feat(document-outline.js): carry a PDF region's caption and interpretation - #1223
Conversation
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — the two-map caption/figure attachment refactor is correct (nearest-gap wins, exact ties resolved by processing order which matches the top-to-bottom region sort), and the new caption/interpretation fields are additive and follow the package's "associated, not moved" convention.
What was reviewed: the attachCaptions two-pass rewrite (claim/caption maps, tie-break correctness against the documented top-to-bottom sort order and verticalGap/horizontallyOverlaps helpers), the new optional caption/interpretation fields on PdfRegion (type-only import from document-schema.js, no Worker-isomorphism violation), and the new/extended tests (sandwiched-figure nearest-caption case, single-occurrence-of-caption-text assertion). Confirmed no other package yet consumes PdfRegion, so this is a self-contained, low-blast-radius change.
Extended reasoning...
Overview
The diff is confined to packages/document-outline.js/src/outline/pdf-regions.ts and its test file. It adds two optional fields (caption: string, interpretation: ContentInterpretation) to the exported PdfRegion interface, and rewrites the attachCaptions second pass from a one-region map into a two-map algorithm that (1) finds each caption candidate's nearest adjacent figure and gap, (2) for each figure keeps only the nearest-gap caption, and (3) produces output where the claimed caption region is still reclassified 'caption' as before, while the winning figure additionally gets the caption text attached via the new caption field. A regionText helper is factored out of duplicated join/trim logic.
Security risks
None. This is pure client-side geometric inference over already-in-memory PDF layout data; no I/O, no external input parsing beyond what already existed, no new dependencies, and the new schema import is type-only (import type { ContentInterpretation } from "document-schema.js"), so it cannot introduce a runtime Node-only dependency and keeps the package Worker-isomorphic per the root and package CLAUDE.md conventions.
Level of scrutiny
I traced the tie-break logic by hand against the actual sort in segmentPdfRegions (.sort((a, b) => b.bounds.yPt - a.bounds.yPt || ...), i.e., descending yPt = top-to-bottom in PDF's bottom-left-origin space), confirming the code comment's claim that the topmost candidate wins strict ties is accurate. I also checked that claimed (caption regions) and captionFor (figure regions) key spaces cannot collide (figures are filtered out of the candidate loop entirely), so the render pass's claimed-then-captionFor check order can't double-apply. I verified no other package in the workspace currently consumes PdfRegion, so the additive fields carry zero cross-package blast radius today.
Other factors
The new "gives a figure the nearer of two candidate captions" test exercises the exact scenario the refactor was built for, and the existing caption test was extended to assert the text is not duplicated across regions (count of exactly 1), directly encoding the "associated, not moved" invariant the PR description and package CLAUDE.md both call out. The change is small, self-contained, well-commented, and the logic checks out under manual trace, so I'm comfortable approving without requiring a human to re-verify the arithmetic.
…ation Two additive fields on `PdfRegion`, following #1197's close-out: the annotation channel landed on content nodes, and a region is the one place an interpretation of a *vector* figure can attach at all. **`interpretation`** reuses `ContentInterpretationSchema` rather than re-minting it -- exported from `document-schema.js` for this. Never set by `segmentPdfRegions`: this package infers geometry and nothing in it calls a model or reads pixels. The field exists because a chart drawn as paths is dozens of sibling rect/line/path items with no single content node whose extent is the chart, so the region is the only container whose content *is* the thing being described -- the rule settled on #1197. A consumer that rasterises a region and reads it puts the result here. **`caption`** records a relationship the pass was already computing and then discarding. `attachCaptions` had to find which figure a short text run belonged to in order to classify that run as a caption at all, and dropped the answer -- so a consumer wanting a figure's own label had to re-derive the adjacency this function had just worked out. It is now recorded in both directions: the text leaf still becomes its own 'caption' region, and the figure it labels carries that text. Associated, not moved, which is the same rule `ContentImageBlock.caption` follows for docx and for the same reason: the caption stays its own region, so a consumer projecting every region's text reads it exactly once. A figure sandwiched between two short runs has two candidates and only one is its label, so the nearer wins -- the same gap that already decides a caption's own confidence. Both fields optional; `segmentPdfRegions`' existing output is unchanged for any page with no figure-and-caption pair, and no consumer needs to know about either. Two notes from writing the tests, in case they save someone time. The figure-side assertion was verified by planting its absence (the test fails with `expected undefined`). And the two-candidate case has to place both runs in a narrow window the segmentation itself forces: wider than the local cut threshold (~1.5x the caption's own font size, below which the run is never split into its own region) and within `CAPTION_GAP_PT`. My first attempt used 14pt and 4pt gaps and produced a single `column` region containing everything -- no figure, no caption, nothing to assert on. Refs #1197.
…st it
An adversarial review found the new "nearer of two candidate captions"
case could not fail for the property it names, and it was right:
`regions` arrives sorted top-to-bottom, so the fixture's nearer caption
was also the later-processed one, and a naive last-writer-wins agreed
with the correct answer on that input. Verified by mutating the
comparison to `if (true)` -- all ten cases still passed. This package
runs Stryker, so that was a live surviving mutant rather than a
theoretical one.
The fixture is inverted so the nearer caption is the one ABOVE, which
is processed first and therefore makes the comparison load-bearing.
Re-verified under the same mutation: it now fails.
Two other review findings, both real:
The "associated, not moved" assertion was a tautology -- it checked the
caption region's own single item is text, which the fixture already
guarantees and which would hold even if the figure had swallowed the
item too. It now counts occurrences of the caption string across every
region and expects exactly one, which is what the claim actually says.
`claimed` stored `{ figure, gap }` and nothing ever read `figure`; it is
now `Map<PdfRegion, number>`. Which figure won is recorded on the
figure's own side, which was the point.
Also documented, since both were inferable only from a sort two hundred
lines away: on an exact tie the run ABOVE the figure wins (and a run
equidistant from two figures is claimed by the upper one), and the two
views deliberately disagree on counts -- both runs in a sandwich are
reclassified 'caption' while only one figure carries text, so the loser
is a 'caption' region labelling nothing.
…ure-unaffected assertion The rebase merge for attachCaptions's bidirectional caption tracking dropped regionText's own definition while removing what looked like a duplicate horizontallyOverlaps declaration -- restored it, since both branches of the merge relied on it. Separately, one direct unit test still asserted that attachCaptions leaves a matched figure region reference-identical to its input. That was true before this feature and is no longer the intended behaviour: a figure that matches a caption now carries that caption's text on its own caption field, so the assertion is updated to expect the merged figure rather than the original object.
f9118f7 to
c9bf0b7
Compare
|
🎉 This PR is included in version 3.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
The follow-up you named when closing #1197 — "extending
document-outline.js's region shape to carry aninterpretationof its own is a clean follow-up on your side — the field group is exported for exactly that reuse". Two additive fields onPdfRegion.interpretationReuses
ContentInterpretationSchemarather than re-minting it. Never set bysegmentPdfRegions— this package infers geometry and nothing in it calls a model or reads pixels.The field exists because a region is the one place an interpretation of a vector figure can attach at all. A chart drawn as paths is dozens of sibling
rect/line/pathitems with no single content node whose extent is the chart, so the region is the only container whose content is the thing being described — which is the rule we settled on #1197. A consumer that rasterises a region (now possible, via #1207's port) and reads it puts the result here.caption— a relationship the pass was already computing and throwing awayThis one wasn't in the original ask, and it's the more immediately useful half.
attachCaptionshas to find which figure a short text run belongs to in order to classify that run as a caption at all — and then dropped the answer. So a consumer wanting a figure's own label had to re-derive the adjacency the function had just worked out.It's now recorded in both directions: the text leaf still becomes its own
captionregion, and the figure it labels carries that text oncaption.Associated, not moved — the same rule
ContentImageBlock.captionfollows for docx, and for the same reason: the caption stays its own region, so a consumer projecting every region's text still reads it exactly once. A figure sandwiched between two short runs has two candidates and only one is its label, so the nearer wins, using the same gap that already decides the caption's own confidence.Why it matters downstream: handing a figure's region to something that reads it is far more useful when the author's own caption comes with it. That's the "give the model the neighbouring text as context" step, and this makes it a field read rather than a geometry re-derivation.
Scope
Both optional. Output is unchanged for any page with no figure-and-caption pair, and no existing consumer needs to know about either.
Verification
Full workspace
pnpm typecheck(70 tasks),pnpm test(39) andpnpm lint(39) pass. Two test additions, and the figure-side assertion was verified by planting its absence — it fails withexpected undefined to be 'Figure 1: …'.One note that may save someone time: the two-candidate test has to place both runs in a narrow window the segmentation itself forces — wider than the local cut threshold (~1.5× the caption's own font size, below which the run is never split into its own region) and within
CAPTION_GAP_PT. My first attempt used 14pt and 4pt gaps and got a singlecolumnregion containing everything: no figure, no caption, nothing to assert on. Worth a line in the module's own comment if you think it's a trap others will hit.Unrelated, but while I'm here
pdf-raster-cpuis merged but not published — 404 on npm,version: 0.0.0. Its README already carries npm badges, so I assume the first release run just hasn't happened. Could you publish it? It's the one thing still blocking us: its header says it has no canvas, no DOM, nonode:*and runs identically under Node, a browser Worker and workerd, which removes the runtime problem we opened #1198 worrying about — we'd rather depend on it than write a second backend against the same port.Refs #1197.