fix: reduce idle CPU usage from 79% to near zero#395
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR addresses high CPU usage and energy consumption ( Changes
Sequence Diagram(s)sequenceDiagram
participant DBM as DatabaseManager
participant NC as NotificationCenter
participant CV as ContentView<br/>(or MainContentView)
participant UI as UI Components
rect rgba(200, 220, 255, 0.5)
Note over DBM,UI: New: Notification-Driven Architecture
DBM->>DBM: Session state changes
DBM->>NC: post(.connectionStatusDidChange)
NC->>CV: deliver notification
CV->>CV: handleConnectionStatusChange()
CV->>UI: update AppState & views
end
rect rgba(255, 220, 200, 0.5)
Note over DBM,UI: Old: Polling via onChange
CV->>CV: onChange(connectionStatusVersions)
CV->>DBM: read version array
CV->>UI: conditional update
end
sequenceDiagram
participant App as NSApplication
participant VCM as VimCursorManager
participant Layer as CABasicLayer
participant UI as Blink Animation
App->>VCM: didResignActive / didBecomeActive
VCM->>VCM: pauseBlink() / resumeBlink()
VCM->>VCM: toggle isPaused flag
alt isPaused == true
VCM->>Layer: stop "blink" animation
else isPaused == false
VCM->>Layer: reattach blink animation
Layer->>UI: animate cursor blink
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@LocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swift`:
- Around line 141-153: The KVO callback can act on a stale window after the
DispatchQueue.main hop; capture the observed window identity and verify it
before proceeding: inside the parentWindow.observe(\.firstResponder...) closure
save the observed window (e.g., let observedWindow = window) and then in the
DispatchQueue.main.async block ensure parentWindow === observedWindow (or
observedWindow === parentWindow) before reading model.activeTextView or walking
the responder chain; only then perform the existing checks and call close() as
necessary to avoid closing a session for a different window.
In `@TablePro/ContentView.swift`:
- Around line 340-360: This block needs to "latch" the view to the session being
closed before deferring the window close to avoid repopulating for a
newly-selected session; set a short-lived marker (e.g., a property like
sessionClosingId or isQueuedForClose) on ContentView when you enter the
guard-failure branch (where currentSession?.id == sid and you teardown
rightPanelState/sessionState) and clear it after the async close completes, then
update any logic that repopulates the view to ignore changes to currentSession
while sessionClosingId/isQueuedForClose == sid/true; reference ContentView,
currentSession, sessions[sid], DispatchQueue.main.async, rightPanelState and
sessionState to find where to latch and where to check.
In `@TablePro/Views/Main/MainContentView.swift`:
- Around line 787-798: The lazy-load block guarded by session.isConnected &&
coordinator.needsLazyLoad currently only checks changeManager.hasChanges; update
it to also guard against any pending tab-level edits so restored tabs don't get
overwritten: before clearing coordinator.needsLazyLoad and calling
coordinator.runQuery() or coordinator.switchDatabase(to:), check
selectedTab.pendingChanges.hasChanges (via
tabManager.selectedTab?.pendingChanges.hasChanges) and return early if true;
ensure you reference the same symbols (session.isConnected,
coordinator.needsLazyLoad, changeManager.hasChanges,
tabManager.selectedTab.pendingChanges.hasChanges, coordinator.runQuery(),
coordinator.switchDatabase(to:)) so the early-exit mirrors the other key-window
lazy-load path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 705d37d9-306f-4daf-9aec-84c1e5c75c0c
📒 Files selected for processing (10)
CHANGELOG.mdLocalPackages/CodeEditSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swiftTablePro/ContentView.swiftTablePro/Core/AI/InlineSuggestionManager.swiftTablePro/Core/Database/DatabaseManager.swiftTablePro/Core/Services/Infrastructure/AppNotifications.swiftTablePro/Core/Vim/VimCursorManager.swiftTablePro/Views/Editor/EditorEventRouter.swiftTablePro/Views/Editor/SQLEditorCoordinator.swiftTablePro/Views/Main/MainContentView.swift
...itSourceEditor/Sources/CodeEditSourceEditor/CodeSuggestion/Window/SuggestionController.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TablePro/Views/Editor/SQLEditorView.swift (1)
55-63:⚠️ Potential issue | 🟡 MinorLength-only check no longer guarantees text match before cursor propagation.
At Line 61, equal-length but different-content text now passes this guard, so stale cursor positions can still be propagated during tab recreation. That weakens the safety check described in the comment above.
Suggested fix
if let controller = coordinator.controller { let currentString = controller.textView.string as NSString let bindingString = text as NSString - if currentString.length != bindingString.length { + if currentString.length != bindingString.length || currentString != bindingString { return } }🤖 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 55 - 63, The current guard compares only lengths (controller.textView.string and text) which allows different-content strings of equal length to pass; replace the length-only check in the coordinator.controller block with a full content equality check between controller.textView.string and the binding text (e.g., compare the two NSString/String values for equality) and return if they differ so cursor propagation only happens when the editor text exactly matches the binding; update the check in the block referencing coordinator.controller, controller.textView.string and text accordingly.
🧹 Nitpick comments (2)
TablePro/Views/AIChat/AIChatPanelView.swift (2)
46-53: Minor redundancy:viewModel.tablesis set in both.onAppearand.task(id:).The
.task(id: tables)modifier runs on initial appear as well as on changes totables, so the assignment on line 48 is redundant with line 51. Consider removing line 48 to consolidate the logic.♻️ Suggested simplification
.onAppear { viewModel.connection = connection - viewModel.tables = tables } .task(id: tables) { viewModel.tables = tables await fetchSchemaContext() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/AIChat/AIChatPanelView.swift` around lines 46 - 53, The viewModel.tables assignment is duplicated: it’s set in both .onAppear and in .task(id: tables); remove the redundant assignment inside the .onAppear block and keep viewModel.tables = tables inside the .task(id: tables) so table updates and initial load are handled in fetchSchemaContext, while preserving viewModel.connection = connection in .onAppear; update AIChatPanelView.swift to delete the viewModel.tables = tables line from the .onAppear closure.
171-178: O(n²) complexity fromfirstIndex(where:)inside ForEach loop.Each iteration calls
firstIndex(where:)which is O(n), making the overall message rendering O(n²). While chat message counts are typically modest, this is less efficient than the previous enumerated approach.Consider using
ziporenumerated()to avoid the repeated linear search:♻️ Suggested fix using enumerated
- ForEach(viewModel.messages) { message in + ForEach(Array(viewModel.messages.enumerated()), id: \.element.id) { msgIndex, message in if message.role != .system { // Extra spacing before user messages to separate conversation turns - if message.role == .user, - let msgIndex = viewModel.messages.firstIndex(where: { $0.id == message.id }), - msgIndex > 0, - viewModel.messages[msgIndex - 1].role == .assistant + if message.role == .user, + msgIndex > 0, + viewModel.messages[msgIndex - 1].role == .assistant {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/AIChat/AIChatPanelView.swift` around lines 171 - 178, The ForEach over viewModel.messages uses firstIndex(where:) inside the loop (searching by message.id) causing O(n²) rendering; change the iteration to an indexed form (use enumerated() or zip with indices) so you have the current index available instead of calling firstIndex(where:), then check the previous message via that index (e.g., inspect viewModel.messages[index - 1].role) to decide spacing; update the ForEach usage and the conditional that currently references firstIndex(where:) and message.id to use the new index-based access.
🤖 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/Services/Formatting/SQLFormatterService.swift`:
- Around line 117-124: The code currently resolves the SQL dialect while holding
keywordRegexLock, causing a potential deadlock; change SQLFormatterService to
move dialect resolution out of the locked section by calling a new helper (e.g.,
resolveDialect(for:)) that obtains the SQLDialectProvider via
SQLDialectFactory.createDialect(for:) / MainActor.assumeIsolated or
DispatchQueue.main.sync as needed, then re-acquire keywordRegexLock only to
cache the provider/result; also replace the duplicate dialect-resolution code in
format(...) with a call to that shared helper so both sites use the same
non-blocking pattern and avoid holding keywordRegexLock across main-queue sync.
In `@TablePro/Extensions/String`+HexDump.swift:
- Around line 11-15: The shared static NumberFormatter hexDumpNumberFormatter is
not thread-safe; remove the hexDumpNumberFormatter property and replace its
usages (e.g., in the hex dump producing method that currently calls
hexDumpNumberFormatter.string(...) around line 73) with the modern, thread-safe
API by calling Int.value.formatted(.number) (or
String(integer.formatted(.number))) where the formatter was used; update any
code paths that expected an optional string from NumberFormatter to handle the
non-optional formatted result and drop the static formatter declaration.
In `@TablePro/Views/Editor/HistoryPanelView.swift`:
- Around line 305-307: In buildSecondaryMetadata(_ entry: QueryHistoryEntry)
replace the hardcoded prefix "Executed: " with a localized string by using
String(localized:) (e.g. String(localized: "Executed: {0}", entry.executedAt) or
compose String(localized:) for the prefix and then append
Self.metadataDateFormatter.string(from: entry.executedAt)); update the code
around buildSecondaryMetadata and Self.metadataDateFormatter usage so the
displayed prefix is produced via String(localized:) rather than a literal.
---
Outside diff comments:
In `@TablePro/Views/Editor/SQLEditorView.swift`:
- Around line 55-63: The current guard compares only lengths
(controller.textView.string and text) which allows different-content strings of
equal length to pass; replace the length-only check in the
coordinator.controller block with a full content equality check between
controller.textView.string and the binding text (e.g., compare the two
NSString/String values for equality) and return if they differ so cursor
propagation only happens when the editor text exactly matches the binding;
update the check in the block referencing coordinator.controller,
controller.textView.string and text accordingly.
---
Nitpick comments:
In `@TablePro/Views/AIChat/AIChatPanelView.swift`:
- Around line 46-53: The viewModel.tables assignment is duplicated: it’s set in
both .onAppear and in .task(id: tables); remove the redundant assignment inside
the .onAppear block and keep viewModel.tables = tables inside the .task(id:
tables) so table updates and initial load are handled in fetchSchemaContext,
while preserving viewModel.connection = connection in .onAppear; update
AIChatPanelView.swift to delete the viewModel.tables = tables line from the
.onAppear closure.
- Around line 171-178: The ForEach over viewModel.messages uses
firstIndex(where:) inside the loop (searching by message.id) causing O(n²)
rendering; change the iteration to an indexed form (use enumerated() or zip with
indices) so you have the current index available instead of calling
firstIndex(where:), then check the previous message via that index (e.g.,
inspect viewModel.messages[index - 1].role) to decide spacing; update the
ForEach usage and the conditional that currently references firstIndex(where:)
and message.id to use the new index-based access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ed74c06-85e9-4309-8381-3a5b370b9596
📒 Files selected for processing (15)
TablePro/Core/Services/Formatting/SQLFormatterService.swiftTablePro/Core/Services/Infrastructure/AnalyticsService.swiftTablePro/Core/Services/Licensing/LicenseManager.swiftTablePro/Core/Services/Query/SchemaProviderRegistry.swiftTablePro/Extensions/String+HexDump.swiftTablePro/Models/Query/QueryResult.swiftTablePro/Views/AIChat/AIChatPanelView.swiftTablePro/Views/Connection/ConnectionFormView.swiftTablePro/Views/Editor/HistoryPanelView.swiftTablePro/Views/Editor/SQLEditorView.swiftTablePro/Views/Filter/FilterRowView.swiftTablePro/Views/Main/Child/MainEditorContentView.swiftTablePro/Views/QuickSwitcher/QuickSwitcherView.swiftTablePro/Views/Structure/ClickHousePartsView.swiftdocs/development/performance-audit.md
💤 Files with no reviewable changes (1)
- TablePro/Views/Filter/FilterRowView.swift
✅ Files skipped from review due to trivial changes (2)
- TablePro/Views/Structure/ClickHousePartsView.swift
- TablePro/Models/Query/QueryResult.swift
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
Fixes #394. TablePro was consuming 79.3% CPU and 54.7 Energy Impact when idle. Root cause analysis identified 5 independent sources of unnecessary sustained work:
NSWindow.didUpdateNotificationwithobject: nilfired 60-120x/sec for ALL windows. Replaced with per-window scoped observers that only check their own editor.CABasicAnimation(.repeatCount = .infinity) ran the compositor continuously even when unfocused. AddedpauseBlink()/resumeBlink()on editor focus/blur and app active/resign.boundsDidChangeNotification) was installed at startup and fired on every pixel of scroll. Now lifecycle-scoped to only exist while ghost text is displayed..onChange(of: connectionStatusVersions[id])observed the entire@Observabledictionary, triggering re-evaluation for ALL connections on any mutation. Replaced with targetedNotificationCenterpublisher filtered by connection ID.didUpdateNotificationfor first responder tracking replaced with KVO onwindow.firstResponderwhich only fires on actual changes.Test plan
Summary by CodeRabbit
Release Notes
Bug Fixes
Performance