Conversation
jspreadsheet-ce only fires onafterchanges for cell-value edits, not for row/column deletion or row reordering. Those structural changes never marked the sheet dirty or triggered a save, so deleting rows (e.g. via da.live/sheet) was silently lost on reload unless followed by an unrelated cell edit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the sinon fake-timer unit test (which needed a dynamic-import pre-warm hack to avoid a race) with a lighter unit test that follows this repo's existing handleSave convention (real setTimeout wait past the debounce), and add a Playwright e2e test that drives the real jspreadsheet-ce grid end-to-end: delete two rows, reload, and verify the deletion actually persisted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
shsteimer
commented
Jul 6, 2026
onmovecolumn mutates the underlying data model the same way onmoverow does (jspreadsheet-ce's moveColumn splices options.data in place), so it was hitting the same silent-loss-on-reload bug that this branch already fixes for row/column delete and row move. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
shsteimer
changed the title
Mirrors the existing row-delete e2e test for ondeletecolumn - deletes two columns via the header context menu and verifies the deletion survives a reload. Row/column move are left to unit-test-only coverage for now; simulating drag-and-drop in Playwright is more fragile than a context-menu click for comparatively low added value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hannessolo
reviewed
Jul 7, 2026
hannessolo
left a comment
hannessolo
left a comment
Contributor
There was a problem hiding this comment.
I think not saving on sort is fine
hannessolo
previously approved these changes
Jul 7, 2026
CI showed both delete tests timing out on the second right-click: jSuites' document-level contextmenu listener dispatches based on document.activeElement, and after the first menu-item click that's still the (now-closed) link - so a follow-up right-click on a new target never reopens a fresh menu. Select both rows/columns first (click + shift-right-click extends the selection) and delete them in a single context-menu action instead of two separate open/click cycles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 7, 2026
… test columnDrag is off so onmovecolumn can never fire; removing the dead binding and its unit test case. Adds e2e coverage for row-move persistence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Selecting a range via shift-right-click doesn't extend the jspreadsheet-ce selection the way a plain shift-click does, so only the last-clicked row/column was ever actually deleted. Switch to click -> shift-click (extend selection) -> right-click (open context menu without collapsing it). Also replace fixed waitForTimeout delays with waiting for the actual debounced save request to complete, since the delay needed varies with machine/network speed and a fixed timeout was masking this selection bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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.
What changed
ondeleterow,ondeletecolumn,onmoverow, andonmovecolumnare now wired to trigger a save, the same asonafterchangesalready does for cell edits. Row/column inserts are left unhooked - a fresh row/column starts empty, and once real content lands in it,onafterchangescaptures it anyway.Also adds Playwright e2e tests that drive a real jspreadsheet-ce grid end-to-end (delete two rows, delete two columns, reload, verify the deletion persisted), plus a unit test following this repo's existing
handleSavetesting convention, covering all four new hooks.Row/column move (
onmoverow/onmovecolumn) are covered at the unit level only - e2e coverage would need to simulate real drag-and-drop, which is more fragile than a context-menu click for comparatively low added value given the unit tests + library source already confirm the mutation path is symmetric to delete.Why
jspreadsheet-ce only fires
onafterchangesfor cell-value edits. Deleting or reordering rows/columns never marked the sheet dirty or triggered a save, so those changes were silently lost on reload unless followed by an unrelated cell edit or an explicit Save click.🤖 Generated with Claude Code