Repository navigation
fix(data-grid): fire onCellEdit once when Enter or Tab commits - #204
Conversation
In cell edit mode, Enter/Tab called commitEdit() and then focused the grid
to hand the keyboard back. That focus blurs the editor synchronously, and
its onBlur ran commitEdit() again: setState({ editing: null }) and the
pending overlay are batched inside the React handler, so this.state still
showed the editor open with the old value, and onCellEdit fired a second
time with the same payload. The author's write-back script ran twice per
edit (double DB writes / audit entries). Batch mode only re-set the same
pending value, so it was harmless there.
An instance flag (editorOpen) now marks the editor open from startEdit
until closeEditor(), which every close path goes through. commitEdit and
the blur handler check the flag instead of the lagging state, so the
blur that follows an Enter/Tab commit is a no-op.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SwdCxkT64ei9WdHpJH5Qyn
New verify page /grid-cell (GridCellEdit): five static work orders in cell edit mode with the reference onCellEdit write-back script and a counter of its runs. /grid runs in batch mode, so it could not show the bug. The e2e test edits a cell and commits it with Enter, with Tab and by clicking away, then cancels one with Escape. It counts onCellEdit in the client by wrapping componentEvents.fireComponentEvent on the DataGrid, found through its React fiber; a counter on the gateway could race and hide a second event. It expects exactly one event per commit, none for Escape, and checks the gateway counter too. The jest suite has no DOM, and the bug lives in React's event batching, so it can't be covered there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwdCxkT64ei9WdHpJH5Qyn
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughDataGrid now tracks editor state independently of React state and closes the editor during commits, cancellations, and blur validation. A new ChangesCell edit commit handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Enter and Tab commits are guarded against a second blur commit, and the verification test checks the intended edit outcomes. No identified issue blocks merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change prevents duplicate edit events in the normal keyboard-and-blur sequence while preserving validation and event payloads. No introduced security issue was identified. Application-specific write-back handlers and production deployment exposure were not available for verification. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…itner-ozzl6w # Conflicts: # CHANGELOG.md
The onCellEdit spy looked for React 16's __reactInternalInstance$ key on the scroll container, so on the CI gateway it found no DataGrid and the new test failed before editing anything. The Perspective 8.3 client ships React 18.2, which names the key __reactFiber$; accept both. Checked in headless Chromium against React 18.2 with createRoot: main still fires onCellEdit twice on Enter and Tab, this branch once, and the spy finds the instance and counts both cases correctly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwdCxkT64ei9WdHpJH5Qyn
What & why
In cell edit mode (
config.editMode: 'cell'), committing an edit with Enter or Tab firedonCellEdittwice with the same payload, so the author's write-back script ran twice per edit (double DB writes, double audit entries).Cause.
onEditorKeyDowncallscommitEdit(), then focuses the grid. That focus blurs the editor synchronously, and the editor'sonBlurrancommitEdit()again. Inside the React handler,setState({ editing: null })and the pending overlay hadn't been applied yet. Sothis.state.editingwas still set and the old value still passed the "changed" check.Fix. An instance flag,
editorOpen, is set instartEditand cleared in a newcloseEditor()helper. Every close path (commit, cancel, the invalid-blur revert, a vanished row/column) goes through that helper.commitEditand the blur handler check the flag instead of the lagging state, so the blur after an Enter/Tab commit does nothing.Test. New verify page
/grid-cell(GridCellEdit): five static rows in cell mode, the reference write-back script, and a counter of script runs./gridruns in batch mode, so it couldn't show the bug. The new e2e test commits with Enter, Tab and click-away, and cancels with Escape. It wrapscomponentEvents.fireComponentEventon the DataGrid (found through its React fiber) and expects exactly oneonCellEditper commit and none for Escape. It also checks the gateway counter. jest is node-only and the bug comes from React's event batching, so it can't be covered there.Checklist
grid: cell mode fires onCellEdit exactly once per committed edit)CHANGELOG.mdupdated under## [Unreleased]ops/schema-guard.shOK)./gradlew buildpasses locally: not run.cd web && npx tsc --noEmit && npm test(769 passed after mergingmain) andnpm run lint(clean) pass.Notes for reviewers
ops/e2e.sh --freshdidn't run.__reactInternalInstance$), but the Perspective 8.3 client ships React 18.2, which uses__reactFiber$. 2af8ee8 accepts both keys.createRoot). Results are the same under both.commitEdit. Whichever merges second gets a small conflict. To resolve it, keep fix(data-grid): edits land on the wrong row or field #201'srowId/locatelogic, add the|| !this.editorOpenguard to its early return, and route itssetState({ editing: null })calls (including the new one incomponentDidUpdate) throughcloseEditor().🤖 Generated with Claude Code
https://claude.ai/code/session_01SwdCxkT64ei9WdHpJH5Qyn
Summary by CodeRabbit