Fix/bug

[datlechin]

No description provided.

[Copilot]

Pull request overview

This PR implements comprehensive fixes for tab state persistence and query execution timing issues. The changes address race conditions during window close, prevent duplicate query executions, and ensure reliable tab state restoration across app restarts.

Key Changes:

• Implemented debounced tab state saving with proper cleanup on window close to prevent data loss
• Added lazy loading mechanism for table tabs that waits for database connection before executing queries
• Refactored tab restoration to prioritize disk-persisted state over session state for better reliability across app restarts

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 14 comments.

File	Description
OpenTable/Views/MainContentView.swift	Core logic changes: added state flags for dismissal/restoration tracking, implemented debounced save mechanism, added connection-ready detection, refactored query execution timing and tab restoration
OpenTable/OpenTableApp.swift	Added notification name for window close events
OpenTable/Core/Storage/TabStateStorage.swift	Added debug logging to track empty query saves and aid troubleshooting
OpenTable/AppDelegate.swift	Modified window close handler to post notification and wait for view cleanup before disconnecting

Comments suppressed due to low confidence (5)

OpenTable/Views/MainContentView.swift:427

• This complex retry logic with magic numbers and timing-sensitive behavior lacks comprehensive documentation. The function would benefit from a doc comment explaining: 1) Why tab restoration needs to wait for connection, 2) Why 5 seconds is the chosen timeout, 3) What happens if timeout occurs, and 4) How this interacts with the lazy load mechanism. This would help future maintainers understand the design decisions.

                        
                        // Wait for connection to be established
                        var retryCount = 0
                        while retryCount < 50 { // Max 5 seconds
                            if let session = DatabaseManager.shared.currentSession,
                               session.isConnected {
                                // Small delay to ensure everything is initialized
                                try? await Task.sleep(nanoseconds: 100_000_000) // 0.1s
                                await MainActor.run {
                                    justRestoredTab = true  // Prevent lazy load from executing again
                                    runQuery()
                                }
                                break
                            }
                            
                            // Wait 100ms and retry
                            try? await Task.sleep(nanoseconds: 100_000_000)
                            retryCount += 1
                        }
                        
                        if retryCount >= 50 {
                            print("[MainContentView] ⚠️ Connection timeout, query not executed")
                        }
                    }
                }
            }
            .onChange(of: selectedTables) { oldTables, newTables in
                // Find newly added table to open
                let added = newTables.subtracting(oldTables)

OpenTable/Core/Storage/TabStateStorage.swift:42

• The debug logging prints call stack symbols when an empty query is being saved. However, this only shows the first 10 stack frames and uses a simple forEach with print. Consider using joined(separator: "\n") for better formatted output, or using a proper logging framework that can capture full stack traces. Additionally, this debug code will execute in DEBUG builds for every save, which could impact performance if saves are frequent.

            // Silent failure - encoding errors are rare and non-critical
        }
    }
    
    /// Load tab state for a connection

OpenTable/Views/MainContentView.swift:601

• The debounce task captures the current tabs state (line 586) but doesn't capture the connection ID. If the connection object changes or becomes invalid during the 500ms delay, the save operation could target the wrong connection or fail silently. Consider capturing connection.id as a local constant alongside tabsToSave and selectedId.

                                    
                                    // Only save if not cancelled and view not being dismissed
                                    guard !Task.isCancelled && !isDismissing else { return }
                                    
                                    // Save the captured tabs state (NOT current state which may have changed)
                                    TabStateStorage.shared.saveTabState(
                                        connectionId: connection.id,
                                        tabs: tabsToSave,
                                        selectedTabId: selectedId
                                    )
                                }
                            }
                        }
                    ),
                    cursorPosition: $cursorPosition,
                    onExecute: runQuery,
                    schemaProvider: schemaProvider

OpenTable/Views/MainContentView.swift:426

• If the connection fails to establish within the timeout period (retryCount >= 50), the needsLazyLoad flag is never set and the query is not executed. However, the flag might have been set elsewhere (e.g., in handleTabChange). This means if the connection was never ready during tab restoration but later becomes ready, the query won't execute because needsLazyLoad was never set to true. Consider setting needsLazyLoad = true in the timeout case to allow the onChange handler to retry when the connection eventually becomes ready.

            }
            .onChange(of: selectedTables) { oldTables, newTables in
                // Find newly added table to open

OpenTable/Views/MainContentView.swift:1852

• The code checks for sessionId and the flags isRestoringTabs and isDismissing, but only saves to disk without updating the session. This is inconsistent with the pattern used in other places (e.g., lines 218-221, 243-246) where both session and disk are updated. Either update the session here as well, or remove the sessionId check since disk saving doesn't require an active session.

        }
        
        // Save state to the old tab before switching
        if let oldId = oldTabId,
            let oldIndex = tabManager.tabs.firstIndex(where: { $0.id == oldId })
        {
            // Save pending changes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[datlechin]

@copilot open a new pull request to apply changes based on the comments in this thread

[Copilot]

@datlechin I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you.

[datlechin]

@copilot review pr

[Copilot]

@datlechin I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.