Skip to content

Harden DOCX table merge import for row-shape edge cases#118

Merged
hackerwins merged 6 commits into
mainfrom
docx-table-merge-hardening
Apr 11, 2026
Merged

Harden DOCX table merge import for row-shape edge cases#118
hackerwins merged 6 commits into
mainfrom
docx-table-merge-hardening

Conversation

@hackerwins

@hackerwins hackerwins commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to PR #117 focused on the cells.length === numCols contract
that 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 the
wrong cell or truncating merged regions at render time.

  • Honor w:gridBefore / w:gridAfter — rows that leave leading or
    trailing grid columns empty now pad with colSpan=0 placeholders so
    they match numCols. Common in Korean government forms.
  • Widen vMerge continue to the owner's colSpan — when a
    vertical-merge owner declares gridSpan=N but the continuation tc
    declares a smaller span (Word can emit this after manual edits), the
    continuation now covers the full merged width.
  • Promote orphan vMerge=continue to a standalone owner — a
    continuation 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.
  • Clamp out-of-range gridSpan — a tc can declare w:gridSpan
    larger than numCols - colIdx. The clamp stops colIdx from
    walking past the grid, which previously left the row longer than
    every other row in the table.
  • Final row-shape normalize — last-line safety net that pads or
    truncates each row to numCols, so downstream code can trust the
    rectangular shape even if any of the above misses a case we have
    not seen yet.

All behavior is gated on tblGrid being present (numCols > 0). When
the fixture has no tblGrid we 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 test goes from 464
to 471 passing tests.

Test plan

  • `pnpm --filter @wafflebase/docs test` — 471 passing
  • `pnpm verify:fast` (pre-commit hook on every commit)
  • Pre-push harness lanes (verify:fast, frontend/backend/cli builds, chunks, entropy)
  • Manual smoke test: reload real-world form.docx and click each cell of a gridBefore/gridAfter row to confirm the cursor lands correctly
  • Manual smoke test: reload a file with a vMerge restart that has gridSpan > 1 and confirm the merged region stays rectangular

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved DOCX table import to enforce grid-aligned rows, clamp overrun spans, and correctly handle vertical-merge continuations and orphan continues so downstream layout is stable.
  • Tests

    • Added thorough tests covering grid padding/truncation, span clamping, vMerge continuation/promote behavior, and gridless-table compatibility.
  • Documentation

    • Added task and lessons notes tracking remaining mapping gaps, verification rules, and importer hardening decisions.

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.
@coderabbitai

coderabbitai Bot commented Apr 11, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e108b7bd-1773-4053-a8fd-ba67e3cfe0d3

📥 Commits

Reviewing files that changed from the base of the PR and between 0a60837 and 2d71e56.

📒 Files selected for processing (4)
  • docs/tasks/active/20260411-docx-table-merge-gaps-lessons.md
  • docs/tasks/active/20260411-docx-table-merge-gaps-todo.md
  • packages/docs/src/import/docx-importer.ts
  • packages/docs/test/import/docx-importer.test.ts

📝 Walkthrough

Walkthrough

This 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 w:trPr w:gridBefore/w:gridAfter padding, and normalizes rows to exactly numCols cells.

Changes

Cohort / File(s) Summary
Task Documentation
docs/tasks/active/20260411-docx-table-merge-gaps-todo.md
New in-progress TODO listing completed merge-correctness fixes (grid skip padding, vMerge owner/continue handling, orphan promotion, gridSpan clamping, per-row normalization), pending mapping gaps, verification commands, and commit/test rules.
Lessons Documentation
docs/tasks/active/20260411-docx-table-merge-gaps-lessons.md
New lessons file describing invariants (rectangular grid requirement), legacy gridless path (numCols === 0) gating, interpretation of colSpan === 0 as covered, and TDD/workflow notes from implementing the merge fixes.
Core Importer Logic
packages/docs/src/import/docx-importer.ts
Added numCols from columnWidths, gated grid-aware behavior on numCols > 0, added readGridSkip(trPr, ...) to parse w:gridBefore/w:gridAfter, padded rows with covered placeholders, clamped cell colSpan to remaining grid, extended vMerge tracking to include colSpan and widened continue coverage to owner's span (with clamping), and added post-parse row pad-or-truncate normalization. No public API changes; new private helper method added.
Test Coverage
packages/docs/test/import/docx-importer.test.ts
Added multiple Vitest cases verifying padding/truncation vs w:tblGrid, w:gridBefore/w:gridAfter behavior, owner gridSpan clamping, orphan vMerge=continue promotion to owner, and backfilling covered positions when continue spans differ from restart.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 I hopped through XML, tracked each span and line,
Padded gaps and clamped the spans to fit the grid just fine.
No orphan merge shall wander, no row will be askew,
The rabbit stamps the table—uniform and true! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: hardening DOCX table merge import to handle row-shape edge cases, which directly aligns with the changeset's core focus on fixing table grid alignment issues.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docx-table-merge-hardening

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Verification: verify:self

