Skip to content

Restore query tabs on restart, and let Copy Path see them (#463) - #468

Merged
erikdarlingdata merged 1 commit into
devfrom
fix-463-restore-query-tabs
Sep 1, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix-463-restore-query-tabs

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 1, 2026 •

Copy link
Copy Markdown
Owner

Closes #463. Reported by @joshdbe in #458.

What was wrong

Close the app with a plan open and it comes back. Close it with a query open — a query you opened from a .sql file, whose path the app has known since #459 — and it is gone.

GetTabFilePath understood exactly one tab shape:

if (tab.Content is DockPanel dp)
{
    foreach (var child in dp.Children)
        if (child is PlanViewerControl v)
            return v.SourceFilePath;
}
return null;

A plan tab is a DockPanel wrapping a PlanViewerControl. A query tab is a QuerySessionControl with nothing around it, and it carries a perfectly good SourceFilePath. It was never asked. SaveOpenPlans builds its list from this method, so it recorded nothing for query tabs and RestoreOpenPlans had nothing to restore.

Evidence, with only the QuerySessionControl branch removed and everything else in this PR left in place:

AQueryOpenedFromAFileIsWrittenDownForTheNextSession [FAIL]
  Assert.Contains() Failure: Item not found in collection
  Collection: []
  Not found:  "/var/folders/.../k5j2vtoq.sql"

An empty collection, with a query tab open that came from a file.

Copy Path, same cause

Copy Path on the tab context menu is gated on GetTabFilePath(tab) != null, so it has never once appeared on a query tab. That is the same defect, not a second one, and it un-hides with the same three lines. It is also the half that is easy to fix by accident and never actually check, so there is a test that clicks the menu item and reads the path back off the clipboard rather than just asserting IsVisible.

Restore has to route now

The saved list holds .sql paths as well as plans, so restore goes through the existing OpenFileByExtension instead of assuming LoadPlanFile. Sending a query file to LoadPlanFile fails XML validation and puts up an error box where the user's query should have been — and at startup, before the main window is visible, that error box is worse than it sounds:

AQueryFileComesBackAsAQueryTabOnTheNextStart [FAIL]
  System.InvalidOperationException : Cannot show window with non-visible owner.
     at Avalonia.Controls.Window.EnsureParentStateBeforeShow(Window owner)

That is the failure with the routing reverted and everything else in place. ShowFileError has a guard for exactly this; ShowError, which LoadPlanFile uses, does not. Not fixed here — out of scope, and it lives in a file I am staying out of — but worth knowing it is there.

The setting

open_plans is renamed to open_tabs now that it holds both kinds of file. An existing settings file is not silently emptied: the old key deserializes into a migration-only property that is merged into OpenTabs on load and then nulled. Nulls are not serialized, so the old key disappears from disk on the first ordinary save rather than lingering. If both keys somehow exist, the current one wins.

SaveOpenPlans and RestoreOpenPlans keep their names — renaming them means editing MainWindow.axaml.cs, which another PR is in the middle of.

What this deliberately does not do

  • Scratch tabs. A query tab that was never saved has no path, so there is nothing to write down and it does not come back. Persisting unsaved buffers is Warn about unsaved query changes, and mark modified tabs #462. There is a test pinning that boundary so it does not get closed by accident.
  • Copy Path on a tab that acquires a file later. The menu item's visibility is decided once, when the tab is built. Save a scratch query and Copy Path stays hidden on that tab until the next restart. Fixing that means recomputing on menu open, inside the tab-header construction another PR currently owns. Follow-up.

Testing

335 passed / 0 failed / 2 skipped before, 343 / 0 / 2 after. Eight new tests, three of them proved red first by reverting one piece of the fix at a time:

Reverted Goes red
the QuerySessionControl branch in GetTabFilePath AQueryOpenedFromAFileIsWrittenDownForTheNextSession, CopyPathIsOfferedOnAQueryTabAndCopiesTheFile
OpenFileByExtension back to LoadPlanFile in restore AQueryFileComesBackAsAQueryTabOnTheNextStart
the legacy-key migration TabsRecordedUnderTheOldSettingsKeyAreStillRestored, TheCurrentSettingsKeyWinsOverTheOldOne

The other three — a scratch tab records nothing, Copy Path stays hidden with no file, a plan still restores as a plan — pass either way by design. They pin the edges, they do not prove the fix.

The migration was also checked against a real settings file rather than only in a unit test: an appsettings.json was hand-edited back to the pre-#463 shape — one plan path under open_plans, no open_tabs — and the suite run against it. Afterwards the file has open_tabs and no open_plans, and the plan named only under the old key is at the head of recent_plans, which nothing but LoadPlanFile puts it there. The old list was read, opened, and retired.

dotnet test still reports "Zero tests ran" under SDK 10.0.302; that is the pre-existing runner integration problem, not this change. Counts above are from the test executable directly.

🤖 Generated with Claude Code

https://claude.ai/code/session_017xj7HmCKrnsz2PWkRKT2Jx

Plan tabs came back after a restart. Query tabs did not, even when the
query had been opened from a file and the path was sitting on the session
control.

GetTabFilePath knew exactly one tab shape: a DockPanel with a
PlanViewerControl inside it. A query tab is a QuerySessionControl with no
wrapper, and it has had a SourceFilePath of its own since #459. It was
simply never asked, so SaveOpenPlans wrote nothing down and
RestoreOpenPlans had nothing to bring back.

The same blind spot hid Copy Path on the tab context menu, which is shown
only when GetTabFilePath answers. It had never appeared on a query tab.

With both kinds of file in one saved list, restore routes on extension
through the existing OpenFileByExtension instead of assuming a plan.
Handing a .sql file to LoadPlanFile produced an "XML is not valid" box
where the user's query should have been.

The setting is renamed open_plans -> open_tabs now that it holds both. A
file written by the previous version is still read: the old key
deserializes into a migration-only property that is merged into OpenTabs
on load and then nulled, so it drops out of the file on the next save
rather than taking a user's restored tabs with it.

Scope: this restores query tabs that came from a file. A scratch tab that
was never saved has no path and still does not come back; persisting
unsaved buffers is #462.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xj7HmCKrnsz2PWkRKT2Jx
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown

Reviewed. No issues found.

  • GetTabFilePath correctly adds the QuerySessionControl branch, and both call sites that depend on it (CollectOpenTabPaths/SaveOpenPlans and the Copy Path visibility gate in CreateTab) pick it up automatically. Verified session.SourceFilePath is set before CreateTab runs in LoadSqlFile, same as the existing PlanViewerControl ordering, so the gate sees the path at construction time as intended.
  • RestoreOpenPlans routing through OpenFileByExtension instead of LoadPlanFile is correct and doesn't introduce a collection-modification hazard — nothing in OpenFileByExtension/LoadSqlFile/LoadPlanFile/TrackRecentPlan touches _appSettings.OpenTabs while the foreach over it is running.
  • MigrateOpenTabs is ordered ahead of MigrateFormatSettings (which can Save mid-Load) specifically so the old open_plans key doesn't get written back out — confirmed that ordering in AppSettingsService.Load. The "only fill when OpenTabs is empty, always null out LegacyOpenPlans" logic matches the stated behavior (current key wins, old key drops out on next save) and the two migration tests exercise both branches directly.
  • Rename is complete — no leftover references to OpenPlans/open_plans outside the migration path.
  • No T-SQL, no execution-plan-XML parsing, and no version-bump-relevant files touched, so those checklist items don't apply here.

New tests (RestoreQueryTabsTests) cover the fix, the two migration branches, and the deliberately out-of-scope edges (scratch tabs, plan tabs unaffected) called out in the PR description.

@erikdarlingdata
erikdarlingdata merged commit 21a6e10 into dev Sep 1, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix-463-restore-query-tabs branch September 1, 2026 20:44
rferraton pushed a commit to rferraton/PerformanceStudio that referenced this pull request Oct 1, 2026
…rikdarlingdata#490)

Session restore had two gaps, both from writing the list only at clean
close (erikdarlingdata#468's original scope): any abnormal exit -- crash, task kill, an
OS "shut down anyway" past the dirty-tab prompt -- restored zero tabs,
and a file-backed plan or query detached into its own window was never
written down at all, because SaveOpenPlans walked MainTabControl alone.

Membership changes now write the list as they happen, debounced one
second so a burst (restore, Close All) lands as one write. The trigger
rides the erikdarlingdata#481 TabContentWatcher rather than the call sites, for exactly
that commit's reason: a persist remembered at sixteen call sites gets
forgotten at the seventeenth. The two changes the strip cannot show get
explicit calls -- the detached register (a window closing changes no
tab) and SaveQueryToPath (a scratch gaining its first file changes no
membership).

Detached windows join the collected set through a second register next
to erikdarlingdata#473's: the prompt register is query-sessions-only because only an
edit can be lost, while persistence needs every FILE-backed detached
window, plans included. Detached entries append after the docked tabs;
on the next start they come back as ordinary docked tabs, deliberately
not re-detached windows.

The crash-loop defense is kept and sharpened: RestoreOpenPlans clears
and saves the empty list BEFORE the first open (it used to clear after
the loop, which only defended against crashes after restore finished),
and each file that opens successfully re-enters through the debounced
writer, which restore flushes synchronously at its end. Net invariant:
a file that crashes the app during load never persists -- it died before
its own re-add -- while everything that opened does. A crash mid-restore
still loses the tabs opened before it (their re-add was pending, the UI
thread never flushed it); accepted, and said so in the code.

OnClosed keeps its save as the final authoritative write, now with the
debounce timer stopped first so no tick lands in a torn-down window.
Under the test host no real timer is armed at all -- the suite shares
one dispatcher, and a stray tick would write one test's tabs over
another's staged state -- so tests drive the flush through a
deterministic seam and assert the redirected settings file (erikdarlingdata#451/erikdarlingdata#487).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant