Repository navigation
Conversation
Four ways a grid edit could be written to the wrong place without any error: - Cell positions are display indexes (pinned first), but edit and paste looked them up in config/columnLayout order, so a pinned column that wasn't first shifted every edit onto its neighbour (#127). effectiveColumns now returns pinned columns first. - The open editor stored a row index. A rebind that inserted a row moved a different record under it and the commit went there (#128). The editor now holds the row id + field, commits by id, and closes if the row or column leaves the view. - onCellEdit's row carried only the one new value, so persisting it undid an earlier unrebound edit of the same row (#129). It now applies every pending edit of that row. - Rows with a missing or shared idField value all answered to one id, so one click selected them all and Delete removed them all (#130). Such rows are now read-only and the grid logs a warning. Pending edits use a structured key, so ids containing '::' no longer split wrong. Closes #127, closes #128, closes #129, closes #130. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (6)
🚧 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. 📝 WalkthroughWalkthroughThe Data Grid now orders pinned columns first, tracks editors by row ID and field, and restricts actions on rows with missing or duplicate IDs. Pending edits use structured keys, reconcile against rebound values, and appear together in cell-edit event rows. ChangesData Grid behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The Data Grid changes keep edits attached to the intended row and column and block interactions on rows with unusable IDs. No actionable merge-blocking issue was identified in the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The changes improve normal edit targeting and reject ambiguous row IDs. However, an unresolved edit can be carried into a later record that reuses the same ID, and the expanded event payload can forward that stale value for persistence. This is a conditional data-integrity risk, not a verified authorization bypass. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The incremental diff adds
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review and test resultsTested
Should fix before merge
Smaller Pre-existing, not from this PR: one Enter in the cell editor fires |
Follow-ups from testing this PR live: - Rows were keyed by index, so a rebind that shifted the edited row remounted the editor's input and dropped the focus: typing, Enter and Escape went nowhere. Rows are now keyed by id. - An editor whose row id became shared after it opened still committed to the first row with that id, with that record's data. locateCell now treats an unusable id as gone, and commit rechecks that the cell is still editable. - A Shift-range selection swept duplicate ids into state.selection, and a selected duplicate id highlighted every row with it. - A pending edit only cleared when the bound value matched it, so a rejected or normalized write-back left the old value shown and re-sent it in the row's next onCellEdit. It now also clears when the cell's bound value changes to anything else (settlePending). - The missing-id warning no longer fires for grids that never select, edit or delete. Docs: README described pending as clearing on any rows change; it never did. CHANGELOG notes the CSV column order and the batch-mode conflict behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Findings 1 to 6 from the review above are addressed in b118016. New tests: unit tests for |
…bundle-639368 # Conflicts: # CHANGELOG.md
1 participant
Fixes four Data Grid bugs where an edit, select or delete hit the wrong row or field without any error.
Changes
effectiveColumnsnow returns pinned columns first, matching the drawn order. Cell positions index that list, so edit and paste now target the column you see.rowId+fieldinstead of a position. Commit looks the row up by id; if the row or column has left the view, the editor closes without writing. The editor is rendered on the cell with that id/field.onCellEditrow dropped pending edits:event.rowis now the bound row plus every pending edit of that row (rowWithPending), so a two-column paste or two quick edits no longer undo each other.unusableRowIdsfinds rows with an empty or sharedidFieldvalue. Those rows can't be selected, edited, pasted into or deleted, and the grid logs aconsole.warnnaming them. Pending edits use a structured key (JSON.stringify([rowId, field])), so ids containing::parse correctly.Docs: CHANGELOG
[Unreleased], README, user manual,onCellEditevent description. No props schema change. For #130 I chose a console warning over a newoutput.*prop.Testing
cd web && npx tsc --noEmit && npm test: 758 passed, with new unit tests for each fix.npm run lint: clean.ops/e2e.sh --fresh: 106 passed. The new e2e test drags Qty onto the pinned Order column and edits the Qty cell. With only the Data Grid: editing a cell edits the wrong field when a pinned column isn't first #127 fix reverted, it fails (no editor opens)./griddemo runs in batch mode and doesn't refresh its rows, so it can't reproduce them live.Closes #127
Closes #128
Closes #129
Closes #130
🤖 Generated with Claude Code
Summary by CodeRabbit
::.