Result: ✅ PASS in 110.7s

Lane Status Duration
sheets:build ✅ pass 12.7s
docs:build ✅ pass 7.2s
verify:fast ✅ pass 55.0s
frontend:build ✅ pass 14.3s
verify:frontend:chunks ✅ pass 0.3s
backend:build ✅ pass 4.5s
cli:build ✅ pass 1.7s
verify:entropy ✅ pass 15.1s

Verification: verify:integration

Result: ✅ PASS

@codecov

codecov Bot commented Apr 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/docs/src/import/docx-importer.ts 86.04% 4 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e4f6e7f and 0a60837.

📒 Files selected for processing (3)
  • docs/tasks/active/20260411-docx-table-merge-gaps-todo.md
  • packages/docs/src/import/docx-importer.ts
  • packages/docs/test/import/docx-importer.test.ts

Comment thread docs/tasks/active/20260411-docx-table-merge-gaps-todo.md
Comment thread packages/docs/src/import/docx-importer.ts
Comment on lines +299 to +300
for (let s = 0; s < gridBefore; s++) cells.push(makeCoveredCell());
let colIdx = gridBefore;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment thread packages/docs/test/import/docx-importer.test.ts
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.
@hackerwins

Copy link
Copy Markdown
Collaborator Author

Thanks for the review — addressed in 2d71e560.

1. Missing paired lessons file ✅ — added docs/tasks/active/20260411-docx-table-merge-gaps-lessons.md alongside the todo, summarizing the cells.length === numCols contract, the colSpan === 0 overloading, and the squash-merge/cherry-pick workflow that produced this PR.

2. gridBefore/gridAfter applied when tblGrid is absent ✅ — confirmed the inconsistency: the clamp and final normalize were gated on numCols > 0 but the skip-marker padding was not. Fixed by gating both readGridSkip calls on numCols > 0.

3. Synthetic placeholders round-trip as bogus <w:vMerge/> ⏭️ — valid but pre-existing, not introduced by this PR. The exporter at packages/docs/src/export/docx-exporter.ts:211-214 serializes every colSpan === 0 cell as <w:vMerge/>, and Doc.mergeCells (packages/docs/src/model/document.ts:713-714) has always set colSpan = 0 for every merged covered cell — including the horizontal-merge placeholders added in #117. There is no existing export test covering merged cells (grep colSpan|gridSpan|vMerge|merge in test/export/docx-exporter.test.ts returns nothing), so any horizontal merge already round-trips as bogus vMerge today. This PR just widens the exposure with gridBefore/gridAfter and the safety-net pad.

The correct fix is exporter-side disambiguation: a covered position absorbed by a prior gridSpan in the same row should not emit a tc at all; a position whose owner lives in an earlier row should emit <w:vMerge/>; and gridBefore/gridAfter should be emitted via trPr skip markers. That's a distinct change and belongs in a follow-up. Filed as item E1 in 20260411-docx-table-merge-gaps-todo.md.

4. Missing gridless-table counterpart tests ✅ — added should not inject gridBefore placeholders when the table has no tblGrid and the matching gridAfter case, both driven TDD-first against the fix in #2. They assert the row keeps exactly the tc count the source shipped.

Test count: 471 → 473 passing. Pre-push harness lanes still green.

@hackerwins
hackerwins merged commit a7af848 into main Apr 11, 2026
1 check passed
@hackerwins
hackerwins deleted the docx-table-merge-hardening branch April 11, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant