feat(coordinator): restore session view state and recover after crashes (#1673)#1680
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: aa3e5628da
ℹ️ 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".
| guard let session = DatabaseManager.shared.activeSessions[connection.id], session.isConnected else { return } | ||
| Task { | ||
| if let database, !database.isEmpty, database != session.activeDatabase { | ||
| await coordinator.switchDatabase(to: database) |
There was a problem hiding this comment.
When a restored session has lastActiveDatabase set to a database different from the current session, this calls the normal switchDatabase(to:) path during launch. That path closes sibling native windows and clears tabManager.tabs after the switch, so the async task can wipe out the tabs/windows that were just restored; restoring the saved context needs a path that updates the session/schema without performing the user-facing tab reset.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func applicationWillTerminate(_ notification: Notification) { | ||
| persistOpenConnectionsForRecovery() |
There was a problem hiding this comment.
Saving LastOpenConnections only from applicationWillTerminate does not support the crash/force-quit recovery this change adds: crashes and SIGKILL-style force quits never run this method, so the recovery list remains stale from a previous clean quit. For example, if the user opens a different connection after launch and the app crashes, the periodic tab files may be up to date but startup will not know to reopen that connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b15a5b16
ℹ️ 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".
| WindowManager.shared.openTab( | ||
| payload: EditorTabPayload(connectionId: connectionId, intent: .restoreOrDefault) | ||
| ) |
There was a problem hiding this comment.
In the crash-recovery path where AppKit restored no windows, activeSessions is empty and this only creates a window payload. Unlike TabRouter.openConnection and TabWindowRestoration, nothing calls DatabaseManager.ensureConnected, so MainSplitViewController stays on ConnectingStateView and never builds MainContentView to run restoreFromDisk; recovered sessions after a crash/force-quit therefore sit on the spinner instead of reopening tabs. Please kick off the normal async connection path for each stored connection after opening the window.
Useful? React with 👍 / 👎.
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } | ||
| ) | ||
| .onAppear { coordinator.applyRestoredCursor(for: tab.id) } |
There was a problem hiding this comment.
For restored query tabs with a nonzero cursor offset, this only mutates MainContentCoordinator.cursorPositions after the editor view is constructed. SQLEditorView writes editorState.cursorPositions out to that binding but never reads the binding back into the CodeEdit SourceEditorState or controller, while the coordinator initializes an empty controller cursor to offset 0; this means the saved offset is cleared from the tab without moving the visible editor caret. The restore path needs to drive the editor state/controller before dropping restoredCursorOffset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0696de7f4a
ℹ️ 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".
| intent: .restoreOrDefault | ||
| ) | ||
| RestorationGroupRegistry.register( | ||
| .init(tabs: [tab], selectedTabId: tab.id), |
There was a problem hiding this comment.
For sessions saved from a coordinator that had more than one in-app tab (for example Redis navigation or legacy/custom tab layouts), those tabs are persisted with the same windowGroupIndex, but the restore path wraps each remaining tab in its own WindowGroup here. Because PersistedTab.windowGroupIndex is never consulted, those tabs reopen as separate native windows/tabs instead of in the original window, so the new layout metadata is ineffective.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f156fc3bb8
ℹ️ 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".
| guard defaults.data(forKey: Keys.general) != nil else { return } | ||
| var general = loadGeneral() | ||
| guard general.startupBehavior == .showWelcome else { return } | ||
| general.startupBehavior = .reopenLast |
There was a problem hiding this comment.
For existing installs that already have GeneralSettings saved with .showWelcome—for example because the user explicitly selected it, or changed another general setting while leaving startup behavior alone—this migration rewrites that saved preference to .reopenLast on the next launch. That makes TablePro reopen prior connections instead of showing the welcome screen despite the stored setting; the default flip should only affect installs with no saved general settings, or otherwise distinguish an unset value from an explicit user choice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51d068474c
ℹ️ 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".
| let normalizedSelectedId = nonPreviewTabs.contains(where: { $0.id == selectedTabId }) | ||
| ? selectedTabId : nonPreviewTabs.first?.id | ||
| scheduleSave(tabs: persisted, selectedTabId: normalizedSelectedId) | ||
| let persisted = windowedTabs.map { $0.tab.toPersistedTab(windowGroupIndex: $0.windowGroupIndex) } |
There was a problem hiding this comment.
When persistence runs while a temporary preview tab is open, this now serializes every windowedTabs entry instead of the previous non-preview subset. PersistedTab does not store isPreview, so a preview tab restored from disk comes back as a normal tab; if the window only contains a preview tab, the old saved state is no longer cleared either. Please filter out tab.isPreview before the empty check, persisted map, and selected-tab normalization in both async and sync save paths.
Useful? React with 👍 / 👎.
1 participant
Closes #1673.
Completes session restore so quitting and relaunching brings you back where you left off. TablePro already persisted tabs per connection and hooked AppKit window restoration; this fills the gaps in what was saved and restored, and makes the default behavior reopen the last session.
What's restored now
Connections reconnect lazily on first interaction, so a missing or unreachable database does not block launch.
Changes
PersistedTabgained sort (persisted by column name, resolved back to indices on first load), page, cursor offset, column widths, and a window-group index, with a defensive decoder that also fixes an existing "a missing field drops the whole tab" fragility.TabDiskStaterecords the active database and schema.convertToPersistedTab, which silently droppederDiagramSchemaKeyandqueryParametersfrom every saved tab.toPersistedTab()is now the single serialization path.prepareTableTabFirstLoad, so the first table query already carries them (no double fetch). Cursor applies when the editor appears.LastOpenConnectionsrecovery list, so a crash or force quit still recovers the session (AppKit's restoration archive is only written on a clean quit). The recovery list is read at launch only when AppKit restored nothing, to avoid double-restore.Scope note: multi-window
TablePro merges all of a connection's windows into one native tab group (
WindowManager.openTabalways merges siblings, one tab per coordinator). Restore brings every tab back in order within that group and persists the window-group index, but it does not recreate separately-detached windows. True detached-window restore needs newWindowManagerdetach support and is left as a follow-up.Tests
PersistedTabRoundTripTests(13) cover each new field, the cursor clamp, truncated-query handling, old-file decode,SortDirectioncodable, andwindowGroupIndex.MultiWindowRestorationTests(5) cover the restoration-group registry and the recovery-list storage.Docs
Updated
docs/features/tabs.mdxanddocs/customization/settings.mdx.