Skip to content

feat(sheet): merged cells#352

Open
SamTV12345 wants to merge 2 commits into
mainfrom
feat/sheet-merged-cells
Open

feat(sheet): merged cells#352
SamTV12345 wants to merge 2 commits into
mainfrom
feat/sheet-merged-cells

Conversation

@SamTV12345

Copy link
Copy Markdown
Member

What

Merged cells for the collaborative spreadsheet, end to end:

Model / ops (Go + TS mirror)Sheet.Merges (top-left anchor → span) plus two ops, mergeCells / unmergeCells, carrying the same rectangle as clearRange. Excel semantics: merging over existing merges absorbs them; unmerge drops every intersecting range. Non-anchor cell content is kept (hidden by the view), so unmerge restores it — no data loss.

Structural interplay — insert/delete rows/cols shift merges like cells: an insert inside a merge grows it, a delete shrinks it, merges fully inside a deleted band (or collapsed to 1x1) are dropped. Transform shifts the op rectangle like clearRange; a rebase past a concurrent delete can collapse a mergeCells op to 1x1, which Apply treats as a converging no-op instead of an error.

View — the anchor td gets rowSpan/colSpan, covered tds go display:none; arrow-key navigation snaps onto merge anchors and steps over the merged block. Ribbon Alignment group gains a merge/unmerge toggle (the editor picks the direction: any merge intersecting the selection → unmerge).

xlsx — merges round-trip through import/export via excelize MergeCell/GetMergeCells.

Tests

  • Go: apply (absorb/unmerge/1x1 no-op), structural shifts (grow/shrink/drop), transform incl. the collapse case, snapshot round-trip, xlsx round-trip.
  • TS: workbookState + transform mirrors (vitest, 86 tests green).
  • E2E (sheet_merge.spec.ts, live 3/3): merge renders colspan/rowspan + hides covered cells + unmerge restores content; merge reaches a second client and survives reload; insert-row-above shifts the merge. All 31 sheet specs green.

Out of scope

Selection auto-expansion to full merges on drag (Excel behavior) — navigation snapping covers the common path; merge-aware sort/fill.

🤖 Generated with Claude Code

Two new ops (mergeCells/unmergeCells, carrying the clearRange rectangle)
with Excel absorb semantics: merging over existing merges swallows them;
unmerge drops every intersecting range. Non-anchor cell content is kept
(hidden by the view) so unmerge restores it. Structural row/col ops
shift merges like cells: insert inside grows, delete shrinks, fully
deleted or 1x1-collapsed merges drop. Transform shifts the rectangle
like clearRange; a rebase past a concurrent delete may collapse a merge
to 1x1, which Apply treats as a no-op instead of an error.

Client mirror in workbookState/transform, view renders the anchor with
rowSpan/colSpan and hides covered tds, arrow navigation snaps onto and
steps over merges, ribbon Alignment group gains a merge/unmerge toggle.
xlsx import/export round-trips merges via excelize MergeCell.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

feat(sheet): merged cells end-to-end (ops, model, UI, xlsx)

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add mergeCells/unmergeCells ops and Sheet.Merges with Excel-style absorb/unmerge semantics.
• Make merges shift/transform with structural edits, persist in snapshots, and round-trip via XLSX.
• Render merges in the grid UI with merge-aware keyboard navigation and a toolbar toggle, with
 tests.
Diagram

graph TD
  T["Toolbar"] --> E["Sheet editor"] --> CS["Client state"] --> V["Grid view"]
  E --> S["Go workbook"] --> J[("Snapshot JSON")]
  X["XLSX I/O"] --> EX{{"excelize"}}
  S --> X
  J --> X

  subgraph Legend
    direction LR
    _ui["UI component"] ~~~ _mdl["Model/ops"] ~~~ _snap[("Persisted data")] ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move covered-cell content into the anchor cell
  • ➕ Simplifies rendering (no hidden-but-preserved cells)
  • ➕ Avoids showing multiple values after unmerge
  • ➖ Introduces data loss/overwrite semantics and makes undo/redo harder
  • ➖ Harder to keep consistent across concurrent edits in collaboration
