feat(sheet): kollaboratives Undo/Redo - #377
Merged
Merged
Conversation
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
SamTV12345
enabled auto-merge (squash)
July 28, 2026 19:30
PR Summary by QodoAdd Collaborative Undo and Redo for Sheets
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Style undo overwrites remote content
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+160
to
+163
| case 'insertRows': | ||
| return [{ ...base(op), type: 'deleteRows', index, count }]; | ||
| case 'insertCols': | ||
| return [{ ...base(op), type: 'deleteCols', index, count }]; |
There was a problem hiding this comment.
1. Insert undo deletes remote edits 🐞 Bug ≡ Correctness
Undoing an inserted row or column unconditionally deletes the entire inserted band. Any cells another collaborator added to that band after the insertion are deleted with it.
Agent Prompt
## Issue description
Undoing `insertRows` or `insertCols` emits an unconditional band deletion, which also deletes cells collaborators subsequently placed in that band.
## Issue Context
Remote non-structural operations do not modify the stored inverse. Implement conflict-aware structural history that either preserves/migrates subsequent remote content or prevents a destructive undo, and add a two-client regression test.
## Fix Focus Areas
- ui/src/js/sheet/undo.ts[160-163]
- ui/src/js/sheet/sheetCollabClient.ts[143-156]
- ui/src/js/sheet/transform.ts[8-23]
- ui/src/js/sheet/undo.test.ts[194-206]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+151
to
+153
| const rebase = (stack: Op[][]): Op[][] => stack.map((entry) => entry.map((o) => transform(o, remoteOp))); | ||
| this.undoStack = rebase(this.undoStack); | ||
| this.redoStack = rebase(this.redoStack); |
There was a problem hiding this comment.
2. Deleted-row undo clobbers cells 🐞 Bug ≡ Correctness
Rebasing a stored cell inverse through a remote row or column deletion clamps its target to the deletion index. Undo then writes the old value into the unrelated cell that shifted into that position.
Agent Prompt
## Issue description
History rebasing uses the normal pending-operation transform, which clamps inverse cell coordinates inside a remotely deleted band and redirects undo onto unrelated shifted cells.
## Issue Context
Introduce history-specific transformation semantics that can remove obsolete point inverses rather than clamping them. Cover both row and column deletion with collaborative regression tests.
## Fix Focus Areas
- ui/src/js/sheet/sheetCollabClient.ts[143-156]
- ui/src/js/sheet/transform.ts[30-69]
- ui/src/js/sheet/undo.test.ts[194-206]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+114
to
+116
| case 'setCell': | ||
| case 'setStyle': | ||
| return [cellRestore(wb, op, sheet, row, col)]; |
There was a problem hiding this comment.
3. Style undo overwrites remote content 🐞 Bug ≡ Correctness
A setStyle operation is inverted as a setCell containing the previous raw value. If a collaborator edits the content afterward, undoing the local formatting restores stale text and erases that edit.
Agent Prompt
## Issue description
Formatting-only operations currently receive a full-cell inverse, allowing style undo to overwrite a collaborator's later content edit.
## Issue Context
Generate a `setStyle` inverse containing only the previous style properties for `setStyle`. Add a two-client test where one client formats a cell, another changes its raw content, and the first client undoes formatting without changing the content.
## Fix Focus Areas
- ui/src/js/sheet/undo.ts[24-35]
- ui/src/js/sheet/undo.ts[113-116]
- ui/src/js/sheet/undo.test.ts[56-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+691
to
+692
| if (mod && !editingNow() && !readOnly) { | ||
| const k = e.key.toLowerCase(); |
There was a problem hiding this comment.
4. Formula undo triggers sheet history 🐞 Bug ≡ Correctness
The global undo/redo shortcut only checks whether the grid is editing, not whether the formula-bar input is focused. Pressing Ctrl/Meta+Z while editing a formula therefore invokes workbook history instead of the input's native text undo.
Agent Prompt
## Issue description
Workbook undo and redo intercept native text-history shortcuts while the user is typing in the formula bar.
## Issue Context
Guard global history shortcuts when the event target is an input, textarea, or editable element, or expose formula-bar editing state. Add browser coverage proving formula text changes locally without changing workbook history.
## Fix Focus Areas
- ui/src/js/sheet/sheetEditor.ts[689-702]
- ui/src/js/sheet/sheetFormulaBar.ts[71-80]
- ui/src/js/sheet/sheetFormulaBar.ts[119-130]
- playwright/specs/sheet_excel_chrome.spec.ts[162-180]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+658
to
+659
| if (which === 'undo') collab.undo(); | ||
| else collab.redo(); |
There was a problem hiding this comment.
5. Undo leaves invalid active sheet 🐞 Bug ≡ Correctness
Undoing an added sheet deletes it without updating activeSheetId. The editor remains pointed at a nonexistent sheet, producing no active tab and an empty grid until another sheet is selected.
Agent Prompt
## Issue description
History replay can delete the currently active sheet without selecting a surviving sheet, leaving editor state inconsistent.
## Issue Context
After undo or redo, verify that `activeSheetId` still exists and select a valid fallback when it does not. Add coverage for adding and activating a sheet, undoing the addition, and verifying that another sheet becomes active.
## Fix Focus Areas
- ui/src/js/sheet/sheetEditor.ts[508-539]
- ui/src/js/sheet/sheetEditor.ts[653-660]
- ui/src/js/sheet/undo.ts[69-80]
- ui/src/js/sheet/sheetTabs.ts[37-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Undo replays inverse ops through the normal collaborative pipeline instead of rolling state back, so it stays safe under concurrency: the inverse is computed against the state before the op, recorded per client, transformed against every remote op that arrives, and then sent like any other edit. Undo therefore only ever reverts your own work, never a collaborator's. invertOp covers the whole op vocabulary: cells and styles (carrying props, not the client-local style id), clearRange, row/column inserts and deletes including the cells, sizes and merges a deletion destroys, merges and the merges they absorbed, dimensions, freeze and the sheet-list ops - deleting a sheet restores its contents. Ops applied within one tick form one history entry, so a paste, a fill or styling a range undoes in a single step without any call-site changes. History is capped at 100 entries; a fresh edit drops the redo branch, a redo keeps it. UI: Undo/Redo buttons in the Home tab that grey out when a stack is empty, plus Ctrl+Z, Ctrl+Y and Ctrl+Shift+Z outside cell editing (inside a cell the browser text undo keeps the keystroke). Known limit: undoing the first-ever resize of a row or column restores the grid default, because the op vocabulary has no way to unset a dimension. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SamTV12345
force-pushed
the
feat/sheet-undo-redo
branch
from
July 28, 2026 19:45
9dc7a64 to
89d8886
Compare
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ansatz
Undo rollt keinen Zustand zurück, sondern spielt inverse Ops durch die normale Kollaborations-Pipeline. Das ist der Punkt, an dem es unter Nebenläufigkeit korrekt bleibt:
applyLocal, bevor sie angewandt wird).Dadurch macht Undo nie die Arbeit eines anderen rückgängig, auch wenn sie zeitlich dazwischen liegt.
invertOp
Deckt das gesamte Op-Vokabular ab:
props, nicht mitstyleId: die Style-Id ist ein clientlokaler Pool-Index, über die Leitung gehen nur Props.Gruppierung
Ops desselben Ticks bilden einen History-Eintrag. Paste, Ausfüllen oder das Formatieren eines Bereichs schreiben je eine Op pro Zelle und werden trotzdem in einem Schritt rückgängig gemacht — ohne Änderung an einer einzigen Aufrufstelle. History bei 100 Einträgen gedeckelt; eine neue Bearbeitung verwirft den Redo-Zweig, ein Redo behält ihn.
UI
Undo/Redo im Home-Tab, ausgegraut wenn der jeweilige Stack leer ist, plus Ctrl+Z, Ctrl+Y und Ctrl+Shift+Z — nur außerhalb der Zellbearbeitung, drinnen gehört der Tastendruck dem Text-Undo des Browsers.
Tests
17 neue Unit-Tests: pro Op-Typ ein Roundtrip gegen einen strukturellen State-Fingerprint (Zellen + aufgelöste Style-Props, Größen, Freeze, Merges), dazu Client-Tests für Gruppierung pro Tick, getrennte Ticks, Redo-Verwerfung, Rebase gegen eine fremde Zeileneinfügung und „Remote-Ops landen nicht in der History". Plus zwei Playwright-Tests (Button- und Tastaturpfad, Mehrzellen-Aktion in einem Schritt). Insgesamt 166 Vitest-Tests grün.
Bekannte Grenze
Das Rückgängigmachen der allerersten Größenänderung einer Zeile/Spalte stellt den Grid-Default (80/22 px) her statt den Eintrag zu entfernen — das Op-Vokabular kennt kein „Dimension zurücksetzen". Optisch identisch, im State nicht bytegleich; im Test explizit festgehalten.
🤖 Generated with Claude Code