Feat/excel m4 structural - #326
Merged
Merged
Conversation
SamTV12345
enabled auto-merge (squash)
July 2, 2026 18:50
PR Summary by QodoSpreadsheet M4: multi-sheet ops, resize/freeze, tabs, sort & filter
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo
1. Invalid sheet ops deadlock
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+174
to
+195
| // attachResizer adds a drag strip to a header cell edge. Dragging previews | ||
| // the size live via inline style and commits once on mouseup via onResize. | ||
| private attachResizer(th: HTMLTableCellElement, axis: 'col' | 'row', index: number): void { | ||
| if (this.opts.readOnly) return; | ||
| const grip = document.createElement('span'); | ||
| grip.className = axis === 'col' ? 'sheet-resizer-col' : 'sheet-resizer-row'; | ||
| grip.addEventListener('mousedown', (e: MouseEvent) => { | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| const startPos = axis === 'col' ? e.clientX : e.clientY; | ||
| const startSize = axis === 'col' ? th.offsetWidth : th.offsetHeight; | ||
| const minSize = axis === 'col' ? 40 : 18; | ||
| let size = startSize; | ||
| const onMove = (me: MouseEvent) => { | ||
| size = Math.max(minSize, startSize + ((axis === 'col' ? me.clientX : me.clientY) - startPos)); | ||
| this.applyDim(axis, index, size); | ||
| }; | ||
| const onUp = () => { | ||
| document.removeEventListener('mousemove', onMove); | ||
| document.removeEventListener('mouseup', onUp); | ||
| this.opts.onResize?.(axis, index, size); | ||
| }; |
There was a problem hiding this comment.
1. Invalid sheet ops deadlock 🐞 Bug ☼ Reliability
The UI can emit invalid setDimension sizes (>4096) and renameSheet names (>128), which the server rejects in Op.Validate() but handleSheetOp returns without sending ACCEPT, leaving SheetCollabClient stuck in committing=true until reload.
Agent Prompt
### Issue description
The client can generate structurally invalid sheet ops (notably `setDimension` sizes above the server’s max and `renameSheet` names exceeding the server’s max). When the server rejects these ops, it does not send an ACCEPT/negative-ACK response, so the client’s collab state machine can remain stuck in a committing state.
### Issue Context
- `setDimension` is generated from drag-resize and has no upper bound.
- `renameSheet` accepts arbitrary prompt input without enforcing the server’s length constraint.
- Server-side validation rejects these inputs.
- WS handler drops the op on error and does not send ACCEPT.
- Client clears `committing` only on ACCEPT.
### Fix Focus Areas
- ui/src/js/sheet/sheetView.ts[174-195]
- ui/src/js/sheet/sheetTabs.ts[45-48]
- lib/sheet/op.go[116-145]
- lib/ws/SheetHandler.go[82-106]
- ui/src/js/sheet/sheetCollabClient.ts[51-69]
### Suggested fix
1. Clamp resize sizes client-side before sending `setDimension`:
- `size = Math.min(size, 4096)` (and possibly also clamp to >=1, though UI already has minSize).
2. Enforce `renameSheet` constraints client-side before sending:
- Reject/trim names >128 characters (and optionally show an inline error).
3. (Optional hardening) Add a server-side negative ACK path for rejected ops so the client can clear `committing` and surface an error instead of silently stalling.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+51
to
+57
| ss := SheetSnapshot{Id: s.Id, Name: s.Name, Cells: cells, FrozenRows: s.FrozenRows, FrozenCols: s.FrozenCols} | ||
| if len(s.ColWidths) > 0 { | ||
| ss.ColWidths = s.ColWidths | ||
| } | ||
| if len(s.RowHeights) > 0 { | ||
| ss.RowHeights = s.RowHeights | ||
| } |
There was a problem hiding this comment.
2. Snapshot shares mutable maps 🐞 Bug ☼ Reliability
Workbook.Snapshot() stores ColWidths/RowHeights map references directly in the snapshot, so the snapshot can be mutated after sheetdoc.Manager.Snapshot() unlocks but before sendSheetVars marshals, creating an inconsistent snapshot relative to the returned head revision.
Agent Prompt
### Issue description
`Workbook.Snapshot()` assigns `s.ColWidths`/`s.RowHeights` directly into the returned snapshot. Because these are reference types, callers can observe later mutations through the snapshot, and there is a real window where the snapshot can become inconsistent with the `head` revision returned by `sheetdoc.Manager.Snapshot()`.
### Issue Context
`sheetdoc.Manager.Snapshot()` returns the snapshot and head under a mutex, but `sendSheetVars()` marshals the snapshot after the mutex is released. If a concurrent op mutates a sheet’s dimension maps between return and marshal, JSON encoding may see a newer map state while still advertising the older `head`.
### Fix Focus Areas
- lib/sheet/snapshot.go[38-60]
- lib/sheetdoc/manager.go[138-148]
- lib/ws/SheetHandler.go[233-243]
### Suggested fix
- In `Workbook.Snapshot()`, deep-copy the new maps before assigning:
- `ss.ColWidths = maps.Clone(s.ColWidths)`
- `ss.RowHeights = maps.Clone(s.RowHeights)`
- (Optional) Consider similarly deep-copying any other mutable reference fields that can be mutated concurrently with snapshot marshalling, or ensure the snapshot is fully materialized into immutable data before releasing locks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+135
to
+145
| // CommitRateLimiting only covers commits (USER_CHANGES / SHEET_OP), as | ||
| // in etherpad-lite. Ephemeral traffic like SHEET_PRESENCE arrives per | ||
| // keystroke and must not burn the commit budget — a drained budget | ||
| // silently drops the commit itself and edits are lost. | ||
| if strings.Contains(decodedMessage, "USER_CHANGES") || strings.Contains(decodedMessage, "SHEET_OP") { | ||
| retrievedSettings.CommitRateLimiting.LoadTest = retrievedSettings.LoadTest | ||
| if err := ratelimiter.CheckRateLimit(ratelimiter.IPAddress(c.ClientIP), retrievedSettings.CommitRateLimiting); err != nil { | ||
| logger.Warn("Rate limit exceeded:", err.Error()) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
3. Substring rate-limit misfires 🐞 Bug ☼ Reliability
WS commit rate limiting is now gated by strings.Contains() over the full JSON message, which can incorrectly apply commit rate limits to non-commit messages whose payload happens to include USER_CHANGES/SHEET_OP as text.
Agent Prompt
### Issue description
The rate-limit gating uses substring search against the full JSON payload. This can misclassify unrelated message types (e.g. chat messages) if user-controlled text includes the trigger substrings, unexpectedly rate limiting non-commit traffic.
### Issue Context
Some WS message types include free-form text fields. Substring searching the entire serialized JSON is not equivalent to checking the actual message `type` discriminator.
### Fix Focus Areas
- lib/ws/client.go[135-145]
- lib/models/ws/ChatMessage.go[23-37]
### Suggested fix
- Replace substring matching with a lightweight parse of the envelope to extract the real message type (e.g. `data.data.type`), then apply `CommitRateLimiting` only when that type equals `USER_CHANGES` or `SHEET_OP`.
- Keep parsing minimal to avoid extra overhead (e.g. unmarshal into a small struct containing only the relevant nested `type` field).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
1. Rate limit payload substring match
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+135
to
+145
| // CommitRateLimiting only covers commits (USER_CHANGES / SHEET_OP), as | ||
| // in etherpad-lite. Ephemeral traffic like SHEET_PRESENCE arrives per | ||
| // keystroke and must not burn the commit budget — a drained budget | ||
| // silently drops the commit itself and edits are lost. | ||
| if strings.Contains(decodedMessage, "USER_CHANGES") || strings.Contains(decodedMessage, "SHEET_OP") { | ||
| retrievedSettings.CommitRateLimiting.LoadTest = retrievedSettings.LoadTest | ||
| if err := ratelimiter.CheckRateLimit(ratelimiter.IPAddress(c.ClientIP), retrievedSettings.CommitRateLimiting); err != nil { | ||
| logger.Warn("Rate limit exceeded:", err.Error()) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Rate limit payload substring match 🐞 Bug ☼ Reliability
Client.readPump() applies commit rate limiting using strings.Contains() on the entire JSON message, so non-commit frames (notably SHEET_PRESENCE) can be incorrectly rate-limited if user-controlled fields contain "USER_CHANGES" or "SHEET_OP". This can drain the commit budget via presence-keystroke traffic and cause actual commits to be skipped (continue), resulting in lost edits.
Agent Prompt
### Issue description
`lib/ws/client.go` currently decides whether to apply `CommitRateLimiting` by doing substring checks against the *entire* websocket JSON payload. Because `SHEET_PRESENCE` includes user-controlled `raw` text, any presence message that happens to contain the substrings `"USER_CHANGES"` or `"SHEET_OP"` will be treated as a commit and can be dropped, which can also exhaust the commit budget and lead to dropped commits.
### Issue Context
- Presence frames contain user input (`raw`) and are sent frequently (per keystroke).
- The rate limit check happens before dispatch/unmarshal and uses `continue` on failure.
### Fix Focus Areas
- lib/ws/client.go[135-145]
### Suggested fix
1. Unmarshal a minimal envelope struct (or otherwise reliably extract the message's logical type) and apply commit rate limiting only when the parsed type is exactly `USER_CHANGES` or `SHEET_OP`.
2. Avoid scanning arbitrary payload strings for routing/limiting decisions; if you keep substring-based routing for legacy reasons, at least make the *rate limiting* decision based on parsed `data.data.type`.
3. Add a regression test: a `SHEET_PRESENCE` frame whose `raw` includes `"SHEET_OP"` must not consume the commit budget / must not be dropped by commit rate limiting.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
addSheet/renameSheet/deleteSheet/moveSheet manage the workbook sheet list (duplicate add and last-sheet delete converge as no-ops). setDimension stores sparse ColWidths/RowHeights per sheet; setFreeze stores FrozenRows/FrozenCols (0/1). Structural row/col ops shift the dimension maps; Transform shifts setDimension indices on the same axis. Ops on a deleted sheet are silent no-ops so late ops cannot poison the ordered-log replay. Snapshot round-trips the new metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
op.ts wire types, workbookState.applyOp and transform port the Go semantics: duplicate add / last-sheet delete / deleted-sheet ops converge as no-ops, dims shift under structural ops, setDimension indices transform on the matching axis. Snapshot loads the metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DomSheetView grows to 200x52 (ponytail: DOM-per-cell; virtualization is the upgrade path), renders sparse col/row pixel overrides, adds header drag-resize grips committing setDimension, and sticky freeze panes via border-separate + position:sticky driven by CSS vars. Bottom tabs bar (add/rename via prompt, delete via context menu, drag reorder) drives the sheet-list ops. Toolbar gains A-Z/Z-A sort (batch setCell with fill-style formula ref shifts), freeze toggles, and a client-local filter dropdown over the focused column (row hiding, no ops). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Also: the filter dropdown repopulates on focus as well as mousedown so keyboard users (and synthetic selection) get fresh values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Snapshot(): clone ColWidths/RowHeights instead of aliasing live maps (snapshot is consumed after the doc lock is released; races with Apply) - resize: only commit setDimension when the size actually changed - resize: row min matches the 22px CSS floor (table cells treat height as min-height, sub-22px rows never rendered) - workbookState.applyOp(setDimension): mirror server validation (axis col/row, integer size 1..4096) instead of storing 0/misrouted axes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SamTV12345
force-pushed
the
feat/excel-m4-structural
branch
from
July 2, 2026 19:33
09ff725 to
1c3d824
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.
No description provided.