2. Maintain a merge index (cell->anchor map) for O(1) lookup
  • ➕ Faster navigation/render decisions if merges become numerous
  • ➕ Avoids per-keypress linear scans in mergeAt()
  • ➖ More complex to keep correct under insert/delete and absorb/unmerge operations
  • ➖ Higher memory use and more edge cases (index rebuild/invalidations)

Recommendation: Keep the PR’s approach: store merges as anchor->span with Excel-like absorb/unmerge and preserve non-anchor content. It aligns with spreadsheet semantics, avoids data loss, and keeps both Go and TS implementations straightforward. If performance becomes an issue (many merges), consider adding a derived index later rather than complicating the canonical model now.

Files changed (18) +587 / -15

Enhancement (13) +318 / -15
apply.goApply merge/unmerge ops and shift merges on structural edits +22/-0

Apply merge/unmerge ops and shift merges on structural edits

• Adds OpMergeCells/OpUnmergeCells handling with Excel semantics (absorbing overlaps; unmerge removes intersecting merges). Integrates merge shifting into insert/delete rows/cols and treats degenerate 1x1 merges as no-ops.

lib/sheet/apply.go

op.goDefine mergeCells/unmergeCells ops and validate range bounds +7/-2

Define mergeCells/unmergeCells ops and validate range bounds

• Adds new op types for merging and unmerging cells. Extends validation to treat these ops like clearRange (Row/Col..EndRow/EndCol rectangle) and improves error messages to include op type.

lib/sheet/op.go

sheet.goAdd Sheet.Merges model and merge shifting utilities +54/-1

Add Sheet.Merges model and merge shifting utilities

• Introduces Span and Sheet.Merges (anchor->span) to represent merged ranges. Implements intersection checks and shiftMerges to grow/shrink/move/drop merges during row/col inserts/deletes, and ensures merges are cloned.

lib/sheet/sheet.go

snapshot.goPersist merges in workbook snapshots +25/-4

Persist merges in workbook snapshots

• Extends snapshots with MergeSnapshot entries and serializes merges deterministically (sorted by anchor). Restores merge ranges when loading a workbook from a snapshot.

lib/sheet/snapshot.go

transform.goTransform merge/unmerge rectangles like clearRange +7/-2

Transform merge/unmerge rectangles like clearRange

• Introduces a hasRange helper so merge/unmerge ops participate in row/col shifting logic for EndRow/EndCol. Keeps merge ops rebasable across structural edits, including collapse scenarios.

lib/sheet/transform.go

export.goExport merged ranges to XLSX +14/-0

Export merged ranges to XLSX

• Adds export of Sheet.Merges by translating anchors/spans into Excel A1 ranges and calling excelize.MergeCell for each merge.

lib/xlsx/export.go

import.goImport merged ranges from XLSX +11/-0

Import merged ranges from XLSX

• Reads merge ranges via excelize.GetMergeCells and converts them into Sheet.Merges entries. Skips invalid/degenerate merge ranges.

lib/xlsx/import.go

op.tsExtend TS op types with mergeCells/unmergeCells +4/-2

Extend TS op types with mergeCells/unmergeCells

• Adds merge/unmerge to the OpType union and documents that endRow/endCol apply to these range-based ops.

ui/src/js/sheet/op.ts

sheetEditor.tsAdd merge/unmerge toggle action and expose merges to the view +30/-0

Add merge/unmerge toggle action and expose merges to the view

• Implements a mergeToggle callback that chooses unmerge if any merge intersects the selection, otherwise merges the selection (skipping 1x1). Exposes a merges() accessor to the sheet view by converting the client merge map into anchor+span objects.

ui/src/js/sheet/sheetEditor.ts

sheetToolbar.tsAdd toolbar button for merge/unmerge +6/-0

Add toolbar button for merge/unmerge

• Extends toolbar callbacks with mergeToggle, adds an icon, and conditionally renders a “Merge / unmerge cells” button in the Alignment group.

ui/src/js/sheet/sheetToolbar.ts

sheetView.tsRender merged cells and make keyboard navigation merge-aware +56/-2

Render merged cells and make keyboard navigation merge-aware

• Adds support for merged ranges: anchor cells get rowSpan/colSpan and covered cells are hidden. Updates arrow-key navigation to snap focus to merge anchors and jump over merged blocks, and tracks merged tds to reset state each render.

ui/src/js/sheet/sheetView.ts

transform.tsTransform merge/unmerge rectangles like clearRange (TS mirror) +6/-2

Transform merge/unmerge rectangles like clearRange (TS mirror)

• Adds a hasRange helper mirroring Go so endRow/endCol coordinates are shifted for merge/unmerge during structural rebases.

ui/src/js/sheet/transform.ts

workbookState.tsAdd merges to client sheet state and apply/shift semantics +76/-0

Add merges to client sheet state and apply/shift semantics

• Adds merges (anchor->span) to SheetState plus helpers for intersection and shifting on insert/delete rows/cols. Implements applyOp handling for mergeCells/unmergeCells with absorb semantics and 1x1 no-op, and extends snapshot load/clone to include merges.

ui/src/js/sheet/workbookState.ts

Tests (5) +269 / -0
merge_test.goGo unit tests for merge semantics, shifting, transform, and snapshot +133/-0

Go unit tests for merge semantics, shifting, transform, and snapshot

• Introduces tests covering merge absorb behavior, unmerge intersection semantics, 1x1 no-op merges, structural grow/shrink/drop rules, Transform collapse behavior, and snapshot round-trip.

lib/sheet/merge_test.go

import_test.goXLSX round-trip test includes merges +4/-0

XLSX round-trip test includes merges

• Extends the existing XLSX round-trip test to include a merge range and asserts it survives export/import.

lib/xlsx/import_test.go

sheet_merge.spec.tsE2E coverage for rendering, collaboration, reload persistence, and shifts +82/-0

E2E coverage for rendering, collaboration, reload persistence, and shifts

• Adds Playwright tests ensuring merged cells render with rowSpan/colSpan and hide covered cells, unmerge restores hidden content, merges sync to a second client and persist across reload, and row insertion shifts merges.

playwright/specs/sheet_merge.spec.ts

transform.test.tsTS transform tests for mergeCells shifting and collapse +11/-0

TS transform tests for mergeCells shifting and collapse

• Adds a test that mergeCells rectangles shift like clearRange under inserts and can collapse under deletes without breaking semantics.

ui/src/js/sheet/transform.test.ts

workbookState.test.tsTS unit tests for merges apply/shift and snapshot load +39/-0

TS unit tests for merges apply/shift and snapshot load

• Introduces a merges-focused test suite covering span storage, overlap absorption, unmerge intersection removal, 1x1 no-op behavior, structural shifting rules, and snapshot load restoration.

ui/src/js/sheet/workbookState.test.ts

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Merge grows on boundary insert ✓ Resolved 🐞 Bug ≡ Correctness
Description
shiftMerges treats the merge end as an exclusive bound (lo+span) but shifts it using
shiftCoord/shiftIdx2, which apply insert shifts for coord >= index. As a result, inserting a
row/col exactly at the merge’s trailing edge (index == anchor+span) incorrectly increases the
merge span by delta, expanding the merge in both Go and the TS mirror when it should remain
unchanged.
Code

lib/sheet/sheet.go[R95-99]

+		// Shift the anchor and the EXCLUSIVE end: shiftCoord clamps in-band
+		// coords to index, which is exactly right for an exclusive bound.
+		nlo := shiftCoord(lo, index, delta)
+		nspan := shiftCoord(lo+span, index, delta) - nlo
+		if nspan <= 0 {
Evidence
In the Go implementation, shiftMerges computes the updated span by shifting both the anchor (lo)
and the exclusive end (lo+span) via shiftCoord, but shiftCoord shifts inserts when `coord >=
index, so an insert at index == lo+span` incorrectly moves the exclusive end forward and increases
nspan. The TS mirror reproduces the same behavior by computing nspan as `shiftIdx2(lo + span,
...) - nlo, and since shiftIdx2 also shifts inserts on coord >= index`, inserting immediately
after the merge similarly shifts the exclusive end and grows the merge span in the optimistic client
state.

lib/sheet/sheet.go[81-114]
lib/sheet/transform.go[59-77]
ui/src/js/sheet/workbookState.ts[45-74]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`shiftMerges` (Go and TS) computes the new merge span by shifting both the anchor (`lo`) and the **exclusive** end (`lo+span`). For inserts, the shared shifters (`shiftCoord` / `shiftIdx2`) shift when `coord >= index`, so when inserting at `index == lo+span` (immediately after the merge), the exclusive end is incorrectly shifted and the merge span grows even though it should remain unchanged.
## Issue Context
Desired behavior:
- insert **inside** merge → merge grows
- insert **at/after** merge end → merge does **not** grow
The current shifters are correct for shifting *cell coordinates*, but when shifting an *exclusive* upper bound during inserts, the comparison must be strict (`coord > index`) to avoid expanding the merge when the insertion is exactly at the trailing edge.
## Fix Focus Areas
- lib/sheet/sheet.go[81-114]
- lib/sheet/transform.go[59-77]
- ui/src/js/sheet/workbookState.ts[45-74]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. XLSX merge docs stale ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The file-level comments for XLSX Import/Export still claim merges are skipped/not stored, but this
PR now imports and exports merges. This mismatch makes the XLSX behavior easy to
misunderstand/maintain incorrectly.
Code

lib/xlsx/export.go[R103-115]

+		for a, sp := range s.Merges {
+			start, err := excelize.CoordinatesToCellName(a.Col+1, a.Row+1)
+			if err != nil {
+				return nil, err
+			}
+			end, err := excelize.CoordinatesToCellName(a.Col+sp.Cols, a.Row+sp.Rows)
+			if err != nil {
+				return nil, err
+			}
+			if err := f.MergeCell(name, start, end); err != nil {
+				return nil, err
+			}
+		}
Evidence
The comments explicitly state merges are skipped/not stored, but the implementation now reads merge
ranges via GetMergeCells and writes them via MergeCell.

lib/xlsx/import.go[17-21]
lib/xlsx/import.go[109-118]
lib/xlsx/export.go[11-15]
lib/xlsx/export.go[103-115]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`lib/xlsx/import.go` and `lib/xlsx/export.go` comments say merges are skipped / not exported because the model does not store them, but the implementation now round-trips merges.
### Issue Context
The code now calls `GetMergeCells` during import and `MergeCell` during export.
### Fix Focus Areas
- lib/xlsx/import.go[17-21]
- lib/xlsx/import.go[109-118]
- lib/xlsx/export.go[11-15]
- lib/xlsx/export.go[103-115]
### Suggested fix
Update the function header comments to reflect the current behavior (merges are imported/exported).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread lib/sheet/sheet.go Outdated
Comment thread lib/xlsx/export.go
qodo: the exclusive merge end was shifted with the >= insert rule, so
inserting a row/col exactly after the merge expanded it. New shiftEnd
(Go + TS mirror) keeps an exclusive bound in place when the insert index
equals it; delete clamping is unchanged. Also refreshes the stale xlsx
Import/Export doc comments that still claimed merges are skipped.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@SamTV12345
SamTV12345 enabled auto-merge (squash) July 18, 2026 11:05
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