fix(datagrid): resolve a row view's index from the table instead of a stale copy

[datlechin]

Found while investigating #2449, and independent of the fix that shipped there (#2455).

The bug

Delete one of several rows added in the same session, then double-click a cell: the editor opens on a different row, or on a row past the end where nothing happens. With VoiceOver attached, cells below the deletion read one row's value under another row's number.

Root cause

DataGridRowView.rowIndex and DataGridCellAccessibilityView.row are copies of a display-row index taken once, when AppKit first builds the view in tableView(_:rowViewForRow:) / tableView(_:viewFor:row:).

NSTableView.insertRows(at:) and removeRows(at:) move an already-built view to its new slot without asking the delegate for it again, so the copy goes stale the moment a row is inserted or removed above it. reloadData(forRowIndexes:columnIndexes:) does not heal it either; only a full reloadData() does, and applyRemovedRows calls updateCache() first, so updateNSView sees structureChanged == false and skips it.

Measured, with the stored index beside AppKit's own answer:

after removeRows(at: [1])   rowViewForRow: calls = 0
  display row 1: stored rowIndex=2   row(for:)=1     cell.storedRow=2  row(for: cell)=1
  display row 2: stored rowIndex=3   row(for:)=2     cell.storedRow=3  row(for: cell)=2
after insertRows(at: [0])   rowViewForRow: calls = 1   (the new row only)
  display row 0: stored rowIndex=0   row(for:)=0
  display row 1: stored rowIndex=0   row(for:)=1     <- two views both claiming 0

Every read of the stale copy then names the wrong row, and there are about thirty of them in DataGridRowView alone: the double-click handler, the cell drawing, the selection wash, the whole context menu and its actions, copy-as-INSERT/UPDATE/JSON/CSV, set-NULL/empty/default, and foreign-key navigation.

The fix

AppKit already tracks the mapping, so it is asked for it rather than mirrored. rowIndex becomes a computed property over NSTableView.row(for:), keeping the assigned value as a seed that answers only for a row view that is in no table, which is the shape DataGridRowViewCopyTests builds. Every existing read site is correct with no further change, and StructureRowViewWithMenu inherits it.

DataGridCellAccessibilityView gets the same treatment, and publishes its row index through accessibilityRowIndexRange() rather than stamping it into setAccessibilityRowIndexRange at build time, so a client reading the tree after a row moved is told where the cell is rather than where it was built.

Rejected: rewriting the stored indices from applyInsertedRows / applyRemovedRows. It duplicates bookkeeping AppKit already does, reaches only the available row views, and needs a matching accessibility remount call of its own. Also rejected: forcing a full reloadData() on every incremental mutation, which throws away the view reuse those calls exist for.

The row number, found by review

Codex's review of the first commit caught the half this left behind. The row-number column is the one column that still mounts a real cell view, and makeRowNumberCell stamps its text, its colour and its accessibility label once. AppKit slides that cell to its new slot without asking the delegate to configure it again, so after removing row 1 the row now at 1 kept reading "3" and announcing "Row 3" while the drawn values beside it had already moved on. Making the data correct without this would have left the number and the values in open disagreement, which is worse than both being wrong together.

That cell is now a DataGridRowNumberCellView that owns its own presentation, and applyInsertedRows / applyRemovedRows renumber the viewport after the mutation. Only the viewport is walked, because only the viewport holds a cell.

Performance

row(for:) is documented as O(visible rows), and it lands on a draw path. Measured on a 5,000-row table with 28 mounted row views: 26 ns per call, 13.2 ms for 500,000 calls. drawCells still hoists it into one local before its per-column loop, next to the onEmphasizedSelection local that is already hoisted there, so a 500-column row resolves it once rather than 500 times.

Verification

• test PASS, 57 cases, 0 failed, across the 7 suites that own these types (84 across 11 on the first commit).
• lint 0 SwiftLint violations. (The wrapper's agent docs step reports one pre-existing stale symbol, CLAUDE.md:216 AXCell, in a file this branch does not touch.)
• New suite DataGridRowIdentityTests, 6 cases: every mounted row reports its own display row; a removal above a mounted row moves its index down; an insert above it moves it up; a detached row view still answers with its seed; an accessibility cell moved by a removal reports the new row's index and speaks the new row's value; and the row number follows a removal, in its text and in its spoken label.
• Negative controls, run separately for each half: with rowIndex and row returning the stored copy again, 3 of the 5 then-existing cases fail, and the other 2 are correct in both worlds by design, one of them deliberately so since it pins the detached-view fallback. With the renumbering removed, rowNumberFollowsTheRemoval fails on its own.

No UI automation: reproducing this needs an editable connection, three added rows and a delete, which does not run deterministically in TableProUITests. The unit suite drives the same AppKit calls the coordinator makes.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.