Repository navigation
Extend paste-into-cell design note to cover #333 - #597
Conversation
The same paste path that wafflebase#528 fixes (multi-block clipboard payload merged into a table cell) turns out to also cause wafflebase#333: pasting a table containing nested tables leaves the pasted copy's inner cell blocks sharing ids with the source, so clicking/typing into a nested table on the pasted copy silently edits the source instead (and the deepest nested level rejects the caret outright). Both bugs live on the same insertBlocks code path and are fixed by the same change (recursive fresh-id regeneration + a findBlockInCells fallback for blocks not yet in the layout-time parent map), so this folds wafflebase#333 into the existing note rather than adding a new file, per docs/design/README.md's guidance to fold single-PR notes into the relevant doc. Design-note-first; implementation already drafted on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 58 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR expands paste-into-cell handling to multi-block insertion and nested-table input routing. It adds cell-aware block chaining, recursive ID and block lookup, recursive nested-table hit testing, demo table controls, updated design/task documentation, and regression tests. ChangesPaste-into-cell behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TextEditor
participant Doc
participant NestedTableResolver
User->>TextEditor: paste table content or click nested cell
TextEditor->>TextEditor: clone blocks with fresh IDs
TextEditor->>Doc: insertBlockAfter chained blocks
Doc-->>TextEditor: preserve containing cell placement
TextEditor->>NestedTableResolver: resolve nested-table offset
NestedTableResolver->>Doc: find owning block
Doc-->>TextEditor: return blockId and offset
TextEditor-->>User: place caret in pasted table
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/design/docs/docs-paste-table-into-cell.md`:
- Around line 139-149: Update the documentation’s verification statements around
the model tests and manual checks to use planned or conditional wording rather
than claiming completion. Revise the sections covering the listed test coverage
and repro/regression checks, or link to concrete landed test and
manual-verification evidence if available; do not imply documentation-only
changes have already exercised or re-ran those checks.
- Around line 18-22: The documentation incorrectly attributes issues `#528` and
`#333` to the same change. Reword the affected note to distinguish the body-only
insertion fix from the separate recursive cloning and _blockParentMap lookup
fallback required for nested pasted tables, while describing both as coordinated
fixes on the shared paste path.
🪄 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 Plus
Run ID: 7b8f867d-4589-47d7-aafc-4bb9a0f76ff1
📒 Files selected for processing (1)
docs/design/docs/docs-paste-table-into-cell.md
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Two wording fixes from PR wafflebase#597's review: - Summary said wafflebase#528 and wafflebase#333 are "fixed by the same change" — reword to describe them as separate fixes (insertBlockAfter chaining for wafflebase#528, recursive fresh-id cloning + findBlockInCells fallback for wafflebase#333) that happen to land on the same insertBlocks code path. - Testing Strategy / Risks described wafflebase#333 test coverage and repro re-verification as already landed, but this PR is documentation only — the wafflebase#333-specific regression test isn't in this diff yet. Reword to distinguish the four existing tests (already on main from wafflebase#528/PR wafflebase#531) from the wafflebase#333 regression test (planned for the implementation PR), and note the manual repro re-verification was against a local draft, not this PR's contents. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pasting a table that contains nested tables left the pasted copy's
inner cell blocks sharing ids with the source: the multi-block paste
path's middle-block clone only refreshed a block's own top-level id
(`{ ...block, id: generateBlockId() }`), not ids nested inside its
table cells. Doc.getBlock / findBlockInCells then resolved those
shared ids back to the source for every level above the deepest
(caret visually correct, input landed in the source instead); the
deepest level's cell wasn't in _blockParentMap yet (only rebuilt at
the next layout pass) and couldn't resolve at all, rejecting the
caret outright.
Swap the middle-block clone for cloneBlockWithFreshIds (already used
by the single-table paste branch) — its refreshBlockIds already
recurses through tableData.rows[].cells[].blocks[], so this alone
fixes the id-sharing. findBlockInCells gains a walkCellsForBlock
recursive fallback for blocks not yet in _blockParentMap (e.g. the
paste's own middle blocks), so getBlock resolves them instead of
throwing "Block not found".
Adds a model-level regression test asserting getBlock resolves a
block inside a freshly pasted nested table before any layout pass has
run, and a Table button in the standalone docs demo for manually
exercising nested-table paste.
Design note: docs/design/docs/docs-paste-table-into-cell.md (wafflebase#333
folded into the existing wafflebase#528 paste-into-cell note, see PR wafflebase#597).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
resolveOffsetInCellAtXY resolved a click into a cell's inner table (2 nesting levels deep), but a table nested inside that inner table's own cell only got a "further nesting" comment and a bail-out that returned a position on the table block itself — which has no text — instead of continuing to drill in. That is what made the deepest level of a 3+-level nested table reject the caret outright: clicking did nothing because the resolved position wasn't inside any editable content. Extract the inner-table resolution into resolveOffsetInNestedTable, a recursive method: it resolves row/column/cell/line for the table it's given, and calls itself when that cell's target line is itself a nested table, instead of stopping after one level. Only the outermost table's rows are page/split-aware (only they have entries in the paginated layout — layoutCellBlocks in table-layout.ts lays out a nested table as a single synthetic line inside its parent cell), so the page-aware Y lookup happens once, in the caller, and the recursive method carries that resolved Y origin forward with plain relative arithmetic for every level below it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/tasks/active/20260730-docs-nested-table-paste-input-routing-todo.md (1)
44-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWork checklist looks stale relative to this PR. The regression test at line 54 appears implemented (
packages/docs/test/model/paste-table-into-cell.test.tsnow contains the#333test). Consider ticking it before merge; per prior learnings the doc should be archived viapnpm tasks:archiverather than banner-marked as complete.🤖 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 `@docs/tasks/active/20260730-docs-nested-table-paste-input-routing-todo.md` around lines 44 - 62, Update the Work checklist to mark the `#333` automated regression test complete, reflecting the existing test in paste-table-into-cell.test.ts. After the remaining work is finished, archive this task document using pnpm tasks:archive rather than adding a completion banner.Source: Learnings
packages/docs/src/view/text-editor.ts (1)
4677-4683: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring contradicts the recursion call. It says
localXis "reused unchanged across levels", but the recursive call at Line 4781 passescellLocalX(cell-content-relative). The code looks right; the comment should say the X coordinate is re-based onto the cell's content box at each level.🤖 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/docs/src/view/text-editor.ts` around lines 4677 - 4683, The docstring for the recursive table layout method incorrectly says localX is reused unchanged. Update the description around tableAbsY and localX to state that each recursive level re-bases the X coordinate to the containing cell’s content box, matching the recursive call’s cellLocalX argument; leave the implementation unchanged.packages/docs/src/model/document.ts (1)
177-204: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFallback walk repeats per recursion level and on every miss.
Two effects worth considering:
- The self-call at Line 191 re-enters
findBlockInCells, which — when its own fast path misses — runs the full body/header/footer walk for the parent table id before returning. So a deep nested lookup can trigger O(depth) full document walks.findBlockis called on hot paths (e.g. the cursor-validity guard inhandleKeyDown); for a genuinely missing id the walk now traverses every table in all three regions on each call.Extracting a
resolveViaParentMap(blockId)helper (fast path only, recursive) and runningwalkCellsForBlockexactly once at the top level would keep the fix while bounding the cost.🤖 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/docs/src/model/document.ts` around lines 177 - 204, Refactor the cell-block lookup around findBlockInCells so the parent-map fast path is isolated in a recursive resolveViaParentMap(blockId) helper that does not perform the full fallback walk. Ensure nested lookups use this helper, while findBlockInCells invokes walkCellsForBlock across document, header, and footer regions exactly once after the fast path fails, including for missing IDs.packages/docs/test/model/paste-table-into-cell.test.ts (1)
152-167: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider also asserting ID disjointness from the source. The
#333root cause was shared IDs;collectIds(Line 8) already exists for exactly this. One extra assertion pins the regression directly:💚 Suggested addition
expect(() => doc.getBlock(deepestBlockId)).not.toThrow(); expect(doc.getBlock(deepestBlockId).id).toBe(deepestBlockId); + // Fresh ids at every nesting level — no sharing with the source table. + const sourceIds = new Set(collectIds(sourceOuter)); + for (const id of collectIds(pasted)) expect(sourceIds.has(id)).toBe(false);🤖 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/docs/test/model/paste-table-into-cell.test.ts` around lines 152 - 167, Add an assertion using the existing collectIds helper to verify that the cloned pasted table's IDs are disjoint from sourceOuter's IDs before or after insertion. Keep the existing deepest-block lookup assertions unchanged, and ensure the check covers all IDs in both the source and cloned structures.
🤖 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 `@docs/tasks/active/20260730-docs-nested-table-paste-input-routing-todo.md`:
- Line 11: Update the “#333's symptom, by nesting depth:” text in the documented
issue section so the leading issue reference is wrapped or otherwise formatted
as inline content, preventing markdownlint from interpreting it as an ATX
heading while preserving the displayed wording.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 4720-4736: Update the merged-cell resolution block around
dataCell0 so scanning stops immediately after the first valid owning cell is
found; use a labeled outer-loop break or an equivalent immediate return,
ensuring later rows cannot overwrite the resolved row and col.
---
Nitpick comments:
In `@docs/tasks/active/20260730-docs-nested-table-paste-input-routing-todo.md`:
- Around line 44-62: Update the Work checklist to mark the `#333` automated
regression test complete, reflecting the existing test in
paste-table-into-cell.test.ts. After the remaining work is finished, archive
this task document using pnpm tasks:archive rather than adding a completion
banner.
In `@packages/docs/src/model/document.ts`:
- Around line 177-204: Refactor the cell-block lookup around findBlockInCells so
the parent-map fast path is isolated in a recursive resolveViaParentMap(blockId)
helper that does not perform the full fallback walk. Ensure nested lookups use
this helper, while findBlockInCells invokes walkCellsForBlock across document,
header, and footer regions exactly once after the fast path fails, including for
missing IDs.
In `@packages/docs/src/view/text-editor.ts`:
- Around line 4677-4683: The docstring for the recursive table layout method
incorrectly says localX is reused unchanged. Update the description around
tableAbsY and localX to state that each recursive level re-bases the X
coordinate to the containing cell’s content box, matching the recursive call’s
cellLocalX argument; leave the implementation unchanged.
In `@packages/docs/test/model/paste-table-into-cell.test.ts`:
- Around line 152-167: Add an assertion using the existing collectIds helper to
verify that the cloned pasted table's IDs are disjoint from sourceOuter's IDs
before or after insertion. Keep the existing deepest-block lookup assertions
unchanged, and ensure the check covers all IDs in both the source and cloned
structures.
🪄 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 Plus
Run ID: 783eceb3-1326-4d26-9fde-38eb41e49d3c
📒 Files selected for processing (7)
docs/design/docs/docs-paste-table-into-cell.mddocs/tasks/active/20260730-docs-nested-table-paste-input-routing-todo.mdpackages/docs/demo.tspackages/docs/index.htmlpackages/docs/src/model/document.tspackages/docs/src/view/text-editor.tspackages/docs/test/model/paste-table-into-cell.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/design/docs/docs-paste-table-into-cell.md
|
Apologies — I jumped ahead of our own process here. Per the repo's design-note-first workflow (note PR → maintainer agreement → implementation), I should have waited for direction/agreement on the design note before pushing the implementation. Instead I pushed the implementation commits ( The implementation is up for reference/early review if useful, but I'm treating the design note as not yet agreed, and I'm happy to hold off on anything further (or roll the implementation back to a separate branch) until there's agreement on the approach. Sorry for the noise. |
The merged-cell resolution in resolveOffsetInNestedTable used a bare break that only exited the inner column loop, so the outer row loop kept scanning against the already-mutated row/col and could re-match a lower row, overwriting the correct merge top-left. Use a labeled `break outer`, matching the sibling resolveHFCellOffset. Also correct the docstring: the X coordinate is re-based onto each cell's content box (the recursion passes cellLocalX), not "reused unchanged across levels". Add an id- disjointness assertion pinning wafflebase#333's shared-id root cause directly, and wrap the leading issue ref in the task note so markdownlint stops reading it as an ATX heading. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hackerwins
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
Summary
Design note for issue #333: pasting a table that contains nested tables
misroutes input on the pasted copy — the deepest nested level rejects the
caret outright, and every level above it visually accepts the caret but
routes typed characters into the corresponding cell of the original
table instead.
This turns out to share its root cause and its fix with #528 (already
merged, PR #531): the multi-block paste path's fresh-id regeneration only
covered a cloned block's own top-level id, not ids nested inside its table
cells, so a pasted nested table's inner cell blocks kept the source's ids.
Rather than opening a new design doc, this folds #333 into the existing
docs-paste-table-into-cell.mdnote perdocs/design/README.md's guidanceto fold single-PR notes into the relevant doc when they land on the same
code path.
This is the design-note-first step (no code yet) — opening for
direction before pushing the implementation. Implementation is already
drafted locally and will follow on this same branch.
Why
Doc.getBlock/findBlockInCellsresolved the shared ids back to thesource table for every level above the deepest (visually-correct caret,
wrong input target); the deepest level's cell wasn't in
_blockParentMapyet (only rebuilt at the next layout pass) and so couldn't resolve at all,
which is why it rejected the caret outright.
The note proposes: use
cloneBlockWithFreshIds(already used for thesingle-table paste branch) for the multi-block branch's middle-block clones
too — its
refreshBlockIdsalready recurses throughtableData.rows[].cells[].blocks[], so this alone fixes the id-sharing —plus a
walkCellsForBlockrecursive fallback infindBlockInCellsforblocks not yet in
_blockParentMap(e.g. the paste's own middle blocks).Test plan
No code changes yet — design note only. The note's Testing Strategy covers
the implementation that will follow: a model-level regression test
(
Doc.getBlockresolves a block inside a freshly pasted nested table thatis not yet in
_blockParentMap), plus manual re-verification of #333's2-level and 3-level repro steps in the
packages/docsstandalone demo.Notes for Reviewers
AI tools assisted with this PR: the root-cause investigation (tracing the
id-sharing bug through
cloneBlockWithFreshIdsandfindBlockInCells) andthe note draft were produced with Claude Code (Claude Sonnet 5). I reviewed
the root-cause analysis and every line of the note.
Fixes #333.
Summary by CodeRabbit