Skip to content

fix: resolve 26 anti-patterns across window lifecycle, memory, connections, and SwiftUI#441

Merged
datlechin merged 6 commits intomainfrom
fix/anti-patterns-cleanup
Mar 24, 2026
Merged

fix: resolve 26 anti-patterns across window lifecycle, memory, connections, and SwiftUI#441
datlechin merged 6 commits intomainfrom
fix/anti-patterns-cleanup

Conversation

@datlechin
Copy link
Collaborator

@datlechin datlechin commented Mar 24, 2026

Summary

  • Comprehensive audit identified 35 anti-patterns and incorrect behaviors across the codebase
  • Fixed 26 issues across P0-P3 priorities; 9 documented as acceptable patterns (won't fix)
  • All fixes compile cleanly; 25 files changed, 2 new files added

P0 - Critical (2 fixes)

  • Tab eviction: Skip selected tab during memory eviction so the visible grid doesn't refresh on window re-focus (matches native macOS behavior of Xcode/Safari/TablePlus)
  • Reconnect race: Wrap reconnectDriver in trackOperation so health monitor pings don't race on non-thread-safe drivers

P1 - High (7 fixes)

  • Cancel redisDatabaseSwitchTask in coordinator teardown (task leak)
  • Add deinit to AIChatViewModel to cancel streaming task
  • Replace 5x fire-and-forget SSH tunnel closes with do/catch + warning logs
  • Log schema/database switch errors instead of try? silent discard
  • Re-fetch session for driver disconnect to avoid stale local reference
  • Add WindowAccessor NSViewRepresentable for reliable window capture (replaces fragile NSApp.keyWindow)
  • Use viewWindow from WindowAccessor in command actions setup

P2 - Medium (9 fixes)

  • Fix weak self optional chaining in LicenseManager/AnalyticsService periodic loops
  • Remove imprecise global changeManager.hasChanges pre-check from eviction (per-tab check is sufficient)
  • Add query staleness detection so stuck queries don't permanently mask dead connections
  • Log warning for synchronous plugin loading fallback on main thread
  • Add deinit to AppSettingsManager for accessibility observer cleanup
  • Split clearChanges() / clearChangesAndUndoHistory() for undo stack preservation
  • Make TabDiskActor.save() throw so callers can handle disk errors
  • Remove fragile subtitle-based window matching; trust WindowLifecycleMonitor
  • Use viewDidMoveToWindow for synchronous window capture (eliminates notification timing race)

P3 - Low (8 fixes)

  • Remove stale isKeyWindow snapshot; notification handlers are sole source of truth
  • Centralize window identifier constants with exact matching (no more .contains())
  • Remove unnecessary DispatchQueue.main.async in AppDelegate notification handlers
  • Add warning log for WindowLifecycleMonitor re-registration with different window
  • Extract shared ExponentialBackoff utility (unifies DatabaseManager + HealthMonitor)
  • executeStartupCommands now returns failures via @discardableResult
  • Replace recursive asyncAfter polling with cancellable Task loop

Test plan

  • Open a table tab, switch to another app, wait >5s, switch back — no refresh flicker
  • Open 3+ tabs, switch away >5s, click a background tab — it lazy-loads correctly
  • Open multiple windows simultaneously — each gets correct viewWindow reference
  • Kill database server during query, wait for auto-reconnect, run another query — no crash
  • Build succeeds with no new warnings

Summary by CodeRabbit

  • New Features

    • Undo history now persists when switching between database tables
    • Health monitor now detects queries that exceed configured timeouts and handles long-running queries more reliably
    • Improved window selection and focus handling for connection and welcome windows
  • Bug Fixes

    • Resolved MongoDB Atlas authentication and TLS verification issues for SRV connections
    • Fixed active-tab refresh behavior when returning to the app
    • Improved logging for SSH tunnel closure and schema/database restore failures
  • Reliability

    • Better persistence/error reporting for tab save operations and background tasks

…tions, and SwiftUI

P0 (Critical):
- Skip selected tab during memory eviction to prevent visible refresh on window re-focus
- Wrap reconnectDriver in trackOperation to prevent health monitor race condition

P1 (High):
- Cancel redisDatabaseSwitchTask in coordinator teardown
- Add deinit to AIChatViewModel to cancel streaming task
- Replace 5x fire-and-forget SSH tunnel closes with error logging
- Log schema/database switch errors instead of silently discarding
- Re-fetch session for driver disconnect to avoid stale reference
- Add WindowAccessor NSViewRepresentable for reliable window capture
- Use viewWindow instead of NSApp.keyWindow in command actions

P2 (Medium):
- Fix weak self in LicenseManager/AnalyticsService periodic task loops
- Remove imprecise global changeManager check from eviction scheduling
- Add query staleness detection so stuck queries don't mask dead connections
- Log warning for synchronous plugin loading fallback
- Add deinit to AppSettingsManager for observer cleanup
- Split clearChanges/clearChangesAndUndoHistory for undo preservation
- Make TabDiskActor.save() throw so callers can handle disk errors
- Remove fragile subtitle-based window matching fallback
- Use viewDidMoveToWindow for synchronous window capture

P3 (Low):
- Remove stale isKeyWindow snapshot from configureWindow
- Centralize window identifier constants with exact matching
- Remove unnecessary DispatchQueue.main.async in AppDelegate handlers
- Add warning log for WindowLifecycleMonitor re-registration
- Unify exponential backoff via shared ExponentialBackoff utility
- Return startup command failures from executeStartupCommands
- Replace recursive asyncAfter with cancellable Task loop
@coderabbitai
Copy link

coderabbitai bot commented Mar 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c0b0f98f-688a-43c4-9beb-64ff18658809

📥 Commits

Reviewing files that changed from the base of the PR and between 260f532 and 07d1dbc.

📒 Files selected for processing (2)
  • TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift
🚧 Files skipped from review as they are similar to previous changes (1)
  • TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift

📝 Walkthrough

Walkthrough

Fixes and refinements across connection, window, persistence, and change-tracking code: MongoDB SRV authSource and TLS handling, improved stuck-query detection and SSH tunnel/logging, preserved undo history across tab switches, exact window-identifier matching, throwing tab-disk saves, centralized backoff, and various lifecycle/cleanup fixes.

Changes

Cohort / File(s) Summary
MongoDB Connection & URI Handling
Plugins/MongoDBDriverPlugin/MongoDBConnection.swift
Force authSource = "admin" when SRV (useSrv) and authSource unset; restrict tlsAllowInvalidCertificates=true to sslMode == "Preferred".
Window Identification & Access
TablePro/AppDelegate+WindowConfig.swift, TablePro/Views/Components/WindowAccessor.swift, TablePro/Views/Connection/WelcomeWindowView.swift
Introduce WindowId enum and exact identifier checks; add WindowAccessor (NSViewRepresentable) for synchronous window capture; replace polling with async retry loop for focusing connection-form window.
Database Manager, Health & Driver
TablePro/Core/Database/DatabaseManager.swift, TablePro/Core/Database/ConnectionHealthMonitor.swift, TablePro/Core/Database/DatabaseDriver.swift
Track query start times for stuck-query detection; delegate backoff to ExponentialBackoff; log SSH tunnel closure errors instead of ignoring; allow health pings for long-running queries; synchronous plugin load warning/log when driver missing.
Change Tracking & Undo History
TablePro/Core/ChangeTracking/DataChangeManager.swift, TablePro/Views/Main/... (multiple MainContentCoordinator extensions, MainContentCommandActions.swift, MainContentCoordinator.swift)
Separate undo history from clearChanges(); add clearChangesAndUndoHistory() and update multiple call sites to clear undo history when discarding/saving; preserve selected tab row data in eviction and cancel redis switch task on teardown.
Persistence & Error Propagation
TablePro/Core/Storage/TabDiskActor.swift, TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift
Make TabDiskActor.save(...) throwing and add logSaveError(...); update callers to try/await and forward save errors to the new logger helper.
Backoff & Utilities
TablePro/Core/Utilities/Connection/ExponentialBackoff.swift, TablePro/Core/Services/Infrastructure/AnalyticsService.swift, TablePro/Core/Services/Licensing/LicenseManager.swift, TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift, TablePro/Core/Storage/AppSettingsManager.swift, TablePro/ViewModels/AIChatViewModel.swift
Add ExponentialBackoff API; tighten self-capture in heartbeat/validation loops; add window-mismatch logging; unregister accessibility observer in deinit; mark streaming task as @ObservationIgnored and cancel in deinit.
Main View & Window Wiring
TablePro/Views/Main/MainContentView.swift
Move window registration to .background { WindowAccessor { ... } }, set viewWindow for command actions, simplify disconnect-on-disappear logic, and add configureWindow(_:) helper.
Changelog
CHANGELOG.md
Add Unreleased → Fixed entries documenting MongoDB auth/TLS fixes, undo-history behavior, stuck-query detection, SSH logging, startup command error accumulation, and various UI/behavior fixes.

Sequence Diagram(s)

sequenceDiagram
    participant App as App / UI
    participant DBM as DatabaseManager
    participant Driver as DatabaseDriver / PluginManager
    participant SSH as SSH Tunnel
    participant HB as ConnectionHealthMonitor

    rect rgba(200,200,255,0.5)
    App->>DBM: request reconnect / execute query
    DBM->>DBM: trackOperation(sessionId) (set queryStartTimes)
    DBM->>Driver: createDriver(for:)
    Driver-->>DBM: driver OR loadPendingPlugins() (sync)
    DBM->>SSH: open tunnel (if needed)
    SSH-->>DBM: success / throw (on error -> log warning)
    DBM->>Driver: run query
    Driver-->>DBM: query in-flight / result
    DBM->>HB: health pings (allow if queryStart exceeded timeout)
    HB-->>DBM: ping result / mark stuck
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐰 I hopped through code with nimble paws,

Auth set to admin for SRV because—
TLS kept honest, pings catch delays,
Undo kept safe through tabbing days.
SSH now logs instead of hush—hip-hop hooray! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main focus: fixing 26 anti-patterns across multiple domains (window lifecycle, memory, connections, SwiftUI). It is concise, specific, and directly reflects the changeset's primary objective.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/anti-patterns-cleanup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
TablePro/Views/Main/MainContentCommandActions.swift (1)

377-382: ⚠️ Potential issue | 🟠 Major

Close the owning window, not whichever window is key later.

discardAndClose() still falls through to performClose(), and that helper resolves NSApp.keyWindow instead of this instance's window. If focus changes while the async save/discard prompt is up, the close can hit a different native window tab than the one that initiated the command.

🔧 Suggested direction
-    private func performClose() {
-        guard let keyWindow = NSApp.keyWindow else { return }
-        let tabbedWindows = keyWindow.tabbedWindows ?? [keyWindow]
+    private func performClose() {
+        guard let window else { return }
+        let tabbedWindows = window.tabbedWindows ?? [window]

         if tabbedWindows.count > 1 {
-            keyWindow.close()
+            window.close()
         } else if coordinator?.tabManager.tabs.isEmpty == true {
-            keyWindow.close()
+            window.close()
         } else {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Views/Main/MainContentCommandActions.swift` around lines 377 - 382,
discardAndClose() currently calls performClose(), which uses NSApp.keyWindow so
if focus changes during the async prompt we may close the wrong window; change
the logic so discardAndClose() closes this view's owning window directly (use
self.window?.close() or pass the specific NSWindow instance into performClose)
instead of relying on NSApp.keyWindow; update performClose signature if needed
(e.g., performClose(window: NSWindow?)) and replace any NSApp.keyWindow lookups
with the explicit window parameter or use self.window within discardAndClose to
ensure the originating window/tab is closed.
TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift (1)

231-235: ⚠️ Potential issue | 🟠 Major

Bind post-save cleanup to the tab that started the save.

This runs after async I/O, but it clears changeManager, resets pendingChanges, and re-runs the query using the tab that happens to be selected at completion time. If the user switches tabs while the commit is running, the new tab gets marked clean and loses its undo history even though it was never saved.

🔧 Suggested scoping
+        let savedTabId = tabManager.selectedTabId
         Task { `@MainActor` in
             let overallStartTime = Date()

             do {
                 guard let driver = DatabaseManager.shared.driver(for: connectionId) else {
@@
-                changeManager.clearChangesAndUndoHistory()
-                if let index = tabManager.selectedTabIndex {
+                if tabManager.selectedTabId == savedTabId {
+                    changeManager.clearChangesAndUndoHistory()
+                }
+                if let savedTabId,
+                   let index = tabManager.tabs.firstIndex(where: { $0.id == savedTabId }) {
                     tabManager.tabs[index].pendingChanges = TabPendingChanges()
                     tabManager.tabs[index].errorMessage = nil
                 }
@@
-                if tabManager.selectedTabIndex != nil && !tabManager.tabs.isEmpty {
+                if tabManager.selectedTabId == savedTabId && !tabManager.tabs.isEmpty {
                     runQuery()
                 }

Also applies to: 262-264

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+SaveChanges.swift
around lines 231 - 235, The post-save cleanup is currently using
tabManager.selectedTabIndex which can change during async I/O; instead capture
the originating tab (e.g., store the starting selectedTabIndex or the tab's
unique identifier before awaiting) and perform
changeManager.clearChangesAndUndoHistory() and the resets on
tabManager.tabs[originatingIndex] (or locate by captured tab id) only if that
tab still exists, leaving other tabs untouched; apply the same change to the
other cleanup site (the block around the symbols at lines referenced, e.g., the
second occurrence that currently sets tabManager.tabs[index].pendingChanges and
.errorMessage) so undo history and pendingChanges are tied to the tab that
initiated the save.
🧹 Nitpick comments (4)
TablePro/Views/Components/WindowAccessor.swift (2)

7-17: Add explicit access control modifiers.

Per coding guidelines, types should have explicit access control. The struct WindowAccessor and final class WindowAccessorView should specify their intended visibility.

♻️ Proposed fix
-struct WindowAccessor: NSViewRepresentable {
+internal struct WindowAccessor: NSViewRepresentable {
     var onWindow: (NSWindow) -> Void
 
     func makeNSView(context: Context) -> WindowAccessorView {
-final class WindowAccessorView: NSView {
+internal final class WindowAccessorView: NSView {
     var onWindow: ((NSWindow) -> Void)?

As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Views/Components/WindowAccessor.swift` around lines 7 - 17, Add
explicit access control to the types and their API: declare the struct
WindowAccessor with an explicit access level (e.g., internal or public as
appropriate), and likewise mark final class WindowAccessorView with the chosen
access level; also apply explicit access control to the onWindow property and
the makeNSView/updateNSView methods (and any other members) so the intended
visibility is clear and consistent.

22-27: Consider guarding against multiple invocations.

viewDidMoveToWindow can be called multiple times if the view hierarchy changes (e.g., view moves between windows). The current implementation will invoke onWindow each time. If the callback should only fire once, consider adding a guard.

♻️ Optional: fire callback only once
 internal final class WindowAccessorView: NSView {
     var onWindow: ((NSWindow) -> Void)?
+    private var didCapture = false
 
     override func viewDidMoveToWindow() {
         super.viewDidMoveToWindow()
-        if let window {
+        if let window, !didCapture {
+            didCapture = true
             onWindow?(window)
         }
     }
 }

This is optional — if the callback is idempotent or intentionally supports re-registration, the current behavior is fine.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Views/Components/WindowAccessor.swift` around lines 22 - 27,
viewDidMoveToWindow can be called multiple times and currently always invokes
onWindow; add a guard boolean (e.g., private var hasInvokedOnWindow = false) and
in viewDidMoveToWindow only call onWindow when window != nil &&
!hasInvokedOnWindow, then set hasInvokedOnWindow = true; if you want to allow
re-firing when the view leaves and re-enters a window, clear hasInvokedOnWindow
when window == nil instead of permanently setting it.
TablePro/AppDelegate+WindowConfig.swift (1)

319-324: Inconsistent window matching in closeRestoredMainWindows().

Line 321 still uses .contains("main") for matching, which is inconsistent with the new exact matching approach elsewhere in this file. Consider using WindowId.main for consistency:

Suggested fix
     func closeRestoredMainWindows() {
         DispatchQueue.main.async {
-            for window in NSApp.windows where window.identifier?.rawValue.contains("main") == true {
+            for window in NSApp.windows where window.identifier?.rawValue == WindowId.main {
                 window.close()
             }
         }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/AppDelegate`+WindowConfig.swift around lines 319 - 324, The window
matching in closeRestoredMainWindows() is inconsistent: replace the substring
check using .contains("main") with an exact match to WindowId.main (e.g.,
compare window.identifier?.rawValue == WindowId.main.rawValue or ==
WindowId.main) so it uses the same precise identifier logic as elsewhere; update
the loop in closeRestoredMainWindows() that iterates NSApp.windows to perform
that exact equality check and close matching windows.
TablePro/Core/Utilities/Connection/ExponentialBackoff.swift (1)

4-17: Add explicit access control.

Per coding guidelines, access control should be specified explicitly on types. The enum defaults to internal, but should be explicitly marked.

Suggested fix
 /// Shared exponential backoff calculator for reconnection delays.
-enum ExponentialBackoff {
+internal enum ExponentialBackoff {
     private static let seeds: [TimeInterval] = [2, 4, 8]

As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@TablePro/Core/Utilities/Connection/ExponentialBackoff.swift` around lines 4 -
17, The enum ExponentialBackoff currently has implicit access control;
explicitly declare its access level by changing the type declaration to include
access control (e.g., "internal enum ExponentialBackoff") so the type follows
the project's guideline; keep the existing private static let seeds and the
static func delay as-is (they retain their explicit/private modifiers), ensuring
only the enum declaration is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Around line 10-13: The Unreleased section currently contains MongoDB bullets
that belong to a different PR; update CHANGELOG.md under the [Unreleased]
(Added/Fixed/Changed) section by either removing the two MongoDB bullets
("MongoDB Atlas connections failing to authenticate" and "MongoDB TLS
certificate verification skipped for SRV connections") or replacing them with
this PR's actual notable change(s) (e.g., "window/memory/connection anti-pattern
cleanup" or a succinct summary of the refactor that fixed those anti-patterns);
ensure the entry clearly names the change and the PR number or author per the
repo's changelog guidelines.

In `@Plugins/MongoDBDriverPlugin/MongoDBConnection.swift`:
- Around line 208-213: The change restricts setting
tlsAllowInvalidCertificates=true to sslMode "Preferred" (using sslMode,
sslEnabled, params in MongoDBConnection.swift), which is a security improvement;
add a concise inline comment above the sslEnabled block explaining that
tlsAllowInvalidCertificates is intentionally only set for "Preferred" to avoid
bypassing certificate validation in "Required" (and that users with self-signed
certs should temporarily use "Preferred" or install proper CA certs and migrate
to "Verify CA"/"Verify Identity"), and update any user-facing docs or README to
mention this behavioral change and migration guidance for self-signed
certificate setups.

In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+MultiStatement.swift:
- Around line 137-138: The code is clearing shared change state unconditionally
after a background multi-statement completion; wrap the mutation of
changeManager (calls to clearChangesAndUndoHistory() and incrementing
reloadVersion) in a guard that checks the originating tabId resolved by the task
matches the currently selected/current tab tracked by the
coordinator/changeManager (e.g., compare originatingTabId to
changeManager.currentTab?.id or coordinator.selectedTabId) and only perform the
clear/reload when they match so switching tabs mid-run won't wipe another tab's
undo history.

In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+QueryHelpers.swift:
- Around line 195-197: The code unconditionally calls
changeManager.clearChangesAndUndoHistory() inside applyPhase1Result, which can
clear another tab's undo/redo when a background query finishes; guard this call
so it only runs when the phase1 result applies to the currently visible/selected
tab (e.g., compare the result's target tab identifier or an isSelected flag for
the tab being updated) and only then, and still check isEditable &&
!changeManager.hasChanges before clearing; update applyPhase1Result to perform
that selected-tab check before invoking
changeManager.clearChangesAndUndoHistory().

---

Outside diff comments:
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+SaveChanges.swift:
- Around line 231-235: The post-save cleanup is currently using
tabManager.selectedTabIndex which can change during async I/O; instead capture
the originating tab (e.g., store the starting selectedTabIndex or the tab's
unique identifier before awaiting) and perform
changeManager.clearChangesAndUndoHistory() and the resets on
tabManager.tabs[originatingIndex] (or locate by captured tab id) only if that
tab still exists, leaving other tabs untouched; apply the same change to the
other cleanup site (the block around the symbols at lines referenced, e.g., the
second occurrence that currently sets tabManager.tabs[index].pendingChanges and
.errorMessage) so undo history and pendingChanges are tied to the tab that
initiated the save.

In `@TablePro/Views/Main/MainContentCommandActions.swift`:
- Around line 377-382: discardAndClose() currently calls performClose(), which
uses NSApp.keyWindow so if focus changes during the async prompt we may close
the wrong window; change the logic so discardAndClose() closes this view's
owning window directly (use self.window?.close() or pass the specific NSWindow
instance into performClose) instead of relying on NSApp.keyWindow; update
performClose signature if needed (e.g., performClose(window: NSWindow?)) and
replace any NSApp.keyWindow lookups with the explicit window parameter or use
self.window within discardAndClose to ensure the originating window/tab is
closed.

---

Nitpick comments:
In `@TablePro/AppDelegate`+WindowConfig.swift:
- Around line 319-324: The window matching in closeRestoredMainWindows() is
inconsistent: replace the substring check using .contains("main") with an exact
match to WindowId.main (e.g., compare window.identifier?.rawValue ==
WindowId.main.rawValue or == WindowId.main) so it uses the same precise
identifier logic as elsewhere; update the loop in closeRestoredMainWindows()
that iterates NSApp.windows to perform that exact equality check and close
matching windows.

In `@TablePro/Core/Utilities/Connection/ExponentialBackoff.swift`:
- Around line 4-17: The enum ExponentialBackoff currently has implicit access
control; explicitly declare its access level by changing the type declaration to
include access control (e.g., "internal enum ExponentialBackoff") so the type
follows the project's guideline; keep the existing private static let seeds and
the static func delay as-is (they retain their explicit/private modifiers),
ensuring only the enum declaration is updated.

In `@TablePro/Views/Components/WindowAccessor.swift`:
- Around line 7-17: Add explicit access control to the types and their API:
declare the struct WindowAccessor with an explicit access level (e.g., internal
or public as appropriate), and likewise mark final class WindowAccessorView with
the chosen access level; also apply explicit access control to the onWindow
property and the makeNSView/updateNSView methods (and any other members) so the
intended visibility is clear and consistent.
- Around line 22-27: viewDidMoveToWindow can be called multiple times and
currently always invokes onWindow; add a guard boolean (e.g., private var
hasInvokedOnWindow = false) and in viewDidMoveToWindow only call onWindow when
window != nil && !hasInvokedOnWindow, then set hasInvokedOnWindow = true; if you
want to allow re-firing when the view leaves and re-enters a window, clear
hasInvokedOnWindow when window == nil instead of permanently setting it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 95412b10-51bd-4838-867f-93f159d74c82

📥 Commits

Reviewing files that changed from the base of the PR and between f1f5707 and f7e276e.

📒 Files selected for processing (27)
  • CHANGELOG.md
  • Plugins/MongoDBDriverPlugin/MongoDBConnection.swift
  • TablePro/AppDelegate+WindowConfig.swift
  • TablePro/Core/ChangeTracking/DataChangeManager.swift
  • TablePro/Core/Database/ConnectionHealthMonitor.swift
  • TablePro/Core/Database/DatabaseDriver.swift
  • TablePro/Core/Database/DatabaseManager.swift
  • TablePro/Core/Services/Infrastructure/AnalyticsService.swift
  • TablePro/Core/Services/Infrastructure/TabPersistenceCoordinator.swift
  • TablePro/Core/Services/Infrastructure/WindowLifecycleMonitor.swift
  • TablePro/Core/Services/Licensing/LicenseManager.swift
  • TablePro/Core/Storage/AppSettingsManager.swift
  • TablePro/Core/Storage/TabDiskActor.swift
  • TablePro/Core/Utilities/Connection/ExponentialBackoff.swift
  • TablePro/ViewModels/AIChatViewModel.swift
  • TablePro/Views/Components/WindowAccessor.swift
  • TablePro/Views/Connection/WelcomeWindowView.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+ChangeGuard.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swift
  • TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift
  • TablePro/Views/Main/MainContentCommandActions.swift
  • TablePro/Views/Main/MainContentCoordinator.swift
  • TablePro/Views/Main/MainContentView.swift
  • docs/development/anti-patterns-tracker.md

- WindowAccessor: deduplicate viewDidMoveToWindow calls, update closure in updateNSView
- AIChatViewModel: add comment explaining nonisolated(unsafe) requirement
- CHANGELOG: add user-visible behavioral changes under [Unreleased]
Prevent clearing another tab's undo state when a background query or
multi-statement execution completes after the user has switched tabs.
@datlechin datlechin merged commit 719122b into main Mar 24, 2026
3 checks passed
@datlechin datlechin deleted the fix/anti-patterns-cleanup branch March 24, 2026 02:36
datlechin added a commit that referenced this pull request Mar 24, 2026
… match

SwiftUI appends -AppWindow-N to the WindowGroup id, so window.identifier
is "main-AppWindow-1" not "main". The exact match from PR #441 broke
isMainWindow, preventing tab grouping and window lifecycle handling.
datlechin added a commit that referenced this pull request Mar 24, 2026
* fix: reduce memory retention after closing tabs

- Clear changeManager state and pluginDriver reference in teardown
- Cancel redisDatabaseSwitchTask in teardown
- Clear cachedTableColumnTypes/Names, tableMetadata, filterState in teardown
- Release editor closures and heavy state (tree-sitter, highlighter) on destroy
- Add releaseHeavyState() to TextViewController for early resource cleanup
- Make InMemoryRowProvider.rowBuffer weak with safe fallback
- Add releaseData() to InMemoryRowProvider for explicit cleanup
- Clear tabProviderCache, sortCache, cachedChangeManager in onTeardown
- Hint malloc to return freed pages after disconnect
- Add deinit logging for RowBuffer and QueryTabManager

* fix: set isKeyWindow when WindowAccessor captures window

WindowAccessor.viewDidMoveToWindow fires after didBecomeKeyNotification,
so isKeyWindow was never set to true. This blocked sidebar table clicks
(guarded by isKeyWindow) and caused new tabs to open as separate windows
instead of attaching to the connection's tab group.

* fix: explicitly attach new windows to existing tab group

openWindow creates the window before tabbingIdentifier is set, so macOS
automatic tabbing doesn't group them. Use addTabbedWindow to explicitly
attach new windows to the connection's existing tab group.

* fix: add diagnostic logging for tab grouping

* fix: add NSLog diagnostic for windowDidBecomeKey

* fix: match SwiftUI window identifiers with hasPrefix instead of exact match

SwiftUI appends -AppWindow-N to the WindowGroup id, so window.identifier
is "main-AppWindow-1" not "main". The exact match from PR #441 broke
isMainWindow, preventing tab grouping and window lifecycle handling.

* fix: use prefix matching for window identifiers across all call sites

SwiftUI appends -AppWindow-N to WindowGroup IDs. Fixed remaining spots
that used .contains("main") or exact match:
- closeRestoredMainWindows: use isMainWindow helper
- closeWindows(withId:): prefix match instead of contains
- ContentView notification filter: prefix match instead of contains

* fix: address code review feedback

- Use queue: .main instead of assumeIsolated in DataGridView teardown observer
- Apply hasPrefix matching to isWelcomeWindow and isConnectionFormWindow
- Gate RowBuffer/QueryTabManager deinit logging behind #if DEBUG
- Add CHANGELOG entries for memory, tab grouping, and sidebar fixes

* fix: guard buffer writes against nil to avoid mutating shared emptyBuffer

* docs: simplify changelog entries
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