Harden DOCX table merge import for row-shape edge cases#118
Conversation
A <w:trPr> can declare w:gridBefore / w:gridAfter to leave N leading or trailing grid columns empty. Such rows ship fewer <w:tc> children than the table has grid columns; without padding we end up with cells.length < numCols and click/layout misalign the row against the grid. The rest of the stack (layout, renderer, click handler, exporter) indexes row.cells[c] by grid column and relies on the makeCoveredCell placeholder contract, so the importer must emit one cell per grid column just like it already does for horizontal merges. Parse the trPr skip markers via a readGridSkip helper, then pad the row with covered placeholders at the front (gridBefore) and at the back (gridAfter). Add vitest fixtures for both so the shape contract is regressed going forward.
When a vertical merge owner declares gridSpan=N but a subsequent continuation tc declares a smaller gridSpan, the row ends up with fewer covered placeholders than the owner's width. Word can emit this shape when the author edits a merged range without fixing the inner continuation cells, and the mismatch leaves downstream cells pointing at the wrong grid columns — cells.length falls short of numCols and click/layout misalign against the grid. Record the owner's colSpan on the vMergeTracker entry at restart, then widen the continuation's placeholder count to the max of the tc's own gridSpan and the owner's colSpan. The whole merged range is now covered regardless of the inner cell's declared span.
Some DOCX writers emit vMerge="continue" for a column that never opened a restart — typically when an author deletes the anchor row but leaves a continuation cell behind. The old importer silently pushed covered placeholders for these tcs, so the grid positions ended up unreachable: no owner to click into, no content surfaced. When the vMergeTracker has no entry for the current column, fall through to the normal owner path so the tc becomes a real cell with its own blocks. This preserves the author's content and keeps every grid position click-routable.
A tc can declare w:gridSpan larger than the tblGrid has room for — the fixture is malformed, or the author deleted a gridCol without touching the cells. Without clamping, colIdx walks past numCols and the row ends up longer than every other row in the table, so every downstream index into row.cells is off by one. Introduce a numCols snapshot from the tblGrid column count and clamp both the owner-path colSpan and the vMerge continue's effective span to numCols - colIdx whenever the declared span overruns the remaining room. When tblGrid is missing (numCols = 0) we cannot know the true column count, so the clamp stays disabled and behavior is unchanged for that case.
Even with gridBefore/gridAfter, owner-span clamping, and orphan vMerge handling, a real-world docx can still produce rows whose tc count disagrees with the tblGrid — e.g. a row that lost a tc during editing without a compensating gridAfter, or an extra tc that runs past the declared column count. Downstream layout, rendering, click routing, and the exporter all assume every row is exactly numCols wide; a single off-shape row drags the whole table out of alignment. After each row is assembled, pad the tail with covered placeholders when cells.length < numCols and truncate when it exceeds numCols. The step is skipped when tblGrid is missing (numCols = 0) so we do not invent a column count.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR hardens DOCX table import: computes table grid columns, enforces grid-aware row/col alignment and clamping, extends vertical-merge tracking to include owner colSpan and widens continue coverage when needed, supports Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Verification: verify:selfResult: ✅ PASS in 110.7s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tasks/active/20260411-docx-table-merge-gaps-todo.md`:
- Around line 1-100: Add the missing paired lessons file for this task: create
docs/tasks/active/20260411-docx-table-merge-gaps-lessons.md alongside the
existing 20260411-docx-table-merge-gaps-todo.md; the lessons file should
summarize what was changed (items 1–5), decisions made (readGridSkip,
vMergeTracker.colSpan, orphan-continue promotion, gridSpan clamping, final row
normalization), test coverage added, and any follow-ups (items 6–12) so the task
pair follows the docs/tasks/active/*-{todo,lessons}.md convention.
In `@packages/docs/src/import/docx-importer.ts`:
- Around line 299-300: makeCoveredCell currently creates synthetic cells with
colSpan: 0 which the exporter (docx-exporter.ts) treats as real vertical-merge
continuations and emits <w:vMerge/>, causing bogus merge markup on round-trip.
Update makeCoveredCell in docx-importer.ts to mark these synthetic padding cells
with a distinct flag (e.g., syntheticCover: true) and set colSpan consistently
for real cells; then modify the exporter logic that serializes colSpan === 0 (in
docx-exporter.ts) to only emit <w:vMerge/> when the cell is not marked synthetic
(i.e., check !cell.syntheticCover before serializing vMerge). Apply the same
synthetic flag change to the other synthetic-creation site mentioned (the block
at 412-422) so exporter can distinguish true vMerge continuations from
gridBefore/gridAfter padding.
- Around line 294-300: The code is inserting gridBefore/gridAfter covered cells
even when the table has no tblGrid (numCols === 0); change the logic in
DocxImporter where gridBefore/gridAfter are applied (the blocks that call
DocxImporter.readGridSkip and then push makeCoveredCell based on
gridBefore/gridAfter) to only apply that padding when the table actually has a
grid (e.g., numCols > 0 or an explicit tblGrid present). Update both occurrences
of this pattern (the trPr/readGridSkip → gridBefore/gridAfter → loop that pushes
makeCoveredCell and the similar block later) so that when numCols === 0 you skip
adding these synthetic covered cells and preserve row shape for gridless tables.
In `@packages/docs/test/import/docx-importer.test.ts`:
- Around line 372-426: Add mirror tests for the gridless-table cases by
duplicating the existing w:gridBefore and w:gridAfter test scenarios but remove
the <w:tblGrid> section in each fixture so the importer exercises the "no
tblGrid" path; use the same helpers (createMinimalDocx and DocxImporter.import)
and assert that no synthetic covered cells are inserted (i.e., cells.length
equals the number of provided <w:tc> and any placeholder cells should not be
created — check colSpan === 0 expectations are applied to the correct indices or
omitted as appropriate), and place these new tests alongside the existing
"should pad leading placeholders for w:gridBefore" and "should pad trailing
placeholders for w:gridAfter" tests in the same file to catch the regression.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 107e809c-8072-4fa4-a1b7-399cff3e9cc3
📒 Files selected for processing (3)
docs/tasks/active/20260411-docx-table-merge-gaps-todo.mdpackages/docs/src/import/docx-importer.tspackages/docs/test/import/docx-importer.test.ts
| for (let s = 0; s < gridBefore; s++) cells.push(makeCoveredCell()); | ||
| let colIdx = gridBefore; |
There was a problem hiding this comment.
These new placeholders round-trip as bogus vMerge continuations.
makeCoveredCell() encodes every synthetic slot as colSpan: 0, but packages/docs/src/export/docx-exporter.ts:208-225 currently serializes any colSpan === 0 cell as <w:vMerge/>. That is fine for real vertical-merge continuations, but gridBefore/gridAfter padding and the final pad/truncate safety net are not vertical merges. Importing then exporting one of these tables will synthesize merge markup that was never in the source document.
This needs a separate placeholder kind, or exporter logic that can distinguish merge coverage from “row-shape only” synthetic cells.
Also applies to: 412-422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs/src/import/docx-importer.ts` around lines 299 - 300,
makeCoveredCell currently creates synthetic cells with colSpan: 0 which the
exporter (docx-exporter.ts) treats as real vertical-merge continuations and
emits <w:vMerge/>, causing bogus merge markup on round-trip. Update
makeCoveredCell in docx-importer.ts to mark these synthetic padding cells with a
distinct flag (e.g., syntheticCover: true) and set colSpan consistently for real
cells; then modify the exporter logic that serializes colSpan === 0 (in
docx-exporter.ts) to only emit <w:vMerge/> when the cell is not marked synthetic
(i.e., check !cell.syntheticCover before serializing vMerge). Apply the same
synthetic flag change to the other synthetic-creation site mentioned (the block
at 412-422) so exporter can distinguish true vMerge continuations from
gridBefore/gridAfter padding.
The other row-shape hardening (out-of-range gridSpan clamp, final pad/truncate normalize) is explicitly disabled when the table ships no <w:tblGrid>, so that the legacy gridless row walk keeps its historical "one entry per tc" shape. The initial gridBefore / gridAfter handling missed that gate: a row with <w:gridBefore> or <w:gridAfter> in a gridless table still grew synthetic covered cells, silently changing its length relative to every other row in the same table. Gate both readGridSkip calls on numCols > 0 so gridless tables stay untouched, and add two regression fixtures (leading + trailing skip marker) that assert the row keeps exactly the tc count the source shipped. Also file a follow-up todo item (E1) for the pre-existing exporter-side treatment of colSpan === 0 as a vertical-merge continuation and create the paired lessons file for this task.
|
Thanks for the review — addressed in 1. Missing paired lessons file ✅ — added 2. 3. Synthetic placeholders round-trip as bogus The correct fix is exporter-side disambiguation: a covered position absorbed by a prior 4. Missing gridless-table counterpart tests ✅ — added Test count: 471 → 473 passing. Pre-push harness lanes still green. |
Summary
Follow-up to PR #117 focused on the
cells.length === numColscontractthat the rest of the docs table stack (layout, renderer, click handler,
exporter) relies on. Five independent edge cases could each leave a
row mis-shaped relative to the
tblGrid, making clicks land on thewrong cell or truncating merged regions at render time.
w:gridBefore/w:gridAfter— rows that leave leading ortrailing grid columns empty now pad with
colSpan=0placeholders sothey match
numCols. Common in Korean government forms.colSpan— when avertical-merge owner declares
gridSpan=Nbut the continuation tcdeclares a smaller span (Word can emit this after manual edits), the
continuation now covers the full merged width.
vMerge=continueto a standalone owner — acontinuation tc with no prior restart (some writers leave these
behind when the anchor row is deleted) was silently turning the
column's grid positions into unreachable placeholders. It now
becomes a real cell with its own blocks.
gridSpan— a tc can declarew:gridSpanlarger than
numCols - colIdx. The clamp stopscolIdxfromwalking past the grid, which previously left the row longer than
every other row in the table.
truncates each row to
numCols, so downstream code can trust therectangular shape even if any of the above misses a case we have
not seen yet.
All behavior is gated on
tblGridbeing present (numCols > 0). Whenthe fixture has no
tblGridwe leave the row walk unchanged.Each fix landed with a dedicated vitest fixture (TDD). The new test
file adds 7 cases;
pnpm --filter @wafflebase/docs testgoes from 464to 471 passing tests.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation