refactor(datagrid): migrate callers to TableRows + delete legacy stack#931
Merged
refactor(datagrid): migrate callers to TableRows + delete legacy stack#931
Conversation
![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: e190a7a2f4
ℹ️ 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".
Comment on lines
+572
to
+573
| if let rs = tab.display.activeResultSet, !rs.resultColumns.isEmpty { | ||
| provider = InMemoryRowProvider( | ||
| rowBuffer: rs.rowBuffer, | ||
| sortIndices: sortIndicesForTab(tab), | ||
| columns: rs.resultColumns, | ||
| columnDefaults: rs.columnDefaults, | ||
| columnTypes: rs.columnTypes, | ||
| columnForeignKeys: rs.columnForeignKeys, | ||
| columnEnumValues: rs.columnEnumValues, | ||
| columnNullable: rs.columnNullable | ||
| ) | ||
| } else { | ||
| let buffer = coordinator.rowDataStore.buffer(for: tab.id) | ||
| provider = InMemoryRowProvider( | ||
| rowBuffer: buffer, | ||
| sortIndices: sortIndicesForTab(tab), | ||
| columns: buffer.columns, | ||
| columnDefaults: buffer.columnDefaults, | ||
| columnTypes: buffer.columnTypes, | ||
| columnForeignKeys: buffer.columnForeignKeys, | ||
| columnEnumValues: buffer.columnEnumValues, | ||
| columnNullable: buffer.columnNullable | ||
| ) | ||
| return rs.tableRows |
There was a problem hiding this comment.
resolvedTableRowsForTab returns activeResultSet.tableRows, but all edit/insert/delete paths now mutate tableRowsStore (via tableRowsMutator in the same view). Because TableRows is a struct, the result-set copy and store copy diverge after the first mutation, so the grid can render stale data while row deltas are applied to a different backing state; in practice this can make edits appear to revert and can desynchronize numberOfRows from inserted/removed row operations.
Useful? React with 👍 / 👎.
datlechin
changed the title
…tatusBarSnapshot rename
…en resignFirstResponder
… offset inside mutation
Load More appended rows to the store but ResultSet.tableRows stayed frozen at applyPhase1Result time. The data grid read through the active ResultSet, so newly fetched rows showed empty cells. Route every TableRows mutation through mutateActiveTableRows on MainContentCoordinator. The helper updates the store, then writes the new value back into the active ResultSet. Result-set switches go through switchActiveResultSet so the store tracks the new active snapshot. Read paths simplify to reading the store directly.
…a across reloads Two related visual regressions: 1. Edit cell -> yellow modified background -> undo -> value reverts but highlight persists. applyDataUndo updated pending state but did not bump reloadVersion, so the visual state cache was gated and the stale modifiedColumns set survived. Bumping reloadVersion forces a rebuild on the next render. 2. FK column arrow and dropdown chevron toggle visible/hidden on each reload of a table tab. applyPhase1Result rebuilt TableRows from scratch and only populated columnDefaults / columnForeignKeys / columnNullable / columnEnumValues when a fresh schema fetch ran. When isMetadataCached returned true (because metadata was in the previous TableRows), no fetch ran and the new TableRows wiped the metadata, which then caused the next reload to refetch -- so every other reload had FK info and every other one didn't. Carry the existing metadata over when the schema fetch is skipped.
Inserting or undoing a row pegged the CPU at 100%. Each mutation went through mutateActiveTableRows, which wrote the live TableRows back into the active ResultSet's @observable tableRows property. That triggered a full SwiftUI re-render of MainEditorContentView (which reads rs.resultColumns / rs.errorMessage / etc), on top of the existing observation triggers from changeManager.reloadVersion and tabManager.tabs. Fast key-repeat undo or paste cascaded re-renders. Move the per-ResultSet snapshot into a save-on-switch model: mutateActiveTableRows now only writes the store, and switchActiveResultSet saves the outgoing snapshot then loads the incoming one. Edits in a pinned result set still survive switching back, but routine inserts / undos / cell edits no longer cross the @observable boundary on the ResultSet. Also short-circuit the inserted-rows scan in DataGridCoordinator.rebuildVisualStateCache: when the grid is unsorted, read changeManager.insertedRowIndices directly instead of iterating every row in TableRows. The full scan only runs in the sorted case where display indices differ from storage indices.
… evicted
`evictInactiveRowData` deliberately skips the currently selected tab
("kept in memory so the user sees no refresh flicker"), but the migrated
tests added a tab and immediately called eviction — the new tab was the
selected tab, so eviction was a no-op and the assertions failed.
Fix: add a second tab so the first becomes background, then assert
eviction on the background tab.
…ing call addNewRow / duplicateSelectedRow now call coordinator.beginEditing(displayRow:column:) synchronously after applyDelta. The editingCell SwiftUI binding plumbing across 7 files is removed since no caller sets it to non-nil anymore. Each row-add press fully completes (commit prior edit, mutate model, apply delta, focus new cell) before the next press fires, so rapid Cmd+Shift+N keeps focus on the latest appended row instead of getting trapped in queued Tasks.
setActiveTableRows now dispatches applyFullReplace to the active grid after every full row swap, routing through a single mutation surface instead of seven direct tableRowsStore.setTableRows callers across navigation, query helpers, multi-statement, FK navigation, and sidebar actions. Without the dispatch, the coordinator's RowID-keyed displayCache survived table switches and returned the previous table's formatted cell values for matching RowIDs, even though the cell views themselves had rebuilt with the new column set.
… add dispatch regression tests The protocol now exposes commitActiveCellEdit and beginEditing alongside the row-delta methods, so its name no longer matches its scope. Renaming to TableViewCoordinating tracks the conforming class TableViewCoordinator and the field DataTabGridDelegate.tableViewCoordinator. TableRowsMutationTests verifies that setActiveTableRows dispatches applyFullReplace exactly once for the active tab and skips background tabs, locking in the displayCache invalidation contract that was missing before commit 0e967c2.
7 tasks
datlechin
added a commit
that referenced
this pull request
Apr 29, 2026
…vider() reads (#934) 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.
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase C.2 of the DataGrid refactor. Migrates every caller of
RowBuffer/InMemoryRowProvider/RowDataStoreto useTableRows/TableRowsStore/TableRowsController, then deletes the three old types. Mutations now emitDeltaevents and the controller drivesNSTableViewdirectly viainsertRows/removeRows/reloadData(forRowIndexes:)— eliminating the SwiftUI counter-driven full reloads.Builds on Phase C.1 (PR #930) which introduced
TableRows,Row, andDelta.Why
The old row-data system had three tightly-coupled types with inconsistent mutation paths (direct buffer writes,
inoutarray refs, provider methods). Sort indices didn't auto-invalidate. Inserted rows lived in a parallelappendedRowsarray. The provider's weak ref toRowBufferfell through to a shared empty buffer with no error. Every cell edit triggered a full SwiftUIreloadVersionbump.After this PR:
TableRows) for cell values and column metadata.TableRowsand returnDelta.DeltatoNSTableViewdirectly — partial reloads, surgical insert/remove animations.RowIDso it survives mutations cleanly.RowID.Commits (12, app green at each step — read in order)
da111ec8TableRowsStorealongsideRowDataStore(12 unit tests)3e98e3c9TableRowsController(apply(_ delta:to:)mapping every Delta case to NSTableView; 11 unit tests)b7bd7a4bTableRowsStore(parallel writes, no behavior change)9b62697f5e1ba86ea2f5a4ce387dc8ebtableRows.editandDeltab4c16432RowOperationsManagermutates TableRows and returns Delta45c51815TableRows.insertInsertedRow(at:values:)for non-tail re-insertion0b7ea835Row.id2d941b41Row.ide190a7a2RowBuffer,InMemoryRowProvider,RowDataStoreEach commit was reviewed for the constraint that the app must continue to build at every step. The user opted out of a worktree for this PR — work was on the main repo's feature branch.
What was deleted in commit 10
TablePro/Models/Query/RowBuffer.swiftTablePro/Models/Query/RowProvider.swift(InMemoryRowProvider,TableRowData)TablePro/Core/Services/Query/RowDataStore.swiftTablePro/Views/Results/RowProviderCache.swiftclosingTabRemovesOnlyThatEntry()inTableRowsStoreTests.grep -rn "RowBuffer\|InMemoryRowProvider\|RowDataStore" TablePro/ TableProTests/→ empty.Notable design decisions (locked from the planning round)
TableRowsis aSendablestruct withContiguousArray<Row>storage. Owns rows + columns + types + metadata.Row.idisRowID.existing(Int) | .inserted(UUID)— replaces today's parallelinsertedRowIndicesset.TableRowsis notEquatable— full row equality on big tables is a footgun;DataGridIdentitykeys on schemaVersion.PendingChanges— composition stays at the coordinator level.sortedIDs). User can re-click the column header to re-sort.cellChanged, prune-against-alive-IDs onrowsRemoved, full clear oncolumnsReplaced/fullReplace.Test plan
This PR is large enough that manual verification matters more than the test suite alone. Suggested smoke tests on a local DB (
tablepro_demoworks in MariaDB or PostgreSQL; both have boolean columns and a 5000-roweventstable):events). VoiceOver navigation works.Verification commands
swiftlint lint --strict xcodebuild -project TablePro.xcodeproj -scheme TablePro -configuration Debug build -skipPackagePluginValidation xcodebuild -project TablePro.xcodeproj -scheme TablePro test -skipPackagePluginValidationFollow-up parking lot (deferred, not in this PR)
TableViewCoordinator.cachedTableRowsmirrorstableRowsProvider(). Two sources of truth in the coordinator. Could be reduced to one if the per-cell delegate path can afford a closure call.MainEditorContentView.resolvedTableRowsForTabandresolvedTableRows(for:)overlap; consolidate.sortCache(sync, small) andcoordinator.querySortCache(async, large) cache the same shape with different keys; merge.tabManager.tabs.removeAll { ... }site needs to calltableRowsStore.removeTableRows(for: tabId).reloadVersion/lastIdentity/lastReapplyVersionnow that mutations drive NSTableView throughDeltadirectly.