Render rows after a table row-split fragment on the same page#170
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (6)
📝 WalkthroughWalkthroughThis PR fixes a bug where non-split table rows on a page following a split fragment fail to render. It introduces a centralized predicate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 48 minutes and 38 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/tasks/active/20260501-table-split-followup-rows-todo.md (1)
1-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the paired lessons file for this non-trivial active task.
This task is substantial, but only the
-todo.mdfile is present. Please adddocs/tasks/active/20260501-table-split-followup-rows-lessons.mdand link it from the task index once available.
As per coding guidelines "docs/tasks/active/*-{todo,lessons}.md: For non-trivial tasks, use paired files indocs/tasks/active/:YYYYMMDD-<slug>-todo.mdandYYYYMMDD-<slug>-lessons.md."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tasks/active/20260501-table-split-followup-rows-todo.md` around lines 1 - 57, Create a paired lessons file named docs/tasks/active/20260501-table-split-followup-rows-lessons.md that summarizes the fix (Option B), the root cause (computeTableRangeForPageLine sweep behavior and dedup in collectTableRenderRanges), the test added (packages/docs/test/view/table-row-split.test.ts), and any reviewer decisions (extracted shouldStartTableRender predicate), plus links to the repro and commit d91c60b3; then update the task index to link this new lessons file next to the existing TODO entry so the pair docs/tasks/active/20260501-table-split-followup-rows-{todo,lessons}.md exist.
🧹 Nitpick comments (1)
packages/docs/src/view/doc-canvas.ts (1)
115-119: ⚡ Quick winUpdate the
collectTableRenderRangesdocstring to match new behavior.After this fix, one table can legitimately produce multiple ranges on a page (split fragment + follow-up rows). The current “one entry per table” wording is now misleading.
Suggested doc comment tweak
- * Collect render args for every table block that has at least one row - * on this page. Returns one entry per table (deduped across PageLines) - * so the background pre-pass touches each table once per page. + * Collect render args for table render passes on this page. + * Usually this is one range per table block, but split-fragment pages + * may yield multiple ranges for the same table (e.g. split fragment + + * follow-up non-split rows) to keep background/content passes aligned.🤖 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 115 - 119, The docstring for collectTableRenderRanges is outdated — it claims there is "one entry per table (deduped across PageLines)" but the function now can emit multiple ranges for a single table (e.g., split fragment plus follow-up rows). Update the comment above the export function collectTableRenderRanges to clearly state that the function collects render args for every table block that has at least one row and may return multiple ranges per table when rows are split across fragments, and remove or reword the "one entry per table" / dedup wording so it matches the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/tasks/active/20260501-table-split-followup-rows-todo.md`:
- Around line 1-57: Create a paired lessons file named
docs/tasks/active/20260501-table-split-followup-rows-lessons.md that summarizes
the fix (Option B), the root cause (computeTableRangeForPageLine sweep behavior
and dedup in collectTableRenderRanges), the test added
(packages/docs/test/view/table-row-split.test.ts), and any reviewer decisions
(extracted shouldStartTableRender predicate), plus links to the repro and commit
d91c60b3; then update the task index to link this new lessons file next to the
existing TODO entry so the pair
docs/tasks/active/20260501-table-split-followup-rows-{todo,lessons}.md exist.
---
Nitpick comments:
In `@packages/docs/src/view/doc-canvas.ts`:
- Around line 115-119: The docstring for collectTableRenderRanges is outdated —
it claims there is "one entry per table (deduped across PageLines)" but the
function now can emit multiple ranges for a single table (e.g., split fragment
plus follow-up rows). Update the comment above the export function
collectTableRenderRanges to clearly state that the function collects render args
for every table block that has at least one row and may return multiple ranges
per table when rows are split across fragments, and remove or reword the "one
entry per table" / dedup wording so it matches the new behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52283360-349a-4d67-bb14-a80ed1595f82
📒 Files selected for processing (6)
docs/design/docs/docs-table-row-splitting.mddocs/tasks/README.mddocs/tasks/active/20260501-table-split-followup-rows-todo.mddocs/tasks/archive/README.mdpackages/docs/src/view/doc-canvas.tspackages/docs/test/view/table-row-split.test.ts
💤 Files with no reviewable changes (1)
- docs/tasks/archive/README.md
f0f62a1 to
7500290
Compare
Verification: verify:selfResult: ✅ PASS in 133.7s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
When a table row is split across pages and the split fragment lands as the first PageLine on the next page, follow-up non-split rows of the same block on that same page were never painted. Layout positioned them correctly (search highlights and selection coordinates worked), but `renderTableContent` was never invoked for them, so cell text and borders were silently dropped — the rows looked like blank space. Root cause: the dedup in `collectTableRenderRanges` and the body loop in `DocCanvas.render` skipped subsequent PageLines that shared the same `blockIndex` as the previous one, assuming the first PageLine's render swept forward over them. That assumption breaks for split fragments because `computeTableRangeForPageLine` intentionally suppresses the sweep for them (so the clipped split pass only covers its own row). Fix: extract a shared `shouldStartTableRender` predicate and have both passes call it. The predicate now also returns true when the previous PL was a split fragment of the same block, so follow-up non-split rows get their own render pass and sweep the remaining rows in one call. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
7500290 to
2917724
Compare
1 participant
Summary
renderTableContentwas never called for them, leaving the rows visually blank.shouldStartTableRenderpredicate so the background pre-pass (collectTableRenderRanges) and the body content pass stay in lockstep. The predicate now also returns true when the previous PageLine was a split fragment of the same block.packages/docs/test/view/table-row-split.test.tsthat builds a 1×3 table whose row 0 splits across two pages and asserts the continuation page emits both the clipped split-fragment range and a follow-up range covering rows 1–2.Why
computeTableRangeForPageLineintentionally suppresses the forward sweep for split fragments (so the clipped split pass only covers its own row). When a split fragment lands as the first PageLine on a page, that single render only covers row N — but the dedup logic in both render passes treated subsequent PageLines as "already covered by the first render" because they shared the sameblockIndex. Result: rows N+1, N+2, … were skipped entirely on that page despite having valid layout positions. This affected real documents — e.g. a 20-row outer table where the section "3. 지원동기 및 포부" (rows 16–17) and "4. 프로젝트 핵심 목표" (rows 18–19) were invisible on page 3 of a shared doc, while Ctrl+F could still find their text.This case was missed in #145 (
d91c60b3, table row splitting). Section 2.5 ofdocs/design/docs/docs-table-row-splitting.mdis updated to document the dedup-with-split rule so future maintainers don't reintroduce it.Test plan
pnpm --filter @wafflebase/docs test— 623 tests pass including the new regression testpnpm verify:fast— full lane passeshttp://localhost:5173/shared/2ec0d5ec-8c46-4b75-a046-b903768b1aab— page 3 sections "3. 지원동기 및 포부" and "4. 프로젝트 핵심 목표" now render with cell content and borderscollectTableRenderRangesand the body loop predicate (both callshouldStartTableRender)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation