Skip to content

Add merge/unmerge context menu and fix multi-page merged-cell rendering#119

Merged
hackerwins merged 26 commits into
mainfrom
feature/docs-table-merge-ux
Apr 11, 2026
Merged

Add merge/unmerge context menu and fix multi-page merged-cell rendering#119
hackerwins merged 26 commits into
mainfrom
feature/docs-table-merge-ux

Conversation

@hackerwins

@hackerwins hackerwins commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Expose merge/unmerge as a first-class table context-menu action with
    drag-time auto-expansion over existing merged cells.
  • Make the docs editor handle merged cells that span a page break
    correctly for rendering, cursor, click, selection highlight, and
    ruler. Every consumer now calls a shared computeMergedCellLineLayouts
    helper so pagination math stays consistent.
  • A handful of unrelated but related bugs that surfaced while testing:
    context menu clipping near the viewport edge, resize cursor flashing
    on borders inside a merged cell, and a ?? vs || color fallback
    that made table text on page 2 pick up the stale selection fillStyle.

What the context menu does now

  • New hybrid slot in docs-table-context-menu.tsx: shows "Merge cells"
    (enabled when a ≥ 2-cell range is active, disabled for a single
    cell) or "Unmerge cells" when the cursor is inside an existing merge.
    Range presence wins, so users can grow an existing merge by
    selecting a larger range.
  • Doc.mergeCells / Doc.splitCell were already in place; this PR adds
    the UI entry point and a pure computeTableMergeContext helper that
    the frontend calls once per menu open.
  • expandCellRangeForMerges runs a fixed-point walk over any merged
    cell the drag selection touches, so the highlight previews the exact
    area that will be merged before the user opens the menu. This
    expansion is applied at both write time (drag/Shift+Arrow) and read
    time (peer cursors, programmatic ranges) via normalizeCellRange.

Merged cells across page breaks

When a merged cell's rowSpan crosses a page break, every part of the
editor needs to agree on which row each line belongs to and where that
row lives on each page. The PR introduces computeMergedCellLineLayouts
in table-renderer.ts and uses it everywhere:

Concern Before After
Text rendering Lines drawn twice (once per page) with canvas clip, producing duplicates near the break Per-line owner row filters + shift so each line draws once with fresh top padding on its page
Borders / background One rect anchored to row 0, half-clipped per page Rect sized to the rows physically on the current page — each slice is a complete 4-sided cell
Cursor placement Anchored to the cell's top-left row's PageLine; Arrow Down on page 1 landed in the empty space below row 0 Finds the target line's owner row via the same helper and anchors to that row's PageLine
Click → offset Single cellPageY from row 0; clicking "2" on page 2 selected "3" Each line's absolute Y is reconstructed from its owner row's PageLine
Click → cell localY = mouseY - row 0 Y for all pages; second-page clicks always fell through to the last row Per-page row band in resolveTableFromMouse; rows only hit-test for the page the cursor is over
Cell-range highlight One big rect extending past row 0's bottom into empty space Split into per-page segments by walking the rowYMap
Cell-internal selection midY += lineHeight loop painted middle lines into empty space on page 1 Iterate cell.lines[startIdx..endIdx], position each via line layouts
Resize cursor Fires on row boundaries even when both sides share a merge or the border is on another page detectTableBorder takes pageFirstRow/pageLastRow and tableData so merge-swallowed or off-page borders are skipped
Ruler target page findPageForPosition returned undefined for cell-internal block IDs, ruler always drew on page 1 Use cursorPixel.y to pick the page; also resolves the block style through the parent map

Other fixes

  • Context menu viewport clamp (docs-table-context-menu.tsx): uses
    useLayoutEffect + offsetWidth/offsetHeight so the clamp isn't
    thrown off by the animate-in zoom-in-95 CSS transform. Bumped the
    viewport padding to 8 px.
  • Resize cursor over merged cells (table-resize.ts): the
    merge-swallowed border detection now takes the page's visible row
    range so hover areas on pages where the row isn't laid out don't
    trigger row-resize.
  • Cell-range highlight width for horizontal merges
    (selection.ts): uses LayoutTableCell.width (which already sums
    spanned columns) instead of a single column's pixel width.
  • ??|| for inline text color (table-renderer.ts): empty
    color strings now fall through to Theme.defaultColor, matching
    doc-canvas.ts's renderRun. Previously page 2's table text
    inherited the stale selection fillStyle.

