perf(phase 4): scope ContentView re-renders + lazy startup - #656
Open
mehmetefeaytas wants to merge 29 commits into
Open
perf(phase 4): scope ContentView re-renders + lazy startup#656mehmetefeaytas wants to merge 29 commits into
mehmetefeaytas wants to merge 29 commits into
Conversation
…y SMC key scan Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…8601 formatter Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…main-actor media publish, stats/battery micro-opt)
…polling suspension Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… when asleep/off Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed + debounced persistence Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…S2-d, YTM omitted to keep phase-1 change)
…p (off-main poll)
…; hoist Defaults reads Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… async file reads Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dering off the main thread + cache Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…disk enumeration Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lineView visualizers, lazy now-playing, off-main icons, lockscreen hosting cache)
… scope re-renders Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…work Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…observe) + lazy startup
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR defers startup work, caches repeated computations and images, moves heavy metrics off the main actor, improves event-driven monitoring, and routes asynchronous media state updates through the main actor. ChangesPerformance and concurrency improvements
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MediaRemote
participant NowPlayingController
participant StreamProcess
MediaRemote->>NowPlayingController: now-playing presence notification
NowPlayingController->>MediaRemote: query application PID
NowPlayingController->>StreamProcess: startStreamingIfNeeded()
MediaRemote-->>NowPlayingController: session disappears
NowPlayingController->>StreamProcess: stopStreaming() after idle delay
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- ThumbnailService: reconcile `cacheKeys` against the NSCache once it exceeds the count limit so the tracking set stays bounded; match the "<path>_" boundary in clearCache(for:) so invalidating /tmp/a no longer evicts /tmp/ab's thumbnails. - BatteryActivityManager: snapshot observers (Array(values)) before invoking, so a callback that adds/removes observers can't trigger a mutation-during-iteration trap. - StatsManager: preserve the network baseline when getifaddrs fails (nil snapshot) instead of collapsing to (0,0), which faked a speed dip and wiped previousNetworkStats. - SystemMediaControllers: drop the volume/mute element caches — they were read/populated from the public API on arbitrary threads while refreshPropertyElements() cleared them on callbackQueue (data race). Recompute on access (cheap probe). HAL listener-leak fix retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DynamicIsland/components/Shelf/Views/ShelfItemView.swift (1)
89-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrag preview never updates once the real thumbnail resolves.
The removed
.onChange(of: viewModel.thumbnail)was the only mechanism that refreshedcachedPreviewImageafter the thumbnail loaded. NowrenderDragPreview()only runs once fromonAppear, using whateverviewModel.thumbnail/viewModel.iconhappen to be at that moment. Since thumbnail loading (loadMetadata()) is async and can finish after this utility-priority task runs — especially for slow QuickLook generations — the composed drag preview can permanently show the generic icon fallback instead of the actual thumbnail for that shelf item.Suggest reintroducing a thumbnail-change listener that only re-renders when the cached preview was built without a real thumbnail (to avoid regressing the "only produced once" perf goal):
🩹 Suggested fix
.onAppear { Task(priority: .utility) { if cachedPreviewImage == nil { cachedPreviewImage = await renderDragPreview() } } viewModel.onQuickLookRequest = { urls in quickLookService.show(urls: urls, selectFirst: true) } } + .onChange(of: viewModel.thumbnail) { _, newThumbnail in + guard newThumbnail != nil, !renderedWithRealThumbnail else { return } + Task(priority: .utility) { + cachedPreviewImage = await renderDragPreview() + renderedWithRealThumbnail = true + } + } .quickLookPresenter(using: quickLookService)(requires a small
renderedWithRealThumbnailstate flag to prevent re-rendering on every subsequent thumbnail-equal update)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/components/Shelf/Views/ShelfItemView.swift` around lines 89 - 104, Update ShelfItemView’s cached drag-preview flow around renderDragPreview and the onAppear task by tracking whether the cached preview was rendered with a real thumbnail, then add a thumbnail-change listener that re-renders only when the existing preview used the fallback icon and a real thumbnail becomes available. Preserve the current single-render behavior once a real thumbnail has been used.
🧹 Nitpick comments (2)
DynamicIsland/DynamicIslandApp.swift (1)
952-971: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDisk I/O in the deferred block still runs on the main thread.
DispatchQueue.main.asynconly pushes this to the next runloop turn —idleAnimationManager.initializeDefaultAnimations()andapplySelectedAppIcon()still perform synchronous disk reads on the main thread, risking a hitch right after the first frame instead of during the (already deferred) launch path.ModelPricingManager.loadInitialPricing()in this same PR already establishes the pattern of offloading toTask.detached(priority: .utility)and hopping back to main only to publish results — worth applying the same approach here for consistency and to fully avoid a post-launch stall.♻️ Sketch: move disk I/O off the main thread
DispatchQueue.main.async { [weak self] in guard let self else { return } - - // Disk I/O: load bundled idle animations off the critical launch path. - self.idleAnimationManager.initializeDefaultAnimations() - - // Disk I/O: read Defaults + load the selected icon image, then apply it. - applySelectedAppIcon() - // Instantiate always-on managers whose init only registers // observers/pollers... _ = self.bluetoothAudioManager _ = self.systemTimerBridge _ = self.lockScreenPanelManager } + +Task.detached(priority: .utility) { [weak self] in + self?.idleAnimationManager.initializeDefaultAnimations() + let icon = /* load selected icon image data here */ + await MainActor.run { + applySelectedAppIcon(with: icon) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/DynamicIslandApp.swift` around lines 952 - 971, Move the disk-loading work in the deferred launch closure—idleAnimationManager.initializeDefaultAnimations() and applySelectedAppIcon()—into a Task.detached(priority: .utility), then hop back to the main actor only for UI or manager updates that require main-thread access. Preserve the existing ordering and behavior, while keeping the always-on manager materialization on the main thread.DynamicIsland/components/Shelf/Services/ThumbnailService.swift (1)
31-44: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
cacheKeysnever shrinks when NSCache auto-evicts entries.
cacheKeysis only pruned inclearCache()/clearCache(for:); NSCache's own eviction (viacountLimit/totalCostLimit/memory pressure) never notifies this set, so it grows unbounded for the process lifetime even though the underlying image cache is bounded.♻️ Use NSCacheDelegate to keep cacheKeys in sync
-actor ThumbnailService { +actor ThumbnailService: NSObject, NSCacheDelegate { static let shared = ThumbnailService() private let cache: NSCache<NSString, NSImage> = { let cache = NSCache<NSString, NSImage>() cache.countLimit = 200 cache.totalCostLimit = 64 * 1024 * 1024 // 64 MB return cache }() + + func cache(_ cache: NSCache<AnyObject, AnyObject>, willEvictObject obj: Any) { + // remove matching key from cacheKeys + }Note: actors can't directly conform to
NSCacheDelegate(which requiresNSObjectProtocol); this likely needs a smallNSObjectdelegate shim that forwards to the actor.Also applies to: 45-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/components/Shelf/Services/ThumbnailService.swift` around lines 31 - 44, Update ThumbnailService’s cache bookkeeping so cacheKeys is removed when NSCache evicts an object automatically. Add an NSObject-based NSCacheDelegate shim, since the actor cannot conform directly, and have it forward eviction callbacks to ThumbnailService; wire it to cache and keep clearCache()/clearCache(for:) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DynamicIsland/components/Shelf/Services/ThumbnailService.swift`:
- Around line 75-81: Update clearCache(for:) so cache key matching only removes
the requested file path or entries within its directory, requiring a
path-component boundary after url.path rather than a raw string prefix. Preserve
removal through cache.removeObject and cacheKeys.remove for each valid match.
In `@DynamicIsland/helpers/AppIcons.swift`:
- Around line 49-56: Update getIcon(bundleID:) to retain the resolved
application URL and pass its .path to getIcon(file:), rather than using
.absoluteString, so downstream filesystem APIs receive a POSIX path.
---
Outside diff comments:
In `@DynamicIsland/components/Shelf/Views/ShelfItemView.swift`:
- Around line 89-104: Update ShelfItemView’s cached drag-preview flow around
renderDragPreview and the onAppear task by tracking whether the cached preview
was rendered with a real thumbnail, then add a thumbnail-change listener that
re-renders only when the existing preview used the fallback icon and a real
thumbnail becomes available. Preserve the current single-render behavior once a
real thumbnail has been used.
---
Nitpick comments:
In `@DynamicIsland/components/Shelf/Services/ThumbnailService.swift`:
- Around line 31-44: Update ThumbnailService’s cache bookkeeping so cacheKeys is
removed when NSCache evicts an object automatically. Add an NSObject-based
NSCacheDelegate shim, since the actor cannot conform directly, and have it
forward eviction callbacks to ThumbnailService; wire it to cache and keep
clearCache()/clearCache(for:) behavior unchanged.
In `@DynamicIsland/DynamicIslandApp.swift`:
- Around line 952-971: Move the disk-loading work in the deferred launch
closure—idleAnimationManager.initializeDefaultAnimations() and
applySelectedAppIcon()—into a Task.detached(priority: .utility), then hop back
to the main actor only for UI or manager updates that require main-thread
access. Preserve the existing ordering and behavior, while keeping the always-on
manager materialization on the main thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 851f911a-d899-49b3-a35b-f3ee57020d31
📒 Files selected for processing (28)
DynamicIsland/ContentView.swiftDynamicIsland/DynamicIslandApp.swiftDynamicIsland/MediaControllers/AmazonMusicController.swiftDynamicIsland/MediaControllers/AppleMusicController.swiftDynamicIsland/MediaControllers/NowPlayingController.swiftDynamicIsland/MediaControllers/SpotifyController.swiftDynamicIsland/MediaControllers/YouTube Music Controller/YouTubeMusicController.swiftDynamicIsland/audio/RealTimeAudioSpectrum.swiftDynamicIsland/components/LockScreen/LockScreenWeatherWidget.swiftDynamicIsland/components/Music/MusicVisualizer.swiftDynamicIsland/components/Music/RealTimeWaveformScrubberView.swiftDynamicIsland/components/Shelf/Services/ThumbnailService.swiftDynamicIsland/components/Shelf/ViewModels/ShelfItemViewModel.swiftDynamicIsland/components/Shelf/Views/ShelfItemView.swiftDynamicIsland/helpers/AppIcons.swiftDynamicIsland/managers/ActivityGate.swiftDynamicIsland/managers/BatteryActivityManager.swiftDynamicIsland/managers/BluetoothAudioManager.swiftDynamicIsland/managers/ClipboardManager.swiftDynamicIsland/managers/DoNotDisturbManager.swiftDynamicIsland/managers/LLMUsage/JSONLUsageParser.swiftDynamicIsland/managers/LLMUsage/ModelPricingManager.swiftDynamicIsland/managers/LockScreenWeatherPanelManager.swiftDynamicIsland/managers/MediaKeyInterceptor.swiftDynamicIsland/managers/StatsManager.swiftDynamicIsland/managers/SystemMediaControllers.swiftDynamicIsland/managers/SystemTimerBridge.swiftDynamicIsland/utils/CPUSensorCollector.swift
- ClipboardManager: replace `MainActor.assumeIsolated` in the poll tick (which would trap if the timer ever ran off the main thread) with a plain Bool cached from a main-actor subscription to `ActivityGate.$shouldSuspendBackgroundWork`. - DoNotDisturbManager: confine all Assertions dispatch-source lifecycle to `pollingQueue`. start/stop were mutating the `assertions*Source` vars on the main thread while the filesystem-event handlers re-armed them on `pollingQueue` — a data race. (Also carries the phase-1 CodeRabbit fixes via merge: ThumbnailService cacheKeys/boundary, BatteryActivityManager snapshot, StatsManager network baseline, SystemMediaControllers cache revert.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- AppIcons.getIcon(bundleID:): use `appURL.path` instead of `.absoluteString`. The getIcon(file:) guard uses fileExists(atPath:), which only accepts POSIX paths, so the "file://…" form made the bundle-ID lookup always return nil. - JSONLUsageParser: replace the per-line `removeSubrange` (which shifted the whole buffer for every newline → quadratic on newline-dense JSONL) with a chunk cursor that emits lines via slices and trims the consumed prefix once per chunk. (Carries the phase-1/2 CodeRabbit fixes via merge — incl. ThumbnailService cacheKeys/boundary and SystemMediaControllers cache revert that Ebullioscopic#655 also flagged.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DynamicIsland/helpers/AppIcons.swift (1)
23-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalidate cached icons when applications change.
The cache is keyed only by file path and has no explicit invalidation. If an app is updated or replaced at the same path while this process remains running, callers can receive the old icon indefinitely. Include the bundle version/modification state in the cache key or invalidate entries when application changes are detected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/helpers/AppIcons.swift` around lines 23 - 38, The cachedIcon(forFile:) implementation must avoid returning stale icons after an application changes at the same path. Incorporate the app bundle’s version or modification state into the NSCache key, or add equivalent change detection that invalidates the affected entry before lookup, while preserving the existing NSWorkspace icon resolution and caching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@DynamicIsland/helpers/AppIcons.swift`:
- Around line 23-38: The cachedIcon(forFile:) implementation must avoid
returning stale icons after an application changes at the same path. Incorporate
the app bundle’s version or modification state into the NSCache key, or add
equivalent change detection that invalidates the affected entry before lookup,
while preserving the existing NSWorkspace icon resolution and caching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b422321-bfca-4aab-ab01-7bb46196e078
📒 Files selected for processing (8)
DynamicIsland/components/Shelf/Services/ThumbnailService.swiftDynamicIsland/helpers/AppIcons.swiftDynamicIsland/managers/BatteryActivityManager.swiftDynamicIsland/managers/ClipboardManager.swiftDynamicIsland/managers/DoNotDisturbManager.swiftDynamicIsland/managers/LLMUsage/JSONLUsageParser.swiftDynamicIsland/managers/StatsManager.swiftDynamicIsland/managers/SystemMediaControllers.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- DynamicIsland/managers/BatteryActivityManager.swift
- DynamicIsland/managers/LLMUsage/JSONLUsageParser.swift
- DynamicIsland/components/Shelf/Services/ThumbnailService.swift
- DynamicIsland/managers/DoNotDisturbManager.swift
- DynamicIsland/managers/SystemMediaControllers.swift
- DynamicIsland/managers/StatsManager.swift
- DynamicIsland/managers/ClipboardManager.swift
Address CodeRabbit review on Ebullioscopic#656: the deferred launch block used DispatchQueue.main.async, which only pushes work to the next runloop turn — the app-icon image read (NSImage(contentsOf:)) still hit disk on the main thread, moving a potential hitch to just after the first frame instead of off-main. Split the icon load into a pure, off-main-safe loadSelectedAppIconImage() (Defaults lookup + disk read, no UI) and assign applicationIconImage back on the main actor from a Task.detached(.utility), matching the existing ModelPricingManager.loadInitialPricing() pattern. initializeDefaultAnimations() stays on the main actor by design: it only resolves bundled resource URLs (no file-content reads) and writes Defaults keys that have UI observers, so offloading it would risk off-main UI updates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address CodeRabbit review on Ebullioscopic#656: appIconPathCache was keyed by file path alone, so once an icon was cached for a path it was returned for the process lifetime. If the app at that path was updated/replaced while Atoll kept running, callers received the stale icon indefinitely. Fold the file's modification date into the cache key. When the bundle changes its mtime changes, the key changes, and a fresh NSWorkspace icon is resolved. stat() is far cheaper than icon(forFile:), so the cache still avoids the expensive lookup on every SwiftUI refresh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Phase 4 (final) of the staged performance pass — cuts the idle re-render storm and cold-start cost. Stacked on #655 → #654 → #653; merge those first (diff includes them until they land). Conservative, behavior-preserving.
Changes
ContentView.swift):isNonNotchScreenno longer walksNSScreen.screenson every body evaluation — the result is cached in@Stateand refreshed only ondidChangeScreenParametersNotification/ selected-screen change (same value, far fewer scans while music/stats tick).StatsManageris no longer an@ObservedObjectofContentView. It was only used imperatively (updateMonitoringState,statsRowCountreads@Defaultflags) — never read reactively — while the live stats UI already has its own@ObservedObjectinNotchStatsView. Dropping the observation removes the 1 Hz whole-body invalidation without changing behavior.DynamicIslandApp.swift): feature managers with an external/view-tree owner (calendarManager,dndManager,downloadManager,idleAnimationManager) becomelazy var; always-on managers with no other owner (bluetoothAudioManager,systemTimerBridge,lockScreenPanelManager) are lazy but materialized one runloop after first frame. Heavy launch work —initializeDefaultAnimations(),applySelectedAppIcon()(disk I/O),playWelcomeSound()— deferred off the first frame. Core managers (vm, coordinator, webcam, extension hosts) stay eager; Defaults migrations and publisher sinks stay eager (correctness/ordering).Expected gains (estimates — verify with Instruments)
Verify on device
Summary by CodeRabbit