Closed
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (59)
📝 WalkthroughWalkthroughThis PR refactors AppState management, converts multiple async operations from synchronous blocking to async/await patterns (SSH verification, theme import), introduces CloudKit conflict resolution with retry logic, migrates notification-driven control flow (window opening, connection forms), and replaces numerous environment injections with singleton access alongside targeted window reference management throughout the codebase. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/ViewModels/AIChatViewModel.swift (1)
346-386:⚠️ Potential issue | 🟠 MajorKeep
streamingTaskfrom retaining the whole view model.
streamingTasklives onself, and this closure now capturesselfstrongly through implicit member access (e.g.,messages,isStreaming,persistCurrentConversation()). This creates a retain cycle—selfretains the task via the property, and the task retainsselfvia the closure capture—that persists until the async stream completes or is cancelled. A slow or stalled provider stream can keep the view model alive indefinitely after the chat panel is dismissed.Use
[weak self]in the task closure to break the cycle, and safely unwrap it when accessing state.Additionally, the cleanup code in the error path clears shared state (
isStreaming,streamingTask,streamingAssistantID) unconditionally. If a new stream is started before an older one completes, the older task's error handler will clobber the new task's state. Tie cleanup to the currentstreamingAssistantIDorstreamingTaskto avoid this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/ViewModels/AIChatViewModel.swift` around lines 346 - 386, The Task closure assigned to streamingTask captures self strongly via implicit member access (messages, isStreaming, persistCurrentConversation(), etc.), creating a retain cycle and risking stale cleanup from old tasks; change the Task to capture [weak self] and early-return if self is nil (or safely unwrap self when accessing members) when iterating the stream, and before mutating shared state (messages, isStreaming, streamingTask, streamingAssistantID, lastMessageFailed, errorMessage) ensure the task is still the active one by comparing either self?.streamingTask to the current Task or self?.streamingAssistantID to the local assistantID so cleanup only affects the matching stream; update both the normal completion block (after the for-await) and the catch block to perform guarded cleanup and call persistCurrentConversation() on the unwrapped self.
🧹 Nitpick comments (12)
TablePro/Views/Filter/FilterRowView.swift (1)
139-144: Optional: extract repeated rounded control styling into a reusable modifier.The same background + rounded clip + rounded stroke block is repeated across menus and text fields. A tiny helper would reduce duplication and make future visual tweaks less error-prone.
♻️ Suggested refactor
+private extension View { + func filterRoundedFieldStyle( + background: Color, + cornerRadius: CGFloat = 4, + stroke: Color = Color(nsColor: .separatorColor), + lineWidth: CGFloat = 0.5 + ) -> some View { + self + .background(background) + .clipShape(RoundedRectangle(cornerRadius: cornerRadius)) + .overlay( + RoundedRectangle(cornerRadius: cornerRadius) + .stroke(stroke, lineWidth: lineWidth) + ) + } +}Then replace repeated blocks like:
-.background(Color(nsColor: .controlBackgroundColor)) -.clipShape(RoundedRectangle(cornerRadius: 4)) -.overlay( - RoundedRectangle(cornerRadius: 4) - .stroke(Color(nsColor: .separatorColor), lineWidth: 0.5) -) +.filterRoundedFieldStyle(background: Color(nsColor: .controlBackgroundColor))Also applies to: 176-181, 204-209, 219-224, 243-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Filter/FilterRowView.swift` around lines 139 - 144, Extract the repeated styling block (background Color(nsColor: .controlBackgroundColor) + clipShape(RoundedRectangle(cornerRadius: 4)) + overlay(RoundedRectangle(cornerRadius: 4).stroke(Color(nsColor: .separatorColor), lineWidth: 0.5))) into a reusable ViewModifier or View extension (e.g., RoundedControlStyle or roundedControlStyle()) and replace the inline blocks in FilterRowView where menus and text fields use it (the occurrences around the menu/text field views currently using clipShape and overlay). Implement the modifier once and apply it to each control so future visual tweaks only change the modifier implementation.TablePro/Views/Editor/SQLEditorView.swift (1)
140-146: Unstructured Task may outlive the view.The unstructured
Task {@mainactorin ... }is not cancelled when the view disappears. If multiple notifications arrive in rapid succession, several Tasks could race, and an older Task completing after a newer one could set stale keywords. The optional chaining oncompletionAdapter?provides nil-safety, but consider whether keyword staleness is acceptable.If ordering matters, you could debounce or cancel in-flight work by storing a
Taskreference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Editor/SQLEditorView.swift` around lines 140 - 146, The unstructured Task in refreshFavoriteKeywords can outlive the view and allow stale results to overwrite newer state; change refreshFavoriteKeywords to store a Task<Void, Never>? property (e.g., favoriteKeywordsTask) on the view, cancel any existing task before creating a new one, then assign the new Task that awaits SQLFavoriteManager.shared.fetchKeywordMap(connectionId: connectionId) and calls completionAdapter?.updateFavoriteKeywords on the main actor; this lets you cancel in-flight work when the view disappears (or before starting a new fetch) and prevents older tasks from racing with newer ones — optionally debounce by delaying before fetch if rapid notifications are expected.TablePro/ViewModels/SidebarViewModel.swift (1)
160-167: RedundantTask {@mainactorin }wrapper on already@MainActorclass.
SidebarViewModelis marked@MainActor(line 56), soonAppear()already runs on the main actor. TheTask {@mainactorin }wrapper is unnecessary—all code within it (driver(for:)lookup, logger calls, andloadTables()) are synchronous. Removing it would reduce nesting without affecting functionality.♻️ Suggested simplification
func onAppear() { guard tables.isEmpty else { Self.logger.debug("onAppear: tables not empty (\(self.tables.count)), skipping") return } - Task { `@MainActor` in - if DatabaseManager.shared.driver(for: connectionId) != nil { - Self.logger.debug("onAppear: driver found, loading tables") - loadTables() - } else { - Self.logger.debug("onAppear: driver is nil for \(self.connectionId.uuidString)") - } + if DatabaseManager.shared.driver(for: connectionId) != nil { + Self.logger.debug("onAppear: driver found, loading tables") + loadTables() + } else { + Self.logger.debug("onAppear: driver is nil for \(self.connectionId.uuidString)") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/ViewModels/SidebarViewModel.swift` around lines 160 - 167, Remove the redundant Task { `@MainActor` in ... } wrapper inside SidebarViewModel's onAppear() since the class is already `@MainActor`; directly execute the synchronous checks and calls (DatabaseManager.shared.driver(for: connectionId), Self.logger.debug(...) and loadTables()) in onAppear() without spawning a Task to reduce nesting and keep behavior identical.TablePro/Models/Connection/DatabaseConnection.swift (1)
365-374: Freeze the persisted key strings explicitly.These keys back serialized
additionalFieldsentries. Right now a case rename/refactor would silently change the stored key because the raw value is inferred from the case name. Please pin each raw value explicitly so storage compatibility does not depend on enum spelling.♻️ Suggested change
private enum AdditionalFieldKey: String { - case mongoAuthSource - case mongoReadPreference - case mongoWriteConcern - case mssqlSchema - case oracleServiceName - case usePgpass - case preConnectScript + case mongoAuthSource = "mongoAuthSource" + case mongoReadPreference = "mongoReadPreference" + case mongoWriteConcern = "mongoWriteConcern" + case mssqlSchema = "mssqlSchema" + case oracleServiceName = "oracleServiceName" + case usePgpass = "usePgpass" + case preConnectScript = "preConnectScript" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Models/Connection/DatabaseConnection.swift` around lines 365 - 374, The AdditionalFieldKey enum's raw values are currently inferred from case names which can break persisted data if cases are renamed; update enum AdditionalFieldKey to assign explicit String raw values for each case (e.g., case mongoAuthSource = "mongoAuthSource", case mongoReadPreference = "mongoReadPreference", case mongoWriteConcern = "mongoWriteConcern", case mssqlSchema = "mssqlSchema", case oracleServiceName = "oracleServiceName", case usePgpass = "usePgpass", case preConnectScript = "preConnectScript") so stored keys remain stable across refactors.TablePro/Core/Sync/SyncMetadataStorage.swift (1)
19-32: Consider logging when falling back to temporary directory.The fallback to
temporaryDirectoryat line 27 should rarely occur on macOS, but if it does, the sync token will be lost on reboot. Adding a log warning would help diagnose unexpected behavior.📝 Proposed logging addition
private let syncTokenFileURL: URL = { guard let appSupport = FileManager.default.urls( for: .applicationSupportDirectory, in: .userDomainMask ).first else { // Fallback to temp directory; should never happen on macOS + Logger(subsystem: "com.TablePro", category: "SyncMetadataStorage") + .warning("Application Support directory unavailable, using temp directory for sync token") return FileManager.default.temporaryDirectory.appendingPathComponent("sync-token.data") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/SyncMetadataStorage.swift` around lines 19 - 32, The syncTokenFileURL lazy closure silently falls back to FileManager.default.temporaryDirectory when the application support directory is unavailable; add a warning log there to surface this rare event. Update the syncTokenFileURL closure (the guard else branch) to call your logger (e.g., processLogger, Logger.shared, or OSLog) and log a clear warning that Application Support was unavailable and the code is using a temporary directory which may lose the sync token on reboot; keep returning the same temporary URL after logging.TablePro/Core/Services/Query/SQLDialectProvider.swift (1)
29-29: DeclareEmptyDialectasinternalexplicitly.The wider visibility makes sense here, but it should be spelled out instead of inherited from Swift’s default access level.
♻️ Proposed fix
-struct EmptyDialect: SQLDialectProvider { +internal struct EmptyDialect: SQLDialectProvider {As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types. Specify on the extension itself, not on individual members."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Query/SQLDialectProvider.swift` at line 29, The struct EmptyDialect currently relies on Swift's default access level; update its declaration to explicitly specify internal access by changing "struct EmptyDialect: SQLDialectProvider" to "internal struct EmptyDialect: SQLDialectProvider" so the visibility is explicit per coding guidelines; locate the EmptyDialect type (conforming to SQLDialectProvider) and add the internal modifier to its declaration.TablePro/Views/Components/WindowAccessor.swift (2)
11-11: Make the helper’s access level explicit.If this is intentionally module-scoped, declare it as
internalinstead of relying on Swift’s default visibility.♻️ Proposed fix
-struct WindowAccessor: NSViewRepresentable { +internal struct WindowAccessor: NSViewRepresentable {As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types. Specify on the extension itself, not on individual members."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Components/WindowAccessor.swift` at line 11, The struct WindowAccessor currently relies on Swift's default (implicit) access level; explicitly declare its access control by adding the desired modifier (e.g., internal) to the type declaration so the helper's scope is clear and follows the guideline to specify access on the type itself; update the declaration of WindowAccessor to include the access specifier (internal/private/public) consistent with its intended module visibility.
11-18: Consider using a callback instead ofBinding<NSWindow?>for better architecture.The current code mitigates the primary retention cycle risk: both
MainContentCoordinatorandMainContentCommandActionsstore the window asweak var window: NSWindow?, preventing the window from retaining itself through the view hierarchy. However, storingNSWindowin@Stateis still fragile and keeps the window alive longer than necessary.A callback-based approach would be cleaner:
- Pass a closure
(NSWindow?) -> VoidtoWindowAccessorinstead of a binding- Call the closure in
updateNSViewto notify the parent view- This avoids storing a window reference in view-owned storage entirely
This improves resilience and makes the ownership model explicit.
🤖 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 11 - 18, Replace the view-owned `@Binding` storage with a callback to avoid storing NSWindow in view state: change WindowAccessor to accept a closure like (NSWindow?) -> Void instead of `@Binding` var window: NSWindow?, keep makeNSView returning NSView(), and in updateNSView(_ nsView: NSView, context: Context) invoke the closure with nsView.window (dispatched on DispatchQueue.main if needed) so the parent receives the window without keeping it in `@State`; update callers to pass a closure that assigns or forwards the window to their weak storage (e.g., MainContentCoordinator/MainContentCommandActions).TablePro/Core/Services/Formatting/SQLFormatterService.swift (1)
108-110: Documentation comment is slightly outdated.The comment states the provider "must be pre-resolved on the main actor," but
resolveDialectProvideris now explicitly thread-safe (per the comment on lines 135-136). Consider updating the doc comment to reflect that callers don't need to worry about actor isolation.📝 Suggested doc update
- /// Get or create the keyword uppercasing regex for a given database type. - /// The dialect provider must be pre-resolved on the main actor before calling this method. + /// Get or create the keyword uppercasing regex for a given database type. private static func keywordRegex(for dialect: DatabaseType, provider: SQLDialectProvider) -> NSRegularExpression? {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Services/Formatting/SQLFormatterService.swift` around lines 108 - 110, Update the doc comment for keywordRegex(for:provider:) to remove the claim that the "dialect provider must be pre-resolved on the main actor" and instead state that callers need not worry about actor isolation because resolveDialectProvider is thread-safe; reference the DatabaseType and SQLDialectProvider parameters and mention resolveDialectProvider to clarify that the provider can be used from any actor or thread when calling keywordRegex.TablePro/Core/Sync/CloudKitSyncEngine.swift (1)
155-172: Silent drop when local record not found.
compactMapsilently returnsnil(line 163) if a conflicted server record has no matching local record. This should be impossible given the flow, but if it occurs, the conflict would be silently ignored. Consider logging a warning for unexpected cases.🔍 Add diagnostic logging
return serverRecords.compactMap { serverRecord in - guard let localRecord = localByID[serverRecord.recordID] else { return nil } + guard let localRecord = localByID[serverRecord.recordID] else { + Self.logger.warning("Conflict resolution: no local record for \(serverRecord.recordID.recordName)") + return nil + } // Copy all locally-set fields onto the server record🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Sync/CloudKitSyncEngine.swift` around lines 155 - 172, The resolveConflicts function silently drops serverRecords with no matching localRecord due to the compactMap guard; modify it to detect this unexpected case and emit a warning instead of silently ignoring the conflict. Specifically, in resolveConflicts (using localByID and serverRecords) replace the compactMap/guard pattern with an explicit iteration (for/forEach or map that preserves nil handling) that logs a warning when localByID[serverRecord.recordID] is nil (include the serverRecord.recordID and any available context) and continues, while still copying fields from localRecord to serverRecord when present.TablePro/Models/UI/FilterSQLPreviewGenerator.swift (1)
12-13: Consider adding explicit access control.Per coding guidelines, access control should be specified explicitly on types. The struct defaults to
internal, but making it explicit improves clarity.♻️ Add explicit access control
/// Generates SQL preview strings from filter state `@MainActor` -struct FilterSQLPreviewGenerator { +internal struct FilterSQLPreviewGenerator {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Models/UI/FilterSQLPreviewGenerator.swift` around lines 12 - 13, The struct FilterSQLPreviewGenerator currently relies on the default internal access level; explicitly declare an access modifier on the type (for example "internal struct FilterSQLPreviewGenerator" or "public struct FilterSQLPreviewGenerator" depending on intended visibility) to follow the coding guideline; update the declaration that begins with "@MainActor struct FilterSQLPreviewGenerator" to include the chosen access control keyword before "struct".TablePro/Theme/ThemeEngine.swift (1)
198-205: Consider capturingselfweakly in the detached task.
Task.detachedcapturesselfstrongly. IfreloadAvailableThemes()is called during teardown or the singleton is somehow replaced, this could cause issues. SinceThemeEngineis a singleton that lives for the app lifetime, this is likely fine in practice, but worth noting.♻️ Optional: Capture self weakly for consistency
func reloadAvailableThemes() { - Task.detached { + Task.detached { [weak self] in let themes = ThemeStorage.loadAllThemes() await MainActor.run { - self.availableThemes = themes + self?.availableThemes = themes } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Theme/ThemeEngine.swift` around lines 198 - 205, The detached Task in reloadAvailableThemes() captures self strongly; change it to capture self weakly by using Task.detached { [weak self] in ... } and immediately unwrap guard let self = self else { return } (or guard let strongSelf = self and use strongSelf) before calling ThemeStorage.loadAllThemes() / await MainActor.run so you avoid retaining ThemeEngine; keep the same calls to ThemeStorage.loadAllThemes() and await MainActor.run { self.availableThemes = themes } after the guard.
🤖 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/Core/Database/DatabaseDriver.swift`:
- Around line 321-325: Update the error handling in WelcomeWindowView and
ConnectionFormView to explicitly catch PluginError.pluginNotLoaded (in the same
catch blocks that currently handle pluginNotInstalled) and implement automatic
retry logic tied to PluginManager.shared.hasFinishedInitialLoad: when
pluginNotLoaded is caught, start a lightweight Task that either awaits a short
delay loop checking PluginManager.shared.hasFinishedInitialLoad before
re-attempting the connection call (or subscribes/observes that flag) or surface
a clear "Retry" UI action that invokes the original connection method; ensure
the retry uses the same method names used to initiate the connection in those
views so behavior is identical on success.
In `@TablePro/Core/Storage/KeychainHelper.swift`:
- Around line 281-298: Before calling SecItemDelete(deleteSyncQuery...) when
kSecAttrSynchronizable is being turned off, present a user confirmation dialog
that clearly states "Turning off iCloud Keychain sync will delete these
credentials from all devices using iCloud Keychain" and requires explicit user
consent; only proceed with constructing deleteSyncQuery and invoking
SecItemDelete if the user confirms, otherwise abort the deletion and leave the
local non‑synchronizable item as-is. Update the Settings UI toggle handler that
flips kSecAttrSynchronizable (the code path that currently performs the
migration and calls deleteSyncQuery/SecItemDelete) to show the dialog and gate
the deletion on the affirmative response, and add a note to the
RELEASE/CHANGELOG documenting this breaking behavior so users are warned in the
release notes.
In `@TablePro/Core/Sync/CloudKitSyncEngine.swift`:
- Around line 120-148: The continuation started in
withCheckedThrowingContinuation (used inside your CloudKit operation wiring
where operation.perRecordSaveBlock and operation.modifyRecordsResultBlock
populate conflicted and call continuation.resume) can be left unresolved if the
process is suspended/terminated; wrap the continuation usage in a
timeout/cancellation guard so the continuation is always resumed: start a Task
that awaits the operation completion but also starts a timeout Task (or use
withThrowingTaskGroup) that, on timeout or Task cancellation, resumes the
continuation with a suitable error (or returns the conflicted array if you
choose a fallback), and ensure you only resume the continuation once (guarding
via a Bool or by checking continuation state) and clean up the operation's
callbacks when timed out; update modifyRecordsResultBlock to early-return if the
continuation has already been resumed.
In `@TablePro/Extensions/Color`+Hex.swift:
- Around line 9-10: The extension declaration for Color is missing an explicit
access level; update the declaration (extension Color) to include the
repository's required access control (for example change it to internal
extension Color { ... }) so the extension itself has an explicit access level
while keeping the existing private static let logger unchanged.
In `@TablePro/ViewModels/AIChatViewModel.swift`:
- Around line 368-387: The post-stream success and error handlers currently
unconditionally clear isStreaming, streamingTask, streamingAssistantID and
update lastMessageFailed/errorMessage and messages, which lets a superseded
stream (A) overwrite state for a newer stream (B); gate all mutations by
verifying the stream identity (e.g. compare streamingAssistantID == assistantID
or compare the Task instance) before doing any teardown, error updates, message
removal, or calling persistCurrentConversation() so only the active stream can
modify state (apply this check inside the success path, the catch block, and any
finally/cleanup code that touches
isStreaming/streamingTask/streamingAssistantID/lastMessageFailed/errorMessage/messages).
In `@TablePro/Views/Connection/WelcomeWindowView.swift`:
- Around line 138-155: The rename alert silently returns on validation failures
(empty or duplicate name) because the alert always dismisses; replace the inline
.alert("Rename Group", isPresented: $showRenameAlert) flow with a custom
modal/sheet bound to $showRenameAlert (or a dedicated `@State` like
showRenameSheet) that contains the TextField bound to renameText, shows
validation messages when the user taps the Rename button (validate trimmed
renameText against empty string and duplicate using groups and
renameGroupTarget), and only call groupStorage.updateGroup(updated) and refresh
groups = groupStorage.loadGroups() when validation passes; keep the sheet open
and surface inline errors (e.g., errorText state) or disable the Rename button
until validation succeeds so the user receives immediate feedback instead of the
alert dismissing silently.
In `@TablePro/Views/Editor/QueryEditorView.swift`:
- Around line 33-35: The Explain control should derive its enabled state from
the view's bound query text instead of the global AppState flag; replace uses of
AppState.shared.hasQueryText in the view body with a computed check against the
view's bound queryText (e.g., non-empty/trimmed) so the Explain button/state
reflects the actual editor contents; update the let hasQuery = ... assignment in
var body and any logic that references hasQuery (or
AppState.shared.hasQueryText) to use the local queryText-based check (retain
trimming/whitespace handling as needed).
In `@TablePro/Views/Main/MainContentView.swift`:
- Around line 530-558: The deferred path can reopen tabs unfiltered because
filter-query reconstruction only runs when the session is already connected; fix
by moving the filter-rebuild logic so it always runs before the connection check
(or ensure the reconnect path re-invokes executeTabQueryWhenReady). Concretely,
in executeTabQueryWhenReady(_:) compute filteredQuery via
coordinator.queryBuilder.buildFilteredQuery using tab.filterState.appliedFilters
(and update tabManager.tabs[tabIndex].query) before checking
DatabaseManager.shared.activeSessions[connection.id].isConnected, and keep the
existing call to coordinator.executeTableTabQueryDirectly() only when connected;
alternatively set coordinator.needsLazyLoad to trigger executeTabQueryWhenReady
rather than leaving tab.query unset.
- Around line 469-485: The code always uses the global default restorePageSize
when rebuilding table tabs, causing restored tabs with a saved custom page size
to get rebuilt SQL with the wrong LIMIT; update the loop that builds table
queries (the restoredTabs iteration that calls QueryTab.buildBaseTableQuery) to
read the tab's saved page size (e.g. restoredTabs[i].pagination.pageSize or
restoredTabs[i].pageSize if that is where it is stored) and pass that value as
the pageSize argument, falling back to restorePageSize only if the tab has no
saved page size.
In `@TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift`:
- Around line 159-198: The view’s .onKeyPress handlers (used with selectedIndex,
allItems, switchToSession, connectToSaved and onDismiss) won’t receive events
because the container isn’t focusable; add the .focusable() modifier to the main
container (e.g. the VStack or List that contains these handlers) so the view can
accept keyboard focus and the arrow/return/Ctrl+J/Ctrl+K handlers actually fire.
---
Outside diff comments:
In `@TablePro/ViewModels/AIChatViewModel.swift`:
- Around line 346-386: The Task closure assigned to streamingTask captures self
strongly via implicit member access (messages, isStreaming,
persistCurrentConversation(), etc.), creating a retain cycle and risking stale
cleanup from old tasks; change the Task to capture [weak self] and early-return
if self is nil (or safely unwrap self when accessing members) when iterating the
stream, and before mutating shared state (messages, isStreaming, streamingTask,
streamingAssistantID, lastMessageFailed, errorMessage) ensure the task is still
the active one by comparing either self?.streamingTask to the current Task or
self?.streamingAssistantID to the local assistantID so cleanup only affects the
matching stream; update both the normal completion block (after the for-await)
and the catch block to perform guarded cleanup and call
persistCurrentConversation() on the unwrapped self.
---
Nitpick comments:
In `@TablePro/Core/Services/Formatting/SQLFormatterService.swift`:
- Around line 108-110: Update the doc comment for keywordRegex(for:provider:) to
remove the claim that the "dialect provider must be pre-resolved on the main
actor" and instead state that callers need not worry about actor isolation
because resolveDialectProvider is thread-safe; reference the DatabaseType and
SQLDialectProvider parameters and mention resolveDialectProvider to clarify that
the provider can be used from any actor or thread when calling keywordRegex.
In `@TablePro/Core/Services/Query/SQLDialectProvider.swift`:
- Line 29: The struct EmptyDialect currently relies on Swift's default access
level; update its declaration to explicitly specify internal access by changing
"struct EmptyDialect: SQLDialectProvider" to "internal struct EmptyDialect:
SQLDialectProvider" so the visibility is explicit per coding guidelines; locate
the EmptyDialect type (conforming to SQLDialectProvider) and add the internal
modifier to its declaration.
In `@TablePro/Core/Sync/CloudKitSyncEngine.swift`:
- Around line 155-172: The resolveConflicts function silently drops
serverRecords with no matching localRecord due to the compactMap guard; modify
it to detect this unexpected case and emit a warning instead of silently
ignoring the conflict. Specifically, in resolveConflicts (using localByID and
serverRecords) replace the compactMap/guard pattern with an explicit iteration
(for/forEach or map that preserves nil handling) that logs a warning when
localByID[serverRecord.recordID] is nil (include the serverRecord.recordID and
any available context) and continues, while still copying fields from
localRecord to serverRecord when present.
In `@TablePro/Core/Sync/SyncMetadataStorage.swift`:
- Around line 19-32: The syncTokenFileURL lazy closure silently falls back to
FileManager.default.temporaryDirectory when the application support directory is
unavailable; add a warning log there to surface this rare event. Update the
syncTokenFileURL closure (the guard else branch) to call your logger (e.g.,
processLogger, Logger.shared, or OSLog) and log a clear warning that Application
Support was unavailable and the code is using a temporary directory which may
lose the sync token on reboot; keep returning the same temporary URL after
logging.
In `@TablePro/Models/Connection/DatabaseConnection.swift`:
- Around line 365-374: The AdditionalFieldKey enum's raw values are currently
inferred from case names which can break persisted data if cases are renamed;
update enum AdditionalFieldKey to assign explicit String raw values for each
case (e.g., case mongoAuthSource = "mongoAuthSource", case mongoReadPreference =
"mongoReadPreference", case mongoWriteConcern = "mongoWriteConcern", case
mssqlSchema = "mssqlSchema", case oracleServiceName = "oracleServiceName", case
usePgpass = "usePgpass", case preConnectScript = "preConnectScript") so stored
keys remain stable across refactors.
In `@TablePro/Models/UI/FilterSQLPreviewGenerator.swift`:
- Around line 12-13: The struct FilterSQLPreviewGenerator currently relies on
the default internal access level; explicitly declare an access modifier on the
type (for example "internal struct FilterSQLPreviewGenerator" or "public struct
FilterSQLPreviewGenerator" depending on intended visibility) to follow the
coding guideline; update the declaration that begins with "@MainActor struct
FilterSQLPreviewGenerator" to include the chosen access control keyword before
"struct".
In `@TablePro/Theme/ThemeEngine.swift`:
- Around line 198-205: The detached Task in reloadAvailableThemes() captures
self strongly; change it to capture self weakly by using Task.detached { [weak
self] in ... } and immediately unwrap guard let self = self else { return } (or
guard let strongSelf = self and use strongSelf) before calling
ThemeStorage.loadAllThemes() / await MainActor.run so you avoid retaining
ThemeEngine; keep the same calls to ThemeStorage.loadAllThemes() and await
MainActor.run { self.availableThemes = themes } after the guard.
In `@TablePro/ViewModels/SidebarViewModel.swift`:
- Around line 160-167: Remove the redundant Task { `@MainActor` in ... } wrapper
inside SidebarViewModel's onAppear() since the class is already `@MainActor`;
directly execute the synchronous checks and calls
(DatabaseManager.shared.driver(for: connectionId), Self.logger.debug(...) and
loadTables()) in onAppear() without spawning a Task to reduce nesting and keep
behavior identical.
In `@TablePro/Views/Components/WindowAccessor.swift`:
- Line 11: The struct WindowAccessor currently relies on Swift's default
(implicit) access level; explicitly declare its access control by adding the
desired modifier (e.g., internal) to the type declaration so the helper's scope
is clear and follows the guideline to specify access on the type itself; update
the declaration of WindowAccessor to include the access specifier
(internal/private/public) consistent with its intended module visibility.
- Around line 11-18: Replace the view-owned `@Binding` storage with a callback to
avoid storing NSWindow in view state: change WindowAccessor to accept a closure
like (NSWindow?) -> Void instead of `@Binding` var window: NSWindow?, keep
makeNSView returning NSView(), and in updateNSView(_ nsView: NSView, context:
Context) invoke the closure with nsView.window (dispatched on DispatchQueue.main
if needed) so the parent receives the window without keeping it in `@State`;
update callers to pass a closure that assigns or forwards the window to their
weak storage (e.g., MainContentCoordinator/MainContentCommandActions).
In `@TablePro/Views/Editor/SQLEditorView.swift`:
- Around line 140-146: The unstructured Task in refreshFavoriteKeywords can
outlive the view and allow stale results to overwrite newer state; change
refreshFavoriteKeywords to store a Task<Void, Never>? property (e.g.,
favoriteKeywordsTask) on the view, cancel any existing task before creating a
new one, then assign the new Task that awaits
SQLFavoriteManager.shared.fetchKeywordMap(connectionId: connectionId) and calls
completionAdapter?.updateFavoriteKeywords on the main actor; this lets you
cancel in-flight work when the view disappears (or before starting a new fetch)
and prevents older tasks from racing with newer ones — optionally debounce by
delaying before fetch if rapid notifications are expected.
In `@TablePro/Views/Filter/FilterRowView.swift`:
- Around line 139-144: Extract the repeated styling block (background
Color(nsColor: .controlBackgroundColor) +
clipShape(RoundedRectangle(cornerRadius: 4)) +
overlay(RoundedRectangle(cornerRadius: 4).stroke(Color(nsColor:
.separatorColor), lineWidth: 0.5))) into a reusable ViewModifier or View
extension (e.g., RoundedControlStyle or roundedControlStyle()) and replace the
inline blocks in FilterRowView where menus and text fields use it (the
occurrences around the menu/text field views currently using clipShape and
overlay). Implement the modifier once and apply it to each control so future
visual tweaks only change the modifier implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1726bb2c-ab93-4316-ad5c-c661927219e9
📒 Files selected for processing (68)
TablePro/AppDelegate+ConnectionHandler.swiftTablePro/AppDelegate+FileOpen.swiftTablePro/AppDelegate+WindowConfig.swiftTablePro/AppDelegate.swiftTablePro/ContentView.swiftTablePro/Core/Database/DatabaseDriver.swiftTablePro/Core/Database/DatabaseManager.swiftTablePro/Core/Plugins/PluginError.swiftTablePro/Core/Plugins/PluginManager.swiftTablePro/Core/Plugins/PluginMetadataRegistry.swiftTablePro/Core/SSH/HostKeyVerifier.swiftTablePro/Core/SSH/LibSSH2TunnelFactory.swiftTablePro/Core/SSH/SSHTunnelManager.swiftTablePro/Core/SchemaTracking/StructureChangeManager.swiftTablePro/Core/Services/Export/ExportService.swiftTablePro/Core/Services/Formatting/SQLFormatterService.swiftTablePro/Core/Services/Infrastructure/WindowOpener.swiftTablePro/Core/Services/Query/SQLDialectProvider.swiftTablePro/Core/Storage/KeychainHelper.swiftTablePro/Core/Sync/CloudKitSyncEngine.swiftTablePro/Core/Sync/SyncCoordinator.swiftTablePro/Core/Sync/SyncMetadataStorage.swiftTablePro/Extensions/Color+Hex.swiftTablePro/Extensions/UserDefaults+RecentDatabases.swiftTablePro/Models/Connection/DatabaseConnection.swiftTablePro/Models/Query/QueryResult.swiftTablePro/Models/Query/QueryTab.swiftTablePro/Models/Schema/ColumnDefinition.swiftTablePro/Models/Schema/ForeignKeyDefinition.swiftTablePro/Models/Schema/IndexDefinition.swiftTablePro/Models/Settings/AppSettings.swiftTablePro/Models/UI/FilterSQLPreviewGenerator.swiftTablePro/Models/UI/FilterState.swiftTablePro/Resources/Localizable.xcstringsTablePro/TableProApp.swiftTablePro/Theme/ThemeEngine.swiftTablePro/Theme/ThemeStorage.swiftTablePro/ViewModels/AIChatViewModel.swiftTablePro/ViewModels/SidebarViewModel.swiftTablePro/Views/Components/SectionHeaderView.swiftTablePro/Views/Components/WindowAccessor.swiftTablePro/Views/Connection/WelcomeWindowView.swiftTablePro/Views/DatabaseSwitcher/DatabaseSwitcherSheet.swiftTablePro/Views/Editor/QueryEditorView.swiftTablePro/Views/Editor/SQLEditorCoordinator.swiftTablePro/Views/Editor/SQLEditorView.swiftTablePro/Views/Filter/FilterRowView.swiftTablePro/Views/Filter/SQLPreviewSheet.swiftTablePro/Views/Import/ImportDialog.swiftTablePro/Views/Import/ImportErrorView.swiftTablePro/Views/Main/Child/MainEditorContentView.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+ChangeGuard.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+FKNavigation.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+MultiStatement.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+Refresh.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swiftTablePro/Views/Main/MainContentCommandActions.swiftTablePro/Views/Main/MainContentCoordinator.swiftTablePro/Views/Main/MainContentView.swiftTablePro/Views/QuickSwitcher/QuickSwitcherView.swiftTablePro/Views/Settings/Appearance/ThemeListView.swiftTablePro/Views/Settings/GeneralSettingsView.swiftTablePro/Views/Settings/SettingsView.swiftTablePro/Views/Structure/SchemaPreviewSheet.swiftTablePro/Views/Toolbar/ConnectionSwitcherPopover.swift
💤 Files with no reviewable changes (1)
- TablePro/AppDelegate.swift
Comment on lines
+321
to
+325
| // If background loading hasn't finished yet, throw a specific error | ||
| // instead of blocking the main thread with synchronous plugin loading. | ||
| if !PluginManager.shared.hasFinishedInitialLoad { | ||
| throw PluginError.pluginNotLoaded(pluginId) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for callers of createDriver and check error handling
rg -n -A 10 'DatabaseDriverFactory\.createDriver' --type swiftRepository: datlechin/TablePro
Length of output: 4118
🏁 Script executed:
# Search for PluginError handling and any retry patterns
rg -n -B 5 -A 5 'PluginError' --type swift | head -100Repository: datlechin/TablePro
Length of output: 8392
🏁 Script executed:
# Check if there's specific handling of PluginError.pluginNotLoaded
rg -n 'pluginNotLoaded' --type swiftRepository: datlechin/TablePro
Length of output: 337
🏁 Script executed:
# Look for hasFinishedInitialLoad usage to understand the retry pattern
rg -n 'hasFinishedInitialLoad' --type swift -A 3 -B 3Repository: datlechin/TablePro
Length of output: 4049
🏁 Script executed:
# Check the full connectToSession method to see if there's retry logic
rg -n -B 5 -A 20 'func connectToSession' --type swiftRepository: datlechin/TablePro
Length of output: 2391
🏁 Script executed:
# Look for any retry patterns or plugin loading wait mechanisms
rg -n 'waitFor\|retry\|while.*hasFinishedInitialLoad' --type swiftRepository: datlechin/TablePro
Length of output: 44
🏁 Script executed:
# Check WelcomeWindowView for how it handles plugin errors and if there's a pattern
rg -n -B 10 -A 10 'pluginNotInstalled' --type swiftRepository: datlechin/TablePro
Length of output: 12605
Add explicit pluginNotLoaded error handling to connection UI views.
The change to throw PluginError.pluginNotLoaded instead of blocking is good for avoiding main-thread freezes, but callers currently don't handle this error appropriately.
In both WelcomeWindowView and ConnectionFormView, only pluginNotInstalled is caught explicitly (lines 556, 1271, 1430). When pluginNotLoaded is thrown, it falls through to generic error handling that displays the message "Please try again in a moment" without providing automatic retry.
To match the error message's promise, update these error handlers to:
- Catch
pluginNotLoadedseparately - Automatically retry the connection after
hasFinishedInitialLoadbecomes true (e.g., using a Task with a small delay or observation of the flag), or - Provide a clear "Retry" button to the user
Without this, users see a cryptic error message requiring manual intervention, defeating the purpose of the non-blocking change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Core/Database/DatabaseDriver.swift` around lines 321 - 325, Update
the error handling in WelcomeWindowView and ConnectionFormView to explicitly
catch PluginError.pluginNotLoaded (in the same catch blocks that currently
handle pluginNotInstalled) and implement automatic retry logic tied to
PluginManager.shared.hasFinishedInitialLoad: when pluginNotLoaded is caught,
start a lightweight Task that either awaits a short delay loop checking
PluginManager.shared.hasFinishedInitialLoad before re-attempting the connection
call (or subscribes/observes that flag) or surface a clear "Retry" UI action
that invokes the original connection method; ensure the retry uses the same
method names used to initiate the connection in those views so behavior is
identical on success.
Comment on lines
+281
to
298
| } else { | ||
| // When opting OUT: delete the stale synchronizable copy so it doesn't | ||
| // linger in iCloud Keychain. This will remove the credential from other | ||
| // devices that have iCloud Keychain enabled. | ||
| let deleteSyncQuery: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecAttrAccount as String: account, | ||
| kSecUseDataProtectionKeychain as String: true, | ||
| kSecAttrSynchronizable as String: true | ||
| ] | ||
| let deleteStatus = SecItemDelete(deleteSyncQuery as CFDictionary) | ||
| if deleteStatus != errSecSuccess, deleteStatus != errSecItemNotFound { | ||
| Self.logger.warning( | ||
| "Migrated item '\(account, privacy: .public)' but failed to delete old synchronizable entry: \(deleteStatus)" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the password sync setting UI has appropriate warning text
rg -n -A5 -B5 'passwordSyncEnabled|keychainPasswordSyncEnabled' --type swift -g '!*.generated.swift' | head -80Repository: datlechin/TablePro
Length of output: 4557
🏁 Script executed:
# First, let's find the SyncSettingsView file and check its full content
wc -l TablePro/Views/Settings/SyncSettingsView.swiftRepository: datlechin/TablePro
Length of output: 112
🏁 Script executed:
# Read the SyncSettingsView.swift file to see the UI implementation
cat -n TablePro/Views/Settings/SyncSettingsView.swift | sed -n '180,210p'Repository: datlechin/TablePro
Length of output: 1583
🏁 Script executed:
# Search for any confirmation dialogs or alerts related to password sync or keychain
rg -n 'alert|confirm|confirmation|warning|dialog' --type swift TablePro/Views/Settings/ | grep -i 'password\|sync\|keychain' || echo "No alerts found with those keywords"Repository: datlechin/TablePro
Length of output: 97
🏁 Script executed:
# More broadly search for presentationMode, sheet, or alert modifiers in SyncSettingsView
rg -n 'presentationMode|\.sheet|\.alert|\.confirmationDialog' --type swift TablePro/Views/Settings/SyncSettingsView.swiftRepository: datlechin/TablePro
Length of output: 44
🏁 Script executed:
# Read the entire SyncSettingsView.swift to see the UI implementation
cat -n TablePro/Views/Settings/SyncSettingsView.swiftRepository: datlechin/TablePro
Length of output: 9300
🏁 Script executed:
# Search for changelog or release notes files
fd -t f -i 'changelog|release|news|history' . | head -20Repository: datlechin/TablePro
Length of output: 849
🏁 Script executed:
# Also search for any documentation about this breaking change
rg -n 'delete.*keychain|remove.*iCloud|opt.out.*password' --type md --type txt | head -20Repository: datlechin/TablePro
Length of output: 44
🏁 Script executed:
# Check the CHANGELOG.md for any entries about password sync or keychain behavior changes
head -100 CHANGELOG.mdRepository: datlechin/TablePro
Length of output: 4189
Add user confirmation before silently deleting iCloud Keychain passwords on opt-out.
The code at lines 285-296 deletes synchronizable keychain entries when users toggle off password sync. This will remove credentials from all other devices with iCloud Keychain enabled, but users are not warned or asked to confirm.
Current state:
- Settings UI (line 125) only states: "Syncs passwords via iCloud Keychain (end-to-end encrypted)"
- No confirmation dialog before deletion
- Behavior change not documented in CHANGELOG.md
Required:
- Add a confirmation dialog when toggling password sync off, explaining that this will delete credentials from all synced devices
- Document this breaking behavior in release notes
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Core/Storage/KeychainHelper.swift` around lines 281 - 298, Before
calling SecItemDelete(deleteSyncQuery...) when kSecAttrSynchronizable is being
turned off, present a user confirmation dialog that clearly states "Turning off
iCloud Keychain sync will delete these credentials from all devices using iCloud
Keychain" and requires explicit user consent; only proceed with constructing
deleteSyncQuery and invoking SecItemDelete if the user confirms, otherwise abort
the deletion and leave the local non‑synchronizable item as-is. Update the
Settings UI toggle handler that flips kSecAttrSynchronizable (the code path that
currently performs the migration and calls deleteSyncQuery/SecItemDelete) to
show the dialog and gate the deletion on the affirmative response, and add a
note to the RELEASE/CHANGELOG documenting this breaking behavior so users are
warned in the release notes.
Comment on lines
120
to
148
| return try await withCheckedThrowingContinuation { continuation in | ||
| var conflicted: [CKRecord] = [] | ||
|
|
||
| operation.perRecordSaveBlock = { recordID, result in | ||
| if case .failure(let error) = result { | ||
| Self.logger.error( | ||
| "Failed to save record \(recordID.recordName): \(error.localizedDescription)" | ||
| ) | ||
| if let ckError = error as? CKError, | ||
| ckError.code == .serverRecordChanged, | ||
| let serverRecord = ckError.serverRecord { | ||
| conflicted.append(serverRecord) | ||
| } else { | ||
| Self.logger.error( | ||
| "Failed to save record \(recordID.recordName): \(error.localizedDescription)" | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| operation.modifyRecordsResultBlock = { result in | ||
| switch result { | ||
| case .success: | ||
| continuation.resume() | ||
| continuation.resume(returning: conflicted) | ||
| case .failure(let error): | ||
| continuation.resume(throwing: error) | ||
| // If the overall operation failed but we have conflicts, return them | ||
| if !conflicted.isEmpty { | ||
| continuation.resume(returning: conflicted) | ||
| } else { | ||
| continuation.resume(throwing: error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CloudKit CKModifyRecordsOperation modifyRecordsResultBlock guaranteed to be called
💡 Result:
CKModifyRecordsOperation.modifyRecordsResultBlock (and the deprecated modifyRecordsCompletionBlock) is intended to be called exactly once as your “final chance” to handle the operation’s outcome after the operation finishes running (success, partial failure, or failure). Apple’s guidance for CloudKit operations also explicitly says your completion handling must cope with the operation not completing its task due to error or explicit cancellation (i.e., cancellation is still an outcome you should expect to be reported via completion). [1], [2]
It is not guaranteed to be called in situations where the operation never gets to finish in your process, e.g.:
- the app/process is suspended/terminated before the operation runs to completion (common with background execution limits / debugger / calling the background fetch completion handler too early). [4]
- the operation is meant to finish after termination: in that case you need long‑lived operations (
isLongLived = true) and then fetch the operation later to receive the cached result after relaunch. [3]
Sources:
[1] https://spec-zone.ru/RU/OSX/documentation/CloudKit/Reference/CKModifyRecordsOperation_class/index.html
[2] https://stackoverflow.com/questions/31936488/ckmodifyrecordsoperation-completion-block-operates-to-early
[3] https://mackuba.eu/notes/wwdc16/cloudkit-best-practices/
[4] https://stackoverflow.com/questions/35859115/cloudkit-ckoperations-dont-run-when-app-is-suspended
🏁 Script executed:
# Verify the exact context of the CloudKitSyncEngine to understand if suspension scenarios apply
cd TablePro && fd CloudKitSyncEngine.swiftRepository: datlechin/TablePro
Length of output: 96
🏁 Script executed:
# Read the CloudKitSyncEngine.swift file to understand full context around lines 120-148
wc -l TablePro/Core/Sync/CloudKitSyncEngine.swiftRepository: datlechin/TablePro
Length of output: 109
🏁 Script executed:
# Get the function containing lines 120-148 and surrounding context
head -160 TablePro/Core/Sync/CloudKitSyncEngine.swift | tail -80 | cat -nRepository: datlechin/TablePro
Length of output: 3969
🏁 Script executed:
# Search for timeout, Task timeout, or task cancellation patterns in this file
rg -i "timeout|task.*cancel|withTimeout|timeout.*continuation" TablePro/Core/Sync/CloudKitSyncEngine.swift -A 2 -B 2Repository: datlechin/TablePro
Length of output: 44
🏁 Script executed:
# Check where this function is called and in what context
rg "withCheckedThrowingContinuation" TablePro/Core/Sync/CloudKitSyncEngine.swift -B 10 -A 30 | head -100Repository: datlechin/TablePro
Length of output: 3554
🏁 Script executed:
# Find the withRetry function to see if it has timeout logic
rg "func withRetry" TablePro/Core/Sync/CloudKitSyncEngine.swift -A 15Repository: datlechin/TablePro
Length of output: 754
🏁 Script executed:
# Check if there's any Task cancellation or timeout handling in the class
rg "Task\." TablePro/Core/Sync/CloudKitSyncEngine.swift | head -20Repository: datlechin/TablePro
Length of output: 120
🏁 Script executed:
# Check the class definition and properties for any timeout/cancellation config
head -50 TablePro/Core/Sync/CloudKitSyncEngine.swift | cat -nRepository: datlechin/TablePro
Length of output: 1817
Add timeout handling for modifyRecordsResultBlock invocation.
modifyRecordsResultBlock is guaranteed to be called exactly once when the operation completes normally. However, CloudKit does not guarantee invocation if the app is suspended or terminated before the operation finishes. The current code lacks timeout or cancellation safeguards, risking indefinite continuation suspension and resource leaks in background sync contexts.
Wrap withCheckedThrowingContinuation with a timeout task (e.g., try await withThrowingTaskGroup or explicit Task cancellation) to ensure the continuation always resolves, even if the operation never completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Core/Sync/CloudKitSyncEngine.swift` around lines 120 - 148, The
continuation started in withCheckedThrowingContinuation (used inside your
CloudKit operation wiring where operation.perRecordSaveBlock and
operation.modifyRecordsResultBlock populate conflicted and call
continuation.resume) can be left unresolved if the process is
suspended/terminated; wrap the continuation usage in a timeout/cancellation
guard so the continuation is always resumed: start a Task that awaits the
operation completion but also starts a timeout Task (or use
withThrowingTaskGroup) that, on timeout or Task cancellation, resumes the
continuation with a suitable error (or returns the conflicted array if you
choose a fallback), and ensure you only resume the continuation once (guarding
via a Bool or by checking continuation state) and clean up the operation's
callbacks when timed out; update modifyRecordsResultBlock to early-return if the
continuation has already been resumed.
Comment on lines
9
to
+10
| extension Color { | ||
| private static let logger = Logger(subsystem: "com.TablePro", category: "Color+Hex") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit access control to the extension declaration.
Please declare access level on the extension itself (e.g., internal extension Color { ... }) to match repo rules.
Suggested patch
-extension Color {
+internal extension Color {
private static let logger = Logger(subsystem: "com.TablePro", category: "Color+Hex")As per coding guidelines, "Always specify access control explicitly (private, internal, public) on extensions and types. Specify on the extension itself, not on individual members."
📝 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
| extension Color { | |
| private static let logger = Logger(subsystem: "com.TablePro", category: "Color+Hex") | |
| internal extension Color { | |
| private static let logger = Logger(subsystem: "com.TablePro", category: "Color+Hex") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Extensions/Color`+Hex.swift around lines 9 - 10, The extension
declaration for Color is missing an explicit access level; update the
declaration (extension Color) to include the repository's required access
control (for example change it to internal extension Color { ... }) so the
extension itself has an explicit access level while keeping the existing private
static let logger unchanged.
Comment on lines
+368
to
387
| isStreaming = false | ||
| streamingTask = nil | ||
| streamingAssistantID = nil | ||
| persistCurrentConversation() | ||
| } catch { | ||
| if !Task.isCancelled { | ||
| Self.logger.error("Streaming failed: \(error.localizedDescription)") | ||
| self.lastMessageFailed = true | ||
| self.errorMessage = error.localizedDescription | ||
| lastMessageFailed = true | ||
| errorMessage = error.localizedDescription | ||
|
|
||
| // Remove empty assistant message on error | ||
| if let idx = self.messages.firstIndex(where: { $0.id == assistantID }), | ||
| self.messages[idx].content.isEmpty { | ||
| self.messages.remove(at: idx) | ||
| if let idx = messages.firstIndex(where: { $0.id == assistantID }), | ||
| messages[idx].content.isEmpty { | ||
| messages.remove(at: idx) | ||
| } | ||
| } | ||
| self.isStreaming = false | ||
| self.streamingTask = nil | ||
| self.streamingAssistantID = nil | ||
| isStreaming = false | ||
| streamingTask = nil | ||
| streamingAssistantID = nil | ||
| } |
There was a problem hiding this comment.
Ignore completion from superseded streams.
These completion/error paths always clear isStreaming, streamingTask, and streamingAssistantID, and the error path can also overwrite lastMessageFailed/errorMessage. If stream A is canceled or replaced and then unwinds after stream B has started, A can tear down B's UI state and persist the wrong conversation. Gate all post-stream mutation on streamingAssistantID == assistantID (or task identity).
Possible fix
- isStreaming = false
- streamingTask = nil
- streamingAssistantID = nil
- persistCurrentConversation()
+ if streamingAssistantID == assistantID {
+ isStreaming = false
+ streamingTask = nil
+ streamingAssistantID = nil
+ persistCurrentConversation()
+ }
} catch {
- if !Task.isCancelled {
+ if !Task.isCancelled && streamingAssistantID == assistantID {
Self.logger.error("Streaming failed: \(error.localizedDescription)")
lastMessageFailed = true
errorMessage = error.localizedDescription
// Remove empty assistant message on error
if let idx = messages.firstIndex(where: { $0.id == assistantID }),
messages[idx].content.isEmpty {
messages.remove(at: idx)
}
}
- isStreaming = false
- streamingTask = nil
- streamingAssistantID = nil
+ if streamingAssistantID == assistantID {
+ isStreaming = false
+ streamingTask = nil
+ streamingAssistantID = nil
+ }
}📝 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
| isStreaming = false | |
| streamingTask = nil | |
| streamingAssistantID = nil | |
| persistCurrentConversation() | |
| } catch { | |
| if !Task.isCancelled { | |
| Self.logger.error("Streaming failed: \(error.localizedDescription)") | |
| self.lastMessageFailed = true | |
| self.errorMessage = error.localizedDescription | |
| lastMessageFailed = true | |
| errorMessage = error.localizedDescription | |
| // Remove empty assistant message on error | |
| if let idx = self.messages.firstIndex(where: { $0.id == assistantID }), | |
| self.messages[idx].content.isEmpty { | |
| self.messages.remove(at: idx) | |
| if let idx = messages.firstIndex(where: { $0.id == assistantID }), | |
| messages[idx].content.isEmpty { | |
| messages.remove(at: idx) | |
| } | |
| } | |
| self.isStreaming = false | |
| self.streamingTask = nil | |
| self.streamingAssistantID = nil | |
| isStreaming = false | |
| streamingTask = nil | |
| streamingAssistantID = nil | |
| } | |
| if streamingAssistantID == assistantID { | |
| isStreaming = false | |
| streamingTask = nil | |
| streamingAssistantID = nil | |
| persistCurrentConversation() | |
| } | |
| } catch { | |
| if !Task.isCancelled && streamingAssistantID == assistantID { | |
| Self.logger.error("Streaming failed: \(error.localizedDescription)") | |
| lastMessageFailed = true | |
| errorMessage = error.localizedDescription | |
| // Remove empty assistant message on error | |
| if let idx = messages.firstIndex(where: { $0.id == assistantID }), | |
| messages[idx].content.isEmpty { | |
| messages.remove(at: idx) | |
| } | |
| } | |
| if streamingAssistantID == assistantID { | |
| isStreaming = false | |
| streamingTask = nil | |
| streamingAssistantID = nil | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/ViewModels/AIChatViewModel.swift` around lines 368 - 387, The
post-stream success and error handlers currently unconditionally clear
isStreaming, streamingTask, streamingAssistantID and update
lastMessageFailed/errorMessage and messages, which lets a superseded stream (A)
overwrite state for a newer stream (B); gate all mutations by verifying the
stream identity (e.g. compare streamingAssistantID == assistantID or compare the
Task instance) before doing any teardown, error updates, message removal, or
calling persistCurrentConversation() so only the active stream can modify state
(apply this check inside the success path, the catch block, and any
finally/cleanup code that touches
isStreaming/streamingTask/streamingAssistantID/lastMessageFailed/errorMessage/messages).
Comment on lines
+138
to
+155
| .alert("Rename Group", isPresented: $showRenameAlert) { | ||
| TextField("Group name", text: $renameText) | ||
| Button(String(localized: "Rename")) { | ||
| let newName = renameText.trimmingCharacters(in: .whitespaces) | ||
| guard !newName.isEmpty, let group = renameGroupTarget else { return } | ||
| let isDuplicate = groups.contains { | ||
| $0.id != group.id && $0.name.lowercased() == newName.lowercased() | ||
| } | ||
| guard !isDuplicate else { return } | ||
| var updated = group | ||
| updated.name = newName | ||
| groupStorage.updateGroup(updated) | ||
| groups = groupStorage.loadGroups() | ||
| } | ||
| Button(String(localized: "Cancel"), role: .cancel) {} | ||
| } message: { | ||
| Text("Enter a new name for the group.") | ||
| } |
There was a problem hiding this comment.
Silent failure on validation errors in rename alert.
When the user enters an empty name or a duplicate group name, the rename action silently returns without any feedback. The alert dismisses but the group is not renamed, which could confuse users.
Consider adding user feedback for validation failures:
🔧 Proposed fix to add validation feedback
.alert("Rename Group", isPresented: $showRenameAlert) {
TextField("Group name", text: $renameText)
Button(String(localized: "Rename")) {
let newName = renameText.trimmingCharacters(in: .whitespaces)
- guard !newName.isEmpty, let group = renameGroupTarget else { return }
+ guard !newName.isEmpty, let group = renameGroupTarget else {
+ // Empty name - keep alert open by not dismissing
+ return
+ }
let isDuplicate = groups.contains {
$0.id != group.id && $0.name.lowercased() == newName.lowercased()
}
- guard !isDuplicate else { return }
+ guard !isDuplicate else {
+ // Duplicate name - keep alert open
+ return
+ }
var updated = group
updated.name = newName
groupStorage.updateGroup(updated)
groups = groupStorage.loadGroups()
}
Button(String(localized: "Cancel"), role: .cancel) {}
} message: {
Text("Enter a new name for the group.")
}Note: SwiftUI .alert buttons dismiss the alert by default after the action runs. The early return statements won't prevent dismissal. To keep the alert open on validation failure, you may need to use a different approach (e.g., a sheet with explicit validation state).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Views/Connection/WelcomeWindowView.swift` around lines 138 - 155,
The rename alert silently returns on validation failures (empty or duplicate
name) because the alert always dismisses; replace the inline .alert("Rename
Group", isPresented: $showRenameAlert) flow with a custom modal/sheet bound to
$showRenameAlert (or a dedicated `@State` like showRenameSheet) that contains the
TextField bound to renameText, shows validation messages when the user taps the
Rename button (validate trimmed renameText against empty string and duplicate
using groups and renameGroupTarget), and only call
groupStorage.updateGroup(updated) and refresh groups = groupStorage.loadGroups()
when validation passes; keep the sheet open and surface inline errors (e.g.,
errorText state) or disable the Rename button until validation succeeds so the
user receives immediate feedback instead of the alert dismissing silently.
Comment on lines
33
to
35
| var body: some View { | ||
| let hasQuery = appState.hasQueryText | ||
| let hasQuery = AppState.shared.hasQueryText | ||
|
|
There was a problem hiding this comment.
Use the bound query text as the source of truth for Explain state.
The control state can drift from the actual editor contents here. This file’s own preview already passes a non-empty queryText, but AppState.shared.hasQueryText can still be false, which leaves Explain disabled until some other code synchronizes the global flag.
♻️ Proposed fix
- let hasQuery = AppState.shared.hasQueryText
+ let hasQuery = !queryText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty📝 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
| var body: some View { | |
| let hasQuery = appState.hasQueryText | |
| let hasQuery = AppState.shared.hasQueryText | |
| var body: some View { | |
| let hasQuery = !queryText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Views/Editor/QueryEditorView.swift` around lines 33 - 35, The
Explain control should derive its enabled state from the view's bound query text
instead of the global AppState flag; replace uses of
AppState.shared.hasQueryText in the view body with a computed check against the
view's bound queryText (e.g., non-empty/trimmed) so the Explain button/state
reflects the actual editor contents; update the let hasQuery = ... assignment in
var body and any logic that references hasQuery (or
AppState.shared.hasQueryText) to use the local queryText-based check (retain
trimming/whitespace handling as needed).
Comment on lines
+469
to
+485
| let restorePageSize = AppSettingsManager.shared.dataGrid.defaultPageSize | ||
| let restoreEditorLang = PluginManager.shared.editorLanguage(for: connection.type) | ||
| let restorePaginationStyle = PluginManager.shared.paginationStyle(for: connection.type) | ||
| let restoreOrderBy = PluginManager.shared.offsetFetchOrderBy(for: connection.type) | ||
| let restorePluginDriver = PluginManager.shared.queryBuildingDriver(for: connection.type) | ||
| let restoreQuote = quoteIdentifierFromDialect(PluginManager.shared.sqlDialect(for: connection.type)) | ||
| var restoredTabs = result.tabs | ||
| for i in restoredTabs.indices where restoredTabs[i].tabType == .table { | ||
| if let tableName = restoredTabs[i].tableName { | ||
| restoredTabs[i].query = QueryTab.buildBaseTableQuery( | ||
| tableName: tableName, | ||
| databaseType: connection.type | ||
| pageSize: restorePageSize, | ||
| editorLanguage: restoreEditorLang, | ||
| paginationStyle: restorePaginationStyle, | ||
| offsetFetchOrderBy: restoreOrderBy, | ||
| pluginDriver: restorePluginDriver, | ||
| quoteIdentifier: restoreQuote |
There was a problem hiding this comment.
Preserve each restored tab’s saved page size.
Line 469 rebuilds every table tab with the current default page size. If a tab was saved with a custom pagination.pageSize, its SQL comes back with the default LIMIT while the tab state still holds the saved size, so the first restore fetch and the next paging action are out of sync.
Suggested fix
- let restorePageSize = AppSettingsManager.shared.dataGrid.defaultPageSize
let restoreEditorLang = PluginManager.shared.editorLanguage(for: connection.type)
let restorePaginationStyle = PluginManager.shared.paginationStyle(for: connection.type)
let restoreOrderBy = PluginManager.shared.offsetFetchOrderBy(for: connection.type)
let restorePluginDriver = PluginManager.shared.queryBuildingDriver(for: connection.type)
let restoreQuote = quoteIdentifierFromDialect(PluginManager.shared.sqlDialect(for: connection.type))
var restoredTabs = result.tabs
for i in restoredTabs.indices where restoredTabs[i].tabType == .table {
if let tableName = restoredTabs[i].tableName {
+ let restorePageSize =
+ restoredTabs[i].pagination.pageSize > 0
+ ? restoredTabs[i].pagination.pageSize
+ : AppSettingsManager.shared.dataGrid.defaultPageSize
restoredTabs[i].query = QueryTab.buildBaseTableQuery(
tableName: tableName,
pageSize: restorePageSize,📝 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
| let restorePageSize = AppSettingsManager.shared.dataGrid.defaultPageSize | |
| let restoreEditorLang = PluginManager.shared.editorLanguage(for: connection.type) | |
| let restorePaginationStyle = PluginManager.shared.paginationStyle(for: connection.type) | |
| let restoreOrderBy = PluginManager.shared.offsetFetchOrderBy(for: connection.type) | |
| let restorePluginDriver = PluginManager.shared.queryBuildingDriver(for: connection.type) | |
| let restoreQuote = quoteIdentifierFromDialect(PluginManager.shared.sqlDialect(for: connection.type)) | |
| var restoredTabs = result.tabs | |
| for i in restoredTabs.indices where restoredTabs[i].tabType == .table { | |
| if let tableName = restoredTabs[i].tableName { | |
| restoredTabs[i].query = QueryTab.buildBaseTableQuery( | |
| tableName: tableName, | |
| databaseType: connection.type | |
| pageSize: restorePageSize, | |
| editorLanguage: restoreEditorLang, | |
| paginationStyle: restorePaginationStyle, | |
| offsetFetchOrderBy: restoreOrderBy, | |
| pluginDriver: restorePluginDriver, | |
| quoteIdentifier: restoreQuote | |
| let restoreEditorLang = PluginManager.shared.editorLanguage(for: connection.type) | |
| let restorePaginationStyle = PluginManager.shared.paginationStyle(for: connection.type) | |
| let restoreOrderBy = PluginManager.shared.offsetFetchOrderBy(for: connection.type) | |
| let restorePluginDriver = PluginManager.shared.queryBuildingDriver(for: connection.type) | |
| let restoreQuote = quoteIdentifierFromDialect(PluginManager.shared.sqlDialect(for: connection.type)) | |
| var restoredTabs = result.tabs | |
| for i in restoredTabs.indices where restoredTabs[i].tabType == .table { | |
| if let tableName = restoredTabs[i].tableName { | |
| let restorePageSize = | |
| restoredTabs[i].pagination.pageSize > 0 | |
| ? restoredTabs[i].pagination.pageSize | |
| : AppSettingsManager.shared.dataGrid.defaultPageSize | |
| restoredTabs[i].query = QueryTab.buildBaseTableQuery( | |
| tableName: tableName, | |
| pageSize: restorePageSize, | |
| editorLanguage: restoreEditorLang, | |
| paginationStyle: restorePaginationStyle, | |
| offsetFetchOrderBy: restoreOrderBy, | |
| pluginDriver: restorePluginDriver, | |
| quoteIdentifier: restoreQuote |
🤖 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 469 - 485, The code
always uses the global default restorePageSize when rebuilding table tabs,
causing restored tabs with a saved custom page size to get rebuilt SQL with the
wrong LIMIT; update the loop that builds table queries (the restoredTabs
iteration that calls QueryTab.buildBaseTableQuery) to read the tab's saved page
size (e.g. restoredTabs[i].pagination.pageSize or restoredTabs[i].pageSize if
that is where it is stored) and pass that value as the pageSize argument,
falling back to restorePageSize only if the tab has no saved page size.
Comment on lines
+159
to
198
| .onExitCommand { onDismiss?() } | ||
| .onKeyPress(.upArrow) { | ||
| if selectedIndex > 0 { | ||
| selectedIndex -= 1 | ||
| } | ||
| return .handled | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Keyboard Navigation | ||
|
|
||
| private func installKeyMonitor() { | ||
| keyMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { event in | ||
| .onKeyPress(.downArrow) { | ||
| let items = allItems | ||
| switch event.keyCode { | ||
| case KeyCode.upArrow.rawValue: | ||
| if selectedIndex > 0 { | ||
| selectedIndex -= 1 | ||
| } | ||
| return nil | ||
| case KeyCode.downArrow.rawValue: | ||
| if selectedIndex < items.count - 1 { | ||
| selectedIndex += 1 | ||
| } | ||
| return nil | ||
| case KeyCode.return.rawValue: | ||
| guard selectedIndex >= 0, selectedIndex < items.count else { return event } | ||
| switch items[selectedIndex] { | ||
| case .session(let session): | ||
| switchToSession(session.id) | ||
| case .saved(let connection): | ||
| connectToSaved(connection) | ||
| } | ||
| return nil | ||
| case KeyCode.escape.rawValue: | ||
| onDismiss?() | ||
| return nil | ||
| case KeyCode.j.rawValue where event.modifierFlags.intersection(.deviceIndependentFlagsMask).contains(.control): | ||
| if selectedIndex < items.count - 1 { | ||
| selectedIndex += 1 | ||
| } | ||
| return nil | ||
| case KeyCode.k.rawValue where event.modifierFlags.contains(.control): | ||
| if selectedIndex > 0 { | ||
| selectedIndex -= 1 | ||
| } | ||
| return nil | ||
| default: | ||
| return event | ||
| if selectedIndex < items.count - 1 { | ||
| selectedIndex += 1 | ||
| } | ||
| return .handled | ||
| } | ||
| } | ||
|
|
||
| private func removeKeyMonitor() { | ||
| if let monitor = keyMonitor { | ||
| NSEvent.removeMonitor(monitor) | ||
| keyMonitor = nil | ||
| .onKeyPress(.return) { | ||
| let items = allItems | ||
| guard selectedIndex >= 0, selectedIndex < items.count else { return .ignored } | ||
| switch items[selectedIndex] { | ||
| case .session(let session): | ||
| switchToSession(session.id) | ||
| case .saved(let connection): | ||
| connectToSaved(connection) | ||
| } | ||
| return .handled | ||
| } | ||
| .onKeyPress(characters: .init(charactersIn: "j"), phases: .down) { keyPress in | ||
| guard keyPress.modifiers.contains(.control) else { return .ignored } | ||
| let items = allItems | ||
| if selectedIndex < items.count - 1 { | ||
| selectedIndex += 1 | ||
| } | ||
| return .handled | ||
| } | ||
| .onKeyPress(characters: .init(charactersIn: "k"), phases: .down) { keyPress in | ||
| guard keyPress.modifiers.contains(.control) else { return .ignored } | ||
| if selectedIndex > 0 { | ||
| selectedIndex -= 1 | ||
| } | ||
| return .handled | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SwiftUI onKeyPress characters phases modifier macOS
💡 Result:
In SwiftUI on macOS (SwiftUI’s hardware-keyboard APIs, introduced around macOS 14 / Xcode 15), you can filter key events by the characters produced and by phase using:
.onKeyPress(characters: CharacterSet, phases: KeyPress.Phases = [.down, .repeat]) { keyPress in
// keyPress.characters, keyPress.modifiers, keyPress.phase
return .handled // or .ignored
}Key points:
characters:is aCharacterSetfilter (e.g..letters,.decimalDigits). (createwithswift.com)phases:controls when you get callbacks:.down,.repeat,.up(and.all), with a default of[.down, .repeat]. (createwithswift.com)- The closure receives a
KeyPresscontaining (commonly used)characters,modifiers, andphase, and you must return.handledor.ignored(a parent returning.handledcan prevent children from receiving events). (avanderlee.com) - Your view must be focused to receive key presses (often:
.focusable()+@FocusState). (avanderlee.com) - This API is considered “new” in the macOS 14 timeframe (earlier OS versions won’t have it). (stackoverflow.com)
Example (listen for digit key release):
struct ContentView: View {
`@FocusState` private var focused: Bool
`@State` private var last: String = "—"
var body: some View {
Text("Last digit released: \(last)")
.padding()
.focusable()
.focused($focused)
.onAppear { focused = true }
.onKeyPress(characters: .decimalDigits, phases: .up) { kp in
last = kp.characters
return .handled
}
}
}
``` ([avanderlee.com](https://www.avanderlee.com/swiftui/key-press-events-detection/))
Citations:
- 1: https://www.createwithswift.com/controlling-keyboard-events-with-keys-and-phases/
- 2: https://www.createwithswift.com/controlling-keyboard-events-with-keys-and-phases/
- 3: https://www.avanderlee.com/swiftui/key-press-events-detection/
- 4: https://www.avanderlee.com/swiftui/key-press-events-detection/
- 5: https://stackoverflow.com/questions/76847277/how-to-make-onkeypress-work-with-textfield-in-swift?utm_source=openai
- 6: https://www.avanderlee.com/swiftui/key-press-events-detection/
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine ConnectionSwitcherPopover.swift
find . -name "ConnectionSwitcherPopover.swift" -type fRepository: datlechin/TablePro
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Read the file and check its content, focusing on the key handler area
wc -l ./TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift
echo "---"
sed -n '150,210p' ./TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift | cat -nRepository: datlechin/TablePro
Length of output: 2613
🏁 Script executed:
#!/bin/bash
# Read from line 1 to ~150 to understand the view structure and check for .focusable()
sed -n '1,160p' ./TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift | cat -nRepository: datlechin/TablePro
Length of output: 7828
🏁 Script executed:
#!/bin/bash
# Check the rest of the file and look for .focusable() modifier
sed -n '140,200p' ./TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift | cat -nRepository: datlechin/TablePro
Length of output: 2614
🏁 Script executed:
#!/bin/bash
# Search for .focusable() in the entire file to check if the view is made focusable
rg -n "focusable" ./TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift
echo "---"
# Also check the entire file context around where this view is used
grep -n "ConnectionSwitcherPopover" ./TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift | head -5Repository: datlechin/TablePro
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Search for where ConnectionSwitcherPopover is used
rg -n "ConnectionSwitcherPopover" --type swift . | grep -v "ConnectionSwitcherPopover.swift"Repository: datlechin/TablePro
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check how ConnectionSwitcherPopover is used in TableProToolbarView.swift
sed -n '60,100p' ./TablePro/Views/Toolbar/TableProToolbarView.swift | cat -nRepository: datlechin/TablePro
Length of output: 2131
🏁 Script executed:
#!/bin/bash
# Search the entire codebase for other uses of .onKeyPress to see if there are examples with .focusable()
rg -B 5 -A 5 "\.onKeyPress" --type swift . | head -100Repository: datlechin/TablePro
Length of output: 8797
🏁 Script executed:
#!/bin/bash
# Check if List automatically receives focus in popovers, or search for focusable in the entire toolbar view
cat -n ./TablePro/Views/Toolbar/TableProToolbarView.swift | sed -n '60,120p'Repository: datlechin/TablePro
Length of output: 2971
Add .focusable() modifier to enable keyboard navigation.
The key handlers are correctly structured, but the view will not receive key press events without .focusable(). The web search confirms that views must be explicitly made focusable with .focusable() to receive .onKeyPress() callbacks—popovers do not automatically grant focus.
Add .focusable() to the VStack or List so that arrow keys, Return, and Ctrl+J/K actually trigger the handlers. Otherwise, all keyboard navigation will silently fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Views/Toolbar/ConnectionSwitcherPopover.swift` around lines 159 -
198, The view’s .onKeyPress handlers (used with selectedIndex, allItems,
switchToSession, connectToSaved and onDismiss) won’t receive events because the
container isn’t focusable; add the .focusable() modifier to the main container
(e.g. the VStack or List that contains these handlers) so the view can accept
keyboard focus and the arrow/return/Ctrl+J/Ctrl+K handlers actually fire.
- VIEW-1.1 (high): replace title-matching window discovery with WindowAccessor NSViewRepresentable - VIEW-1.2 (high): replace all NSApp.keyWindow (22 occurrences) with coordinator.window - VIEW-8.2 (medium): extract onAppear into coordinator.configureWindow/registerWindowLifecycle - VIEW-6.1 (high): replace NSEvent.addLocalMonitorForEvents with .onKeyPress in ConnectionSwitcherPopover - APP-2.3 (high): remove openWindow from WindowOpener singleton, use NotificationCenter - MODEL-1.2 (high): simplify QueryTab equality to id-based, add contentHash for change detection - MODEL-1.8 (medium): replace UUID() on ColumnInfo/IndexInfo/ForeignKeyInfo with deterministic ids
Service layer: - DB-1: offload DatabaseManager driver setup to nonisolated helper - SVC-4: remove redundant NSLock from ExportService (@mainactor is sufficient) - PLG-3: audit PluginMetadataRegistry thread-safety, document contract Storage + sync: - STG-2: move CKServerChangeToken from UserDefaults to Application Support file - STG-3: delete stale iCloud Keychain copy on sync opt-out - CKS-2: change CloudKit save policy to .ifServerRecordUnchanged with conflict retry - APP-3.4: clean up transient connection Keychain entries after use SwiftUI views: - VIEW-1.3: replace NSAlert.runModal() with SwiftUI .alert in WelcomeWindowView - VIEW-1.4: extract duplicated connection-ready logic into single method - VIEW-2.2: replace NSNotificationCenter observer with async .task - VIEW-4.1: use minWidth/minHeight instead of fixed frame on Settings - VIEW-4.2: remove direct AppSettingsManager.shared from GeneralSettingsView - VIEW-5.1: remove focusConnectionFormWindow() polling loop - VIEW-7.1: deduplicate DoubleClickView into shared DoubleClickDetector
Models: - MODEL-1.1: make buildBaseTableQuery accept dependencies as parameters - MODEL-1.3: add isEvicted computed property, document RowBuffer aliasing - MODEL-1.4: create AdditionalFieldKey enum for type-safe dictionary keys - MODEL-1.5: extract FilterSQLPreviewGenerator from FilterStateManager - MODEL-1.9: verify SharedSidebarState cleanup on disconnect ViewModels: - VM-5.1: replace NSLog with os.Logger in SidebarViewModel - VM-5.2: document Binding storage contract on SidebarViewModel - VM-5.3: remove unnecessary [weak self] from @mainactor Tasks - APP-8.1: add @mainactor to @objc notification handlers - APP-8.2: document legacy notification migration path - VIEW-8.1: standardize on AppState.shared direct access Theme: - THEME-7.1: document ThemeEngine.shared View extension pattern - THEME-7.3: make importTheme async, dispatch reloadAvailableThemes to background
- Replace deprecated .cornerRadius() with .clipShape(RoundedRectangle()) in 8 files - Remove dead nonisolated deinit from AppDelegate - Replace DispatchQueue.main.async with Task in @mainactor SQLEditorCoordinator - Add @mainactor to UserDefaults+RecentDatabases.trackDatabaseAccess - Add Logger warning on invalid hex color fallback in Color+Hex - Document AppLanguage.apply() side effect - Use ThemeEngine typography in ConnectionSwitcherPopover (replace hardcoded sizes)
datlechin
force-pushed
the
chore/batch5-cleanup
branch
from
7385db9 to
f5eb348
Compare
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
Batch 5 (final) of anti-pattern fixes. Addresses 20 low-priority cleanup items.
Changes
.cornerRadius()with.clipShape(RoundedRectangle())— 12 occurrences, 8 filesnonisolated deinitfromAppDelegateDispatchQueue.main.asyncwithTaskin@MainActorSQLEditorCoordinator— 4 occurrences@MainActortoUserDefaults+RecentDatabases.trackDatabaseAccessLoggerwarning on invalid hex color fallback inColor+HexAppLanguage.apply()side effectThemeEnginetypography inConnectionSwitcherPopover— replace 4 hardcoded font sizesSkipped (intentional)
Selector("showSettingsWindow:")— standard macOS pattern, no alternativehasInitializedinMainContentView— guards tab restoration beyond coordinator initTask { @MainActor in }inDatabaseSwitcherSheet.moveSelection— needed for SwiftUI warningTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Improvements