fix(datagrid): show the selected row in the inspector and row operations under a value filter (#1837)#1839
Conversation
…ow when a value filter is active (#1837)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 196e8d5f5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for displayIndex in selectedIndices.sorted() { | ||
| if let row = DisplayRowMapping.row(forDisplay: displayIndex, displayIDs: displayIDs, in: tableRows) { | ||
| allRows.append(Array(row.values)) |
There was a problem hiding this comment.
Resolve filtered selections when saving inspector edits
When a value filter is active, this now populates the inspector edit state from the row resolved by display index, but the Save action still calls saveSidebarEdits, which iterates selectionState.indices and indexes tableRows.rows[rowIndex] directly. Selecting display row 1 for underlying row 2 will therefore show/edit row 2 here, then pressing Save generates SQL using row 1's original values/PK. Please resolve the selected display indices in the save path before building the RowChanges.
Useful? React with 👍 / 👎.
Fixes #1837.
Problem
With a JSON column, selecting a row showed a different row's JSON in the right-side Details inspector. The reporter read it as "delivered timestamp missing," but the inspector was showing another row's value entirely.
Root cause
GridSelectionState.indicescome fromNSTableView.selectedRowIndexes, so they are display-row positions. The grid renders correctly because it maps display -> row viaTableViewCoordinator.displayRow(at:), but the inspector (and the copy/delete/duplicate row operations) indexedTableRows.rows[...]with the raw position. Display position equals array index only whendisplayIDs(valueFilteredIDs ?? sortedIDs) is nil; a per-column "Filter by value" makes them diverge, so the inspector and row operations acted on the wrong underlying row.Apple's HIG (Panels) says an inspector "automatically updat[es] its contents when the item changes or when people select a new item," and mature clients (DataGrip, TablePlus, Postico) bind the value inspector to the live (row, column) coordinate. This does neither.
Changes
DisplayRowMapping(new): the single place that maps a display position to a row givendisplayIDs.TableViewCoordinator.displayRow(at:)/tableRowsIndex(forDisplayRow:)delegate to it.selectedRowDataForSidebarandupdateSidebarEditState(including the sidebar-editonFieldChangedclosure) resolve the selected row throughDisplayRowMappinginstead of raw indexing.columnIndexinstead of a per-fetchUUID, so it follows the selected row and resets only when the columns change. Removed the fragileonChange(of: selectedRowData?.count)reset.RowID(existing rows soft-deleted by display index with resolved values, inserted rows removed by storage index) and re-selects in display space; duplicate resolves the source and selects the new row's display position. All three branch only when a filter is active, so the unfiltered path is unchanged.DisplayRowMapping, never indexTableRows.rowswith a display position.Tests
DisplayRowMappingTests: the regression test. Under a filter, display index 1 resolves to array row 2, not row 1.MultiRowEditState: a column-keyed lookup resolves to the new row's value after a selection change.RowOperationsManager.deleteRows: existing rows marked deleted at their display index with the given values; inserted rows removed by storage index.UI automation for the value-filter + inspector flow needs a live database fixture, so it is not deterministic in
TableProUITests; the unit tests above cover the resolution logic instead.Known limitation
Deleting an inserted (unsaved) row while a value filter is active and other pending edits exist leans on
PendingChanges' single-index-space shift logic. This fix handles the common cases and never regresses the unfiltered path; making that combination bulletproof would need a RowID-based change-tracking redesign, which is out of scope here.