fix(table): keep glyphs straddling a table outer edge (19% of negatives were losing their sign) - #14
Conversation
charsInCell assigns a glyph to whichever cell contains its centre. That is the right rule for an interior boundary -- it decides which of two candidate cells owns a straddling glyph, and every glyph still lands somewhere. At the table outer edge there is no competing cell, so the same rule does not disambiguate, it deletes. On 3M 2018 10-K page 58 the value "(16,048)" ends with a ")" centred at x=537.879 while the last column ends at x=537.871. It missed by 0.008pt, about a nine-thousandth of an inch, and was dropped -- turning the accounting notation for -16,048 into a plain 16,048. Across that filing five financial statements the same geometry flipped the sign of 20 of 103 negative numbers, 19%, with every magnitude still correct. A sign error that leaves plausible-looking output is worse than a missing value: a gap is detectable downstream, this is not. Outermost cells now also keep glyphs that merely overlap the outer edge. The widening is bounded by the straddling glyph itself, so it cannot reach unrelated page content. Interior boundaries are untouched, and charsInCell keeps its exact previous behaviour for every other caller. Re-measured on the same five pages: 103 of 103 negatives now survive. The regression test pins the real coordinates rather than round synthetic ones, because the failure only reproduces at that margin.
Reviewer's GuideModify table text assembly to keep glyphs that straddle a table’s outer horizontal edges, introduce a more general cell-edge-aware glyph selection helper, and add targeted regression tests to guard against both the original data-loss bug and unintended widening of table cells. Sequence diagram for glyph assignment with outer table edgessequenceDiagram
participant assembleTableText
participant charsInCellEdges
assembleTableText->>assembleTableText: iterate tb.CellsGrid rows
assembleTableText->>assembleTableText: find first,last nonzero columns
assembleTableText->>charsInCellEdges: charsInCellEdges(chars, cell, ci==first, ci==last)
activate charsInCellEdges
charsInCellEdges->>charsInCellEdges: compute hMid,vMid
alt [vMid outside cell vertical range]
charsInCellEdges-->>charsInCellEdges: skip glyph
else [vMid inside]
alt [interior column]
charsInCellEdges-->>charsInCellEdges: in = hMid within cell.X0..cell.X1
else [outerRight && glyph straddles right edge]
charsInCellEdges-->>charsInCellEdges: in = true
else [outerLeft && glyph straddles left edge]
charsInCellEdges-->>charsInCellEdges: in = true
end
charsInCellEdges-->>charsInCellEdges: append glyph if in
end
charsInCellEdges-->>assembleTableText: []Char cellChars
deactivate charsInCellEdges
assembleTableText->>assembleTableText: emit cell text from cellChars
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
charsInCellEdgessignature with two booleans (outerLeft,outerRight) makes the call sites a bit opaque; consider using a small enum or separate helpers for left/right outer edges to make intent clearer and avoid misconfiguration. - The new comments embed a lot of filing-specific narrative; consider trimming them down to the minimal explanation of the edge-case geometry and linking to the detailed ticket instead to keep the codebase focused and easier to skim.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `charsInCellEdges` signature with two booleans (`outerLeft`, `outerRight`) makes the call sites a bit opaque; consider using a small enum or separate helpers for left/right outer edges to make intent clearer and avoid misconfiguration.
- The new comments embed a lot of filing-specific narrative; consider trimming them down to the minimal explanation of the edge-case geometry and linking to the detailed ticket instead to keep the codebase focused and easier to skim.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fixes a silent sign-flip on financial data. Stacked on #9 — that branch is the base, so merge #9 first.
The bug
charsInCellassigns a glyph to whichever cell contains its centre. That is correct for an interior boundary: it decides which of two candidate cells owns a straddling glyph, and every glyph still lands somewhere.At a table's outer edge there is no competing cell. The same rule stops disambiguating and starts deleting.
How it showed up
3M 2018 10-K, page 58, consolidated balance sheet. The value
(16,048)ends with a)spanning x=536.691..539.067, so its centre is 537.879. The last column ends at 537.871.It missed by 0.008pt — about a nine-thousandth of an inch — and was dropped.
(16,048)became16,048: accounting notation for −16,048, read back as +16,048.Across that filing's five financial statements:
Every magnitude was already correct — only signs were wrong. That is what makes it dangerous: the output looks entirely plausible, so nothing downstream can flag it. A missing value is detectable; a flipped sign is not.
The fix
Outermost cells also keep glyphs that merely overlap the outer edge. The widening is bounded by the straddling glyph itself, so it cannot reach unrelated page content — only a glyph genuinely crossing the table's own boundary.
charsInCell: keeps its exact previous behaviour for every other caller (delegates with both flags false, pinned by a test).Verification
go build,go vet,go test ./... -count=1 -race— all green.pdftotext -layoutcross-check. Both confirm the parentheses exist in the document.The regression test pins the real coordinates rather than round synthetic ones; the failure only reproduces at that 0.008pt margin.
Not fixed here
The opening
(still lands in the adjacent column (| ( | 16,135) |). That one is recoverable — the glyph exists, joining the row restores the value — so it is cosmetic rather than corrupting, and it needs column-boundary work rather than an edge rule. Same for| December 3 | 1, |. Tracked in HAL-520.Closes HAL-520
Summary by Sourcery
Preserve glyphs that straddle a table’s outer edges so negative values retain their signs while keeping existing midpoint behaviour for interior boundaries unchanged.
Bug Fixes:
Tests: