perf(capture): route window capture through ScreenCaptureKit to cut SkyLight leaks#613
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates window image capture from synchronous SkyLight private API to async ScreenCaptureKit paths. It introduces async ScreenCaptureKit bridging with bounds/display validation, updates menu-bar color and refresh pipelines to use async captures with generation-based staleness tracking, and implements selective routing where visible menu-bar items use the SCK path while offscreen items fall back to SkyLight. ChangesAsync ScreenCaptureKit Capture Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Collaborator
|
.build |
|
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift (1)
775-781:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep off-screen items out of the SCK recent-move path.
This early return now sends hidden/always-hidden items through
individualCapture, which is SCK-backed. Those are the exact off-display windows this PR routes to SkyLight elsewhere, andrunLiveRefreshLoop()also skips refreshes for the same 2-second window, so their cache can stay blank/stale right after a move.🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift` around lines 775 - 781, The early-return that forces individualCapture during a recent move (in MenuBarItemImageCache when calling appState.itemManager.lastMoveOperationOccurred) incorrectly sends hidden/always-hidden items into the SCK-backed path; change the condition so the single-item capture is only used for items that are visible/on-screen (i.e., guard the return with a visibility check such as negating capturable.isHidden or capturable.alwaysHidden), allowing hidden/off-screen items to bypass individualCapture so they remain on the SkyLight/composite path and keep their cache blank/stale as intended (also consistent with runLiveRefreshLoop's 2-second skip).
🧹 Nitpick comments (1)
ThawTests/MarkerPairResolverTests.swift (1)
293-313: ⚡ Quick winAdd a characterization test for width-only matching.
The suite currently checks width mismatch rejection, but it does not lock in the intended behavior where equal width with different height should still resolve. Adding that test will prevent accidental regression to full-size matching.
Proposed test addition
+ /// Same width but different height should still pair: resolver + /// intentionally matches by width only. + func testSameWidthDifferentHeightStillResolves() { + let icons = [icon(windowID: 1, title: "Item-0", size: CGSize(width: 116, height: 27))] + let markers = [marker(windowID: 2, title: "com.example.widget", size: CGSize(width: 116, height: 33), owningPID: 100)] + let result = MarkerPairResolver.resolve( + unresolvedIcons: icons, + markers: markers, + thawBundleID: thawBundleID, + ccBundleID: ccBundleID, + pidToBundleID: { _ in "com.example.widget" }, + bundleIDToPID: { _ in 100 } + ) + XCTAssertEqual(result.count, 1) + XCTAssertEqual(result.first?.resolvedPID, 100) + }🤖 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 `@ThawTests/MarkerPairResolverTests.swift` around lines 293 - 313, Add a new characterization test that verifies MarkerPairResolver.resolve treats equal widths as a match even when heights differ: create an icon and marker where width is identical but height differs, call MarkerPairResolver.resolve (use the same thawBundleID/ccBundleID/pidToBundleID/bundleIDToPID pattern used in existing tests) and assert that the resolver returns the expected pairing instead of empty; reference the resolve method and add the test alongside testSizeMismatchProducesNoResult (name it something like testWidthMatchesDespiteHeightDifference) so future changes to full-size matching are caught.
🤖 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 `@MenuBarItemService/SourcePIDCache.swift`:
- Around line 421-546: The code currently gates both marker-pair resolution and
the expensive diagnostics on unresolvedWindows which still contains marker
windows; compute a filtered set of unresolved icon candidates (e.g.,
unresolvedIconWindowIDs = unresolvedWindows filtered by non-bundle-ID-shaped
titles / using the same predicate used to build icons/unresolvedInfos) and use
that set instead of unresolvedWindows to decide whether to run
MarkerPairResolver.resolve and the diagnostic re-walk; keep using
MarkerPairResolver.resolve(...) / the icons/resolutions variables and still
remove resolved icon IDs from state.pids and unresolvedWindows, but iterate and
log against unresolvedIconWindowIDs (and unresolvedWindowInfos derived from it)
so marker windows no longer keep the hot path permanently active.
In `@Shared/Bridging/Bridging.swift`:
- Around line 606-615: The function captureWindowsImageSCK currently accepts any
non-empty subset of windowIDs (scWindows), which can produce incorrect
composites if some requested windows are missing; change the guard to require an
exact match by comparing scWindows.count to windowIDs.count and fail fast when
they differ (log a warning including requested vs matched IDs/counts via
diagLog.warning and return nil) so callers cleanly fallback when
SCShareableContent resolves only a subset; reference the variables windowIDs,
scWindows, content.windows and the function captureWindowsImageSCK when making
this change.
In `@Shared/Utilities/DiagnosticLogger.swift`:
- Around line 149-177: The file publishes the FileHandle
(fileHandleLock.withLock { $0 = handle }) before writing the header, allowing
concurrent log(...) calls to race ahead; move the header write into the same
critical section so handle publication and header.write(data) occur atomically.
Specifically, under the same lock used to publish the handle (fileHandleLock) —
and, if you need to update currentLogFileLock, perform that update inside the
same locked region as well — set the handle, set currentLogFileLock (if
required), then convert header to data and call handle.write(data) before
releasing the lock so no log(...) can be written before the header is emitted.
In `@Thaw/MenuBar/IceBar/IceBarColorManager.swift`:
- Around line 169-180: The current fire-and-forget Task inside updateWindowImage
lets callers (and updateColorInfo) run before windowImage is actually set and
allows older tasks to overwrite state after stopPeriodicRefresh; change
updateWindowImage so callers can await the capture: either make
updateWindowImage async and await ScreenCapture.captureWindowsAsync directly
before assigning windowImage, or persist the spawned Task (e.g. windowImageTask)
and have callers await windowImageTask.value before calling updateColorInfo;
additionally cancel that Task in stopPeriodicRefresh and/or use a simple
generation token checked before writing windowImage to prevent stale tasks from
overwriting state.
In `@Thaw/MenuBar/Search/MenuBarSearchModel.swift`:
- Around line 85-103: The async Task started around
ScreenCapture.captureWindowsAsync can complete after the UI has changed and
overwrite averageColorInfo with a stale value; store and manage a token or
cancellable Task for the capture (e.g., a property like captureTask: Task<Void,
Never>? or captureToken: Int) before launching the Task, cancel the previous
Task or increment the token when the panel visibility changes, capture the
current token into the Task closure, and before assigning self.averageColorInfo
(and after await) verify the token still matches (or that the Task hasn’t been
cancelled) and that self is still valid; reference the existing Task creation,
ScreenCapture.captureWindowsAsync, MenuBarAverageColorInfo, and averageColorInfo
when making the change.
In `@Thaw/Permissions/Permission.swift`:
- Around line 100-106: The current implementation cancels the shared
hasPermissionCancellable at the start of waitForPermission(), making concurrent
calls cancel each other; change waitForPermission() to create a new,
invocation-scoped AnyCancellable (e.g. local variable inside the await
withCheckedContinuation) instead of overwriting the shared
hasPermissionCancellable, and ensure that continuation is resumed and the local
cancellable is cancelled when permission changes or when the task is cancelled;
alternatively implement a fan-out by storing multiple continuations and
attaching a single shared sink (using $hasPermission.sink) that resumes all
pending continuations rather than replacing hasPermissionCancellable—adjust
configureCancellables()/hasPermissionCancellable usage accordingly so the
per-invocation wait is not cancelled by subsequent calls.
In `@ThawTests/AdvancedSettingsSnapshotTests.swift`:
- Around line 43-46: Update AdvancedSettingsSnapshotTests.swift to add
assertions that verify the three newly populated boolean properties:
useOptionClickToShowAlwaysHiddenSection, useLCSSortingOnNotchedDisplays, and
enableMenuBarItemOverflow. Locate the test blocks where
useDoubleClickToShowAlwaysHiddenSection is already asserted (occurrences around
the noted ranges) and add matching XCTAssertEqual (or appropriate
XCTAssertTrue/XCTAssertFalse) checks comparing the decoded/defaulted model's
properties against the expected false values for
useOptionClickToShowAlwaysHiddenSection, useLCSSortingOnNotchedDisplays, and
enableMenuBarItemOverflow so any decode/defaulting regressions for these keys
will fail the test.
---
Outside diff comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift`:
- Around line 775-781: The early-return that forces individualCapture during a
recent move (in MenuBarItemImageCache when calling
appState.itemManager.lastMoveOperationOccurred) incorrectly sends
hidden/always-hidden items into the SCK-backed path; change the condition so the
single-item capture is only used for items that are visible/on-screen (i.e.,
guard the return with a visibility check such as negating capturable.isHidden or
capturable.alwaysHidden), allowing hidden/off-screen items to bypass
individualCapture so they remain on the SkyLight/composite path and keep their
cache blank/stale as intended (also consistent with runLiveRefreshLoop's
2-second skip).
---
Nitpick comments:
In `@ThawTests/MarkerPairResolverTests.swift`:
- Around line 293-313: Add a new characterization test that verifies
MarkerPairResolver.resolve treats equal widths as a match even when heights
differ: create an icon and marker where width is identical but height differs,
call MarkerPairResolver.resolve (use the same
thawBundleID/ccBundleID/pidToBundleID/bundleIDToPID pattern used in existing
tests) and assert that the resolver returns the expected pairing instead of
empty; reference the resolve method and add the test alongside
testSizeMismatchProducesNoResult (name it something like
testWidthMatchesDespiteHeightDifference) so future changes to full-size matching
are caught.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 464caf4b-f0c1-438f-9062-f4036c8f6513
📒 Files selected for processing (38)
MenuBarItemService/SourcePIDCache.swiftMenuBarItemService/main.swiftShared/Bridging/Bridging.swiftShared/Utilities/DiagnosticLogger.swiftShared/Utilities/MarkerPairResolver.swiftThaw.xcodeproj/project.pbxprojThaw/MenuBar/IceBar/IceBarColorManager.swiftThaw/MenuBar/MenuBarItems/LayoutReconciler.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/PendingLedger.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/Search/MenuBarSearchModel.swiftThaw/Permissions/Permission.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Utilities/ScreenCapture.swiftThawTests/AdvancedSettingsSnapshotTests.swiftThawTests/AnchorDestinationTests.swiftThawTests/LayoutReconcilerTests.swiftThawTests/MarkerPairResolverTests.swiftThawTests/MenuBarTestFixtures.swiftThawTests/MenuBarTestFixturesTests.swiftThawTests/PartitionUnmanagedUIDsTests.swiftThawTests/PendingRehideTagIdentifiersTests.swiftThawTests/PlanFullSortSequenceTests.swiftThawTests/PlanLCSMoveSequenceTests.swiftThawTests/PlanLeftmostMoveTests.swiftThawTests/PlanNotchOverflowTests.swiftThawTests/PlanPendingMoveTests.swiftThawTests/PlanSectionOrderTests.swiftThawTests/PlanUnmanagedPlacementTests.swiftThawTests/ProfileExportBundleTests.swiftThawTests/ProfileTests.swiftThawTests/SavedPositionLookupTests.swiftThawTests/SelectWindowForBatchScanTests.swiftThawTests/ShouldPersistSavedOrderTests.swift
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Thaw/MenuBar/IceBar/IceBarColorManager.swift (2)
51-54: 💤 Low valueScreen change handler doesn't update colorInfo after awaiting the capture.
Other similar patterns (lines 105-111, 129-133, 162-168) await
updateWindowImagethen callupdateColorInfo. This path only refreshes the image. If the screen changes without a subsequent frame change,colorInforemains stale until the throttled frame observer fires or the panel moves.Consider adding
updateColorInfoafter the await for consistency with the other notification-driven paths.🤖 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 `@Thaw/MenuBar/IceBar/IceBarColorManager.swift` around lines 51 - 54, The screen-change Task currently awaits updateWindowImage(for: screen) but doesn't refresh colorInfo, leaving stale state; after the await in that Task (the same pattern used elsewhere), call updateColorInfo() (or the existing method used in lines 105-111 / 129-133 / 162-168) on self to recompute colorInfo so the notification-driven path mirrors the other handlers that await updateWindowImage(for:) and then invoke updateColorInfo().
244-254: 💤 Low value
updateAllPropertiesis now fire-and-forget despite the comment suggesting "fresh capture".The method wraps everything in a
Task, so callers won't observe updatedcolorInfoimmediately after the call returns. If any caller depends on synchronous read-after semantics, this could cause stale reads.If all call sites are tolerant of eventual consistency, this is fine. Otherwise, consider exposing an async variant for callers that need to await.
🤖 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 `@Thaw/MenuBar/IceBar/IceBarColorManager.swift` around lines 244 - 254, The current updateAllProperties(with:screen:) is fire-and-forget which can cause stale reads; introduce an async variant (e.g. updateAllPropertiesAsync(with:screen:) async) that performs await updateWindowImage(for:) then calls updateColorInfo(with:screen:) so callers can await fresh captures, and keep the existing synchronous updateAllProperties as a thin wrapper that spawns Task { await updateAllPropertiesAsync(with:screen:) } to preserve current non-blocking behavior for callers that don't need to await; update call sites that require read-after semantics to call/await updateAllPropertiesAsync instead.
🤖 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.
Nitpick comments:
In `@Thaw/MenuBar/IceBar/IceBarColorManager.swift`:
- Around line 51-54: The screen-change Task currently awaits
updateWindowImage(for: screen) but doesn't refresh colorInfo, leaving stale
state; after the await in that Task (the same pattern used elsewhere), call
updateColorInfo() (or the existing method used in lines 105-111 / 129-133 /
162-168) on self to recompute colorInfo so the notification-driven path mirrors
the other handlers that await updateWindowImage(for:) and then invoke
updateColorInfo().
- Around line 244-254: The current updateAllProperties(with:screen:) is
fire-and-forget which can cause stale reads; introduce an async variant (e.g.
updateAllPropertiesAsync(with:screen:) async) that performs await
updateWindowImage(for:) then calls updateColorInfo(with:screen:) so callers can
await fresh captures, and keep the existing synchronous updateAllProperties as a
thin wrapper that spawns Task { await updateAllPropertiesAsync(with:screen:) }
to preserve current non-blocking behavior for callers that don't need to await;
update call sites that require read-after semantics to call/await
updateAllPropertiesAsync instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 267ee3bc-7c91-4558-ac39-1268e7f4b754
📒 Files selected for processing (4)
Shared/Bridging/Bridging.swiftThaw/MenuBar/IceBar/IceBarColorManager.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/Search/MenuBarSearchModel.swift
…kyLight leaks The `leaks` tool against a fresh Thaw process on macOS 26.4.1 attributed 250 `NSMutableDictionary` instances (~42 KB of ~44 KB total leaked bytes) to a single allocation site: `CGRectCreateDictionaryRepresentation` inside the private `SLSWindowListCreateImageFromArrayProxying`, called by every `SLWindowListCreateImageFromArray` invocation. The dictionary is allocated with retain count 1 and never released. The path was being exercised by every Thaw window-capture call site: `MenuBarItemImageCache.individualCapture`, `compositeCapture`, and `refreshImages`, plus the three color-sampler call sites in `MenuBarManager.updateAverageColorInfo`, `IceBarColorManager.updateWindowImage`, and `MenuBarSearchModel.updateAverageColorInfo`. The leak lives inside Apple's SkyLight framework, so there is no way to release the dictionary from Swift; the only levers we have are to stop calling SkyLight or to call it less. This change introduces SCK-backed async equivalents `Bridging.captureWindowsImageSCK` (using `SCScreenshotManager.captureImage(contentFilter:configuration:)` with an `SCContentFilter(display:including:)` filter) and corresponding `ScreenCapture.captureWindowsAsync` / `captureWindowAsync` wrappers, then migrates every capture call site whose target windows fit within display bounds. The menu-bar item cache (`compositeCapture`, `individualCapture`) becomes async and routes through SCK; the three color samplers keep their public synchronous signatures but wrap the capture in a `Task` so the SCK call runs off the main actor and the resulting state mutation hops back to the owning `@MainActor` class via the captured `self`. The `refreshImages` path cannot move to SCK: it captures items in the hidden and always-hidden sections, which Thaw positions at large negative x past the display's left edge. Both SCK filter shapes reject those windows on macOS 26: `SCContentFilter(display:including:)` returns error -3812 (sourceRect outside display bounds) and `SCContentFilter(desktopIndependentWindow:)` returns error -3811 (stream start failure). To shrink that residual floor, `refreshImages` gains an opt-in `viaSCK` parameter and `runLiveRefreshLoop` is restructured: tick-global guards (`lastMoveOperationOccurred`, `isResettingLayout`) hoist out of the per-section loop, the `.visible` section refreshes individually via the leak-free SCK path, and `.hidden` plus `.alwaysHidden` items batch into a single SkyLight call per tick. A full all-sections refresh now leaks one dictionary per tick instead of three. On a fresh `MallocStackLogging=1` launch with the menu bar idle, hidden + always-hidden sections expanded, and `Settings → Menu Bar Layout` opened, `leaks` reports 122 leaks / 10.8 KB total: 53 `NSMutableDictionary` instances (all from the batched SkyLight `refreshImages` call) and 19 framework CGRegion leaks from AppKit's `_regionForOpaqueDescendants` display-cycle path. Versus the original baseline of 507 leaks / 44 KB that is a 79% reduction in dictionary leak count and a 76% reduction in leaked bytes. The remaining 53 instances are the irreducible Apple-framework floor until either `SLSWindowListCreateImageFromArrayProxying` stops dropping the dictionary or SCK adds support for capturing windows positioned outside any `SCDisplay.frame`. Apple Feedback drafts for the SkyLight leak, the AppKit CGRegion leak, and a separate `AppIntents` `NSProgress` retain cycle observed when materializing `SetFocusFilterIntent` metadata are tracked separately and will be filed via Feedback Assistant.
Tightens the SCK migration's edges in response to follow-up review. Five small fixes across four files; no leak-rate change, all behavioural correctness.
`Bridging.captureWindowsImageSCK` now requires an exact match between the requested `windowIDs` and the `SCWindow`s resolved from `SCShareableContent`. The previous `!scWindows.isEmpty` guard let a partial subset through, which is unsafe for cache composites (the post-capture crop math assumes every requested window's bounds is covered) and for the color samplers (a missing `menuBarWindow` would silently produce a wallpaper-only strip whose `averageColor` no longer represents the menu bar). On mismatch we now log the requested vs matched counts plus the missing IDs and return `nil` so callers fall back cleanly to SkyLight or skip the tick.
`IceBarColorManager` adds a generation-token pattern to `updateWindowImage`. The function becomes `async`, bumps `windowImageGeneration` before suspending, and writes `windowImage` after the SCK await only if the generation still matches. A new `clearWindowImage()` helper bumps + nils together so `stopPeriodicRefresh` and the notification sink can invalidate any capture in flight from before the clear; the previous fire-and-forget shape let a late completion undo `self.windowImage = nil`. Four sinks and `updateAllProperties` are now wrapped in `Task { await updateWindowImage(...); updateColorInfo(...) }` so the post-refresh color read sees the freshly captured image instead of the previous cycle's leftover, which closes the visible ~5 s color lag users would see after a theme change or first IceBar open while the previous async capture was still in flight. `updateAllProperties` keeps its synchronous signature so `IceBar.show()` doesn't ripple async upstream.
`MenuBarManager.updateAverageColorInfo` is split into a sync fire-and-forget wrapper and an awaitable `updateAverageColorInfoAsync()` that uses `withTaskGroup` for concurrent per-screen captures and collects results back on the enclosing `@MainActor` so the `averageColors` / `averageColorInfo` writes are complete before `await` returns. `captureAdaptiveColorWithRetry` is rewritten as a single async retry loop that awaits each capture before reading `averageColors`; the previous fire-and-forget call meant the post-call read saw stale state and burned all ten retries even when the first capture would have succeeded. The wake-poll Timer sink wraps its body in a `Task` that awaits the async variant before reading `after = averageColors`, so the stabilization detector (`wakePollDidChange`, `wakePollStableCount`, `wakePollPrevColors`) sees real frame-to-frame changes instead of always-stale snapshots that previously prevented it from ever marking the wake as settled within the 10 s window.
`MenuBarSearchModel.updateAverageColorInfo` gets the same generation-token treatment as `IceBarColorManager`. A new `clearAverageColorInfo()` helper bumps the token and nils `averageColorInfo`; the panel-visibility-off sink and the screen-params sink now route through it, and the capture Task captures the generation before suspending and skips its write if the token has advanced. Closes the previously-possible races where a late capture undid an intentional clear or where out-of-order completions from rapid screen switches let a stale color overwrite a fresher one. The public function stays sync because no callers in this file read `averageColorInfo` immediately after.
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 (2)
Thaw/MenuBar/MenuBarManager.swift (1)
489-574:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply average-color updates atomically per refresh run.
updateAverageColorInfo()is still launched from multiple timers/notifications, so two refreshes can overlap and the slower older one can resume later and overwriteaverageColorInfo. This loop also only patches successful captures intoaverageColors, which leaves stale entries behind on failure;captureAdaptiveColorWithRetry()then treats those old keys as a successful fresh capture and can stop retrying too early. Build a per-run snapshot, gate it with a generation/task token, and replace the targeted entries in one commit.🤖 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 `@Thaw/MenuBar/MenuBarManager.swift` around lines 489 - 574, updateAverageColorInfoAsync can run concurrently and older slower runs may overwrite newer results and leave stale averageColors entries when captures fail; collect per-run results into a local dictionary keyed by displayID, attach a short-lived generation/token for the run (e.g. increment a menuBarAverageGeneration property or use Task local) and, after the TaskGroup completes, perform one atomic commit on the MainActor: replace averageColors entries for exactly the target displayIDs with the new values (setting nil / removing the key for displays that failed to capture) and update averageColorInfo for activeDisplayID only if the commit token/generation still matches; update code in updateAverageColorInfoAsync (and references to averageColors / averageColorInfo) to use this snapshot-and-commit approach so concurrent runs cannot interleave writes or leave stale keys.Thaw/MenuBar/IceBar/IceBarColorManager.swift (1)
188-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
windowImagebefore awaiting the new capture.
updateWindowImage(for:)invalidates stale completions, but it still leaves the previouswindowImagein place until a new image arrives. Every caller on Lines 105-110, 129-133, 162-167, and 249-252 immediately recomputes color after awaiting this method, so a nil/guarded-out capture here will reuse the last cycle’s image instead of failing closed.Suggested fix
private func updateWindowImage(for screen: NSScreen) async { + windowImageGeneration += 1 + let generation = windowImageGeneration + windowImage = nil + let windows = WindowInfo.createWindows(option: .onScreen) let displayID = screen.displayID guard let menuBarWindow = WindowInfo.menuBarWindow(from: windows, for: displayID), @@ let windowIDs = [menuBarWindow.windowID, wallpaperWindow.windowID] let bounds = withMutableCopy(of: wallpaperWindow.bounds) { $0.size.height = 1 } - - // Stamp our generation before suspending. If the counter advances while - // we await (a clearWindowImage, a stopPeriodicRefresh, or a newer - // updateWindowImage call), our completion is stale and must skip the - // write so we don't undo intentional clears or clobber a fresher image. - windowImageGeneration += 1 - let generation = windowImageGeneration let image = await ScreenCapture.captureWindowsAsync( with: windowIDs, screenBounds: bounds, option: .nominalResolution🤖 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 `@Thaw/MenuBar/IceBar/IceBarColorManager.swift` around lines 188 - 215, In updateWindowImage(for:) clear the existing cached image before the async capture so callers won't see a stale image if the capture fails or is cancelled: after bumping windowImageGeneration (the generation/local variable already used to detect staleness) set windowImage to nil before calling ScreenCapture.captureWindowsAsync(with:screenBounds:option:), then continue with the existing guard that compares generation and assigns the new image; this ensures callers that await updateWindowImage(for:) cannot observe the previous cycle's windowImage when the new capture is nil or skipped.
🤖 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 `@Shared/Bridging/Bridging.swift`:
- Around line 633-639: The display selection in captureWindowsImageSCK currently
uses intersects and must instead require a single display whose frame fully
contains both effectiveBounds and unionBounds; update the guard that finds a
display (currently using content.displays.first(where: {
$0.frame.intersects(...) })) to search for a display where
display.frame.contains(effectiveBounds) && display.frame.contains(unionBounds),
and if none is found keep the existing diagLog.warning and return nil so the SCK
path is only attempted when the union fits entirely within one display.
- Around line 645-648: captureWindowsImageSCK currently sets
configuration.ignoreShadowsDisplay using "options.contains(.boundsIgnoreFraming)
|| options.isEmpty", which incorrectly makes an empty options set behave like
.boundsIgnoreFraming; change the assignment to only honor the explicit
.boundsIgnoreFraming flag (i.e., set configuration.ignoreShadowsDisplay based
solely on options.contains(.boundsIgnoreFraming)) so empty options do not
implicitly enable frameless behavior; update the line referencing
configuration.ignoreShadowsDisplay and options.contains(.boundsIgnoreFraming)
accordingly.
---
Outside diff comments:
In `@Thaw/MenuBar/IceBar/IceBarColorManager.swift`:
- Around line 188-215: In updateWindowImage(for:) clear the existing cached
image before the async capture so callers won't see a stale image if the capture
fails or is cancelled: after bumping windowImageGeneration (the generation/local
variable already used to detect staleness) set windowImage to nil before calling
ScreenCapture.captureWindowsAsync(with:screenBounds:option:), then continue with
the existing guard that compares generation and assigns the new image; this
ensures callers that await updateWindowImage(for:) cannot observe the previous
cycle's windowImage when the new capture is nil or skipped.
In `@Thaw/MenuBar/MenuBarManager.swift`:
- Around line 489-574: updateAverageColorInfoAsync can run concurrently and
older slower runs may overwrite newer results and leave stale averageColors
entries when captures fail; collect per-run results into a local dictionary
keyed by displayID, attach a short-lived generation/token for the run (e.g.
increment a menuBarAverageGeneration property or use Task local) and, after the
TaskGroup completes, perform one atomic commit on the MainActor: replace
averageColors entries for exactly the target displayIDs with the new values
(setting nil / removing the key for displays that failed to capture) and update
averageColorInfo for activeDisplayID only if the commit token/generation still
matches; update code in updateAverageColorInfoAsync (and references to
averageColors / averageColorInfo) to use this snapshot-and-commit approach so
concurrent runs cannot interleave writes or leave stale keys.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ca1a6f5-412e-4045-8e49-838543c138be
📒 Files selected for processing (6)
Shared/Bridging/Bridging.swiftThaw/MenuBar/IceBar/IceBarColorManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/Search/MenuBarSearchModel.swiftThaw/Utilities/ScreenCapture.swift
Two follow-up tightenings in `Bridging.captureWindowsImageSCK`. No leak-rate change, both behavioural correctness for edge cases the existing callers happen not to hit today. Display selection now requires a single `SCDisplay` whose `frame` fully contains BOTH the effective capture bounds AND the union of all selected windows. The previous chain (`intersects(effectiveBounds)` → `intersects(unionBounds)` → `displays.first`) could pick a display that only partially overlapped the windows, and `SCContentFilter(display:including:)` silently clips at `display.frame`, so anything outside the chosen display was lost from the capture without an error. Tightening to `contains` returns a clean `nil` for cross-display spans and frame-reporting quirks so callers fall back (cache paths excluded-item retry) or skip the tick (`refreshImages viaSCK: true`) instead of feeding clipped pixels to downstream crops and `averageColor`. The diagLog warning now reports both bounds so the rejection is debuggable. `configuration.ignoreShadowsDisplay` no longer treats `options.isEmpty` as if `.boundsIgnoreFraming` were set. The legacy `SLWindowListCreateImageFromArray` API treats an empty option set as the default capture (framing and shadows preserved); only the explicit `.boundsIgnoreFraming` flag suppresses them. The previous `|| options.isEmpty` clause would have given an unexpected frameless capture to any future caller that relied on the function's `options: CGWindowImageOption = []` default. All current callers pass either `.boundsIgnoreFraming` (cache paths) or `.nominalResolution` (color samplers), so observable behaviour is unchanged today.
|
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
3 participants
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.
What does this PR do?
Migrates Thaw's window capture path from the leaking SkyLight private API (
SLWindowListCreateImageFromArray) to ScreenCaptureKit (SCScreenshotManager.captureImage(contentFilter:configuration:)) for every call site whose target windows fit within display bounds, and batches the residual SkyLight calls that genuinely cannot move to SCK. Eliminates ~79% of theNSMutableDictionaryleaks reported byleakson macOS 26.4.1 and reduces total leaked bytes by ~76%.PR Type
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path:
N/A. The public surfaces of all migrated functions are preserved.
refreshImagesonly gains a newviaSCK: Bool = falseparameter with a default that matches prior behaviour.compositeCaptureandindividualCapturebecomeasync, but they areprivatetoMenuBarItemImageCache. The three color-sampler functions (MenuBarManager.updateAverageColorInfo,IceBarColorManager.updateWindowImage,MenuBarSearchModel.updateAverageColorInfo) keep their synchronous signatures and externally observable behaviour.What is the current behavior?
Every call to
Bridging.captureWindowsImage(the wrapper around the privateSLWindowListCreateImageFromArray) leaks oneCFMutableDictionaryinsideSLSWindowListCreateImageFromArrayProxyingviaCGRectCreateDictionaryRepresentation.leakson a freshMallocStackLogging=1Thaw process attributed ~250NSMutableDictionaryinstances (~42 KB of ~44 KB total leaked bytes) to that one allocation site, exercised by six distinct Thaw call sites:MenuBarItemImageCache.individualCapture,compositeCapture, andrefreshImages, plusMenuBarManager.updateAverageColorInfo,IceBarColorManager.updateWindowImage, andMenuBarSearchModel.updateAverageColorInfo. The leak is inside Apple's SkyLight framework, so the dictionary cannot be released from Swift; the only levers are to stop calling SkyLight or to call it less.Issue Number: N/A
What is the new behavior?
Adds
Bridging.captureWindowsImageSCKas an async, leak-free SCK replacement (usingSCContentFilter(display:including:)+SCScreenshotManager.captureImage(contentFilter:configuration:)) and exposes it viaScreenCapture.captureWindowsAsync/captureWindowAsync. MigratesMenuBarItemImageCache.compositeCaptureandindividualCaptureto the SCK path (both becomeasync). Migrates the three color-sampler functions while preserving their sync signatures by wrapping the capture in aTask; the SCK call runs off the main actor and state mutation resumes on the owning@MainActorclass. ForrefreshImages, where items live past the display's left edge and both SCK filter shapes fail (-3812for display-bounded,-3811for desktopIndependentWindow),refreshImagesgains an opt-inviaSCK: Bool = falseparameter andrunLiveRefreshLoopis restructured: tick-global guards (lastMoveOperationOccurred,isResettingLayout) hoist out of the per-section loop, the.visiblesection refreshes individually via the leak-free SCK path, and.hiddenplus.alwaysHiddenitems batch into a single SkyLight call per tick instead of two separate ones. Net result on testing across normal exercise: 122 leaks / 10.8 KB total versus the 507 leaks / 44 KB baseline; the remaining 53NSMutableDictionaryinstances are the irreducible Apple-framework floor.PR Checklist
Other information
Verified empirically rather than via unit tests, since the bug surfaces in
leaksoutput not in functional assertions. Workflow: build withxcodebuild -scheme Thaw-configuration Debug build, launch withMallocStackLogging=1, exercise all four scenarios (menu bar idle, ThawBar visible-section, ThawBar hidden + always-hidden expanded,Settings → Menu Bar Layoutopen), thenleaks <pid> --fullStacks --groupByTypeand inspect attribution. The diagnostic delta is the most useful artefact: before the change, dictionary leaks scale with total capture-call count across all six call sites; after, they scale only with the batchedrefreshImagesSkyLight call frequency. Comments atThaw/Utilities/ScreenCapture.swiftandShared/Bridging/Bridging.swiftdocument the SCK display-bounds limitation and the specific error codes (-3812/-3811) so future maintainers don't re-investigate.Notes for reviewers
The
refreshImagesviaSCKparameter defaults tofalsedeliberately so any future external caller keeps the SkyLight semantics (offscreen-capable). Only the in-loop call site inrunLiveRefreshLoopopts in totrue, and only for.visible. Per-section routing is by section enum rather than runtime bounds-check because the section identity is already known at the call site and items in.hidden/.alwaysHiddenare off-screen by definition (control items observed at x = -3985 and x = -9245 respectively). Color-samplerTaskwraps trade a small change in update timing for leak elimination: paths that previously ranupdateWindowImage(); updateColorInfo();back-to-back will now read the previous cycle'swindowImageuntil the next async capture lands. On the 5 s periodic refresh this is imperceptible, but it's worth a quick visual smoke when exercising IceBar adaptive backgrounds. Apple Feedback drafts for the SkyLightCGRectCreateDictionaryRepresentationleak, the AppKit_regionForOpaqueDescendantsCGRegionleak, and a separateAppIntentsNSProgressretain cycle (observed when System Settings materializesSetFocusFilterIntentmetadata) are prepared and will be filed via Feedback Assistant; FB numbers will be added as in-code comments once issued.Summary by CodeRabbit
New Features
Bug Fixes
Refactor