Fix four DOCX / rendering bugs found in v0.3.2 smoke test#117
Conversation
Two preexisting DOCX importer bugs surfaced during the v0.3.2 browser smoke test: bold/italic/strikethrough toggles with w:val="0" are treated as ON, and the tbl gridCol lookup recurses into nested tables. Plan covers both fixes with failing-test-first ordering and inline-XML test fixtures to match the existing style. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
OOXML uses <w:b w:val="0"/>, <w:i w:val="0"/>, and <w:strike w:val="0"/> (and the equivalent "false") to explicitly clear an inherited style on a run. Missing val means on. The importer was only checking element presence, so every explicit "off" override became an "on", which surfaced as heavy unintended strikethrough (and bold/italic) when importing real-world Korean form documents containing hundreds of such overrides. Extract a small isToggleOn helper and route b/i/strike through it. Underline and highlight already handled w:val="none" correctly and are unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
getElementsByTagNameNS recurses into every descendant, so the outer <w:tbl>'s column parse was absorbing <w:gridCol> elements from every nested table inside it. The row walk already used direct-child iteration, so the asymmetry collapsed outer column widths whenever a table wrapped other tables. In real-world Korean form documents this hit every "별첨 인적 사항" sheet: the outer 1-column wrapper picked up four nested 3-column grids, reported 12+ columns, and the real single column shrank to about 8% of the content width with every row wrapping vertically in a narrow strip on later pages. Walk direct children for the outer <w:tblGrid> and its <w:gridCol> entries via a small findDirectChild helper, keeping the grid and row walks symmetric. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors table rendering into separate background and content passes; fixes DOCX import table parsing to use direct-child Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Canvas as doc-canvas
participant Layout as Layout Data
participant TR as table-renderer
Canvas->>Layout: computeTableRangeForPageLine(pl)
Canvas->>Canvas: collectTableRenderRanges(page)
Canvas->>TR: renderTableBackgrounds(tableData, layout, x,y, startRow,endRow)
TR->>Canvas: fillRect (cell backgrounds)
Canvas->>Canvas: draw selection highlight (between passes)
Canvas->>TR: renderTableContent(tableData, layout, x,y, startRow,endRow)
TR->>Canvas: fillText / stroke (text & borders)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 120.6s
Verification: verify:integrationResult: ✅ PASS |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/docs/test/import/docx-style-map.test.ts (1)
90-95: Add an italicw:val="false"parity test.Small coverage gap: Line 90-95 checks italic
"0"but not"false"(already covered for bold/strike). Adding one keeps the toggle matrix symmetric and regression-proof.As per coding guidelines: "
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests for critical business logic and edge cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/test/import/docx-style-map.test.ts` around lines 90 - 95, Add a new unit test in the same test suite (packages/docs/test/import/docx-style-map.test.ts) mirroring the existing one for w:val="0" but using '<w:i w:val="false"/>' instead: parse the XML into an element, call mapRunProperties(el), and assert that the returned style.italic is undefined; reference the existing test around mapRunProperties to follow the same structure and naming convention so bold/strike and italic parity are covered.
🤖 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-import-bugs-todo.md`:
- Around line 1-152: Create a new paired lessons file named
20260411-docx-import-bugs-lessons.md that records the outcome of the task in
20260411-docx-import-bugs-todo.md: summarize what was fixed (Bug A: style
toggles — mention isToggleOn helper; Bug B: table gridCols traversal — mention
directChildByLocalName or direct-child tblGrid/gridCol walk), list blockers
encountered, tests added (style-matrix and table column regression cases), any
decisions or trade-offs, and follow-up actions (release cut, verification
commands like pnpm --filter `@wafflebase/docs` test / pnpm verify:fast); keep it
brief, structured (Outcome, Blockers, Tests, Follow-ups), and committed
alongside the two bug-fix commits.
In `@packages/docs/src/import/docx-importer.ts`:
- Around line 225-227: The parsing of the table column width token `w` (via
el.getAttributeNS(W, 'w') || el.getAttribute('w:w')) can yield non-numeric
values that parseInt() turns into NaN and then pollutes
colWidthsRaw/columnWidths; validate the parsed value before pushing: call
parseInt(..., 10), check with Number.isFinite(parsed) or !Number.isNaN(parsed)
and push the numeric value only when valid, otherwise push the fallback 1;
update the code around the variables `w`, `colWidthsRaw`, and `columnWidths` in
docx-importer (where parseInt is used) to perform this numeric validation and
fallback.
---
Nitpick comments:
In `@packages/docs/test/import/docx-style-map.test.ts`:
- Around line 90-95: Add a new unit test in the same test suite
(packages/docs/test/import/docx-style-map.test.ts) mirroring the existing one
for w:val="0" but using '<w:i w:val="false"/>' instead: parse the XML into an
element, call mapRunProperties(el), and assert that the returned style.italic is
undefined; reference the existing test around mapRunProperties to follow the
same structure and naming convention so bold/strike and italic parity are
covered.
🪄 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: 0630bc64-4fc0-4caf-b7f3-df60af691562
📒 Files selected for processing (5)
docs/tasks/active/20260411-docx-import-bugs-todo.mdpackages/docs/src/import/docx-importer.tspackages/docs/src/import/docx-style-map.tspackages/docs/test/import/docx-importer.test.tspackages/docs/test/import/docx-style-map.test.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Renames the task file from docx-import-bugs to v0.3.2-smoke-test-fixes to cover the selection-hidden-by-cell- background rendering bug surfaced during the same form.docx smoke test. Adds Bug C (table renderer split + doc-canvas two-pass render order) and flags the related inline run background limitation as a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Selecting text inside a cell that has a backgroundColor produced no visible selection: the editor's page render drew the highlight layers first and then drew the table, whose opaque cell bg fillRect covered the translucent blue overlay. The theme's selectionColor is rgba(.., .., .., 0.3) and is designed to sit on top of backgrounds, so the fix is to draw the cell backgrounds before any highlights instead of changing colors or alpha. Split renderTable into renderTableBackgrounds (fillRect pass only) and renderTableContent (text + borders), keeping the existing renderTable as a facade for tests and single-pass callers. In doc-canvas, add a background pre-pass before the search/peer/local selection highlights and call renderTableContent from the main text loop. The row-range computation (forward peek over consecutive PageLines + backward extension for rowSpan owners from previous pages) is extracted into two helpers so both passes stay in lockstep. Inline run backgrounds sit inside the text pass and still cover selections — tracked as a follow-up in the task plan. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/docs/src/view/doc-canvas.ts (1)
518-525: Avoid the extraindexOf()scan in the hot render loop.
DocCanvas.render()runs often, and this adds a linear lookup for every first table line on the page. Switching the outer loop to an indexed loop, or reusing the range computed incollectTableRenderRanges(), keeps the new two-pass path cheaper on large documents.♻️ Minimal cleanup
- for (const pl of page.lines) { + for (let plIndex = 0; plIndex < page.lines.length; plIndex++) { + const pl = page.lines[plIndex]; // Render horizontal-rule blocks as a thin line if (layout) { const block = layout.blocks[pl.blockIndex]?.block; @@ - const plIndex = page.lines.indexOf(pl); if (plIndex === 0 || page.lines[plIndex - 1]?.blockIndex !== pl.blockIndex) { const range = computeTableRangeForPageLine(page, lb, pl, plIndex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/src/view/doc-canvas.ts` around lines 518 - 525, DocCanvas.render currently does a page.lines.indexOf(pl) inside the hot render loop which causes an extra linear scan; change the loop over page.lines to an indexed for loop (use for (let i = 0; i < page.lines.length; i++) { const pl = page.lines[i]; ... }) or else reuse the precomputed ranges from collectTableRenderRanges instead of recomputing via computeTableRangeForPageLine, and replace uses of plIndex derived from indexOf with the loop index (or the mapped precomputed range) to eliminate the O(n) indexOf call in render.packages/docs/test/view/table-renderer.test.ts (1)
130-189: Add one regression test at theDocCanvas.render()layer.These tests prove the split renderer APIs, but they would still pass if
packages/docs/src/view/doc-canvas.tsregressed to drawing selection before both passes, or skipped the background pre-pass entirely. A small canvas-spy test aroundDocCanvas.render()would lock down the actual Bug C failure mode.As per coding guidelines, "Write tests for critical business logic and edge cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs/test/view/table-renderer.test.ts` around lines 130 - 189, Add a regression test that calls DocCanvas.render() (the class/method DocCanvas.render) with a recording/mock canvas context and asserts that the renderer still performs the background pass before drawing text and that both passes run (i.e., one fillRect call for the background, four fillText calls for content, stroke invoked) and that the invocationCallOrder for the single background fillRect occurs before the first fillText; implement this by reusing the existing makeRecordingCtx() pattern used in table-renderer.test.ts to create ctx spies, invoking new DocCanvas(...).render(...) with the same tableData/layout fixture (or a minimal equivalent), and asserting counts and ordering to lock down the bug described.
🤖 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-v0.3.2-smoke-test-fixes-todo.md`:
- Around line 1-24: Add a paired lessons file next to the todo: create
docs/tasks/active/20260411-v0.3.2-smoke-test-fixes-lessons.md containing the
retrospective/lessons learned for this non-trivial task (summary of root causes
and fixes for Bug A, Bug B, Bug C, testing notes, and any follow-ups), ensuring
the filename matches the todo slug and follows the project's paired-files
convention referenced in the todo.
---
Nitpick comments:
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 518-525: DocCanvas.render currently does a page.lines.indexOf(pl)
inside the hot render loop which causes an extra linear scan; change the loop
over page.lines to an indexed for loop (use for (let i = 0; i <
page.lines.length; i++) { const pl = page.lines[i]; ... }) or else reuse the
precomputed ranges from collectTableRenderRanges instead of recomputing via
computeTableRangeForPageLine, and replace uses of plIndex derived from indexOf
with the loop index (or the mapped precomputed range) to eliminate the O(n)
indexOf call in render.
In `@packages/docs/test/view/table-renderer.test.ts`:
- Around line 130-189: Add a regression test that calls DocCanvas.render() (the
class/method DocCanvas.render) with a recording/mock canvas context and asserts
that the renderer still performs the background pass before drawing text and
that both passes run (i.e., one fillRect call for the background, four fillText
calls for content, stroke invoked) and that the invocationCallOrder for the
single background fillRect occurs before the first fillText; implement this by
reusing the existing makeRecordingCtx() pattern used in table-renderer.test.ts
to create ctx spies, invoking new DocCanvas(...).render(...) with the same
tableData/layout fixture (or a minimal equivalent), and asserting counts and
ordering to lock down the bug described.
🪄 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: c8678f07-f7ed-4910-9b46-5eff5566fe2e
📒 Files selected for processing (4)
docs/tasks/active/20260411-v0.3.2-smoke-test-fixes-todo.mdpackages/docs/src/view/doc-canvas.tspackages/docs/src/view/table-renderer.tspackages/docs/test/view/table-renderer.test.ts
Clicking the right half of a merged cell imported from DOCX did nothing: in a row like [A, B(gridSpan=4)] the cursor would not move inside B unless the click landed near the left edge of the merged area. The data-model contract (set by Doc.mergeCells in the model) is that a row's cells array has one entry per grid column, with covered positions marked colSpan=0. Layout, renderer, click handler, and exporter all index row.cells[c] by grid column and rely on that contract. The importer honored it only for vMerge: a horizontal gridSpan > 1 pushed just the owner and advanced colIdx, leaving cells.length shorter than numCols. Click handling then skipped the owner-search branch because cells[c] was undefined, not a placeholder with colSpan=0. Extract a module-level makeCoveredCell helper and pad (colSpan − 1) placeholders after every owner push. Also fix the vMerge=continue branch so a gridSpan combined with vMerge pushes one placeholder per grid column it covers instead of just one regardless of gridSpan. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The codecov/patch status was failing PRs that legitimately drop coverage — for example, a refactor that splits an exhaustively tested file into two helpers forces an artificial "raise coverage" commit even when the underlying behavior is still fully exercised. Coverage is a trailing indicator of test discipline, not a merge gate. Add codecov.yml with `informational: true` on both project and patch status so the coverage diff still posts on every PR (reviewers can still see the number) but never blocks. If we later want to hard-gate on a specific subset, per-flag entries can override the default without reverting this file. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Add <w:i w:val="false"/> parity test to close the toggle matrix (bold/italic/strike × "0"/"false"/"1"). - Guard the tblGrid w:w parser against non-numeric values with Number.isFinite + positive check, falling back to a unit weight so malformed input degrades to an even share. - Replace page.lines.indexOf(pl) in the hot render loop with an indexed for loop so DocCanvas.render stays O(n) per page. - Add a static import-scan regression guard in table-renderer.test.ts asserting doc-canvas imports only renderTableBackgrounds / renderTableContent, never the renderTable facade — catches the specific drift class that would reintroduce Bug C. - Add the paired lessons file 20260411-v0.3.2-smoke-test-fixes-lessons.md capturing the four bugs' root causes, fix patterns, and cross-cutting meta-lessons (real-document smoke tests, coverage as trailing indicator, static guards over heavy integration setup). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-v0.3.2-smoke-test-fixes-todo.md`:
- Around line 263-272: The tests block for Bug C (the three checklist items
referencing renderTableBackgrounds, renderTableContent, and renderTable) is
misplaced after Bug D and duplicates a "### Tests" heading; move that entire
block (the checklist lines mentioning
renderTableBackgrounds/renderTableContent/renderTable) into the Bug C section
immediately after the Bug C content (just before the Bug D heading) and remove
the duplicate checklist where it currently appears so there is only one "###
Tests" under Bug C.
🪄 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: 0f995ee8-4ec2-4b2a-a230-b05798371297
📒 Files selected for processing (8)
codecov.ymldocs/tasks/active/20260411-v0.3.2-smoke-test-fixes-lessons.mddocs/tasks/active/20260411-v0.3.2-smoke-test-fixes-todo.mdpackages/docs/src/import/docx-importer.tspackages/docs/src/view/doc-canvas.tspackages/docs/test/import/docx-importer.test.tspackages/docs/test/import/docx-style-map.test.tspackages/docs/test/view/table-renderer.test.ts
✅ Files skipped from review due to trivial changes (1)
- codecov.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/docs/test/import/docx-style-map.test.ts
- packages/docs/test/view/table-renderer.test.ts
- packages/docs/test/import/docx-importer.test.ts
- packages/docs/src/view/doc-canvas.ts
DOCX import/export shipped in PR #114 (merged to main as bbce164 on 2026-04-11) as part of v0.3.2. The 20260410-docx-import-export-todo.md plan was the execution blueprint for that work — every task from Phase 1 prerequisite features through Phase 4 frontend integration exists in the codebase, but only the last five checkboxes were ever ticked because the execution pipeline ran end-to-end faster than the manual checkbox update. The plan is accurate in spirit; mark all items complete and move it to archive/2026/04/. Also regenerate docs/tasks/archive/README.md via `pnpm tasks:index`. The index picks up two entries that were physically archived in PR #110 (header/footer and page break task plans) but never made it into the README because the index regenerator wasn't run at that time. Restoring them now keeps the published archive list consistent with the filesystem. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
User confirmed the browser smoke test on PR #117 passed: the four bugs are fixed in the rendered form.docx — no unintended strikethrough, no narrow-strip tables on the "별첨" pages, visible selection inside colored cells, and correct cursor placement when clicking anywhere inside a horizontally merged cell. Check off the remaining planned tests (all implemented in docx-style-map.test.ts, docx-importer.test.ts, and the new table-renderer.test.ts — 448 → 464), the PR opened / review addressed / archive steps, and add an Outcome section summarizing the verification. Archive both the todo and lessons files together, regenerate the tasks index. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
# Conflicts: # docs/tasks/README.md # docs/tasks/archive/README.md
Summary
Four bugs surfaced during the v0.3.2 browser smoke test against a
real-world Korean form document (46 tables, 513 explicit
strikethrough overrides, 509 explicit bold/italic overrides, 6
tables wrapping nested tables, cells with background colors, and
horizontally merged cells). All four are editor / import path
only — no changes to server, schema, or Yorkie / S3 integration.
v0.3.2 is kept as-is; these ship in v0.3.3.
Bug A —
w:val="0"treated as ON for bold / italic / strikedocx-style-map.tschecked only element presence, so<w:b w:val="0"/>,<w:i w:val="0"/>,<w:strike w:val="0"/>(and
"false") all forced the toggle ON instead of OFF. OOXMLuses these to explicitly clear an inherited style per-run; a
missing val is the shorthand for "on". Underline and highlight
already handled
w:val="none"correctly and are unchanged.Fix: small
isToggleOnhelper used for all three.Bug B — Nested-table gridCol leak into outer columnWidths
tblEl.getElementsByTagNameNS('gridCol')recurses into everydescendant, so an outer table wrapping nested tables absorbed
every nested
<w:gridCol>into its own column count. The<w:tr>walk already used direct-child iteration, so the grid lookup was
the asymmetric survivor. In the real-world form this collapsed
single-column wrapper tables to ~8% of the content width on every
"별첨" page, with every row wrapping vertically in a narrow strip.
Fix: walk direct children for
<w:tblGrid>and its<w:gridCol>entries via a small
findDirectChildhelper, keeping the gridand row walks symmetric.
Bug C — Selection highlight hidden under table cell background
Selecting text inside a cell that has a
backgroundColorproducedno visible selection: the editor's page render drew the highlight
layers (search, peer cursor, local selection) first, then drew
tables, and
renderTableinternally painted cell backgrounds withopaque
fillRects that covered the translucent blue selectionoverlay.
Theme.selectionColorisrgba(66, 133, 244, 0.3)andis designed to sit on top of backgrounds, so the right fix is to
draw the cell backgrounds before any highlights.
Fix: split
renderTableintorenderTableBackgrounds(fillRectpass only) and
renderTableContent(text + borders), keepingthe existing
renderTableas a facade for tests and single-passcallers. In
doc-canvasadd a background pre-pass before theselection highlights and call
renderTableContentfrom the maintext loop. The row-range computation (forward peek over
consecutive PageLines + backward extension for rowSpan owners
from previous pages) is extracted into two helpers so both
passes stay in lockstep.
Inline run backgrounds sit inside the text pass and still cover
selections — same class of bug but a bigger refactor, tracked as
a follow-up in the task plan.
Bug D — Horizontal merge placeholders missing from imported rows
Clicking the right half of a merged cell imported from DOCX did
nothing: in a row like
[A, B(gridSpan=4)]the cursor refusedto move inside B unless the click landed near the left edge.
The data-model contract — set by
Doc.mergeCellsin the modeland honored by layout, renderer, click handler, and exporter —
is that a row's
cellsarray has one entry per grid column,with covered positions marked
colSpan === 0. The importerhonored this only for vertical merges; a horizontal
gridSpanpushed just the owner and advanced the colIdx, so the cells
array was shorter than numCols. The click handler's owner
fallback (gated on
cell?.colSpan === 0) silently skippedbecause
cells[c]wasundefined, not a placeholder.Fix: extract a
makeCoveredCellhelper and pad(colSpan − 1)placeholders after every owner push. Also fixthe edge case where a vMerge=continue
tchas its own gridSpangreater than 1 — all covered grid columns now get placeholders
instead of just one.
Commits
f0420cf4— Task plan for v0.3.3 DOCX import bug fixescca6489d— Respectw:val="0"for bold / italic / strike (Bug A)40539722— Scope DOCXtblGridlookup to direct children (Bug B)e706447b— Broaden task plan scope to cover all smoke test fixesb0f786fa— Draw table cell backgrounds before the selection highlight (Bug C)a7bac962— Pad horizontal merge placeholders in DOCX table import (Bug D)Test plan
pnpm --filter @wafflebase/docs test— 462 passed (was 448)docx-style-map.test.ts: 15 → 21 (+6 Bug A cases)docx-importer.test.ts: 12 → 16 (+2 Bug B + 2 Bug D cases)table-renderer.test.ts: new, 4 cases asserting the two-passsplit for Bug C (backgrounds pass has no
fillText/stroke,content pass has no cell
fillRect, facade preserves relativeorder)
pnpm verify:fast— 29 files, 462 tests passpnpm verify:selfpre-push hook — 6 lanes passform.docxin production tovisually confirm no unintended strikethrough, no narrow-strip
tables on later pages, a visible selection inside colored
cells, and that clicking anywhere inside a merged cell
lands the cursor correctly
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores