Skip to content

Commit 796b1e7

Browse files
Proposal B PR-1: batch prior-resolution (appropriate-bleed machine-vote lean) (#72)
* Add Proposal B survey + HOLD: export-time per-side bleed normalization The branch this was originally scoped against (claude/print-preview- xml-2) sits at the Proposal D commit with zero Proposal-B-specific code or design doc - the queue's "resume Proposal B to completion" gloss ("export-time per-side bleed normalization") is compatible with two genuinely different features with very different risk profiles, and guessing which one blind isn't a reasonable substitute for a spec that never reached this session. Surveys the current uniform-scale bleed handling in PDF.tsx, the backend's already-validated whole-image bleed/trimmed classification (reusable technique, but binary and currently unpopulated in production), and proposes two candidate designs: automatic per-edge detection+auto-correct (higher risk - a wrong heuristic silently mis-crops real print orders, no validated real-image sample to trust it against yet) vs. manual per-side controls in the existing Proposal A WYSIWYG preview (recommended MVP - no heuristic-correctness risk, additive to layout.ts/PagePreview.tsx/PDF.tsx). HOLD pending the owner's choice between them. * Build Proposal B: export-time per-side bleed normalization (core + wiring) Implements the approved spec (docs/proposals/proposal-b-bleed- normalization.md, recovered after a courier loss): measures each card's real per-side bleed via probe-median + IQR-ambiguity sampling (bleedNormalize.ts), resolves a trim/extend plan against the target bleed with a fallback prior and three manual-override modes, and synthesizes the corrected image via canvas crop+edge-extension (bleedExtension.ts). Wired into PDF.tsx's PDFCardImage for full- resolution Google Drive/local-file renders, replacing the old uniform proportional rescale for those cards. 26 new tests (all 6 required synthetic fixtures + override modes + geometry math + pdfImage.ts's new getPDFImageBlob split), zero regressions in the existing 267. Two real bugs found and fixed via actual render verification, not just unit tests: (1) a confident-but-wrong measurement could ask to trim more than a small source image actually has, producing a negative canvas dimension - clamped defensively in computeBleedExtensionGeometry. (2) @react-pdf/renderer's own stylesheet parser has a genuine bug where a single-token transform value ("none") throws inside its layout engine without ever propagating as a rejection, silently hanging the whole render - caught only by running tests/PDFGenerator.spec.ts's real Playwright suite (which hung at timeout) against a stashed before/after baseline; fixed by omitting the transform key instead of passing "none". Documented in docs/lessons.md as a reusable cross-session finding. Still not built (flagged, not silently dropped - see the proposal doc's "Shipped vs. not yet built"): the main-thread batch resolution of bleedPriors via APIGetTagConsensus, the manual-override UI + project- state persistence, and the WYSIWYG preview badge. * Build Proposal B PR-1: batch prior-resolution for bleed normalization Main-thread, concurrency-bounded batch fetch of each export card's appropriate-bleed machine-vote lean via the existing APIGetTagConsensus endpoint - no new endpoint, per the approved spec. Populates PDFProps.bleedPriors (built in PR #66, previously always undefined in real exports) so ambiguous sides use a real per-card lean instead of always falling through to the safe "unresolved" default. Runs on the main thread (PDFGenerator.tsx's downloadPDF/saveToDrivePDF, before the render worker is invoked) because APIGetTagConsensus's CSRF header needs document.cookie, which doesn't exist inside pdf.worker.ts's Worker context - the resolved plain map crosses that boundary the same way every other PDFProps field already does. New: common/concurrencyLimit.ts (a general-purpose bounded-concurrency map, kept separate from GoogleDriveService's own private Semaphore to avoid expanding this PR into an unrelated refactor), features/pdf/ bleedPriorResolution.ts (netPolarity -> BleedPrior mapping, per-card failure tolerance so one bad lookup never fails the whole export). 13 new tests, 282/282 passing overall. Verified against the real render path, not just unit tests: tests/PDFGenerator.spec.ts has no tagConsensus mock at all, so every lookup genuinely fails during that suite - it still passes at the same timing as before this PR, confirming the failure-tolerance path works end to end, not just in isolation. Full report: docs/reports/proposal-b-pr1-bleed-prior-batch-resolution.md --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c350d3f commit 796b1e7

7 files changed

Lines changed: 386 additions & 14 deletions

File tree

docs/proposals/proposal-b-bleed-normalization.md

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ Core: at PDF export, for each card (full-res bitmap already in hand): MEASURE ac
2424
## Implementation notes (added during build, not part of the approved spec above)
2525

