Conversation
…vider() reads Removes the cachedTableRows stored property on TableViewCoordinator. It mirrored what tableRowsProvider() returns and was kept in sync via four writers (initial load, updateNSView, updateCache, releaseData). The two-sources-of-truth setup was a parking-lot item from PR #931 — under it, every reader had to trust the cache had been refreshed, and forgetting to refresh produced stale-value bugs. Each reader now captures rows once at the top of its scope: - persistColumnLayoutToStorage - updateCache (the only true cache update; cachedRowCount/Count still derived) - releaseData (just drops the now-removed field) - DataGridView+CellPaste (anchor column count check) - DataGridView+Sort (sortDescriptorsDidChange + menuNeedsUpdate header context menu) - DataGridView+RowActions (undoInsertRow no longer needs the explicit refresh) Net -6 LOC, one fewer field on the coordinator, and no possibility of cache/source drift. Smoke-tested: column header context menu, click-to-sort, cell paste, undo-insert all behave as before.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d6e954fed
ℹ️ 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".
| let tableRows = tableRowsProvider() | ||
| guard !tableRows.columns.isEmpty else { return } |
There was a problem hiding this comment.
Using tableRowsProvider() as the source of column names makes layout persistence fail during teardown paths where row data is removed before SwiftUI dismantles the grid. In performClose, removeTableRows(for:) runs before tabs are removed, so dismantleNSView can call persistColumnLayoutToStorage() after the provider already returns an empty TableRows; this new guard then exits early and skips saving widths/order for the current table. Previously the coordinator’s cached snapshot allowed persistence even after store teardown.
Useful? React with 👍 / 👎.
1 participant
Summary
Drops the
cachedTableRowsstored property onTableViewCoordinator. It was a mirror of whattableRowsProvider()returns, kept in sync via four writers (initial load,updateNSView,updateCache,releaseData). Parking-lot item from PR #931 — under it, every reader had to trust the cache had been refreshed, and a missing refresh produced stale-value bugs.What changed
Each reader now captures the live rows once at the top of its scope:
persistColumnLayoutToStorageupdateCache(the only legitimate cache update —cachedRowCount/cachedColumnCountstill derived)releaseData(just drops the now-removed field reset)DataGridView+CellPaste(anchor column count check)DataGridView+Sort(sortDescriptorsDidChange+menuNeedsUpdateheader context menu)DataGridView+RowActions(undoInsertRowno longer needs the explicit refresh)Net −6 LOC, one fewer field on the coordinator, no possibility of cache/source drift.
Test plan
Follow-up parking lot (deferred)
MainEditorContentView.resolvedTableRowsForTabandresolvedTableRows(for:)overlap; consolidatesortCache(sync, small) andcoordinator.querySortCache(async, large) cache the same shape with different keys; mergelastIdentityearly-return + convertmetadataChanged(FK reload) andpaginationChanged(scroll-to-top) to delta dispatch