Design + plan

  • Design: `docs/design/docs/docs-tables.md` — new "Cell Range
    Normalization" and "Cell Merge UX" sections describe the shared
    helper, hybrid slot, and expansion loop.
  • Task files: `docs/tasks/active/20260411-docs-table-merge-ux-todo.md`
    and `20260411-docs-table-merge-ux-lessons.md`. Lessons captures the
    pagination follow-ups, the ?? vs || pitfall, and the dist-import
    boundary that burned the first manual smoke test.

Test plan

  • `pnpm verify:fast` — 490/490 docs tests passing, lint + architecture
    checks clean.
  • Manual smoke in dev (multi-page 3×3 table with first column 3-row merge,
    content `1\n2\n3`):
    • Drag cells → "Merge cells" enabled, merges correctly.
    • Right-click a merged cell → "Unmerge cells", restores cells.
    • Single non-merged cell right-click → "Merge cells" disabled.
    • Drag range touching an existing merge → highlight auto-expands
      to cover the full merge before the menu opens.
    • 1+2 row split across pages → page 1 shows "1", page 2 shows
      "2\n3" as a complete cell with top padding.
    • 2+1 row split → page 1 shows "1\n2", page 2 shows "3".
    • Click on page 2 cell → ruler updates to page 2.
    • Click after "2" in page 2's merged cell → cursor lands after
      "2", not after "3".
    • Hover empty space below row 0 on page 1 → no resize cursor.
    • Hover inside a merged cell's centre → no resize cursor.
    • Context menu at the bottom of the viewport → clamped so
      "Delete table" and its padding are visible.
    • Cell-internal Cmd+A + cell-range selection across the page
      break → highlights only the text, no empty-space bleed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Merge/Unmerge combined action added to the table context menu.
  • Improvements

    • More accurate selection, cursor, rendering and hover previews for merged cells across paginated pages; selection highlights and copy behavior better reflect merged boundaries.
  • Documentation

    • Added design/spec, UX lessons, and a multi-step implementation plan for table merge behavior.
  • Tests

    • New tests covering merge behavior and cell-range expansion.

hackerwins and others added 25 commits April 11, 2026 21:38
Pure helpers in selection.ts: findMergeTopLeft walks back from a covered
cell to its merge top-left, and expandCellRangeForMerges runs a fixed-point
loop to grow a bounding rectangle until it fully contains every merged cell
it touches. These will back the drag-time auto-expand of cell-range
selections so users see the exact area that will be merged.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
normalizeCellRange now optionally takes TableData and runs the fixed-point
expander before returning. normalizeRange looks the table up from layout
and threads it through, so peer cursor rendering and programmatic ranges
get the expanded rectangle even without a write-time normalization pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The drag-selection and Shift+Arrow cross-cell paths now run the cell range
through expandCellRangeForMerges before storing it on the selection. The
highlight is built from this stored value, so users see the exact area
that will be merged before they open the context menu. applyTableCellStyle,
which reads tableCellRange directly without normalization, also gets the
expanded range for free.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Returns 'none' | 'canMerge' | 'canUnmerge' plus the data the menu needs to
act. Pure function over Doc, parent map, cursor, and selection range — no
DOM, no editor reference, fully unit-testable. Range presence wins over
canUnmerge so a user inside an existing merged cell can still grow the
merge by selecting a wider range.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Thin wrapper around computeTableMergeContext that the frontend context menu
calls when it opens, so the merge/unmerge slot can render the right label,
icon, and enabled state without duplicating selection logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The auto-expansion path will routinely produce ranges that contain a
pre-existing merged cell, so we lock in the current Doc.mergeCells
behavior: outer top-left absorbs inner span and inner content, every
other cell ends up covered.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The Cell section of the table context menu now exposes one slot whose
label, icon, and enabled state follow editor.getTableMergeContext().
A single non-merged cell shows "Merge cells" disabled (discoverability),
a 2+ cell range shows it enabled, and a merged cell shows "Unmerge cells".
Replaces the previous Split-only entry, which was unreachable for users
who never knew the merge API existed.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Design additions to docs/design/docs/docs-tables.md cover cell range
normalization (fixed-point bounding-rect expansion over merged cells)
and the hybrid Merge/Unmerge context-menu slot. The paired task files
under docs/tasks/active/ document the bite-sized TDD plan used to
implement them and capture lessons about the docs-package dist import
boundary and the cursor-on-covered-cell pitfall.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
buildCellRangeRects was drawing each non-covered cell in the range using
the anchor column's pixel width and the anchor row's height, so a merge
top-left with colSpan/rowSpan > 1 was highlighted only over its original
single-cell footprint. The new Merge UX exposes this constantly because
selections are now auto-expanded to include any merged cell they touch.

Use LayoutTableCell.width (which already sums spanned columns) and sum
rowHeights over the source cell's rowSpan for the height.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
detectTableBorder previously considered every column/row boundary a valid
resize handle regardless of whether a merged cell swallowed that segment
at the cursor's row/column. Hovering the middle of a 1×3 merged cell
showed col-resize, which looked broken.

Locate the hover row/col, walk to each neighboring cell's merge top-left
via findMergeTopLeft, and skip the border when both sides share a merge.
Call sites in text-editor.ts now thread tableData through so the detector
can run the merge check.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Right-clicking near the bottom or right edge of the viewport previously
produced a menu that overflowed and was partially clipped. Measure the
rendered menu in a useLayoutEffect, shift it up/left if it overflows,
and re-run the check when showColors toggles (it grows the menu).

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The container has an animate-in fade-in-0 zoom-in-95 class. useLayoutEffect
fires while the menu is still scaled at 95%, so getBoundingClientRect
reports a height smaller than the final laid-out size. Clamping on that
scaled height leaves the bottom clipped after the animation completes.
offsetWidth/offsetHeight report unscaled layout dimensions and fix it.
Also bump the viewport padding from 4px to 8px for a little breathing room.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
resolveTableCellClick was reading tl.rowYOffsets directly, which are in
the table's own contiguous coordinate system and do not account for the
gap between pages when a table splits across them. Clicking the first
row on page 2 of a 1+2 split 3x3 table produced a localY far larger than
any rowYOffset, so the function fell through to its last-row fallback
and picked the wrong row.

Iterate paginatedLayout.pages[].lines for the table's blockIndex,
reconstruct each row's absolute Y as pageY + pl.y, and pick the row
whose band contains mouseY. The caller now passes canvas-logical mouseX
and mouseY directly and the old localX/localY threading is gone from
both the click and drag-selection call sites.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
buildCellRangeRects previously derived each row's Y from
(tablePageY - rowYOffsets[0]) + rowYOffsets[r], which assumes the table
renders in a single contiguous band. When the table spans pages, rows
after the break live on a new page whose top is not related to the
first row's Y, so the highlight ended up stacked near the top of page 1
instead of on the actual rows.

Build a row → absolute Y map by walking paginatedLayout.pages[].lines
for the table and use that per-row Y in the rect output.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When a merged cell's rowSpan crosses a page break, both pages re-draw
the full cell content at different virtual origins. The canvas clip
masks whatever does not fit in each page, but lines near the break
land inside both pages' content areas and appear twice.

Fix by assigning each line of a merged cell to a single owner row based
on its center Y in table-logical coordinates, then skipping the line
when the owner row is not inside the current page's physical row
range. renderTableContent now takes a pageStartRow parameter so the
caller can distinguish "swept back for a merged cell" (renderStartRow)
from "first row actually laid out on this page" (pageStartRow). The
same filter gates list markers so they follow their block's first line.
Non-merged cells on swept-back rows are also skipped outright since
their content was already drawn on an earlier page.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The previous pass deduped merged-cell text across a page break, but the
cell outline was still drawn as one full rectangle per page with the
canvas clip trimming either the bottom or the top. That left the page 1
slice with no visible bottom border and the page 2 slice with no visible
top border, so the split looked like one half-open shape on each side.

Recompute the cell's visible row range (Math.max(r, pageStart) to
Math.min(r + rowSpan, rowEnd)) and size the background fill and the
four border lines to that clipped range. Each page now renders the
merged cell slice as a fully closed four-sided cell that matches the
adjacent rows on the same page.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Two follow-ups to the merged-cell page-break rendering:

Cursor placement: resolvePositionPixel anchored the cursor to the cell's
top-left row's PageLine and added the line's y within the cell. For a
merged cell split across pages, lines owned by later rows landed in the
empty space on page 1 instead of following their row to page 2. Find
the line's owner row in table-logical coordinates, look up that row's
PageLine, and derive the cursor Y from it so Arrow Down across a page
break lands on the actual line on the next page.

Border hit-testing: resolveTableFromMouse mapped mouseY through a
single tableOriginY taken from row 0, which made localY meaningless
for pages past the first. Walk each page's line range for the table,
only resolve inside the band the cursor is actually over, and return
pageFirstRow/pageLastRow so detectTableBorder can ignore row borders
belonging to rows on a different page. Hovering empty space below row
0 on page 1 no longer flashes a row-resize cursor.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
renderTableContent now shifts merged-cell text on a continuation page so
the first visible line sits at (visible-row top + padding). The cursor
computation in resolvePositionPixel needed the same adjustment or the
caret would hover above where the text actually draws.

Extract findMergedCellOwnerRow and computeMergedCellLineShift as shared
helpers in table-renderer.ts and use them from peer-cursor.ts. The
cursor now finds ownerRow, looks up the cell's first row on that page,
and adds the same shift the renderer uses so the caret lands on the
rendered text line.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The previous heuristic assigned each line to a row based on where its
center fell in the table-logical Y axis. When row heights were larger
than the line height (e.g. three 20-px lines merged into three 28-px
rows), two lines packed into the same row and the third line overlapped
the row-2 boundary. Splitting at a page break then showed "1, 2, 3" on
page 1 with the tail line straddling the break, instead of "3" moving
cleanly to page 2.

Replace findMergedCellOwnerRow + computeMergedCellLineShift with a
single computeMergedCellLineLayouts helper that walks lines in order
and advances to the next spanned row as soon as one fills up. Each
returned entry is { ownerRow, runLineY } in table-logical coordinates,
so the renderer and resolvePositionPixel can drive their Y from the
same source and stay aligned on continuation pages.

Both call sites now look up lineLayouts[targetIdx] instead of running
separate owner/shift math, and for rowSpan === 1 the result matches the
old "cellY + padding + line.y" formula so non-merged cells render
unchanged.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
buildCellRangeRects drew one rect per selected cell anchored at the
cell's top row with the full row-span height summed from rowHeights.
For a merged cell whose span crosses a page break that rect extended
down past row 0's actual bottom on page 1, painting the highlight into
the empty area below the table on page 1 and leaving no highlight on
page 2's slice.

Walk the cell's spanned rows via the rowYMap, detect a page break when
the next row's absolute Y is not equal to the current segment's
(top + accumulated height), flush the current rect on the break, and
start a new segment anchored at the new row's page position. Non-merged
cells (rowSpan === 1) produce a single segment so the existing output
is unchanged.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The multi-line branch of buildRects advanced midY by the first line's
height to paint "middle" rows, which assumes the cell's text flows on a
single continuous Y axis. Selecting all content in a merged cell that
spans a page break then painted the middle-line strip into the empty
space below row 0 on page 1, because lines 2 and 3 actually live on
page 2 at completely different absolute Y values.

Find the cell's start/end line indices, run the same
computeMergedCellLineLayouts helper the renderer uses, and emit one
rect per line in [startLineIdx, endLineIdx]. Each rect's Y is looked up
from the owner row's PageLine, so the highlight lands on whichever page
actually renders that line. Width is cell-clamped in the middle and
start/end-clamped on the edge lines.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
resolveOffsetInCellAtXY computed a single cellPageY from the cell's
top-left row's PageLine, then matched the click against cell.lines[li].y
on a continuous Y axis. For a merged cell split across pages, line 1 and
line 2 live on a later page at different absolute Y values, so a click
next to "2" on page 2 fell through the loop and returned the last line's
offset ("3" in the 1\n2\n3 example).

Replace the single-origin math with computeMergedCellLineLayouts: build
each line's absolute rendered Y by looking up its owner row's PageLine
and adding the row-local delta, then pick the first line whose
(lineAbsY + line.height) passes the click. A row → PageLine cache keeps
the per-click cost bounded.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
renderTableContent set ctx.fillStyle / strokeStyle with
\`style.color ?? Theme.defaultColor\`. The ?? operator only treats null
and undefined as missing, so an inline whose \`color\` is an empty string
left the canvas state unchanged — setting fillStyle to '' silently
keeps whatever was there before, which happened to be the blue local-
selection rect color on any page that drew a selection highlight. Table
text on page 2 (selected merged cell) then rendered in the selection
color instead of the theme's text color while page 1 stayed white
because nothing had set fillStyle to a selection color yet.

Switch to \`style.color || Theme.defaultColor\` to match the non-table
renderRun path (doc-canvas.ts:673). Empty strings now correctly fall
through to the theme default, and fillText/strokeText always produce
the intended color regardless of the stale canvas state left by an
earlier drawing pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Captures the patterns and pitfalls discovered after the initial plan:
the shared line-layout helper, per-page row filter, ?? vs || for
inline colors, center-of-line ownership failure mode, cell-internal
multi-line rect path, and per-page resolveTableFromMouse.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
findPageForPosition walks top-level document blocks, so a cursor whose
blockId belongs to a table cell resolved to undefined and the ruler
fell back to page 0. Clicking a cell on page 2 then kept the ruler
indicator on page 1.

Use the already-computed cursorPixel.y instead: walk paginatedLayout
pages and pick the one whose Y range contains the cursor. That handles
table cells, merged-cell continuations, header/footer contexts, and
non-table blocks with the same code path. Also resolve the cursor's
block style through the parent-map when the cursor is inside a cell so
the ruler's indent markers reflect the table block's own style.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@coderabbitai

coderabbitai Bot commented Apr 11, 2026

Copy link
Copy Markdown

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Adds merge-aware table selection, per-line merged-cell layout and page-aware rendering, a pure merge-context computation API exposed via EditorAPI, a hybrid Merge/Unmerge context menu, and tests/doc plans for fixed-point range normalization and merge UX behavior.

Changes

Cohort / File(s) Summary
Design & Task Docs
docs/design/docs/docs-tables.md, docs/tasks/active/20260411-docs-table-merge-ux-lessons.md, docs/tasks/active/20260411-docs-table-merge-ux-todo.md
Specs and implementation plan: cell-range normalization invariant, expandCellRangeForMerges, UX rules (merge precedence), and a stepwise todo for helpers, editor API, and context-menu updates.
Public API Export
packages/docs/src/index.ts
Re-exported TableMergeContext type from view layer to package public surface.
Merge Context Helper
packages/docs/src/view/table-merge-context.ts
Added TableMergeContext union and pure computeTableMergeContext(...) to determine none/canMerge/canUnmerge based on cursor, selection, and table data.
Selection & Range Helpers
packages/docs/src/view/selection.ts
Added findMergeTopLeft and expandCellRangeForMerges; normalizeCellRange optionally uses TableData for fixed-point expansion; updated rect builders to use per-line merged layouts and page Y mapping.
Merged-Cell Line Layout
packages/docs/src/view/table-renderer.ts
Added MergedCellLineLayout and computeMergedCellLineLayouts; page-clipped background/content rendering with optional pageStartRow; per-line rendering for merged cells and anchored list markers.
Render Pipeline Propagation
packages/docs/src/view/doc-canvas.ts
Extended TableRenderRange and computeTableRangeForPageLine to compute and pass pageStartRow through render/background/content.
Cursor/Peer Cursor
packages/docs/src/view/peer-cursor.ts
Refactored resolvePositionPixel to use per-line merged layouts for cursor Y; added fallback for end-of-content placement.
Editor API & Ruler Logic
packages/docs/src/view/editor.ts
Added EditorAPI.getTableMergeContext(): TableMergeContext; improved cursor page derivation via blockParentMap and pixel/Y fallbacks.
Mouse/Hit-testing & Text Editor
packages/docs/src/view/text-editor.ts
Page-aware table hit-testing, resolveTableFromMouse returns page row bounds, resolveTableCellClick iterates page lines, expand drag selections with expandCellRangeForMerges, and merged-cell line selection via computeMergedCellLineLayouts.
Resize / Border Detection
packages/docs/src/view/table-resize.ts
detectTableBorder accepts tableData and page row bounds; added cellsShareMerge to suppress border hits inside a merged region.
Frontend Context Menu
packages/frontend/src/app/docs/docs-table-context-menu.tsx
Added mergeCtx state via editor.getTableMergeContext() on right-click; replaced split-only entry with hybrid Merge/Unmerge slot; viewport clipping via useLayoutEffect.
Tests
packages/docs/test/view/cell-range-expand.test.ts, packages/docs/test/view/table-merge-context.test.ts, packages/docs/test/model/table.test.ts
Unit/integration tests for findMergeTopLeft, expandCellRangeForMerges, computeTableMergeContext, and regression test for mergeCells over existing merged regions.

Sequence Diagram

sequenceDiagram
    actor User
    participant ContextMenu as Docs Table<br/>Context Menu
    participant Editor as Editor API
    participant Doc as Doc Model
    participant BlockParent as BlockParentMap
    participant TableData as Table<br/>Layout Data
    participant StateCompute as Merge Context<br/>Computer

    User->>ContextMenu: Right-click on table
    ContextMenu->>Editor: getTableMergeContext()
    Editor->>BlockParent: resolve cursor -> cell/block
    Editor->>Doc: fetch block (table) & selection.range
    Editor->>TableData: read TableData (layout) if available
    Editor->>StateCompute: computeTableMergeContext(doc, blockParentMap, cursorPos, selectionRange)
    
    alt Cursor in table & selection spans ≥2 cells
        StateCompute-->>Editor: { state: 'canMerge', range }
    else Cursor in merged cell & no selection
        StateCompute-->>Editor: { state: 'canUnmerge', cell }
    else
        StateCompute-->>Editor: { state: 'none' }
    end

    Editor-->>ContextMenu: TableMergeContext
    ContextMenu->>ContextMenu: render Merge/Unmerge slot (label, enabled)
    ContextMenu->>User: display menu
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐰 With twitching whiskers, I hop and mend,

Cells that spanned now gently blend.
Lines find owners, pages smile wide,
Merge or split — I guide with pride.
A tiny rabbit, in code I hide.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately and clearly describes the main change: adding a merge/unmerge context menu and fixing multi-page merged-cell rendering, which aligns with the substantial changes across selection expansion, rendering, and context menu.

✏️ 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 feature/docs-table-merge-ux

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 114.8s

Lane Status Duration
sheets:build ✅ pass 12.7s
docs:build ✅ pass 7.5s
verify:fast ✅ pass 57.9s
frontend:build ✅ pass 14.9s
verify:frontend:chunks ✅ pass 0.3s
backend:build ✅ pass 4.7s
cli:build ✅ pass 1.8s
verify:entropy ✅ pass 15.0s

Verification: verify:integration

Result: ✅ PASS

@codecov

codecov Bot commented Apr 11, 2026

Copy link
Copy Markdown

@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: 6

🤖 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/design/docs/docs-tables.md`:
- Around line 323-344: Update the paragraph to accurately describe the current
behavior: state that Selection.setRange() stores the raw tableCellRange and that
normalizeRange() only expands the range when a consumer reads it (not
proactively persisting the expanded bounding rect), so programmatic writers do
not get an automatically persisted expanded selection; reference the
functions/table fields by name (selection.setRange, normalizeRange,
selection.range.tableCellRange, tableCellRange) and clarify that consumers
trigger normalization (e.g., rendering, copy, merge) rather than the value being
mutated on set.

In `@docs/tasks/active/20260411-docs-table-merge-ux-lessons.md`:
- Around line 79-98: Remove the duplicate lesson paragraphs so each unique
lesson appears only once: keep a single instance of the note about
`@wafflebase/docs` being imported from dist (mentioning EditorAPI and
editor.getTableMergeContext) and a single instance of the table-merge cursor
note (mentioning expandCellRangeForMerges, TableCellRange.end,
resolveTableCellClick and currentCA/tableCellRange.end), deleting the redundant
copy so the file contains one authoritative entry for each topic.

In `@docs/tasks/active/20260411-docs-table-merge-ux-todo.md`:
- Around line 501-503: The note incorrectly claims pos (cursor target) follows
the expanded tableCellRange.end; in reality the implemented behavior keeps pos
on the raw hit-test cell (currentCA) after merge expansion, which causes the
cursor to land on a covered cell — update the documentation note to describe the
actual implemented behavior (that pos remains tied to currentCA/hit-test cell
rather than tableCellRange.end) and mention the potential regression so future
edits don't assume the note describes a fix; reference pos, currentCA, and
tableCellRange.end so reviewers can find and confirm the described behavior.

In `@packages/docs/src/view/selection.ts`:
- Around line 305-320: lineIdxForOffset currently treats an offset exactly at a
visual-line boundary as belonging to the previous line, which breaks
wrapped-cell selection; change lineIdxForOffset to accept a lineAffinity (or a
boolean/enum bias) and use it when comparing remaining to lineChars (i.e., treat
equality as belonging to the next line when affinity is forward, and to the
previous line when affinity is backward), then thread that affinity through the
selection callers that compute startLineIdx and endLineIdx (replace current
calls that pass only start.offset/end.offset with calls that pass the
corresponding lineAffinity for start and end) so boundary offsets resolve
consistently with startPixel/endPixel affinities.

In `@packages/docs/src/view/table-merge-context.ts`:
- Around line 37-62: The selectionRange.tableCellRange is used without
normalization; update the logic around tableBlockId / cr to first retrieve
tableData (via doc.getBlock(tableBlockId).tableData), then normalize and clamp
cr: compute startRow = Math.min(cr.start.rowIndex, cr.end.rowIndex), endRow =
Math.max(...), startCol = Math.min(...), endCol = Math.max(...), and clamp
startRow/endRow to [0, tableData.rows.length-1] and startCol/endCol to [0,
tableData.rows[startRow].cells.length-1] (or use max columns across rows) before
computing rowSpan/colSpan and returning the range object so the canMerge branch
uses an ordered, bounded range.

In `@packages/docs/src/view/table-renderer.ts`:
- Around line 30-60: computeMergedCellLineLayouts currently always starts yInRow
at cellPadding (top-aligned); change the function to accept the available
vertical space or a starting offset and the cell's verticalAlign so merged cells
can be top/ middle/ bottom aligned. Specifically, update
computeMergedCellLineLayouts signature to include either (verticalAlign:
'top'|'middle'|'bottom', availableHeight: number) or a precomputed startYOffset;
inside the function compute totalLinesHeight = sum(line.height), compute slack =
availableHeight - totalLinesHeight - 2*cellPadding, then set yInRow =
cellPadding + Math.max(0, alignmentFactor)*slack where alignmentFactor is 0 for
top, 0.5 for middle, 1 for bottom; keep the existing row-wrap logic (currentRow
increment when yInRow + line.height exceeds rowHeights[currentRow] -
cellPadding) and proceed as before so hit-testing/rendering reflect the chosen
vertical alignment.
🪄 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: f1ab493f-ce7f-4dbd-9857-3b5fdc6aec12

📥 Commits

Reviewing files that changed from the base of the PR and between a7af848 and a2945e1.

📒 Files selected for processing (16)
  • docs/design/docs/docs-tables.md
  • docs/tasks/active/20260411-docs-table-merge-ux-lessons.md
  • docs/tasks/active/20260411-docs-table-merge-ux-todo.md
  • packages/docs/src/index.ts
  • packages/docs/src/view/doc-canvas.ts
  • packages/docs/src/view/editor.ts
  • packages/docs/src/view/peer-cursor.ts
  • packages/docs/src/view/selection.ts
  • packages/docs/src/view/table-merge-context.ts
  • packages/docs/src/view/table-renderer.ts
  • packages/docs/src/view/table-resize.ts
  • packages/docs/src/view/text-editor.ts
  • packages/docs/test/model/table.test.ts
  • packages/docs/test/view/cell-range-expand.test.ts
  • packages/docs/test/view/table-merge-context.test.ts
  • packages/frontend/src/app/docs/docs-table-context-menu.tsx

Comment thread docs/design/docs/docs-tables.md Outdated
Comment thread docs/tasks/active/20260411-docs-table-merge-ux-lessons.md
Comment thread docs/tasks/active/20260411-docs-table-merge-ux-todo.md Outdated
Comment thread packages/docs/src/view/selection.ts Outdated
Comment thread packages/docs/src/view/table-merge-context.ts
Comment on lines +30 to +60
export function computeMergedCellLineLayouts(
cellLines: LayoutTableCell['lines'],
cellTopRow: number,
rowSpan: number,
cellPadding: number,
rowYOffsets: number[],
rowHeights: number[],
): MergedCellLineLayout[] {
const numRows = rowYOffsets.length;
const spanEnd = Math.min(cellTopRow + rowSpan, numRows);
const result: MergedCellLineLayout[] = [];
let currentRow = cellTopRow;
let yInRow = cellPadding;

for (const line of cellLines) {
if (
rowSpan > 1 &&
currentRow + 1 < spanEnd &&
yInRow + line.height > rowHeights[currentRow] - cellPadding
) {
currentRow++;
yInRow = cellPadding;
}
result.push({
ownerRow: currentRow,
runLineY: rowYOffsets[currentRow] + yInRow,
});
yInRow += line.height;
}
return result;
}

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

computeMergedCellLineLayouts() hardcodes top alignment for merged cells.

The helper always starts at cellPadding and has no way to receive the aligned starting Y or available slack. Now that rendering, cursor hit-testing, peer cursor placement, and selection rects all share this helper, any merged cell with verticalAlign: 'middle' | 'bottom' will render and interact as top-aligned.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/docs/src/view/table-renderer.ts` around lines 30 - 60,
computeMergedCellLineLayouts currently always starts yInRow at cellPadding
(top-aligned); change the function to accept the available vertical space or a
starting offset and the cell's verticalAlign so merged cells can be top/ middle/
bottom aligned. Specifically, update computeMergedCellLineLayouts signature to
include either (verticalAlign: 'top'|'middle'|'bottom', availableHeight: number)
or a precomputed startYOffset; inside the function compute totalLinesHeight =
sum(line.height), compute slack = availableHeight - totalLinesHeight -
2*cellPadding, then set yInRow = cellPadding + Math.max(0,
alignmentFactor)*slack where alignmentFactor is 0 for top, 0.5 for middle, 1 for
bottom; keep the existing row-wrap logic (currentRow increment when yInRow +
line.height exceeds rowHeights[currentRow] - cellPadding) and proceed as before
so hit-testing/rendering reflect the chosen vertical alignment.

- table-renderer: skip per-row distribution for merged cells with
  verticalAlign != 'top'. The new helper always packed content to the
  top, which regressed single-page middle/bottom-aligned merged cells.
  Top-aligned cells (the common case) still use the per-row layout so
  the pagination fixes in this PR keep working.
- table-merge-context: run expandCellRangeForMerges on the selection's
  tableCellRange before returning canMerge. Drag/Shift+Arrow already
  expand at write time, but programmatic Selection.setRange() callers
  do not, and a partially-overlapping range used to drive the menu
  toward an invalid merge rectangle.
- selection.lineIdxForOffset: accept lineAffinity and treat an offset
  at a visual-line boundary as belonging to the next line under
  'forward' affinity. Matches the start/end affinity that
  resolvePositionPixel already uses, so wrapped cell selections no
  longer emit stray rects on adjacent lines.
- docs-tables.md: clarify that normalization now happens at both write
  time (drag/Shift+Arrow) and read time (normalizeRange), and note
  that Selection.setRange itself does not mutate the stored range.
- lessons.md: de-duplicate the dist-import and covered-cell notes that
  were accidentally pasted twice.
- todo.md: fix the Task 3 note that described the original bad draft
  ('pos follows expanded end') — the implemented code keeps pos tied
  to the raw currentCA to avoid landing on a covered cell.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@hackerwins
hackerwins merged commit fdf30e0 into main Apr 11, 2026
1 check passed
@hackerwins
hackerwins deleted the feature/docs-table-merge-ux branch April 11, 2026 14:36
hackerwins added a commit that referenced this pull request Apr 11, 2026
Merged via #119. Move the todo/lessons pair into
docs/tasks/archive/2026/04/ and regenerate the task index.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@coderabbitai coderabbitai Bot mentioned this pull request May 1, 2026
7 tasks
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