fix: improve visual parity diagnostics and layout - #88
Conversation
|
Warning Review limit reached
Next review available in: 52 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR fixes layout parity issues in folio-core: hidden table row support end-to-end (schema, conversion, measure, render), header/footer margin extension refinements with per-section even/odd selection, additive paragraph spacing, justified line-wrap tolerance, TOC/allCaps formatting fixes, cached field fallback, and a CG Times font mapping. It also extends the parity CLI tooling with output/max-pages options, new diagnostic scripts, DOM extraction improvements, and text normalization. ChangesCore Layout Parity Fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Parity CLI Tooling and Text Normalization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant PagedEditor
participant layoutPipeline
participant headerFooterMargins
participant renderPage
PagedEditor->>layoutPipeline: sectionHeaderFooterRefs with evenAndOddHeaders
layoutPipeline->>layoutPipeline: buildSectionHfExtenderContent()
layoutPipeline->>headerFooterMargins: extendSectionBreakMargins(sectionContent)
headerFooterMargins-->>layoutPipeline: extended margins per section
renderPage->>renderPage: applySectionHeaderFooterOptions(evenAndOddHeaders)
sequenceDiagram
participant CLI as parity CLI
participant WordTruth as wordTruth.ts
participant FolioExtract as folioExtract.ts
participant Compare as compare.ts
CLI->>WordTruth: getWordTruth() + limitGeomPages(maxPages)
CLI->>FolioExtract: extract(docxPath, {maxPages})
FolioExtract-->>CLI: truncated pages
CLI->>WordTruth: getWordPagePngs(docxPath, {maxPages})
CLI->>Compare: compareGeoms(word, folio)
Compare-->>CLI: report
CLI->>CLI: writeJsonReport(report, outputPath)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses several layout parity issues between Word and Folio, including handling hidden table rows, reserving paragraph spacing inside table cells, refining header/footer trailing paragraph suppression, and preventing paragraph-mark formatting from bleeding onto visible text. It also introduces new CLI diagnostic tools for comparing reports and tracing source elements. The review feedback suggests optimizing performance in measureParagraph.ts by inlining the lineWidthTolerance calculation to avoid closure overhead in a hot loop, and improving structural safety in measureBlocks.ts by populating empty cell measures for hidden rows to prevent potential runtime crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const lineWidthTolerance = (): number => { | ||
| if (!isJustifiedParagraph) { | ||
| return WIDTH_TOLERANCE; | ||
| } | ||
| return Math.max(WIDTH_TOLERANCE, currentLine.availableWidth * JUSTIFY_SHRINK_TOLERANCE_RATIO); | ||
| }; |
There was a problem hiding this comment.
The closure lineWidthTolerance is created on every call to measureParagraph and then called repeatedly inside the hot word/character loop. We can completely eliminate this closure and its function call overhead by inlining the calculation of widthTolerance directly inside the loop where it is defined.
| // Extract word (includes trailing space if present) | ||
| const word = text.slice(charIndex, nextBreak); | ||
| const wordWidth = measureTextWidth(word, style); | ||
| const widthTolerance = lineWidthTolerance(); |
There was a problem hiding this comment.
Inline the widthTolerance calculation here to avoid the overhead of calling the lineWidthTolerance closure function inside the hot word/character loop.
| const widthTolerance = lineWidthTolerance(); | |
| const widthTolerance = isJustifiedParagraph | |
| ? Math.max(WIDTH_TOLERANCE, currentLine.availableWidth * JUSTIFY_SHRINK_TOLERANCE_RATIO) | |
| : WIDTH_TOLERANCE; |
| if (sourceRow?.hidden) { | ||
| row.height = 0; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
For hidden rows, row.cells is left unpopulated (or empty []), which could lead to undefined errors if any other part of the layout, paginator, or serialization pipeline attempts to access rowMeasure.cells[cellIdx] by index (assuming it matches the source row's cells length). Populate row.cells with empty/zeroed cell measures for hidden rows to ensure structural consistency and prevent potential runtime crashes.
| if (sourceRow?.hidden) { | |
| row.height = 0; | |
| continue; | |
| } | |
| if (sourceRow?.hidden) { | |
| row.height = 0; | |
| row.cells = (sourceRow.cells ?? []).map(() => ({ | |
| blocks: [], | |
| width: 0, | |
| height: 0, | |
| })); | |
| continue; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/layout-painter/renderPage.test.ts (1)
548-575: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest doesn't exercise the
evenAndOddHeadersgate.The test "ignores even footers unless odd/even headers are enabled" uses
sectionPageNumber: 1(odd), so even footers are ignored regardless of theevenAndOddHeadersflag — the old code (sectionPageNumber % 2 === 0) also producesuseEven = false. To properly test the gate, usesectionPageNumber: 2(even) withoutevenAndOddHeadersand assert the default footer is selected. That distinguishes old from new behavior.🧪 Proposed fix
const applied = applySectionHeaderFooterOptions( { ...page, number: 2, - sectionPageNumber: 1, + sectionPageNumber: 2, headerFooterRefs: { footerDefault: "default-footer", footerEven: "even-footer", }, fragments: [], }, pageOptions, { footerContentByRId: new Map([ ["default-footer", headerFooterTextContent("Default footer")], ["even-footer", headerFooterTextContent("Even footer")], ]), }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/layout-painter/renderPage.test.ts` around lines 548 - 575, The test in renderPage.test.ts for applySectionHeaderFooterOptions does not actually cover the evenAndOddHeaders gate because sectionPageNumber is odd, so update the scenario to use an even sectionPageNumber and keep evenAndOddHeaders unset/false, then assert the default footer is chosen. Use the existing test case name and the applySectionHeaderFooterOptions helper to verify that even footers are ignored only when the odd/even headers feature is not enabled.
🧹 Nitpick comments (6)
packages/core/src/layout-engine/paginator.test.ts (1)
90-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-page additive-spacing test.
Both spacing tests only cover the page-break edge case where
trailingSpacingresets to 0. Neither directly verifies the core behavior change — that on the same page,spaceAfterfrom one fragment andspaceBeforefrom the next now sum (spaceBefore + trailingSpacing) instead of collapsing viaMath.max.✅ Proposed additional test
test("preserves explicit spaceBefore at the top of a new page", () => { const paginator = createPaginator({ pageSize: { w: 100, h: 100 }, margins: { top: 10, right: 10, bottom: 10, left: 10 }, }); paginator.addFragment({ kind: "paragraph" } as never, 70, 0, 20); const result = paginator.addFragment({ kind: "paragraph" } as never, 20, 5, 0); expect(paginator.pages.length).toBe(2); expect(result.y).toBe(15); }); + + test("composes spaceAfter and spaceBefore additively on the same page", () => { + const paginator = createPaginator({ + pageSize: { w: 100, h: 100 }, + margins: { top: 10, right: 10, bottom: 10, left: 10 }, + }); + + paginator.addFragment({ kind: "paragraph" } as never, 20, 0, 10); + const result = paginator.addFragment({ kind: "paragraph" } as never, 20, 5, 0); + + expect(paginator.pages.length).toBe(1); + // gap should be spaceAfter(10) + spaceBefore(5) = 15, not max(10,5) = 10 + expect(result.y).toBe(10 + 20 + 15); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/layout-engine/paginator.test.ts` around lines 90 - 117, Add a same-page additive-spacing test in paginator.test.ts to cover the core spacing behavior in createPaginator/addFragment: verify that when two fragments fit on the same page, the next fragment’s y position reflects spaceBefore plus the previous fragment’s trailing spacing (spaceAfter) instead of using Math.max. Place the test alongside the existing "paginator block spacing" cases and assert the expected combined offset on the same page, using addFragment and the returned y value to confirm the additive behavior.packages/core/src/layout-engine/measure/measureParagraph.ts (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the rationale for the justify tolerance ratio.
Every other tolerance/behavioral constant in this file carries a comment explaining its derivation or citing the parity bug it fixes;
JUSTIFY_SHRINK_TOLERANCE_RATIO = 0.012has none. Since this scales the wrap tolerance up to several px on full-width lines (directly affecting visible text overflow for justified paragraphs), a short note on how 1.2% was derived would help future maintainers avoid re-tuning it blindly.📝 Suggested comment
const WIDTH_TOLERANCE = 0.5; +// Word's justification engine compresses/expands inter-word spacing to keep +// lines flush; this ratio approximates that squeeze tolerance as a fraction +// of line width so wider lines can absorb proportionally more overflow +// before wrapping. Empirically tuned — see <issue/PR reference>. const JUSTIFY_SHRINK_TOLERANCE_RATIO = 0.012;Also applies to: 725-730
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/layout-engine/measure/measureParagraph.ts` around lines 54 - 55, Add a short explanatory comment for JUSTIFY_SHRINK_TOLERANCE_RATIO in measureParagraph.ts, alongside the existing tolerance constants, describing why 0.012 (1.2%) was chosen and what behavior it preserves for justified text wrapping. Keep the note consistent with the other rationale comments in this file and mention the relevant wrap/overflow behavior handled by measureParagraph so future changes don’t retune it without context.packages/core/src/prosemirror/conversion/toProseDoc.ts (1)
246-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the TOC branch in
getInheritedRunFormatting.
hasDirectRunFormatting(formatting)does not affect the result here:suppressParagraphMarkFormatting(baseRunFormatting, undefined, formatting)returnsbaseRunFormattinganyway, so this can just returnbaseRunFormattingdirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/prosemirror/conversion/toProseDoc.ts` around lines 246 - 263, The TOC branch in getInheritedRunFormatting is more complex than needed because hasDirectRunFormatting(formatting) does not change the outcome. In toProseDoc.ts, simplify the fieldType === "TOC" path inside getInheritedRunFormatting to return baseRunFormatting directly instead of conditionally calling suppressParagraphMarkFormatting, and leave the rest of the formatting flow unchanged.Source: Coding guidelines
parity/__tests__/cli.test.ts (1)
5-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
--max-pagesvalidation tests.The suite covers
--outputparsing and validation thoroughly but doesn't test--max-pageserror cases (missing value, zero, negative, non-integer). Adding these would guard against regressions in the validation logic at lines 90-97 ofcli.ts.♻️ Suggested additional tests
test("requires a path after --output", () => { expect(() => parseArgs(["fixture.docx", "--output"])).toThrow("--output requires a file path"); expect(() => parseArgs(["fixture.docx", "--output", "--json"])).toThrow( "--output requires a file path", ); }); + + test("requires a positive integer after --max-pages", () => { + expect(() => parseArgs(["fixture.docx", "--max-pages"])).toThrow( + "--max-pages requires a positive integer", + ); + expect(() => parseArgs(["fixture.docx", "--max-pages", "0"])).toThrow( + "--max-pages requires a positive integer", + ); + expect(() => parseArgs(["fixture.docx", "--max-pages", "-5"])).toThrow( + "--max-pages requires a positive integer", + ); + expect(() => parseArgs(["fixture.docx", "--max-pages", "abc"])).toThrow( + "--max-pages requires a positive integer", + ); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@parity/__tests__/cli.test.ts` around lines 5 - 19, Add test coverage for the parseArgs path in parity/__tests__/cli.test.ts to validate --max-pages error handling alongside the existing --output cases. Create tests that exercise parseArgs with --max-pages missing a value, set to 0, negative, and a non-integer, and assert each throws the same validation error expected by the cli.ts parsing logic. Use parseArgs as the primary symbol to locate the behavior and keep the tests focused on the maxPages flag parsing branch.parity/folioExtract.ts (1)
553-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the new
rectForhelper in the fallback path to eliminate duplicated union-rect logic.Lines 559-577 manually compute the union ink rect with the same logic as the new
rectForfunction defined at lines 396-412. Replace the manual computation withrectFor(segmentEls)and fall back to the line element's bounding rect when it returnsnull.♻️ Proposed refactor
const segmentEls = Array.from( lineEl.querySelectorAll(".layout-run, .layout-list-marker"), ) as HTMLElement[]; - let rect = lineEl.getBoundingClientRect(); - let inkLeft = Number.POSITIVE_INFINITY; - let inkTop = Number.POSITIVE_INFINITY; - let inkRight = Number.NEGATIVE_INFINITY; - let inkBottom = Number.NEGATIVE_INFINITY; - for (const segmentEl of segmentEls) { - const segmentRect = segmentEl.getBoundingClientRect(); - if (segmentRect.width <= 0 || segmentRect.height <= 0) continue; - inkLeft = Math.min(inkLeft, segmentRect.left); - inkTop = Math.min(inkTop, segmentRect.top); - inkRight = Math.max(inkRight, segmentRect.right); - inkBottom = Math.max(inkBottom, segmentRect.bottom); - } - if (inkRight > inkLeft && inkBottom > inkTop) { - rect = new DOMRect(inkLeft, inkTop, inkRight - inkLeft, inkBottom - inkTop); - } + const rect = rectFor(segmentEls) ?? lineEl.getBoundingClientRect();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@parity/folioExtract.ts` around lines 553 - 586, The fallback path in folioExtract’s line handling duplicates the union-rect logic already implemented in rectFor, so replace the manual inkLeft/inkTop/inkRight/inkBottom computation with a call to rectFor(segmentEls). If rectFor returns a rect, use it for the line; otherwise keep the existing lineEl.getBoundingClientRect() fallback before calling toRawLine.parity/divergences.ts (1)
99-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd exhaustiveness checks to the
divergencePage,divergenceText, anddivergenceQueryTextswitches.These switches cover all current
DivergenceKindcases but lack adefaultbranch with anevercheck. If a new divergence kind is added, the switches would silently fall through, returningundefined— causing downstream issues likenormalizeLineText(row.text)receivingundefined.divergenceMagnitude(line 159) already has adefaultcase, making this inconsistent.As per coding guidelines: "Use TypeScript discriminated unions, branded types, and exhaustive checks when they make a class of bug structurally impossible."
♻️ Proposed fix for all three switch functions
const divergencePage = (divergence: Divergence): number | undefined => { switch (divergence.kind) { case "page-count": return undefined; case "pagination": return divergence.wordPage; case "line-break": case "missing-line": case "extra-line": case "x-drift": case "y-drift": case "width-drift": case "text-mismatch": return divergence.page; + default: { + const _exhaustive: never = divergence; + throw new Error(`Unhandled divergence kind: ${_exhaustive}`); + } } };Apply the same
defaultblock todivergenceTextanddivergenceQueryText.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@parity/divergences.ts` around lines 99 - 150, Add exhaustive `never` checks to `divergencePage`, `divergenceText`, and `divergenceQueryText` so a new `DivergenceKind` cannot silently return `undefined`. Update each switch in `parity/divergences.ts` to include a `default` branch that forces compile-time exhaustiveness, matching the existing pattern used by `divergenceMagnitude`, and ensure the relevant `Divergence` cases are handled explicitly by those functions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@parity/sourceTrace.ts`:
- Around line 210-229: The blockText helper currently ignores math runs, so
paragraphs containing math are reduced to empty text and later search logic can
miss them. Update blockText in parity/sourceTrace.ts to handle the MathRun
branch alongside the existing text, field, and tab cases, using the math run’s
plainText value (as already used by summarizeRun) so math content is included in
the extracted block text.
---
Outside diff comments:
In `@packages/core/src/layout-painter/renderPage.test.ts`:
- Around line 548-575: The test in renderPage.test.ts for
applySectionHeaderFooterOptions does not actually cover the evenAndOddHeaders
gate because sectionPageNumber is odd, so update the scenario to use an even
sectionPageNumber and keep evenAndOddHeaders unset/false, then assert the
default footer is chosen. Use the existing test case name and the
applySectionHeaderFooterOptions helper to verify that even footers are ignored
only when the odd/even headers feature is not enabled.
---
Nitpick comments:
In `@packages/core/src/layout-engine/measure/measureParagraph.ts`:
- Around line 54-55: Add a short explanatory comment for
JUSTIFY_SHRINK_TOLERANCE_RATIO in measureParagraph.ts, alongside the existing
tolerance constants, describing why 0.012 (1.2%) was chosen and what behavior it
preserves for justified text wrapping. Keep the note consistent with the other
rationale comments in this file and mention the relevant wrap/overflow behavior
handled by measureParagraph so future changes don’t retune it without context.
In `@packages/core/src/layout-engine/paginator.test.ts`:
- Around line 90-117: Add a same-page additive-spacing test in paginator.test.ts
to cover the core spacing behavior in createPaginator/addFragment: verify that
when two fragments fit on the same page, the next fragment’s y position reflects
spaceBefore plus the previous fragment’s trailing spacing (spaceAfter) instead
of using Math.max. Place the test alongside the existing "paginator block
spacing" cases and assert the expected combined offset on the same page, using
addFragment and the returned y value to confirm the additive behavior.
In `@packages/core/src/prosemirror/conversion/toProseDoc.ts`:
- Around line 246-263: The TOC branch in getInheritedRunFormatting is more
complex than needed because hasDirectRunFormatting(formatting) does not change
the outcome. In toProseDoc.ts, simplify the fieldType === "TOC" path inside
getInheritedRunFormatting to return baseRunFormatting directly instead of
conditionally calling suppressParagraphMarkFormatting, and leave the rest of the
formatting flow unchanged.
In `@parity/__tests__/cli.test.ts`:
- Around line 5-19: Add test coverage for the parseArgs path in
parity/__tests__/cli.test.ts to validate --max-pages error handling alongside
the existing --output cases. Create tests that exercise parseArgs with
--max-pages missing a value, set to 0, negative, and a non-integer, and assert
each throws the same validation error expected by the cli.ts parsing logic. Use
parseArgs as the primary symbol to locate the behavior and keep the tests
focused on the maxPages flag parsing branch.
In `@parity/divergences.ts`:
- Around line 99-150: Add exhaustive `never` checks to `divergencePage`,
`divergenceText`, and `divergenceQueryText` so a new `DivergenceKind` cannot
silently return `undefined`. Update each switch in `parity/divergences.ts` to
include a `default` branch that forces compile-time exhaustiveness, matching the
existing pattern used by `divergenceMagnitude`, and ensure the relevant
`Divergence` cases are handled explicitly by those functions.
In `@parity/folioExtract.ts`:
- Around line 553-586: The fallback path in folioExtract’s line handling
duplicates the union-rect logic already implemented in rectFor, so replace the
manual inkLeft/inkTop/inkRight/inkBottom computation with a call to
rectFor(segmentEls). If rectFor returns a rect, use it for the line; otherwise
keep the existing lineEl.getBoundingClientRect() fallback before calling
toRawLine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b4b835a-19f8-4ba8-a7a7-4322b7c8fced
📒 Files selected for processing (41)
.changeset/fix-lma-layout-parity.mdpackages/core/src/controller/layoutPipeline.tspackages/core/src/layout-bridge/convert/headerFooterLayout.test.tspackages/core/src/layout-bridge/convert/headerFooterLayout.tspackages/core/src/layout-bridge/convert/toFlowBlocks.tspackages/core/src/layout-engine/index.tspackages/core/src/layout-engine/measure/measureBlocks.test.tspackages/core/src/layout-engine/measure/measureBlocks.tspackages/core/src/layout-engine/measure/measureParagraph.test.tspackages/core/src/layout-engine/measure/measureParagraph.tspackages/core/src/layout-engine/paginator.test.tspackages/core/src/layout-engine/paginator.tspackages/core/src/layout-engine/types.tspackages/core/src/layout-painter/renderPage.test.tspackages/core/src/layout-painter/renderPage.tspackages/core/src/layout-painter/renderParagraph-right-tab.test.tspackages/core/src/layout-painter/renderParagraph.tspackages/core/src/layout-painter/renderTable.tspackages/core/src/layout-painter/renderTableFragment.test.tspackages/core/src/paged-layout/headerFooterMargins.tspackages/core/src/prosemirror/attrs/index.tspackages/core/src/prosemirror/conversion/fromProseDoc.tspackages/core/src/prosemirror/conversion/toProseDoc.test.tspackages/core/src/prosemirror/conversion/toProseDoc.tspackages/core/src/prosemirror/extensions/nodes/TableExtension.tspackages/core/src/prosemirror/schema/nodes.tspackages/core/src/utils/fontResolver.test.tspackages/core/src/utils/fontResolver.tspackages/react/src/paged-editor/PagedEditor.tsxparity/__tests__/cli.test.tsparity/__tests__/compare.test.tsparity/__tests__/textNorm.test.tsparity/cli.tsparity/compare.tsparity/diffReport.tsparity/divergences.tsparity/folioExtract.tsparity/inspect.tsparity/sourceTrace.tsparity/textNorm.tsparity/wordTruth.ts
| const blockText = (block: FlowBlock): string => { | ||
| if (block.kind === "paragraph") { | ||
| return block.runs | ||
| .map((run) => { | ||
| if (run.kind === "text") return run.text; | ||
| if (run.kind === "field") return run.fallback ?? ""; | ||
| if (run.kind === "tab") return "\t"; | ||
| return ""; | ||
| }) | ||
| .join(""); | ||
| } | ||
| if (block.kind === "table") { | ||
| return block.rows | ||
| .flatMap((row) => row.cells) | ||
| .flatMap((cell) => cell.blocks) | ||
| .map(blockText) | ||
| .join(" "); | ||
| } | ||
| return ""; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
blockText skips math runs, causing false negatives for math content.
The Run union includes MathRun (which has a plainText field, as referenced in summarizeRun at line 204), but blockText returns "" for math runs. If a paragraph contains math content, the search at line 297 would miss it.
🐛 Proposed fix
.map((run) => {
if (run.kind === "text") return run.text;
if (run.kind === "field") return run.fallback ?? "";
if (run.kind === "tab") return "\t";
+ if (run.kind === "math") return run.plainText;
return "";
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const blockText = (block: FlowBlock): string => { | |
| if (block.kind === "paragraph") { | |
| return block.runs | |
| .map((run) => { | |
| if (run.kind === "text") return run.text; | |
| if (run.kind === "field") return run.fallback ?? ""; | |
| if (run.kind === "tab") return "\t"; | |
| return ""; | |
| }) | |
| .join(""); | |
| } | |
| if (block.kind === "table") { | |
| return block.rows | |
| .flatMap((row) => row.cells) | |
| .flatMap((cell) => cell.blocks) | |
| .map(blockText) | |
| .join(" "); | |
| } | |
| return ""; | |
| }; | |
| const blockText = (block: FlowBlock): string => { | |
| if (block.kind === "paragraph") { | |
| return block.runs | |
| .map((run) => { | |
| if (run.kind === "text") return run.text; | |
| if (run.kind === "field") return run.fallback ?? ""; | |
| if (run.kind === "tab") return "\t"; | |
| if (run.kind === "math") return run.plainText; | |
| return ""; | |
| }) | |
| .join(""); | |
| } | |
| if (block.kind === "table") { | |
| return block.rows | |
| .flatMap((row) => row.cells) | |
| .flatMap((cell) => cell.blocks) | |
| .map(blockText) | |
| .join(" "); | |
| } | |
| return ""; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@parity/sourceTrace.ts` around lines 210 - 229, The blockText helper currently
ignores math runs, so paragraphs containing math are reduced to empty text and
later search logic can miss them. Update blockText in parity/sourceTrace.ts to
handle the MathRun branch alongside the existing text, field, and tab cases,
using the math run’s plainText value (as already used by summarizeRun) so math
content is included in the extracted block text.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests