fix(editor): make result pinning reachable for a single query result (#1855)#1856
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 7d6808f888
ℹ️ 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".
| if let lastPinned = tabManager.tabs[tabIdx].display.resultSets.last(where: \.isPinned) { | ||
| switchActiveResultSet(to: lastPinned.id, in: tabId) | ||
| tabManager.mutate(at: tabIdx) { $0.display.removeUnpinnedResults() } | ||
| return |
There was a problem hiding this comment.
When a query tab has any pinned result, this branch switches to the pinned result and returns before clearing tab.execution.errorMessage/lastExecutedAt/status fields, unlike the unpinned-only path below. If a user pins a result, runs a single-statement query that fails (handleQueryExecutionError stores the error on tab.execution without adding a ResultSet), and then chooses Clear Results, the error banner remains because executionErrorBanner falls back to tab.execution.errorMessage after the active pinned result has no error. Please clear the same execution fields in the pinned branch as well.
Useful? React with 👍 / 👎.
1 participant
Closes #1855.
The problem
Result pinning already shipped in v0.27.0.
ResultSet.isPinnedexists and every execution path already preserves pinned results. But the only way to pin was a context menu on the result tab bar, and that bar only rendered when a tab had 2+ results.So you had to pin before running the next query, yet you could only pin once you already had two results. For a single-statement query the pin was unreachable. That is why the reporter concluded the feature did not exist, and why their only workaround was batching statements into one execution.
The fix
The result tab bar now shows for every query result, so the pin, the label, and the close button are always reachable. Table tabs are unchanged and keep their current look.
Pin is also a real command now: Pin Result / Unpin Result in the View menu, bound to
Cmd+Option+P, with a state-reflecting title. The HIG requires this: a command that lives only in a context menu is unreachable to Full Keyboard Access. Hovering a result tab shows the SQL that produced it, so a pinned result says which query it came from.Under the hood
The
pinned + newResultsdestination rule was copy-pasted across three execution paths. It is now a singleTabDisplayState.replaceUnpinnedResults(with:), directly unit-tested including the case where every result is pinned. That duplication is how DBeaver shipped "pinned tab gets overwritten anyway" three separate times (dbeaver#35072, dbeaver#35238, dbeaver#10280); the rule needs one home and a total answer for where the next result lands.Latent bugs this surfaced, also fixed
DataChangeManagercould have generated UPDATEs against the wrong table. That was a data-corruption path.One ordering detail worth flagging for review:
switchActiveResultSetwrites the registry's current rows back into the outgoing result set, so removing the unpinned results before switching would write the cleared rows into the pinned one. Clear switches first, then removes.clearKeepsPinnedResultAndRowsguards exactly that.Design notes
macOS has no HIG pattern for pinning, but Safari and Notes fix the vocabulary (Pin / Unpin) and Xcode's preview-vs-permanent tab is the behavioural precedent: the unpinned pane is the replaceable slot, and pinning promotes out of it. DataGrip, DBeaver, MySQL Workbench, and TablePlus all converge on a per-result-tab pin toggle, and all of them show a result tab even for a single result.
Tests
New
ResultPinningTests(10 tests): the destination rule keeps pinned results, appends new ones, and retargets the active result, including when every result is pinned; Clear Results keeps a pinned result and its rows; closing a pinned result is refused; pin toggling and gating; a pinned result blocks tab reuse; reusing a tab for another table drops its result sets.108 tests pass across the affected suites (
ResultPinningTests,ClearQueryResultsTests,SidebarNavigationResultTests,ReplaceTabContentDefaultSortResetTests,OpenTableTabTests,MultiConnectionNavigationTests,MainContentCoordinatorTabSwitchTests,QueryTabManagerRecordingTests,AIChatInsertQueryTests,SwitchDatabaseTests). SwiftLint--strictis clean on every changed file.Known limitation, not addressed here
Background-tab memory eviction only accounts for the active result set's rows. Non-active result sets, pinned or from a multi-statement run, keep their full
TableRowsoutside the memory budget. That gap predates this change and already applies to multi-statement results; folding it in would have ballooned the scope. Worth a follow-up issue.