fix(datagrid): keep the query tab when navigating to a foreign key reference - #2018
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
Problem
From a query tab: run a query whose results contain a foreign key column, then click the FK arrow in a cell. The referenced table opens, but the query tab is gone. Its SQL and its results are both lost, and there is no way back to them.
Root cause
navigateToFKReferencedecided "reuse the current tab" vs. "open a new tab" onopenInNewTab || changeManager.hasChanges.changeManager.hasChangesonly tracks pending cell edits, so running aSELECTnever set it. A plain click therefore fell through toQueryTabManager.replaceTabContent, which rewrites the selected tab in place:tabTypebecomes.table,content.queryis overwritten with the generated base-table SELECT, anddisplay.resultSetsandexecution.lastExecutedAtare cleared. The tab keeps its UUID, so nothing in the app sees a tab closing. The work is just overwritten.The predicate that should have stopped this already existed.
QueryTab.holdsQueryWorkcarries the comment "A tab holding query work must not be silently reused in place", andisActiveTabReusablehonours it. FK navigation was the one caller that never asked.Fix
Extract the shared rule into
selectedTabHoldsProtectedContent(pending cell edits, query work, or a committable create-table design) and have bothisActiveTabReusableand the FK path derive from it, instead of two hand-rolled guards that had already drifted apart.isActiveTabReusablekeeps its extra sidebar-only checks (applied filters, user sort, pinned results). Those are deliberately not in the FK gate: FK navigation applies a filter itself, so every table tab reached by a previous FK hop has applied filters, and including them would make chaineda -> b -> cnavigation open a new tab at every hop.Plain-click in-place navigation between table tabs is unchanged. That behaviour is documented in
docs/features/data-grid.mdxand was added deliberately in af0f332 (#1421, #1427), and its test still passes as written. The defect was narrower: replacing a tab that holds work the user authored.Also fixed on the same path
The in-place branch was missing four steps its sibling
reuseActiveTabalready performs:saveLastFilters(for:)- the outgoing table's saved filters were dropped on every hop.cancelTableLoad(for:)- the tab id is reused, so an in-flight load could complete after the swap and race the new content in.restoreLastHiddenColumnsForTable()-replaceTabContentwipescolumnLayout, so saved hidden columns for the target table never came back.promotePreviewTab()- a preview source tab stayed replaceable by a later unrelated click.Repeat clicks
Because a plain FK click from a query tab now opens a tab, clicking the same reference twice would otherwise leave two identical tabs. It now returns to the tab it already opened, matching on table, database, schema and the exact filter predicate. A click on a different referenced row always opens its own tab and never re-filters a tab opened for another row. Cmd-click never reuses.
Testing
swiftlint lint --strictis clean on all changed files. 47 tests pass with no failures (FKNavigationTests,OpenTableTabTests,QueryTabProtectionTests).New:
main)selectedTabHoldsProtectedContentNo existing test's assertions changed. All five pre-existing
FKNavigationTestsand all sixisActiveTabReusabletests pass unmodified, which confirms the extraction is behaviour-preserving. I also verified by inspection that a preview query tab is unreachable (isPreviewis only ever set true alongsidetabType = .table, andPersistedTabdoes not carry it), which is the one case where the rewrite could have diverged.Notes
MainContentCoordinator.openTabInNewWindowis a seam so the decision is testable. The test suite deliberately never callsWindowManager.openTab, because it builds a realTabWindowControllerandNSWindow; its only test file covers the pure statictabbingIdentifier(for:)helper.Two follow-ups, deliberately not in this PR:
openTableTab'snavigationModel == .inPlacebranch is a secondreplaceTabContentcall site with no content guard. It is unreachable from FK navigation (Redis database switching only), but Redis tabs can be query tabs, so the same bug is reachable there. It is a one-line guard now that this predicate exists.Ctrl+left/right, Sequel AceCtrl-Opt-left/right, TablePlus since 6.3.2). Worth considering as a feature.