refactor(perf): datagrid signal architecture and row data extraction#919
Merged
refactor(perf): datagrid signal architecture and row data extraction#919
Conversation
Phase A: persistence - Add tabStructureVersion on QueryTabManager, bumped only on tab add/remove/rename/replaceTabContent and on user query dispatch. - Replace .onChange(of: tabManager.tabs) with .onChange(of: tabManager.tabStructureVersion). Remove handleTabsChange; add handleStructureChange. - Remove per-keystroke saveLastQuery from queryTextBinding; remove now-unused saveLastQuery on TabPersistenceCoordinator. Phase B: inspector - Move updateSidebarEditState inside the 50ms inspector debounce so per-row-click N×M field configuration coalesces. - Drop coordinator.tableMetadata?.tableName from inspectorTrigger; metadata-driven inspector refresh now flows through the existing .task(id: tableName) modifier with an explicit scheduleInspectorUpdate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e deltas - Rename QueryTab.resultVersion to schemaVersion (column shape only). ResultSet keeps its own resultVersion for result-set tabs. - Add dataGridDidInsertRows/dataGridDidRemoveRows/dataGridDidReplaceAllRows to DataGridViewDelegate. DataTabGridDelegate forwards to the table view coordinator. - TableViewCoordinator gains applyInsertedRows/applyRemovedRows/applyFullReplace that call NSTableView.insertRows / removeRows / reloadData directly, refresh visual state, and invalidate identity so subsequent updateNSView passes don't short-circuit. - Row ops (add, delete, duplicate, undo, redo, paste) drive the new delta path instead of bumping a counter. RowOperationsManager.deleteSelectedRows now returns physicallyRemovedIndices so soft-deletes stay on the existing reloadVersion path while inserted-row removals get NSTableView animation. - querySortCache invalidates per tab on row-count changes. - Sort completion routes a full reload via the delegate; remove the redundant changeManager.reloadVersion bumps. - applyMultiStatementResults: keep the schemaVersion bump, drop the redundant reloadVersion bump. - Pin toggle no longer mutates a tab counter; TabDisplayState.resultSets is already @observable. - Wire MainContentCoordinator.dataTabDelegate weak ref in MainEditorContentView.onAppear / clear in teardown.
…per render - Extend DataGridIdentity with tabType, tableName, and primaryKeyColumns so updateNSView's identity guard catches all configuration fields that should drive a column rebuild. Drop the legacy initializer. - Move DataTabGridDelegate property assignments out of the dataGridView(tab:) body. Stable refs are set once in onAppear; isEditable / isView / tableName / safeModeLevel drive a focused refresh via .onChange. - DataGridConfiguration was already Equatable; no change.
QueryTab is now pure metadata. Row data (RowBuffer) lives in a RowDataStore keyed by tab.id, owned by MainContentCoordinator. Mutations to row data no longer flow through SwiftUI's @observable tracking on tabManager.tabs. - New RowDataStore (@mainactor, @observable, store @ObservationIgnored): buffer(for:), existingBuffer(for:), setBuffer, removeBuffer, evict, evictAll(except:), tearDown. - Drop rowBuffer and the resultRows/resultColumns/columnTypes/columnDefaults/columnForeignKeys/columnEnumValues/columnNullable proxy properties from QueryTab. Update == and init(from:) accordingly. - Migrate every read and write site (QueryHelpers, MultiStatement, LoadMore, RowOperations, Filtering, FKNavigation, Navigation, Discard, SaveChanges, SidebarActions, SidebarSave, TabSwitch, WindowLifecycle, CommandActions, plus inspector and editor views) to use coordinator.rowDataStore.buffer(for: tab.id). - replaceTabContent on QueryTabManager no longer resets a per-tab buffer; callers reset via setBuffer. - Eviction routes through rowDataStore.evictAll(except:); teardown clears the store. - Tests updated to read row data from the coordinator's rowDataStore.
…sion double-bump - DataGridCoordinator.applyInsertedRows / applyRemovedRows / applyFullReplace now call updateCache() before mutating the table view, so numberOfRows(in:) returns the post-mutation count when NSTableView synchronously validates. - QueryTabManager: drop the explicit tabStructureVersion bump from each add* method. The didSet on tabs already bumps when the ID array changes, and the explicit bump made every add count as 2 increments. replaceTabContent keeps its bump because same-id mutation does not change the ID array.
…f-arch # Conflicts: # TablePro/Views/Main/Child/MainEditorContentView.swift # TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift
![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: d28e6548b4
ℹ️ 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
+16
to
+17
| if oldValue.map(\.id) != tabs.map(\.id) { | ||
| tabStructureVersion += 1 |
There was a problem hiding this comment.
tabStructureVersion only increments when the tab ID list changes, but persistence now listens exclusively to that counter; any in-place tab updates (same ID) like title/file metadata edits will not trigger handleStructureChange, so those edits are lost on crash/restart until some unrelated structural change happens. This regression was introduced by narrowing the didSet condition to ID changes only.
Useful? React with 👍 / 👎.
Comment on lines
+383
to
384
| coordinator?.rowDataStore.evictAll(except: nil) | ||
| coordinator?.tabManager.tabs.removeAll() |
There was a problem hiding this comment.
This path now evicts row data but then removes all tabs without clearing RowDataStore, leaving one retained RowBuffer entry per closed tab in a long-lived empty window; reopening/closing tabs repeatedly grows retained metadata indefinitely. Before this refactor, buffers were owned by tabs and were released on tabs.removeAll(), so this is a new memory-retention regression.
Useful? React with 👍 / 👎.
…protocol - New protocol RowDeltaApplying captures the row-delta surface (applyInsertedRows/applyRemovedRows/applyFullReplace) so DataTabGridDelegate can be tested with a fake without standing up an NSTableView. TableViewCoordinator conforms via empty extension. - DataTabGridDelegate.tableViewCoordinator switches to (any RowDeltaApplying)?. - Unit tests added: RowDataStore (10 cases), RowProviderCache (9 cases), QueryTabManager.tabStructureVersion semantics (10 cases), DataTabGridDelegate forwarding (4 cases). - Drop a stale comment in QueryTabManager.init() (CLAUDE.md no-comments rule). - Guard the selectedTabId .onChange against caching a provider over an evicted RowBuffer.
- MainEditorContentView: the selectedTabId onChange and onAppear hooks called rowDataStore.buffer(for:) inside the eviction guard. buffer(for:) creates an empty RowBuffer on miss, so the guard could leak ghost entries (e.g. on first switch to a tab that has not loaded yet, or on view re-appear after teardown). Use existingBuffer(for:) so the guard only runs when a buffer actually exists, and skip cacheRowProvider for fresh / evicted tabs. - TabStructureVersionTests: cover drag-reorder (tabs.swapAt) so the didSet ID-array-change check is regression-tested.
…fix tests Dead code removal: - ResultSet.resultVersion was write-only (3 sites set it, 0 sites read). Field gone, 5 dead writes removed in QueryHelpers, QueryParameters, MultiStatement, and two LoadMore paths. - TabPersistenceCoordinator.saveLastQuery was deleted in the persistence-decoupling phase but loadLastQuery was left orphaned. Drop loadLastQuery on the coordinator, drop saveLastQuery / loadLastQuery / lastQueryFileURL / lastQueryDirectory on TabDiskActor, drop the legacy lastQuery migration loop and key prefix. Per-keystroke crash recovery is gone; user-dispatched queries still bump tabStructureVersion so committed work persists. Test fixes: - TriggerStructTests: drop metadataTableName, rename resultVersion -> schemaVersion. Old signature was broken by phases B and C. - DataGridIdentityTests: convert hiddenColumns: argument to configuration: DataGridConfiguration. Phase D added tabType / tableName / primaryKeyColumns to the identity through the configuration init. - CommandActionsDispatchTests: drop selectedRowIndices binding, pass coordinator.selectionState. - MainStatusBarLayoutTests: pass StatusBarSnapshot instead of QueryTab. - SaveCompletionTests: drop selectedRowIndices inout from row op calls; assert on coordinator.selectionState.indices instead. - RowOperationsManagerTests: deleteSelectedRows now returns DeleteRowsResult; assert on .nextRowToSelect. - AnyChangeManagerTests: drop dataManager: / structureManager: argument labels (single any ChangeManaging init), and switch the changes property to rowChanges. - TabDiskActorTests + TabPersistenceCoordinatorTests: drop the lastQuery round-trip / nil / empty / whitespace / 500KB-cap tests since the feature is gone.
- Add 4 RowOperationsManagerTests cases for the DeleteRowsResult.physicallyRemovedIndices contract: empty selection returns empty, deleting only existing rows leaves it empty (soft-delete via change manager), deleting inserted rows reports indices descending, mixed selection reports only the inserted indices. - Drop the stale 'Last-query strings are stored in a sibling directory' doc comment on TabDiskActor; the directory and feature are gone.
This was referenced Apr 27, 2026
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
Architectural refactor of the datagrid pipeline. Splits the overloaded
resultVersioncounter into purpose-built signals, decouples persistence and inspector from row writes, and moves row data out ofQueryTabinto a per-coordinatorRowDataStore. SwiftUI no longer sees row mutations as tab-array mutations, and NSTableView gets surgical insert/remove deltas instead of full reloads.What changed
Signal taxonomy
QueryTab.schemaVersion(renamed fromresultVersion): column shape only.QueryTabManager.tabStructureVersion: tab add / remove / replace / user query dispatch. Drives persistence viahandleStructureChange(replaceshandleTabsChange).changeManager.reloadVersion: cell edits only.DataGridViewDelegate.dataGridDidInsertRows / dataGridDidRemoveRows / dataGridDidReplaceAllRows: row-shape mutations go straight to NSTableView.RowDataStore(new, per coordinator, keyed by tab.id): row data lives here, not on QueryTab.Persistence
.onChange(of: tabManager.tabs)replaced with.onChange(of: tabManager.tabStructureVersion).saveLastQueryremoved;runQuerybumps the structure counter so user-dispatched work is crash-recoverable.Inspector
updateSidebarEditStatemoved inside the existing 50ms debounce. Was running synchronously per row click.coordinator.tableMetadataremoved frominspectorTrigger;.task(id: tableName)callsscheduleInspectorUpdateafter metadata loads.Row operations
addNewRow,deleteSelectedRows,duplicateSelectedRow,pasteRows,undoInsertRow,undoLastChange,redoLastChangenow driveNSTableView.insertRows/removeRows/reloadDatathrough the delegate. NoresultVersionbump, no SwiftUI re-eval cycle for row mutations.RowOperationsManager.deleteSelectedRowsreturnsphysicallyRemovedIndicesso soft-deleted (existing) rows stay on thechangeManager.reloadVersionpath while inserted-row removals get NSTableView animation.querySortCacheinvalidated per tab on row-count changes.Spurious bumps removed
resultVersion(TabDisplayState.resultSetsis@Observable).changeManager.reloadVersion; routes a single replace through the delegate.applyMultiStatementResultsno longer double-signals.Identity guards
DataGridConfigurationalready conformed toEquatable.DataGridIdentityextended withtabType,tableName,primaryKeyColumnssoupdateNSViewshort-circuits when nothing structural changed.DataTabGridDelegateproperties wired inonAppear/.onChange, not in the dataGridView body.Row data extraction
RowDataStore(@MainActor @Observable, store@ObservationIgnored).rowBufferand 7 proxy properties (resultRows,resultColumns,columnTypes,columnDefaults,columnForeignKeys,columnEnumValues,columnNullable) fromQueryTab.coordinator.rowDataStore.buffer(for: tab.id).ResultSet.rowBuffer(separate, ondisplay.resultSets) kept as-is per design.Correctness details
applyInsertedRows/applyRemovedRows/applyFullReplacecallupdateCache()before mutating the table view, sonumberOfRows(in:)returns the post-mutation count when NSTableView synchronously validates.tabStructureVersiondriven bytabs.didSet(catches all add/remove/external mutations); explicit bump only inreplaceTabContent(same-id mutation) andrunQuery.Test plan