refactor(datagrid): drop DataGridIdentity + version-counter bridge#938
Merged
refactor(datagrid): drop DataGridIdentity + version-counter bridge#938
Conversation
…ter bridge (Phase D-b) Completes Phase D. The remaining metadataChanged (FK column reload) and paginationChanged (scroll-to-top) branches in updateNSView were the last consumers of DataGridIdentity. Both convert to explicit dispatch on TableViewCoordinating: - refreshForeignKeyColumns(): reloads visible FK column cells (lifted verbatim from the metadataChanged branch). Called from MainContentCoordinator+QueryHelpers after the columnEnumValues mutation that bumps metadataVersion. The other metadataVersion bump (in setupResultsForExecutedQuery) is followed immediately by setActiveTableRows, which already dispatches applyFullReplace and renders FK columns fresh — no extra dispatch needed there. - scrollToTop(): scrolls to row 0 if any rows exist. Trickier than refreshFK because pagination bumps the version BEFORE runQuery is fired; the scroll has to wait until the new data lands. Added pendingScrollToTopAfterReplace flag on MainContentCoordinator. paginateAfterConfirmation sets it; notifyFullReplaceIfActive checks and clears it after applyFullReplace fires. With both branches gone, DataGridIdentity has no consumers. Drops: - struct DataGridIdentity (entire type, 25 LOC) - var lastIdentity on TableViewCoordinator - The early-return short-circuit in updateNSView - previousIdentity capture and the metadataChanged / paginationChanged variables - metadataVersion / paginationVersion / schemaVersion props on DataGridView (no remaining readers) - The matching call-site args in MainEditorContentView.dataGridView and TableStructureView - DataGridIdentityTests entirely (-105 LOC test file) The 'initial pre-warm display cache' check (was `previousIdentity == nil || previousIdentity?.rowCount == 0`) becomes `oldRowCount == 0, rowDisplayCount > 0` — same intent, derived from the existing cachedRowCount instead of the dropped identity. Without the early-return, every SwiftUI body re-eval runs updateNSView's full body. After previous Phase D + the cachedTableRows / sort-cache / FK-routing PRs, that body is mostly cheap idempotent assignments and Equatable guards. Smoke-tested cell edit + undo, add row + undo, table switch, FK column display, pagination scroll, sort. Net -165 LOC including the deleted test file.
datlechin
changed the title
![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: 2e59834b42
ℹ️ 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".
|
|
||
| mutate(&self.tabManager.tabs[idx].pagination) | ||
| self.tabManager.tabs[idx].paginationVersion += 1 | ||
| self.pendingScrollToTopAfterReplace = true |
There was a problem hiding this comment.
pendingScrollToTopAfterReplace is a single global flag set during pagination, but it is consumed only when notifyFullReplaceIfActive runs for whichever tab is currently selected. If the user paginates tab A and switches to tab B before A’s query result applies, tab A’s setActiveTableRows path returns early (tabManager.tabs[idx].id == tabId guard fails), so A never gets its scroll-to-top; later, an unrelated full replace on tab B can consume the flag and scroll B unexpectedly. This is a real behavior regression from the old paginationVersion-driven logic for cross-tab timing.
Useful? React with 👍 / 👎.
…spatch tests The original Bool flag could strand if pagination's runQuery failed before setActiveTableRows fired — the flag would stay set, and the next setActiveTableRows from any other code path (undo, FK navigation, multi-statement query) would unexpectedly scroll to top. Changing to Set<UUID> scopes the intent per-tab. Pagination inserts the tabId; notifyFullReplaceIfActive checks via remove() and only dispatches scrollToTop on a matching hit. A stranded entry stays scoped to that one tab and only fires when that tab next receives a full replace — narrow blast radius. Adds three regression tests: - scrollToTopFiresOnPendingFlag: setActiveTableRows on the active tab WITH the flag set dispatches scrollToTop and clears the flag - scrollToTopFlagIsScopedPerTab: a flag set for tab A doesn't fire when tab B is replaced; tab A's entry stays - scrollToTopSkippedWhenFlagAbsent: setActiveTableRows without the flag doesn't scroll
9 tasks
datlechin
added a commit
that referenced
this pull request
Apr 29, 2026
…+ fix delete-row visual regression (#940) * refactor(coordinator): migrate RowOperations to selectedTabAndIndex + fix invalidateCachesForUndoRedo regression Two changes: 1. RowOperations migration. Six methods (addNewRow, deleteSelectedRows, duplicateSelectedRow, handleUndoResult, pasteRows, updateCellInTab) used the selectedTabIndex + bounds-check + tabs[index] pattern; now use selectedTabAndIndex which returns (tab, index) atomically. Three read-only methods (copySelectedRowsToClipboard, copySelectedRowsWithHeaders, copySelectedRowsAsJson) drop the index entirely and use selectedTab. undoInsertRow only needs the tabId so it uses selectedTab?.id. Behavior unchanged. 2. Bug fix discovered during RowOperations smoke test: pressing Delete on an existing row didn't show the red background until the user clicked another row. Phase D-b (PR #938) dropped the versionChanged branch in reloadAndSyncSelection, which used to reload visible rows on any changeManager.reloadVersion bump. invalidateCachesForUndoRedo cleared the displayCache and rebuilt the visualStateCache but never told NSTableView to re-query, so visible cell views kept rendering the pre-change state. Adding a reloadData(forRowIndexes: visibleRows, columnIndexes: allCols) call after the cache invalidation restores the lost behavior. Same root cause covers undo/redo of cell edits, deletes, and inserts. * test+refactor: lock invalidateCachesForUndoRedo dispatch, tighten copy/undo paths Two follow-ups to the review: 1. Read-only copy paths (copySelectedRowsToClipboard, copySelectedRowsWithHeaders, copySelectedRowsAsJson) and undoInsertRow now use selectedTabAndIndex instead of selectedTab. selectedTab has a fallback to tabs.first when selectedTabId is nil — the old selectedTabIndex pattern did not. Switching to selectedTabAndIndex (with _ for unused index) restores strict equivalence with the pre-refactor behavior. Closes the subtle init-transient divergence the review flagged. 2. New RowOperationsDispatchTests covers the dispatch wiring restored by this PR's bug fix: - softDeleteDispatchesInvalidate: deleteSelectedRows on existing rows fires invalidateCachesForUndoRedo (the path that triggers the visible-rows reload). Without this, a future refactor could silently re-drop the reload (as Phase D-b accidentally did). - physicalDeleteDispatchesDelta: deleting an inserted row fires applyDelta and bypasses invalidate — guards the branching in deleteSelectedRows so the two paths don't get tangled.
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
Completes the Phase D arc: removes the entire SwiftUI counter-driven reload bridge from the data grid. The
metadataChanged(FK column reload) andpaginationChanged(scroll-to-top) branches inreloadAndSyncSelectionwere the last consumers ofDataGridIdentity, and they're now explicit dispatch calls onTableViewCoordinating. With both gone,DataGridIdentityhas no consumers and the early-return short-circuit goes with it.Net −165 LOC including the deleted
DataGridIdentityTests(no longer applicable).What changes
New
TableViewCoordinatingmethodsrefreshForeignKeyColumns()— reloads visible FK column cells. Logic lifted verbatim from the oldmetadataChangedbranch.scrollToTop()—tableView.scrollRowToVisible(0)if any rows exist.Dispatch wiring
MainContentCoordinator+QueryHelpers.swift:495(thecolumnEnumValuesenrichment that bumpsmetadataVersion) now also callsrefreshForeignKeyColumns()if the affected tab is active. The othermetadataVersionbump (line 290 insetupResultsForExecutedQuery) is followed immediately bysetActiveTableRows, which already dispatchesapplyFullReplaceand renders FK columns fresh — no extra dispatch needed there.paginateAfterConfirmationbumpspaginationVersionBEFORErunQueryfires; the scroll has to wait until the new data lands. AddedpendingScrollToTopAfterReplaceflag onMainContentCoordinator.paginateAfterConfirmationsets it;notifyFullReplaceIfActivechecks and clears it afterapplyFullReplacefires.Deletions
struct DataGridIdentity(25 LOC)var lastIdentityonTableViewCoordinatorcurrentIdentity == coordinator.lastIdentityearly-return short-circuit inupdateNSViewpreviousIdentitycapture,metadataChanged/paginationChangedvariablesmetadataVersion,paginationVersion,schemaVersionprops onDataGridView(no remaining readers)MainEditorContentView.dataGridViewandTableStructureViewlastIdentity = nilwrites inapplyInsertedRows/applyRemovedRows/applyFullReplaceDataGridIdentityTests.swift(entire file, 105 LOC)Subtle preserved behavior
The "initial pre-warm display cache" check (was
previousIdentity == nil || previousIdentity?.rowCount == 0) becomesoldRowCount == 0, rowDisplayCount > 0— same intent, derived from the existingcachedRowCountinstead of the dropped identity.Performance note
Without the early-return, every SwiftUI body re-eval runs
updateNSView's full body. After the previous Phase D +cachedTableRowscollapse + sort-cache merge + FK paste-routing PRs, that body is mostly cheap idempotent assignments and Equatable guards (syncDisplayFormats,rebuildVisualStateCacheis version-guarded,updateColumnsonly runs ifcolumnsChanged, etc.). For typical tables this is microseconds per re-eval — cost is negligible compared to what the early-return was protecting against.Test plan
Phase D recap
This closes out the multi-PR arc that started with PR #933:
reloadVersion/lastReloadVersioncachedTableRowsmirrorresolvedTableRowshelperslastIdentity+ version-counter bridgeThe data grid no longer has any SwiftUI counter machinery for cell edits, row inserts/removals, FK metadata refreshes, or pagination scrolls. All four are explicit dispatches on
TableViewCoordinating. Tab switches still dispatch throughsetActiveTableRows→applyFullReplace. Sort changes dispatch throughdataGridDidReplaceAllRows.