Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds preview-tab support: single-click opens a temporary preview tab; double-click or editing promotes it to a permanent tab. Changes touch tab models and persistence, window lifecycle and subtitles, sidebar double‑click detection, navigation/coordinator flows, settings/localization, build config, and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant Sidebar as Sidebar UI
participant Coordinator as MainContentCoordinator
participant TabMgr as QueryTabManager
participant WLM as WindowLifecycleMonitor
participant Window as NSWindow
participant Persist as TabPersistenceCoordinator
User->>Sidebar: Single-click table
Sidebar->>Coordinator: request open (preview mode)
Coordinator->>WLM: previewWindow(for: connectionId)?
alt preview exists
WLM-->>Coordinator: (windowId, window)
Coordinator->>TabMgr: replace preview tab content / select preview tab
else no preview
Coordinator->>Window: create preview window
Coordinator->>WLM: register(window, connectionId, windowId, isPreview: true)
Coordinator->>TabMgr: addPreviewTableTab(tableName)
end
Coordinator->>Window: set subtitle "<name> — Preview"
User->>Sidebar: Double-click preview row
Sidebar->>Coordinator: promotePreviewTab()
Coordinator->>TabMgr: set selectedTab.isPreview = false
Coordinator->>WLM: setPreview(false, for: windowId)
Coordinator->>Window: update subtitle (remove " — Preview")
Coordinator->>Persist: save non-preview tabs (or clear if none)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift (1)
39-50:⚠️ Potential issue | 🟠 MajorPersist only a selected tab that actually survives filtering.
If the active tab is a preview tab,
selectedTabIdis still written even though that tab was removed frompersisted.restoreFromDisk()then hands back a selection that does not exist instate.tabs, which can leave the restored window with no valid selected tab. Normalize the selection againstnonPreviewTabsbefore saving, and reuse the same normalization insaveNowSync.Proposed fix
internal func saveNow(tabs: [QueryTab], selectedTabId: UUID?) { let nonPreviewTabs = tabs.filter { !$0.isPreview } guard !nonPreviewTabs.isEmpty else { clearSavedState() return } let persisted = nonPreviewTabs.map { convertToPersistedTab($0) } let connId = connectionId - let selectedId = selectedTabId + let persistedIds = Set(nonPreviewTabs.map(\.id)) + let selectedId = selectedTabId.flatMap { persistedIds.contains($0) ? $0 : nil } + ?? nonPreviewTabs.first?.id Task { await TabDiskActor.shared.save(connectionId: connId, tabs: persisted, selectedTabId: selectedId) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift` around lines 39 - 50, The saveNow implementation can persist a selectedTabId that was filtered out as a preview; normalize the selection against nonPreviewTabs before saving and apply the same logic in saveNowSync: compute nonPreviewTabs = tabs.filter { !$0.isPreview }, derive persisted = nonPreviewTabs.map { convertToPersistedTab($0) }, then set selectedId to selectedTabId only if it exists in nonPreviewTabs.map { $0.id } (otherwise nil), and pass that normalized selectedId into TabDiskActor.shared.save (and the synchronous saveNowSync path) so restoreFromDisk() will never return a selection that was removed.
🧹 Nitpick comments (1)
TablePro.xcodeproj/project.pbxproj (1)
2478-2522: Build configuration updates look correct, but appear unrelated to PR objectives.The MQLExport Release and SQLImport Debug configurations have properly aligned settings for their respective plugin targets. The configuration blocks correctly reference their own plugin-specific values (Info.plist paths, principal classes, and bundle identifiers).
However, these Xcode project configuration changes don't appear directly related to the "preview tabs" feature described in the PR objectives. Consider clarifying in the PR description why these build configuration updates are included, or splitting them into a separate PR for cleaner change tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro.xcodeproj/project.pbxproj` around lines 2478 - 2522, The build config changes for targets referencing MQLExportPlugin (Release block with INFOPLIST_FILE = Plugins/MQLExportPlugin/Info.plist and INFOPLIST_KEY_NSPrincipalClass = "$(PRODUCT_MODULE_NAME).MQLExportPlugin") and SQLImportPlugin (Debug block with INFOPLIST_FILE = Plugins/SQLImportPlugin/Info.plist and INFOPLIST_KEY_NSPrincipalClass = "$(PRODUCT_MODULE_NAME).SQLImportPlugin") are unrelated to the "preview tabs" feature; either document their purpose in the PR description (explain why Release/Debug buildSettings and PRODUCT_BUNDLE_IDENTIFIER changes for these plugin targets are necessary) or move these XCBuildConfiguration edits into a separate PR so the preview-tabs changes remain focused and reviewable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/AppDelegate.swift`:
- Around line 519-522: Change the brittle hasPrefix check so we match the exact
connection name or require the preview delimiter instead of allowing substrings:
in the loop that computes hasActiveSession
(DatabaseManager.shared.activeSessions.values.contains { ... }), replace
window.subtitle.hasPrefix($0.connection.name) with a safe check such as
window.subtitle == $0.connection.name || window.subtitle ==
"\($0.connection.name) — Preview" (or check for prefix + the exact delimiter
string), using the same preview delimiter used elsewhere so "Prod" won't match
"Production".
In `@TablePro/ContentView.swift`:
- Around line 188-191: The fallback subtitle check erroneously uses
hasPrefix(currentSession?.connection.name ?? ""), which matches when
currentSession is nil and conflates names with shared prefixes; update the
condition in the notification handling to only match when currentSession exists
and the subtitle exactly equals the connection name (e.g., compare
notificationWindow.subtitle == currentSession?.connection.name using optional
binding or a direct optional-equality check) so only the intended window is
considered before WindowLifecycleMonitor registration completes.
- Around line 222-224: The double-click currently calls
sessionState.coordinator.promotePreviewTab(), which promotes the sidebar's
coordinator instead of the actual preview coordinator; change the handler to
locate the preview coordinator (the coordinator created/returned by
MainContentCoordinator.openPreviewTab(...), or the existing preview coordinator
property/method on the main coordinator) and call promotePreviewTab() on that
preview coordinator instance rather than on sessionState.coordinator so the
preview window's tab is promoted correctly.
In `@TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift`:
- Around line 67-72: The early return in saveNowSync(tabs:selectedTabId:) when
nonPreviewTabs.isEmpty preserves a stale persisted session; instead, detect the
no-permanent-tabs case and explicitly clear or overwrite the persisted session
(e.g., write an empty tabs list and nil selectedTabId or call the existing
persistence clear/overwrite routine) so that quitting with only preview tabs
does not restore an earlier session; update the logic in saveNowSync to perform
that clear/overwrite before returning.
In `@TablePro/Models/Settings/AppSettings.swift`:
- Around line 484-486: TabSettings' synthesized Decodable will throw when older
saved settings lack enablePreviewTabs; implement a custom init(from:) on
TabSettings that uses container.decodeIfPresent(Bool.self, forKey:
.enablePreviewTabs) and falls back to true (or to
TabSettings.default.enablePreviewTabs) when nil so older data backfills
enablePreviewTabs correctly; keep the struct Codable/Equatable and maintain the
static `default` value.
In `@TablePro/Resources/Localizable.xcstrings`:
- Around line 6008-6010: The "Enable preview tabs" Localizable.xcstrings entry
is empty and needs Vietnamese (vi) and Simplified Chinese (zh-Hans)
translations; open the "Enable preview tabs" key and add localized string values
for vi and zh-Hans (matching the existing formatting/quoting in
Localizable.xcstrings), and do the same for the related keys referenced
(14366–14368) so those settings’ label/help texts are localized for vi and
zh-Hans as well.
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift:
- Around line 180-186: The code currently uses
WindowLifecycleMonitor.shared.findWindow(for: connectionId) which finds the
first visible window for the connection and may update the wrong window's
subtitle; change this to use a new or existing direct lookup
WindowLifecycleMonitor.shared.window(for: windowId) and set its subtitle to
connection.name (and keep the existing
WindowLifecycleMonitor.shared.setPreview(false, for: wid) call) so the exact
promoted window (identified by windowId) has its preview suffix removed rather
than any connection-wide window found by findWindow(for:).
In `@TablePro/Views/Main/MainContentView.swift`:
- Around line 639-646: persistableTabs removes preview tabs but the saved
selectedTabId (tabManager.selectedTabId) may point to a preview and thus be
invalid on restore; before calling coordinator.persistence.saveNow(tabs:
persistableTabs, selectedTabId: ...), check if tabManager.selectedTabId exists
in persistableTabs (compare IDs) and if not, replace it with
persistableTabs.first?.id (or nil/omit saving if empty), and only call
coordinator.persistence.clearSavedState() when persistableTabs.isEmpty; update
the call site around persistableTabs/newTabs/isPreview and
coordinator.persistence.saveNow to pass the adjusted selectedTabId.
- Around line 251-253: The code uses hasPrefix(connectionName) to detect a
visible window for a session which causes false matches (e.g., "Prod Replica"
matching "Prod"); update the check in the hasVisibleWindow closure to match the
full subtitle instead of prefix-matching — replace
window.subtitle.hasPrefix(connectionName) with a strict equality check (e.g.,
window.subtitle == connectionName) or a full, case-insensitive comparison if
needed; keep the rest of the NSApp.windows.contains closure and variable name
hasVisibleWindow unchanged.
In `@TablePro/Views/Sidebar/DoubleClickDetector.swift`:
- Around line 26-36: SidebarDoubleClickView currently intercepts all mouse
events and prevents the row from receiving single-clicks; update
SidebarDoubleClickView (reference: class SidebarDoubleClickView, method
mouseUp(with:), property onDoubleClick) to only handle double-clicks by either
forwarding non-double-click mouse events to the next responder (call
nextResponder?.mouseUp(with: event) for clickCount != 2) or override hitTest(_:)
to return nil unless the event is a double-click so the overlay doesn't become
the mouse target for single clicks; keep acceptsFirstResponder as false and
ensure onDoubleClick() is invoked only for event.clickCount == 2.
---
Outside diff comments:
In `@TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift`:
- Around line 39-50: The saveNow implementation can persist a selectedTabId that
was filtered out as a preview; normalize the selection against nonPreviewTabs
before saving and apply the same logic in saveNowSync: compute nonPreviewTabs =
tabs.filter { !$0.isPreview }, derive persisted = nonPreviewTabs.map {
convertToPersistedTab($0) }, then set selectedId to selectedTabId only if it
exists in nonPreviewTabs.map { $0.id } (otherwise nil), and pass that normalized
selectedId into TabDiskActor.shared.save (and the synchronous saveNowSync path)
so restoreFromDisk() will never return a selection that was removed.
---
Nitpick comments:
In `@TablePro.xcodeproj/project.pbxproj`:
- Around line 2478-2522: The build config changes for targets referencing
MQLExportPlugin (Release block with INFOPLIST_FILE =
Plugins/MQLExportPlugin/Info.plist and INFOPLIST_KEY_NSPrincipalClass =
"$(PRODUCT_MODULE_NAME).MQLExportPlugin") and SQLImportPlugin (Debug block with
INFOPLIST_FILE = Plugins/SQLImportPlugin/Info.plist and
INFOPLIST_KEY_NSPrincipalClass = "$(PRODUCT_MODULE_NAME).SQLImportPlugin") are
unrelated to the "preview tabs" feature; either document their purpose in the PR
description (explain why Release/Debug buildSettings and
PRODUCT_BUNDLE_IDENTIFIER changes for these plugin targets are necessary) or
move these XCBuildConfiguration edits into a separate PR so the preview-tabs
changes remain focused and reviewable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2694b6d-d145-4a89-b2dd-6c39867cc2bf
📒 Files selected for processing (19)
CHANGELOG.mdTablePro.xcodeproj/project.pbxprojTablePro/AppDelegate.swiftTablePro/ContentView.swiftTablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swiftTablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swiftTablePro/Models/Query/EditorTabPayload.swiftTablePro/Models/Query/QueryTab.swiftTablePro/Models/Settings/AppSettings.swiftTablePro/Resources/Localizable.xcstringsTablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swiftTablePro/Views/Main/MainContentCoordinator.swiftTablePro/Views/Main/MainContentView.swiftTablePro/Views/Main/SidebarNavigationResult.swiftTablePro/Views/Settings/GeneralSettingsView.swiftTablePro/Views/Sidebar/DoubleClickDetector.swiftTablePro/Views/Sidebar/SidebarView.swiftTableProTests/Models/PreviewTabTests.swiftTableProTests/Views/SidebarNavigationResultTests.swift
TablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TablePro/Views/Sidebar/DoubleClickDetector.swift (1)
26-41: Consider a shared monitor for efficiency with many rows.Each
SidebarDoubleClickViewinstance creates its own local event monitor. If the sidebar has many table rows, this results in many active monitors, each filtering the same events. This works correctly but is suboptimal.💡 Alternative: shared monitor approach
Consider a singleton/shared monitor that tracks all registered views and dispatches to the appropriate callback. This would reduce overhead from O(n) monitors to O(1):
final class DoubleClickMonitor { static let shared = DoubleClickMonitor() private var monitor: Any? private var registeredViews = NSHashTable<SidebarDoubleClickView>.weakObjects() func register(_ view: SidebarDoubleClickView) { registeredViews.add(view) installMonitorIfNeeded() } func unregister(_ view: SidebarDoubleClickView) { registeredViews.remove(view) removeMonitorIfEmpty() } // ... dispatch to matching view in handler }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Sidebar/DoubleClickDetector.swift` around lines 26 - 41, Multiple SidebarDoubleClickView instances each install their own NSEvent local monitor in viewDidMoveToWindow (using the monitor property), which is inefficient; refactor by creating a shared singleton (e.g., DoubleClickMonitor.shared) that holds a single NSEvent monitor and an NSHashTable of weak SidebarDoubleClickView references, then change SidebarDoubleClickView to call register(self) in viewDidMoveToWindow when it appears and unregister(self) when removed, and update the shared monitor's event handler to dispatch double-clicks to the appropriate registered view callbacks (onDoubleClick) and to tear down the monitor when the registry is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@TablePro/Views/Sidebar/DoubleClickDetector.swift`:
- Around line 26-41: Multiple SidebarDoubleClickView instances each install
their own NSEvent local monitor in viewDidMoveToWindow (using the monitor
property), which is inefficient; refactor by creating a shared singleton (e.g.,
DoubleClickMonitor.shared) that holds a single NSEvent monitor and an
NSHashTable of weak SidebarDoubleClickView references, then change
SidebarDoubleClickView to call register(self) in viewDidMoveToWindow when it
appears and unregister(self) when removed, and update the shared monitor's event
handler to dispatch double-clicks to the appropriate registered view callbacks
(onDoubleClick) and to tear down the monitor when the registry is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d465b0f4-ebb0-4885-beb0-57652793cbc3
📒 Files selected for processing (2)
TablePro/ContentView.swiftTablePro/Views/Sidebar/DoubleClickDetector.swift
- Replace hasPrefix subtitle matching with exact match to prevent false positives - Use live tab state instead of immutable payload for preview detection on re-registration - Normalize selectedTabId against non-preview tabs in persistence - Clear stale state in saveNowSync instead of skipping - Use exact window(for:) lookup in promotePreviewTab instead of findWindow - Add backward-compatible Codable init for TabSettings - Refactor DoubleClickDetector to use single shared event monitor - Add Vietnamese translations for preview tab settings
- First table click in empty window creates a preview tab when preview mode is enabled - Subsequent clicks replace the preview tab content in-place - Only opens a new native tab when all existing tabs are permanent (promoted)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift:
- Around line 140-172: The race happens because previewWindow(for:) can be nil
until MainContentView registration completes; to fix it, add a connection-scoped
"pending preview" entry when calling WindowOpener.shared.openNativeTab(payload)
and consult that pending entry before deciding to create a new preview: update
the branch that calls WindowLifecycleMonitor.shared.previewWindow(for:
connectionId) to also check a PendingPreviewRegistry (e.g., keyed by
connectionId) and treat a pending windowId/coordinator as an existing preview,
preventing a second open; ensure the pending entry is set immediately before
openNativeTab(payload) is invoked and cleared/updated when the actual
coordinator/window registers (in the same code paths that create
Self.coordinator(for:) or when registration fails/timeouts) so subsequent clicks
correctly reuse the pending/soon-to-be-registered preview.
In `@TablePro/Views/Main/MainContentView.swift`:
- Around line 636-650: The promotion mutates tabManager.tabs but the persistence
logic still uses the pre-promotion snapshot newTabs, causing stale/premature
clears; after calling coordinator.promotePreviewTab() recompute the persistable
set from the live tabManager.tabs (not newTabs) before calling
coordinator.persistence.clearSavedState() or saveNow(...) — alternatively, if
you want the promotion to drive persistence, return early after
promotePreviewTab() and let the subsequent tabManager.onChange handler perform
persistence; update references to newTabs when computing persistableTabs and
normalizedSelectedId to use tabManager.tabs and tabManager.selectedTabId (and
keep calls to coordinator.persistence.clearSavedState() / saveNow(...)
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0addeee-5705-4b2b-9e24-1a78fd281654
📒 Files selected for processing (9)
TablePro/AppDelegate.swiftTablePro/ContentView.swiftTablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swiftTablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swiftTablePro/Models/Settings/AppSettings.swiftTablePro/Resources/Localizable.xcstringsTablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swiftTablePro/Views/Main/MainContentView.swiftTablePro/Views/Sidebar/DoubleClickDetector.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift
- TablePro/AppDelegate.swift
- TablePro/Models/Settings/AppSettings.swift
- TablePro/Resources/Localizable.xcstrings
Comment on lines
+140
to
+172
| // Check if a preview window already exists for this connection | ||
| if let preview = WindowLifecycleMonitor.shared.previewWindow(for: connectionId) { | ||
| if let previewCoordinator = Self.coordinator(for: preview.windowId) { | ||
| previewCoordinator.tabManager.replaceTabContent( | ||
| tableName: tableName, | ||
| databaseType: connection.type, | ||
| isView: isView, | ||
| databaseName: databaseName, | ||
| isPreview: true | ||
| ) | ||
| if let tabIndex = previewCoordinator.tabManager.selectedTabIndex { | ||
| previewCoordinator.tabManager.tabs[tabIndex].showStructure = showStructure | ||
| previewCoordinator.tabManager.tabs[tabIndex].pagination.reset() | ||
| AppState.shared.isCurrentTabEditable = !isView && !tableName.isEmpty | ||
| previewCoordinator.toolbarState.isTableTab = true | ||
| } | ||
| preview.window.makeKeyAndOrderFront(nil) | ||
| previewCoordinator.runQuery() | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // No preview window exists: create one | ||
| let payload = EditorTabPayload( | ||
| connectionId: connection.id, | ||
| tabType: .table, | ||
| tableName: tableName, | ||
| databaseName: databaseName, | ||
| isView: isView, | ||
| showStructure: showStructure, | ||
| isPreview: true | ||
| ) | ||
| WindowOpener.shared.openNativeTab(payload) |
There was a problem hiding this comment.
Preview reuse still races the first window registration.
previewWindow(for:) only becomes non-nil after the new preview window finishes MainContentView registration. A fast second sidebar click can still take the "No preview window exists" path here and open a second preview native tab instead of replacing the first one. ContentView's double-click handler queries the same registry, so the same gap can also leave the fresh preview unpromoted. Track a connection-scoped pending preview coordinator/window id as soon as openNativeTab(...) is requested, and consult that until WindowLifecycleMonitor catches up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift
around lines 140 - 172, The race happens because previewWindow(for:) can be nil
until MainContentView registration completes; to fix it, add a connection-scoped
"pending preview" entry when calling WindowOpener.shared.openNativeTab(payload)
and consult that pending entry before deciding to create a new preview: update
the branch that calls WindowLifecycleMonitor.shared.previewWindow(for:
connectionId) to also check a PendingPreviewRegistry (e.g., keyed by
connectionId) and treat a pending windowId/coordinator as an existing preview,
preventing a second open; ensure the pending entry is set immediately before
openNativeTab(payload) is invoked and cleared/updated when the actual
coordinator/window registers (in the same code paths that create
Self.coordinator(for:) or when registration fails/timeouts) so subsequent clicks
correctly reuse the pending/soon-to-be-registered preview.
Comment on lines
+636
to
+650
| // Promote preview tab if user has interacted with it | ||
| if let tab = tabManager.selectedTab, tab.isPreview, tab.hasUserInteraction { | ||
| coordinator.promotePreviewTab() | ||
| } | ||
|
|
||
| // Persist tab changes (exclude preview tabs from persistence) | ||
| let persistableTabs = newTabs.filter { !$0.isPreview } | ||
| if persistableTabs.isEmpty { | ||
| coordinator.persistence.clearSavedState() | ||
| } else { | ||
| let normalizedSelectedId = persistableTabs.contains(where: { $0.id == tabManager.selectedTabId }) | ||
| ? tabManager.selectedTabId : persistableTabs.first?.id | ||
| coordinator.persistence.saveNow( | ||
| tabs: newTabs, | ||
| selectedTabId: tabManager.selectedTabId | ||
| tabs: persistableTabs, | ||
| selectedTabId: normalizedSelectedId |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Persistence API and call sites =="
rg -n -C 4 'restoreFromDisk\(|saveNow\(|clearSavedState\('
echo
echo "== Persistence implementation =="
fd '.*Persistence.*\.swift$' -x sh -c '
echo "--- $1 ---"
sed -n "1,260p" "$1"
echo
echo "scoping hints:"
rg -n -C 2 "windowId|connectionId|UserDefaults|FileManager|selectedTabId" "$1" || true
' sh {}Repository: datlechin/TablePro
Length of output: 46246
Use live tab state after auto-promotion, not pre-promotion snapshot.
promotePreviewTab() mutates tabManager.tabs, but the persistence block still filters the pre-promotion newTabs snapshot. The first sort/edit on a preview tab therefore persists from stale state, and if all tabs in newTabs remain preview-only, clearSavedState() is called — which wipes saved state for the entire connection, affecting all other windows on that connection before the promotion-triggered onChange arrives. Recompute from tabManager.tabs after promotion, or return early and let the promotion-triggered onChange own persistence.
💾 Suggested fix
- // Persist tab changes (exclude preview tabs from persistence)
- let persistableTabs = newTabs.filter { !$0.isPreview }
+ // Persist tab changes from the live state after any promotion above
+ let persistableTabs = tabManager.tabs.filter { !$0.isPreview }
if persistableTabs.isEmpty {
coordinator.persistence.clearSavedState()
} else {
let normalizedSelectedId = persistableTabs.contains(where: { $0.id == tabManager.selectedTabId })
? tabManager.selectedTabId : persistableTabs.first?.id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Promote preview tab if user has interacted with it | |
| if let tab = tabManager.selectedTab, tab.isPreview, tab.hasUserInteraction { | |
| coordinator.promotePreviewTab() | |
| } | |
| // Persist tab changes (exclude preview tabs from persistence) | |
| let persistableTabs = newTabs.filter { !$0.isPreview } | |
| if persistableTabs.isEmpty { | |
| coordinator.persistence.clearSavedState() | |
| } else { | |
| let normalizedSelectedId = persistableTabs.contains(where: { $0.id == tabManager.selectedTabId }) | |
| ? tabManager.selectedTabId : persistableTabs.first?.id | |
| coordinator.persistence.saveNow( | |
| tabs: newTabs, | |
| selectedTabId: tabManager.selectedTabId | |
| tabs: persistableTabs, | |
| selectedTabId: normalizedSelectedId | |
| // Promote preview tab if user has interacted with it | |
| if let tab = tabManager.selectedTab, tab.isPreview, tab.hasUserInteraction { | |
| coordinator.promotePreviewTab() | |
| } | |
| // Persist tab changes from the live state after any promotion above | |
| let persistableTabs = tabManager.tabs.filter { !$0.isPreview } | |
| if persistableTabs.isEmpty { | |
| coordinator.persistence.clearSavedState() | |
| } else { | |
| let normalizedSelectedId = persistableTabs.contains(where: { $0.id == tabManager.selectedTabId }) | |
| ? tabManager.selectedTabId : persistableTabs.first?.id | |
| coordinator.persistence.saveNow( | |
| tabs: persistableTabs, | |
| selectedTabId: normalizedSelectedId |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Views/Main/MainContentView.swift` around lines 636 - 650, The
promotion mutates tabManager.tabs but the persistence logic still uses the
pre-promotion snapshot newTabs, causing stale/premature clears; after calling
coordinator.promotePreviewTab() recompute the persistable set from the live
tabManager.tabs (not newTabs) before calling
coordinator.persistence.clearSavedState() or saveNow(...) — alternatively, if
you want the promotion to drive persistence, return early after
promotePreviewTab() and let the subsequent tabManager.onChange handler perform
persistence; update references to newTabs when computing persistableTabs and
normalizedSelectedId to use tabManager.tabs and tabManager.selectedTabId (and
keep calls to coordinator.persistence.clearSavedState() / saveNow(...)
unchanged).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swift (1)
67-91:⚠️ Potential issue | 🟡 MinorPropagate
showStructureon the first tab open.When the window is empty, this path creates the tab but never applies
showStructureto it. The first “open structure” action in a fresh window will therefore still open the data view.Suggested fix
if let tabIndex = tabManager.selectedTabIndex { + tabManager.tabs[tabIndex].showStructure = showStructure tabManager.tabs[tabIndex].isView = isView tabManager.tabs[tabIndex].isEditable = !isView tabManager.tabs[tabIndex].pagination.reset() AppState.shared.isCurrentTabEditable = !isView && tableName.isEmpty == false toolbarState.isTableTab = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift around lines 67 - 91, The newly created first tab never receives the incoming showStructure flag; after creating the tab via tabManager.addPreviewTableTab(...) or tabManager.addTableTab(...), when you check tabManager.selectedTabIndex and mutate the created tab, also set tabManager.tabs[tabIndex].showStructure = showStructure (alongside the existing assignments to isView, isEditable, pagination.reset(), etc.) so the initial "open structure" action is applied on the first tab open.
♻️ Duplicate comments (1)
TablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swift (1)
153-205:⚠️ Potential issue | 🟠 MajorPreview reuse can still open duplicate preview windows.
This still has the same registration gap as before:
previewWindow(for:)only becomes non-nilafter the newly opened preview window registers. A fast second click can reach Line 194 and callopenNativeTab(...)again, breaking the “one preview at a time” behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift around lines 153 - 205, The preview reuse race happens because previewWindow(for:) only reports an existing preview after the new window registers, so a fast second click can call WindowOpener.shared.openNativeTab(...) again; fix by introducing a "pending preview" guard (e.g., a Set of connection IDs) in WindowOpener or WindowLifecycleMonitor that is checked before calling openNativeTab and set immediately when you intend to create a preview, and ensure that set is cleared when the window actually registers (WindowLifecycleMonitor registration callback) or if creation fails/when the window closes; update the MainContentCoordinator navigation flow to check/mark that pending set around previewWindow(for:) and WindowOpener.shared.openNativeTab to guarantee only one preview creation in flight per connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift:
- Around line 67-91: The newly created first tab never receives the incoming
showStructure flag; after creating the tab via
tabManager.addPreviewTableTab(...) or tabManager.addTableTab(...), when you
check tabManager.selectedTabIndex and mutate the created tab, also set
tabManager.tabs[tabIndex].showStructure = showStructure (alongside the existing
assignments to isView, isEditable, pagination.reset(), etc.) so the initial
"open structure" action is applied on the first tab open.
---
Duplicate comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+Navigation.swift:
- Around line 153-205: The preview reuse race happens because
previewWindow(for:) only reports an existing preview after the new window
registers, so a fast second click can call
WindowOpener.shared.openNativeTab(...) again; fix by introducing a "pending
preview" guard (e.g., a Set of connection IDs) in WindowOpener or
WindowLifecycleMonitor that is checked before calling openNativeTab and set
immediately when you intend to create a preview, and ensure that set is cleared
when the window actually registers (WindowLifecycleMonitor registration
callback) or if creation fails/when the window closes; update the
MainContentCoordinator navigation flow to check/mark that pending set around
previewWindow(for:) and WindowOpener.shared.openNativeTab to guarantee only one
preview creation in flight per connection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43e2110d-1c88-4e18-85a9-53d83a3d21cc
📒 Files selected for processing (1)
TablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swift
This was referenced Mar 11, 2026
4 tasks
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
window.subtitlematching to usehasPrefixso preview suffix doesn't break connection detectionTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization