From 3d34534049f5db32ada1c7bc008edbfaae3fe80d Mon Sep 17 00:00:00 2001 From: Sameer Deolalikar <96577638+sameerdeolalikar@users.noreply.github.com> Date: Sun, 27 Sep 2026 00:31:41 +0200 Subject: [PATCH] fix: preserve cell identity in commit callbacks --- src/ActiveCell.tsx | 38 +++++------- src/Spreadsheet.test.tsx | 130 +++++++++++++++++++++++++++++++++++++++ src/Spreadsheet.tsx | 9 ++- src/reducer.ts | 30 ++++++--- src/types.ts | 9 ++- 5 files changed, 181 insertions(+), 35 deletions(-) diff --git a/src/ActiveCell.tsx b/src/ActiveCell.tsx index d077c1439..f6f7faa57 100644 --- a/src/ActiveCell.tsx +++ b/src/ActiveCell.tsx @@ -46,6 +46,7 @@ const ActiveCell: React.FC = (props) => { ); const initialCellRef = React.useRef(undefined); + const prevModeRef = React.useRef("view"); const prevActiveRef = React.useRef(null); const prevCellRef = React.useRef(undefined); @@ -69,37 +70,32 @@ const ActiveCell: React.FC = (props) => { React.useEffect(() => { const prevActive = prevActiveRef.current; const prevCell = prevCellRef.current; - prevActiveRef.current = active; - prevCellRef.current = cell; - - if (!prevActive || !prevCell) { - return; - } - - // Commit + const prevMode = prevModeRef.current; const coordsChanged = - active?.row !== prevActive.row || active?.column !== prevActive.column; - const exitedEditMode = mode !== "edit"; + active?.row !== prevActive?.row || active?.column !== prevActive?.column; - if (coordsChanged || exitedEditMode) { + // An edit belongs to the cell where it started, even after navigation. + if (prevMode === "edit" && (coordsChanged || mode !== "edit")) { const initialCell = initialCellRef.current; - if (prevCell !== initialCell) { + const nextCell = coordsChanged ? prevCell : cell; + if (prevActive && nextCell !== initialCell) { commit([ { prevCell: initialCell || null, - nextCell: prevCell, - }, - ]); - } else if (!coordsChanged && cell !== prevCell) { - commit([ - { - prevCell, - nextCell: cell || null, + nextCell: nextCell || null, + point: prevActive, }, ]); } - initialCellRef.current = cell; } + + if (mode === "edit" && (prevMode !== "edit" || coordsChanged)) { + // Typing to replace clears the value on entry; retain the pre-edit cell. + initialCellRef.current = coordsChanged ? cell : prevCell; + } + prevActiveRef.current = active; + prevCellRef.current = cell; + prevModeRef.current = mode; }); const DataEditor = (cell && cell.DataEditor) || props.DataEditor; diff --git a/src/Spreadsheet.test.tsx b/src/Spreadsheet.test.tsx index ec498ee51..1865c79e4 100644 --- a/src/Spreadsheet.test.tsx +++ b/src/Spreadsheet.test.tsx @@ -579,3 +579,133 @@ function getHTMLCollectionIndexOf( function getSpreadsheetElement(): Element { return safeQuerySelector(document, ".Spreadsheet"); } + +describe("cell commit identity", () => { + test("clearing a different cell reports the cleared cell, not the last edit", () => { + const onCellCommit = jest.fn(); + const data = [[{ value: "A" }, { value: "B" }]]; + const { container } = render( + + ); + const cells = container.querySelectorAll("td"); + fireEvent.mouseDown(cells[1]); + fireEvent.keyDown( + safeQuerySelector(container, ".Spreadsheet__active-cell"), + { key: "Enter" } + ); + fireEvent.change(safeQuerySelector(container, "input"), { + target: { value: "edited B" }, + }); + fireEvent.keyDown(safeQuerySelector(container, "input"), { key: "Enter" }); + fireEvent.mouseDown(cells[0]); + onCellCommit.mockClear(); + fireEvent.keyDown( + safeQuerySelector(container, ".Spreadsheet__active-cell"), + { key: "Delete" } + ); + expect(onCellCommit).toHaveBeenCalledTimes(1); + expect(onCellCommit).toHaveBeenCalledWith( + data[0][0], + { value: undefined }, + { row: 0, column: 0 } + ); + }); + + test("entering and leaving an unchanged cell does not commit; editing retains its old value", () => { + const onCellCommit = jest.fn(); + const data = [[{ value: "original" }]]; + const { container } = render( + + ); + fireEvent.mouseDown(safeQuerySelector(container, "td")); + expect(onCellCommit).not.toHaveBeenCalled(); + fireEvent.keyDown( + safeQuerySelector(container, ".Spreadsheet__active-cell"), + { key: "Enter" } + ); + fireEvent.keyDown(safeQuerySelector(container, "input"), { key: "Enter" }); + expect(onCellCommit).not.toHaveBeenCalled(); + fireEvent.keyDown( + safeQuerySelector(container, ".Spreadsheet__active-cell"), + { key: "Enter" } + ); + fireEvent.change(safeQuerySelector(container, "input"), { + target: { value: "changed" }, + }); + fireEvent.keyDown(safeQuerySelector(container, "input"), { key: "Enter" }); + expect(onCellCommit).toHaveBeenCalledTimes(1); + expect(onCellCommit).toHaveBeenCalledWith( + data[0][0], + { value: "changed" }, + { row: 0, column: 0 } + ); + }); + + test("changing the callback does not replay a previously delivered commit", () => { + const first = jest.fn(); + const second = jest.fn(); + const data = [[{ value: "A" }]]; + const { container, rerender } = render( + + ); + fireEvent.mouseDown(safeQuerySelector(container, "td")); + fireEvent.keyDown( + safeQuerySelector(container, ".Spreadsheet__active-cell"), + { key: "Delete" } + ); + first.mockClear(); + rerender(); + expect(second).not.toHaveBeenCalled(); + }); +}); + +test("pasting a row reports each destination coordinate exactly once", () => { + const onCellCommit = jest.fn(); + const data = [[{ value: "A" }, { value: "B" }]]; + const { container } = render( + + ); + fireEvent.mouseDown(safeQuerySelector(container, "td")); + fireEvent.paste(safeQuerySelector(container, ".Spreadsheet__active-cell"), { + clipboardData: { getData: () => "X\tY" }, + }); + expect(onCellCommit).toHaveBeenCalledTimes(2); + expect(onCellCommit).toHaveBeenNthCalledWith( + 1, + data[0][0], + { value: "X" }, + { row: 0, column: 0 } + ); + expect(onCellCommit).toHaveBeenNthCalledWith( + 2, + data[0][1], + { value: "Y" }, + { row: 0, column: 1 } + ); +}); + +test("cut and paste commits the source removal and destination separately", () => { + const onCellCommit = jest.fn(); + const data = [[{ value: "A" }, { value: "B" }]]; + const { container } = render( + + ); + const cells = container.querySelectorAll("td"); + fireEvent.mouseDown(cells[0]); + fireEvent.cut(safeQuerySelector(container, ".Spreadsheet__active-cell"), { + clipboardData: { setData: jest.fn() }, + }); + fireEvent.mouseDown(cells[1]); + fireEvent.paste(safeQuerySelector(container, ".Spreadsheet__active-cell"), { + clipboardData: { getData: () => "A" }, + }); + expect(onCellCommit).toHaveBeenCalledTimes(2); + expect(onCellCommit).toHaveBeenNthCalledWith(1, data[0][0], null, { + row: 0, + column: 0, + }); + expect(onCellCommit).toHaveBeenNthCalledWith(2, data[0][1], data[0][0], { + row: 0, + column: 1, + }); +}); diff --git a/src/Spreadsheet.tsx b/src/Spreadsheet.tsx index f791fd552..6cfdcddbd 100644 --- a/src/Spreadsheet.tsx +++ b/src/Spreadsheet.tsx @@ -295,13 +295,18 @@ const Spreadsheet = ( }, [state.mode, onModeChange]); // Listen to last commit changes - const prevLastCommitRef = React.useRef( + const prevLastCommitRef = React.useRef( state.lastCommit ); React.useEffect(() => { if (state.lastCommit && state.lastCommit !== prevLastCommitRef.current) { + prevLastCommitRef.current = state.lastCommit; for (const change of state.lastCommit) { - onCellCommit(change.prevCell, change.nextCell, state.lastChanged); + onCellCommit( + change.prevCell, + change.nextCell, + change.point ?? state.lastChanged + ); } } }, [onCellCommit, state.lastChanged, state.lastCommit]); diff --git a/src/reducer.ts b/src/reducer.ts index cb370e6fd..3a22f3889 100644 --- a/src/reducer.ts +++ b/src/reducer.ts @@ -189,9 +189,17 @@ export default function reducer( ? Matrix.unset(state.copied.start, state.model.data) : state.model.data; const commit: Types.StoreState["lastCommit"] = []; + if (state.cut && state.copied) { + commit.push({ + point: state.copied.start, + prevCell: Matrix.get(state.copied.start, state.model.data) || null, + nextCell: null, + }); + } for (const point of selectedRange || []) { const currentCell = Matrix.get(point, state.model.data); commit.push({ + point, prevCell: currentCell || null, nextCell: cell || null, }); @@ -234,10 +242,16 @@ export default function reducer( row: point.row + state.copied.start.row, column: point.column + state.copied.start.column, }; + commit = [ + ...commit, + { + point: prevPoint, + prevCell: Matrix.get(prevPoint, acc.data) || null, + nextCell: null, + }, + ]; nextData = Matrix.unset(prevPoint, acc.data); } - - commit = [...commit, { prevCell: cell || null, nextCell: null }]; } if (!Matrix.has(nextPoint, paddedData)) { @@ -249,6 +263,7 @@ export default function reducer( commit = [ ...commit, { + point: nextPoint, prevCell: currentCell, nextCell: cell || null, }, @@ -303,7 +318,7 @@ export default function reducer( if (state.mode === "view" && state.active) { const selectedRange = state.selected.toRange(state.model.data); if (selectedRange?.size() === 1) { - return edit(clear(state)); + return edit(clear(state, false)); } return edit(state); } @@ -356,7 +371,7 @@ function edit(state: Types.StoreState): Types.StoreState { return { ...state, mode: "edit" }; } -function clear(state: Types.StoreState): Types.StoreState { +function clear(state: Types.StoreState, notify = true): Types.StoreState { if (!state.active) { return state; } @@ -379,6 +394,7 @@ function clear(state: Types.StoreState): Types.StoreState { const cell = Matrix.get(point, state.model.data); const clearedCell = clearCell(cell); changes.push({ + point, prevCell: cell || null, nextCell: clearedCell || null, }); @@ -388,7 +404,7 @@ function clear(state: Types.StoreState): Types.StoreState { return { ...state, model: new Model(createFormulaParser, newData), - ...commit(changes), + ...(notify ? commit(changes) : {}), }; } @@ -448,8 +464,8 @@ const keyDownHandlers: KeyDownHandlers = { ArrowRight: go(0, +1), Tab: go(0, +1), Enter: edit, - Backspace: clear, - Delete: clear, + Backspace: (state) => clear(state), + Delete: (state) => clear(state), Escape: blur, }; diff --git a/src/types.ts b/src/types.ts index fa47a46af..de395ce77 100644 --- a/src/types.ts +++ b/src/types.ts @@ -60,7 +60,7 @@ export type StoreState = { >; dragging: boolean; lastChanged: Point | null; - lastCommit: null | CellChange[]; + lastCommit: null | CommitChanges; }; export type CellChange = { @@ -203,9 +203,8 @@ export type CornerIndicatorProps = { export type CornerIndicatorComponent = React.ComponentType; -export type CommitChanges = Array<{ - prevCell: Cell | null; - nextCell: Cell | null; -}>; +export type CommitChanges = Array< + CellChange & { point?: Point } +>; export type CreateFormulaParser = (data: Matrix) => FormulaParser;