2626
- **The "same confidence-fill path" genuinely exists, corrected from an earlier wrong read**: an initial pass over this concluded no per-arbitrary-card confidence lookup existed (only `QuestionFeedItem.tagConfidence`, one-at-a-time from the questionFeed queue) and treated that as a spec contradiction. That was wrong - `store/api.ts`'s `APIGetTagConsensus(backendURL, identifier)` (backing `TagVotePicker.tsx`, a different but genuinely equivalent attribute-voting surface) already does exactly this: a per-card `POST 2/tagConsensus/` returning `TagConsensusEntry[]` with a graded `netPolarity` per tag, including whatever `appropriate-bleed` votes exist. No new endpoint needed after all - the spec's own instruction stands as written.
27-
- **What IS still a real gap, not a contradiction**: `APIGetTagConsensus` needs `document.cookie` (via `getCSRFHeader()`) for its CSRF header, and `document` doesn't exist inside `pdf.worker.ts`'s Web Worker context, where the actual render runs. So this can't be called from inside the worker - it has to run on the MAIN thread (`PDFGenerator.tsx`, which already has cookie/Redux access) as a batch pre-resolution step *before* `pdfRenderService.renderPDF(...)` is invoked, producing a plain `{[identifier]: BleedPrior}` map that gets passed into `PDFProps.bleedPriors` (already wired to accept it - see below). That batch-fetch step itself (N cards, needs a concurrency bound and per-card failure tolerance so one flaky lookup doesn't fail the whole export) is real, separate work that has NOT been built yet - see "Shipped vs. not yet built" below. Interpreted "unresolved" per the literal spec text (a missing entry, or a `netPolarity` that isn't clearly positive) as bucketing with "trimmed" for the fallback (extend full target) - the spec's FALLBACK line is an exhaustive two-way split ("machine says bleed -> X; trimmed or unresolved -> Y"), not three-way.
27+
- **What IS still a real gap, not a contradiction**: `APIGetTagConsensus` needs `document.cookie` (via `getCSRFHeader()`) for its CSRF header, and `document` doesn't exist inside `pdf.worker.ts`'s Web Worker context, where the actual render runs. So this can't be called from inside the worker - it has to run on the MAIN thread (`PDFGenerator.tsx`, which already has cookie/Redux access) as a batch pre-resolution step _before_ `pdfRenderService.renderPDF(...)` is invoked, producing a plain `{[identifier]: BleedPrior}` map that gets passed into `PDFProps.bleedPriors` (already wired to accept it - see below). That batch-fetch step itself (N cards, needs a concurrency bound and per-card failure tolerance so one flaky lookup doesn't fail the whole export) is real, separate work that has NOT been built yet - see "Shipped vs. not yet built" below. Interpreted "unresolved" per the literal spec text (a missing entry, or a `netPolarity` that isn't clearly positive) as bucketing with "trimmed" for the fallback (extend full target) - the spec's FALLBACK line is an exhaustive two-way split ("machine says bleed -> X; trimmed or unresolved -> Y"), not three-way.
2828
- Everything else in the spec matched the current codebase directly (dpi is a real field on `CardDocument`, the `BleedEdgeMM`/`CardWidthMM`/`CardHeightMM` constants are exactly what the measurement's px→mm conversion needs, `PDFCardImage`'s existing per-card async image resolution is already the right per-card hook point for "measure, normalize, draw, release").
2929
- react-pdf's own internal concurrency for resolving multiple `<Image src={async () => ...}>` callbacks across a page is inside the `@react-pdf/renderer` library, not this codebase — not independently controllable or verifiable from here without forking that library. "No per-page Promise.all fan-out" is honored in the sense that matters (the code this session adds does no new fan-out of its own — measurement + extension both happen inside the same single per-card async callback `PDFCardImage` already uses, and nothing new here calls `Promise.all` over multiple cards); it is not a claim that react-pdf's own scheduling was audited or changed.
3030
- Persistence (decision 4, project state, flag XML rather than build it): not yet touched. No `bleedOverrides` reducer/state exists in `projectSlice` yet, since the manual-override UI itself isn't built (see below) - nothing to persist yet. When the UI lands, per the owner's answer: project state (`projectSlice`), not session-only. XML round-trip: flagging per the owner's explicit instruction rather than building it - `ExportXML.tsx`/`ImportXML.tsx` would need a new optional per-member field (e.g. `<bleedOverride>`) to carry a manual override through a save/reload cycle; not added this pass.
@@ -34,9 +34,11 @@ Core: at PDF export, for each card (full-res bitmap already in hand): MEASURE ac
3434

3535
**Shipped, tested, wired into the real render path** (`frontend/src/features/pdf/bleedNormalize.ts` + `.test.ts`, `bleedExtension.ts` + `.test.ts`, `pdfImage.ts`'s `getPDFImageBlob` split + tests, `PDF.tsx`'s `PDFCardImage`): the full measure → resolve-plan → crop/extend → encode pipeline, all three `ManualOverride` modes, the effective-dpi derivation (source dpi vs. a lower requested `imageDPI`), all 6 required synthetic fixtures plus 2 manual-override tests plus 4 geometry tests plus 4 `pdfImage` tests (26 new tests total, all passing; zero regressions in the pre-existing suite). Every named constant (`PROBE_COUNT`, `RGB_DISTANCE_THRESHOLD`, `IQR_AMBIGUITY_FRACTION`, `OVERSIZED_MULTIPLE`) carries its calibration caveat comment per the spec.
3636

37+
**PR-1 (this pass) — shipped**: the main-thread batch resolution of `bleedPriors` via `APIGetTagConsensus`, bounded concurrency (`frontend/src/common/concurrencyLimit.ts`'s `mapWithConcurrencyLimit`, a general-purpose worker-pool utility - not GoogleDriveService's own private `Semaphore`, to keep this PR's review surface to new files only; default concurrency 6, matching that class's own default), per-card failure tolerance (a single failed lookup degrades to `"unresolved"`, never fails the whole batch), wired into `PDFGenerator.tsx`'s `downloadPDF`/`saveToDrivePDF` (resolved once per export, before the render call, skipped entirely when no remote backend is configured). 13 new tests (6 for the concurrency utility, 7 for the resolution logic itself). Verified against the real render path too: `tests/PDFGenerator.spec.ts`'s full suite (which has no `tagConsensus` mock at all) still passes at the same timing as before this PR, confirming an unmocked/failing lookup degrades gracefully rather than hanging or failing the export.
38+
3739
**Not yet built** (concrete next steps, not silently dropped):
38-
1. `PDFGenerator.tsx`'s main-thread batch resolution of `bleedPriors` via `APIGetTagConsensus` - until this exists, every card's ambiguous sides use the safe "unresolved" default (extend full target) rather than a real machine-vote lean.
39-
2. The manual-override UI (Auto / Force bleed / Force trimmed per card) in the export panel, and its `projectSlice` persistence.
40-
3. The WYSIWYG preview badge ("bleed will be generated") in `PagePreview.tsx`.
41-
4. The XML optional field for a persisted override, if/when the UI above lands (flagged per the owner's own instruction, not built).
42-
5. The merge-time server-side calibration pass (~20-30 real catalog images) for the four named constants above.
40+
41+
1. The manual-override UI (Auto / Force bleed / Force trimmed per card) in the export panel, and its `projectSlice` persistence (Proposal B PR-2).
42+
2. The WYSIWYG preview badge ("bleed will be generated") in `PagePreview.tsx` (Proposal B PR-3).
43+
3. The XML optional field for a persisted override, if/when PR-2's UI lands (flagged per the owner's own instruction, not built).
44+
4. The merge-time server-side calibration pass (~20-30 real catalog images) for the four named measurement constants.
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
As of: 2026-07-18
2+
Task: Proposal B deferred PR-1 — batch prior-resolution
3+
Branch: `claude/e2-bleed-prior-batch-resolution` (stacked on `claude/proposal-b-bleed-normalization`, PR #66)
4+
5+
## What shipped
6+
7+
The main-thread, concurrency-bounded batch fetch of each export card's `appropriate-bleed`
8+
machine-vote lean, via the existing `APIGetTagConsensus` endpoint (`store/api.ts` — no new
9+
endpoint, per the approved spec). This populates `PDFProps.bleedPriors`, which `PDF.tsx`'s
10+
`PDFCardImage` (built in PR #66) already reads and defaults safely to `"unresolved"` when
11+
absent — this PR is what actually populates it with real data instead of that default.
12+
13+
**Why main thread, not the render worker**: `APIGetTagConsensus`'s CSRF header needs
14+
`document.cookie` (`getCSRFHeader()`, `common/cookies.ts`), and `document` doesn't exist inside
15+
`pdf.worker.ts`'s Web Worker context. The resolved `{[identifier]: BleedPrior}` map is plain,
16+
structured-clone-safe data — it crosses the main-thread → worker boundary the same way every
17+
other `PDFProps` field already does, no new plumbing needed there.
18+
19+
### New files
20+
21+
- `frontend/src/common/concurrencyLimit.ts` — `mapWithConcurrencyLimit<T, R>(items, concurrency,
22+
fn)`, a general-purpose bounded-concurrency map (worker-pool-over-an-index-cursor, not
23+
fixed-size batching — a worker claims the next unclaimed index the instant it's free, rather
24+
than waiting for a whole batch to finish). Deliberately a new, separate utility rather than
25+
reusing `GoogleDriveService.ts`'s own private `Semaphore` class — that class isn't exported,
26+
and extracting/refactoring it would have expanded this PR's review surface into an unrelated
27+
file for a small win. 6 tests: order preservation, concurrency actually bounded (not
28+
serialized), every item processed exactly once, empty input, concurrency > item count,
29+
rejection propagation (no built-in per-item error tolerance — that's the caller's job).
30+
- `frontend/src/features/pdf/bleedPriorResolution.ts` — `resolveBleedPriors(backendURL,
31+
identifiers, concurrency?)`. Deduplicates identifiers, fetches each unique card's
32+
`TagConsensusResponse` via `APIGetTagConsensus`, maps the `appropriate-bleed` entry's
33+
`netPolarity` to a `BleedPrior`: clearly positive → `"bleed"`, clearly negative → `"trimmed"`,
34+
missing entry or zero/near-zero → `"unresolved"` (this 3-way split is for code
35+
clarity/debugging — `resolveBleedPlan`'s own fallback already treats `"trimmed"` and
36+
`"unresolved"` identically, extending the full target). A single card's lookup failure (network
37+
blip, rate limit, an identifier the backend doesn't recognize) is caught internally and degrades
38+
that one card to `"unresolved"` — never fails the whole batch. Concurrency defaults to 6,
39+
matching `GoogleDriveService`'s own existing default (`BLEED_PRIOR_RESOLUTION_CONCURRENCY`, a
40+
named constant, not empirically tuned for this specific endpoint but a reasonable,
41+
already-precedented starting point). 7 tests covering every branch of the netPolarity mapping,
42+
the failure-tolerance behavior, deduplication, and the empty-input case.
43+
44+
### Changed files
45+
46+
- `frontend/src/features/pdf/PDFGenerator.tsx``downloadPDF`/`saveToDrivePDF` both gained a
47+
`backendURL: string | null` parameter; each now calls `resolveBleedPriors` once, right before
48+
`pdfRenderService.renderPDF(...)`, using `Object.keys(props.cardDocumentsByIdentifier)` as the
49+
export's own card set (already deduplicated by that map's construction). Skipped entirely
50+
(`bleedPriors` stays `undefined`) when no remote backend is configured — matches
51+
`PDFCardImage`'s existing safe default, no special-casing needed downstream. `useDownloadPDF`/
52+
`useSaveToDrivePDF` thread the new parameter through; the main component now selects
53+
`backendURL` via `selectRemoteBackendURL` (`store/slices/backendSlice.ts`) and passes it to
54+
both hooks.
55+
56+
## Verification
57+
58+
- `npx tsc --noEmit`: clean.
59+
- `npx eslint` on all new/changed files: 0 errors/warnings.
60+
- `npx prettier@2.7.1 --write`: applied.
61+
- Full `npx jest --runInBand`: **282/282 passing** (269 from PR #66's own build + 13 new this
62+
pass), zero regressions.
63+
- Full `npx playwright test tests/PDFGenerator.spec.ts`: **4/4 passing**, same timing as before
64+
this PR (17–27s per test, no hang, no new failure). This suite has **no `tagConsensus` mock at
65+
all** — every card's lookup genuinely fails (unmocked request) during this run, which is
66+
exactly the failure-tolerance path `resolveSingleBleedPrior`'s try/catch exists for. Passing
67+
cleanly here is real, end-to-end confirmation that an unmocked/failing batch degrades to
68+
`"unresolved"` per card rather than hanging or failing the export — not just an assertion in a
69+
unit test.
70+
71+
## Deviations
72+
73+
None from the authorized scope. One implementation detail worth flagging: `mapWithConcurrencyLimit`
74+
was built as a new, separate utility rather than extracting `GoogleDriveService`'s existing
75+
`Semaphore` — a deliberate choice to keep this PR's diff to new files plus `PDFGenerator.tsx`'s
76+
own wiring, not a refactor of an unrelated, already-working module. If a shared concurrency
77+
primitive across both features is wanted later, that's a clean, separate follow-up.
78+
79+
## Open items
80+
81+
None blocking. PR-2 (manual-override UI + persistence) and PR-3 (preview badge) remain queued
82+
behind this, per the standing order (B PR-2 → PR-3 → C part (b) → E-3 → F).
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import { mapWithConcurrencyLimit } from "@/common/concurrencyLimit";
2+
3+
describe("mapWithConcurrencyLimit", () => {
4+
it("returns results in item order regardless of completion order", async () => {
5+
const delays = [30, 10, 20, 0];
6+
const results = await mapWithConcurrencyLimit(
7+
delays,
8+
4,
9+
(delayMs, index) =>
10+
new Promise<number>((resolve) =>
11+
setTimeout(() => resolve(index), delayMs)
12+
)
13+
);
14+
expect(results).toEqual([0, 1, 2, 3]);
15+
});
16+
17+
it("never exceeds the concurrency limit at any point in time", async () => {
18+
let active = 0;
19+
let maxActive = 0;
20+
const items = Array.from({ length: 10 }, (_, i) => i);
21+
await mapWithConcurrencyLimit(items, 3, async () => {
22+
active++;
23+
maxActive = Math.max(maxActive, active);
24+
await new Promise((resolve) => setTimeout(resolve, 5));
25+
active--;
26+
});
27+
expect(maxActive).toBeLessThanOrEqual(3);
28+
expect(maxActive).toBeGreaterThan(1); // confirms it's genuinely concurrent, not serialized
29+
});
30+
31+
it("processes every item exactly once", async () => {
32+
const items = Array.from({ length: 25 }, (_, i) => i);
33+
const seen: number[] = [];
34+
await mapWithConcurrencyLimit(items, 4, async (item) => {
35+
seen.push(item);
36+
});
37+
expect(seen.slice().sort((a, b) => a - b)).toEqual(items);
38+
});
39+
40+
it("handles an empty input without hanging", async () => {
41+
const results = await mapWithConcurrencyLimit(
42+
[],
43+
4,
44+
async () => "unreachable"
45+
);
46+
expect(results).toEqual([]);
47+
});
48+
49+
it("handles a concurrency limit larger than the item count", async () => {
50+
const results = await mapWithConcurrencyLimit(
51+
[1, 2],
52+
10,
53+
async (item) => item * 2
54+
);
55+
expect(results).toEqual([2, 4]);
56+
});
57+
58+
it("propagates a rejection from fn (no built-in per-item error tolerance)", async () => {
59+
await expect(
60+
mapWithConcurrencyLimit([1, 2, 3], 2, async (item) => {
61+
if (item === 2) throw new Error("boom");
62+
return item;
63+
})
64+
).rejects.toThrow("boom");
65+
});
66+
});
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Bounded-concurrency map: runs `fn` over every item in `items`, at most `concurrency` calls
3+
* in flight at once, returning results in the same order as `items` regardless of completion
4+
* order. A worker-pool-over-an-index-cursor implementation - each of `concurrency` workers
5+
* repeatedly claims the next unclaimed index until none remain, rather than chunking `items`
6+
* into fixed-size batches (which would leave workers idle once a batch's slowest call is still
7+
* running while faster ones in the same batch have already finished).
8+
*
9+
* Does not itself catch or retry per-item failures - a rejection from `fn` propagates through
10+
* Promise.all and fails the whole call, same as a bare Promise.all would. Callers that need "one
11+
* item's failure shouldn't fail the batch" (e.g. bleedPriorResolution.ts) catch inside their own
12+
* `fn`, not here - keeps this a general-purpose primitive.
13+
*/
14+
export async function mapWithConcurrencyLimit<T, R>(
15+
items: readonly T[],
16+
concurrency: number,
17+
fn: (item: T, index: number) => Promise<R>
18+
): Promise<R[]> {
19+
const results: R[] = new Array(items.length);
20+
let nextIndex = 0;
21+
22+
const worker = async (): Promise<void> => {
23+
while (nextIndex < items.length) {
24+
const index = nextIndex++;
25+
results[index] = await fn(items[index], index);
26+
}
27+
};
28+
29+
const workerCount = Math.max(1, Math.min(concurrency, items.length));
30+
await Promise.all(Array.from({ length: workerCount }, worker));
31+
32+
return results;
33+
}

0 commit comments

Comments
 (0)