refactor: reduce reliance on screen capture APIs for menu bar icons#346
Conversation
…mission as non-failure
…ontrolItem.windowID
…temImageCache - Add locking for failedCaptures to ensure thread safety - Use reserveCapacity for arrays and dictionaries to optimize memory usage - Refactor failure handling logic for clarity and reliability - Minor code cleanups and improved diagnostics
Introduce utilities to resolve and render menu bar icons without screen capture permissions. Add a provider to map system menu items to SF Symbols and implement a catalog reader that uses heuristic scoring and regex to extract optimal tray icons from third-party app bundles. Include caching and fallback mechanisms (such as app icon silhouettes) to ensure robust and performant icon rendering.
diazdesandi
added
feature
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
- refactor: remove `updateAverageColorInfo` from `MenuBarManager` and `MenuBarSearchModel` - refactor: delete `ScreenCapture` utility and related usage - refactor: remove `ScreenRecordingPermission` checks from `AppPermissions` and `Permission` - chore: update localization strings for removed screen recording permissions - refactor: clean `MenuBarLayoutSettingsPane` UI of screen recording controls - feat: add sample JSON manifest for app icons keyed by bundle identifier
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Thaw/MenuBar/Appearance/MenuBarOverlayPanel.swift (1)
462-466:⚠️ Potential issue | 🟠 MajorThis turns the wallpaper-backed overlay path into a permanent no-op.
draw(_:)still only renders the wallpaper-backed background whendesktopWallpaperis non-nil, andshow()/the timers still keep scheduling.desktopWallpaperupdates. With this method always clearing the image, theshowsMenuBarBackground == falsefull/split path can never rebuild its wallpaper content anymore. If that mode is intentionally gone, the callers/config should be pruned in the same PR; otherwise this is a visible regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/MenuBar/Appearance/MenuBarOverlayPanel.swift` around lines 462 - 466, The current updateDesktopWallpaper(for:with:) unconditionally clears desktopWallpaper which prevents the wallpaper-backed overlay from ever being rebuilt; instead, only clear desktopWallpaper when the overlay should not display wallpaper (e.g., when showsMenuBarBackground is true or display is inactive), otherwise capture and set desktopWallpaper so draw(_:) can render it; update the logic in updateDesktopWallpaper(for:with:) to conditionally set desktopWallpaper (rather than always nil) and ensure callers like show() and the .desktopWallpaper timer still trigger this method to refresh the image when needed.Thaw/Events/HIDEventManager.swift (1)
818-823:⚠️ Potential issue | 🟠 MajorDon't lock out tooltips while
ControlItem.windowIDis still resolving.In
Thaw/MenuBar/ControlItem/ControlItem.swift,windowIDis lazily resolved and can stayniluntil its delayed retry runs. If that happens here, this branch falls through toreturn, buttooltipHoveredWindowIDwas already set tohoveredID, so Lines 792-795 suppress every later retry while the pointer stays on the same control item. Please keep a pre-resolution fallback here or clear the hover token on this miss so the tooltip can recover once the window ID appears.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/Events/HIDEventManager.swift` around lines 818 - 823, The current branch in HIDEventManager handling hoveredID exits early when a ControlItem's lazy ControlItem.windowID is still nil, which leaves tooltipHoveredWindowID set and prevents later retries; update the branch that checks appState.menuBarManager.sections.contains(where:) to treat a matching ControlItem with a nil windowID as a pre-resolution match (set displayName = Constants.displayName) or, if you prefer not to show the fallback tooltip, explicitly clear tooltipHoveredWindowID (set it to nil) before returning so the hover token doesn't block subsequent resolution; reference tooltipHoveredWindowID and ControlItem.windowID in your change inside the HIDEventManager handling code.
🧹 Nitpick comments (11)
Thaw/MenuBar/Search/MenuBarSearchPanel.swift (1)
512-529: Use an explicit loading state instead ofmanagedItems.isEmpty.This branch can no longer distinguish "initial refresh in progress" from "refresh finished but the cache is still empty," so the panel can end up showing “Loading menu bar items…” forever. Driving this view from a real loading/error state would keep the placeholder truthful now that the permission-specific empty state is gone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/MenuBar/Search/MenuBarSearchPanel.swift` around lines 512 - 529, The view currently uses managedItems.isEmpty/hasItems to decide whether to show the “Loading menu bar items…” placeholder, which cannot distinguish initial loading from a legitimately empty cache; change the view to drive its UI from an explicit loading/error state exposed by the model (e.g. add a Bool or enum like model.isLoading / model.loadingState on the view model that is updated by the refresh logic) and replace the hasItems check with something like: if model.isLoading show the loading placeholder, else if model.displayedItems.isEmpty show the empty-state, else show SectionedList(selection: $model.selection, items: $model.displayedItems, isEditing: model.editingItemTag != nil). Ensure the model’s refresh path sets isLoading true at start and false on completion (and surfaces any error) so MenuBarSearchPanel’s mainContent renders correctly.Thaw/MenuBar/IceBar/IceBar.swift (1)
599-629: Consider extracting shared view modifiers to reduce duplication.Lines 599-628 nearly duplicate lines 572-598, sharing the same padding, background, contentShape, overlay, animation, and accessibility modifiers. While the fallback to app icons is a good UX improvement, this introduces ~30 lines of duplicated view modifier chains.
Example: Extract shared modifiers to a ViewBuilder helper
`@ViewBuilder` private func itemContainer<Content: View>( `@ViewBuilder` content: () -> Content ) -> some View { content() .padding(.horizontal, 3) .background { RoundedRectangle(cornerRadius: 16, style: .continuous) .fill((isLightBackground ? Color.black : Color.white).opacity(isHovered ? 0.15 : 0)) .padding(.vertical, 3) } .contentShape(Rectangle()) .overlay { IceBarItemClickView( item: item, tooltipDelay: tooltipDelay, leftClickAction: leftClickAction, rightClickAction: rightClickAction, onHover: { hovering in isHovered = hovering } ) } .animation(.easeInOut(duration: 0.15), value: isHovered) .accessibilityLabel(item.displayName) .accessibilityAction(named: "left click", leftClickAction) .accessibilityAction(named: "right click", rightClickAction) }Then use:
if let image { itemContainer { Image(nsImage: image) .interpolation(.high) .antialiased(true) .resizable() .frame(width: size.width, height: size.height) } } else if let appIcon = item.sourceApplication?.icon ?? item.owningApplication?.icon { itemContainer { Image(nsImage: appIcon) .interpolation(.high) .antialiased(true) .resizable() .aspectRatio(contentMode: .fit) .frame(width: iconSize, height: iconSize) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/MenuBar/IceBar/IceBar.swift` around lines 599 - 629, The duplicated view-modifier chain around the Image in the fallback appIcon branch (same modifiers used earlier) should be extracted into a shared ViewBuilder helper (e.g., itemContainer) so both branches reuse the same padding, background, contentShape, overlay (IceBarItemClickView with tooltipDelay, leftClickAction, rightClickAction and onHover updating isHovered), animation (bound to isHovered), and accessibility modifiers (accessibilityLabel/action). Implement a private func itemContainer<Content: View>(`@ViewBuilder` content: () -> Content) -> some View that applies those modifiers using the existing symbols isLightBackground, isHovered, item, tooltipDelay, leftClickAction, rightClickAction, and replace the duplicated chains in both the Image(nsImage: image) and Image(nsImage: appIcon) branches with calls to itemContainer { /* image view only */ }.Thaw/Resources/Localizable.xcstrings (1)
10080-10087: Backfill localized variants for newly added icon-override strings.
These keys are ready in source language, but non-English locales will fall back to English until translated. Consider queuing them in the next l10n sync before release.Also applies to: 11817-11820, 17189-17196, 18095-18098, 19616-19619, 23339-23342, 24752-24755
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/Resources/Localizable.xcstrings` around lines 10080 - 10087, The new source-language strings ("Export to Clipboard" and "Export your overrides to share or reset back to application defaults.") were added but not backfilled into non‑English Localizable.xcstrings, so non‑English locales will fall back to English; add translated entries for these keys into each locale's Localizable.xcstrings (and the other new keys referenced by ranges 11817-11820, 17189-17196, 18095-18098, 19616-19619, 23339-23342, 24752-24755) or enqueue them for the next l10n sync so each locale contains the corresponding localized key/value pairs rather than falling back to English.Thaw/Utilities/AssetCatalogReader.swift (3)
25-34: Consider usingDiagLoginstead ofprint()for consistency.Line 30 uses
print()for the manifest loading error, while the rest of the codebase usesDiagLog. This message won't appear in the app's structured logging.♻️ Proposed fix
+ private static let diagLog = DiagLog(category: "AssetCatalogReader") + static let manifest: [String: AppMapping] = { guard let url = Bundle.main.url(forResource: "AppIcons", withExtension: "json"), let data = try? Data(contentsOf: url), let decoded = try? JSONDecoder().decode([String: AppMapping].self, from: data) else { - print("⚠️ Failed to load AppIcons.json manifest") + diagLog.warning("Failed to load AppIcons.json manifest") return [:] } return decoded }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/Utilities/AssetCatalogReader.swift` around lines 25 - 34, The error message in AssetCatalogReader's static property manifest currently uses print("⚠️ Failed to load AppIcons.json manifest"); replace that print call with the project's structured logger DiagLog (e.g., DiagLog.error or appropriate DiagLog API) so the manifest load failure is logged consistently; update the manifest initialization block in AssetCatalogReader to call DiagLog with a clear message and include any available error context if present.
155-160: Merge nestedifstatements for clarity.The static analyzer suggests merging the nested
ifstatements. This is a minor readability improvement.♻️ Proposed fix
// TIER 1: The Known-App Manifest (Instant & 100% Accurate) - if let mapping = manifest[bundleID] { - if let result = resolveIcon(mapping.defaultIcon, bundle: bundle, forceTemplate: mapping.forceTemplate == true) { - return result - } + if let mapping = manifest[bundleID], + let result = resolveIcon(mapping.defaultIcon, bundle: bundle, forceTemplate: mapping.forceTemplate == true) + { + return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/Utilities/AssetCatalogReader.swift` around lines 155 - 160, Merge the nested optional-bindings into a single if-let condition: locate the block that checks `if let mapping = manifest[bundleID] { if let result = resolveIcon(mapping.defaultIcon, bundle: bundle, forceTemplate: mapping.forceTemplate == true) { return result } }` and combine the two lets so the `mapping` and `result` are bound in one `if let` statement (keep the `resolveIcon` call and the `forceTemplate` check intact) and return `result` inside the single branch.
104-107: Hardcoded/usr/bin/xcrunpath.The static analyzer flagged the hardcoded executable path. While
/usr/bin/xcrunis a standard location on macOS and unlikely to change, consider usingwhich xcrunor allowing configuration for edge cases (e.g., custom Xcode toolchain paths).That said, this is low risk for a macOS-only app targeting standard developer tools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/Utilities/AssetCatalogReader.swift` around lines 104 - 107, The code in extractAssetNames(from:) hardcodes "/usr/bin/xcrun"; change Process invocation to resolve xcrun dynamically (e.g. use "/usr/bin/env" as the executable and prepend "xcrun" to the arguments: process.executableURL = URL(fileURLWithPath: "/usr/bin/env") and process.arguments = ["xcrun", "--sdk", "macosx", "assetutil", "--info", carURL.path]) or alternatively resolve the full path by running "which xcrun" once and caching it; also consider allowing the xcrun path to be injected/configured for custom toolchains.Thaw/MenuBar/MenuBarItems/MenuBarIconProvider.swift (2)
140-167: Remove unuseditemparameter or prefix with underscore.The static analyzer correctly identifies that
itemis not used in this function. The icon is derived fromappState.settings.general.iceIcon, not from the item itself.♻️ Proposed fix
`@MainActor` private static func renderThawControlItem( - _ item: MenuBarItem, + _ _: MenuBarItem, appState: AppState, canvasSize: CGSize, scale: CGFloat ) -> MenuBarItemImageCache.CapturedImage? {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/MenuBar/MenuBarItems/MenuBarIconProvider.swift` around lines 140 - 167, The parameter `item` in the static function renderThawControlItem is unused; either remove it from the function signature or rename it to `_ item` (underscore-prefixed) to silence the analyzer and clarify intent; update all callers of renderThawControlItem accordingly (or adjust their invocations if you only rename the parameter) so the function signature change is consistent across references such as renderThawControlItem(_:appState:canvasSize:scale:) in MenuBarIconProvider.
169-217: Clock format may not match user's system preferences.The hardcoded template
"EEE d MMM h:mm a"may differ from the user's actual menu bar clock format, which is configurable in System Preferences. Users who have customized their clock (e.g., 24-hour format, no date, no day) will see a mismatch in the layout pane.Consider reading the user's clock preferences from
com.apple.menuextra.clockdefaults if a more accurate representation is desired, or document that this is an approximation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/MenuBar/MenuBarItems/MenuBarIconProvider.swift` around lines 169 - 217, renderClockText currently forces a fixed DateFormatter template ("EEE d MMM h:mm a") which can mismatch the user's System Preferences (24‑hour, hide date/day, etc.); update renderClockText to respect the user's actual menu bar clock settings by reading the com.apple.menuextra.clock defaults and constructing the DateFormatter format accordingly (or fall back to a locale-aware DateFormatter if no settings found), using the existing DateFormatter/formatting logic inside renderClockText and preserving the same drawing/layout code paths and return type MenuBarItemImageCache.CapturedImage.Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift (1)
496-506: Static analysis: Nested closure depth.The static analyzer flagged excessive closure nesting at line 503. The code is still readable, but could be refactored for clarity.
♻️ Optional: Extract inner closure to reduce nesting
- images = images.filter { key, _ in - if key.isSystemItem { - return allValidTags.contains(key) - } - return allValidTags.contains(where: { $0.matchesIgnoringWindowID(key) }) - } + images = images.filter { key, _ in + key.isSystemItem + ? allValidTags.contains(key) + : allValidTags.contains(where: { $0.matchesIgnoringWindowID(key) }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift` around lines 496 - 506, Refactor the nested closure inside the images.filter call to a small helper function to reduce closure depth: create a local function (e.g., func isValidImageKey(_ key: MenuItemKey) -> Bool) that returns key.isSystemItem ? allValidTags.contains(key) : allValidTags.contains(where: { $0.matchesIgnoringWindowID(key) }), then replace the filter body with images = images.filter { key, _ in isValidImageKey(key) } and keep the subsequent call to validateAndCleanupInvalidEntries() unchanged.Thaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swift (1)
153-155: Consider:UserDefaults.didChangeNotificationfires for all defaults changes.This notification fires whenever any UserDefaults value changes, not just the icon overrides key. For a busy app, this could cause unnecessary state refreshes. Consider using
UserDefaults.publisher(for:)with key-value observing for more precise updates.♻️ Optional refinement using KVO
// Replace the onReceive with targeted KVO: .onReceive( UserDefaults.standard.publisher(for: \.menuBarItemIconOverrides) .map { _ in () } ) { _ in hasOverrides = !AssetCatalogReader.overrides.isEmpty }Note: This requires adding an
@objc dynamiccomputed property or using the raw key path ifmenuBarItemIconOverridesisn't already KVO-compliant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Thaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swift` around lines 153 - 155, Replace the broad NotificationCenter-based listener used in the onReceive block (currently listening to UserDefaults.didChangeNotification) with a targeted UserDefaults publisher for the specific icon-overrides key so hasOverrides is only updated when that key changes; specifically, switch the onReceive to use UserDefaults.standard.publisher(for: ...) for the menuBarItemIconOverrides key (or a KVO-compliant key path) and keep the body that sets hasOverrides = !AssetCatalogReader.overrides.isEmpty; if the key isn’t KVO-compliant, make it so (e.g., add an `@objc` dynamic wrapper or use the raw key path publisher) so the change notifications are precise.Shared/Utilities/AXHelpers.swift (1)
71-75: Consider: Redundant synchronization when called from background queue.
resolveTitles(for:)wraps the body inqueue.sync, but the only caller (MenuBarItem.getMenuBarItemsLegacyMethod) already dispatches toDispatchQueue.global(qos: .userInteractive). This results in a background thread blocking onqueue.syncwhich then dispatches to the same (or compatible) global concurrent queue.While not incorrect, this adds synchronization overhead. The
queue.syncwrapper is appropriate for the single-call accessors (liketitle(for:)) to serialize AX API access, but forresolveTitleswhich performs many AX calls internally, consider whether the wrapper is necessary or if the internal calls to individual AX APIs already provide sufficient serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Shared/Utilities/AXHelpers.swift` around lines 71 - 75, resolveTitles(for:) currently wraps resolveTitlesBody(for:) in queue.sync which redundantly blocks a background caller (MenuBarItem.getMenuBarItemsLegacyMethod) and adds unnecessary serialization; remove the outer queue.sync and call resolveTitlesBody(for:) directly from resolveTitles(for:) so the bulk AX work runs on the caller's background queue, or alternatively change to queue.async if you need to offload work onto the serial queue—adjust only resolveTitles(for:) (leave resolveTitlesBody(for:) and other single-accessors like title(for:) that rely on queue.sync intact) and ensure MenuBarItem.getMenuBarItemsLegacyMethod still dispatches to a background queue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MenuBarItemService/Listener.swift`:
- Around line 40-42: The current diagnostic log prints raw AX title from
SourcePIDCache.shared.pidAndTitle(for: window) which may contain user content;
change the diagLog.debug call to avoid emitting the actual axTitle (e.g., log
axTitle != nil or replace with a fixed "<redacted>" or length flag) while
leaving the returned value .sourcePID(pid, axTitle) unchanged; update the debug
message at the diagLog.debug site so it records only presence/absence or a
redacted token for axTitle instead of the verbatim string.
In `@Thaw/MenuBar/ControlItem/ControlItem.swift`:
- Around line 243-259: retryWindowIDResolution currently bails when
statusItem.button?.window is nil and never attempts to resolve _resolvedWindowID
from the menu-bar window list; update it to iterate the results of
Bridging.getMenuBarWindowList(option: .itemsOnly), compute/obtain each window's
screen rect (using the existing CGSGetScreenRectForWindow or equivalent helper),
compare those rects against the status item/button frame (or the control's known
bounds) to find the best match, set _resolvedWindowID to the matching
CGWindowID, and emit the same Self.diagLog.debug message including the resolved
id and self.identifier.rawValue when found so failed snapshot-diff attempts can
recover.
- Around line 96-100: The accessibility title is being set to the internal token
controlItem.identifier.rawValue which exposes developer identifiers to assistive
tech; change the value passed to button.setAccessibilityTitle to the
user-facing, localized label from MenuBarSection.displayString (use
controlItem.section.displayString or the equivalent accessor on the ControlItem
that returns the localized label) so VoiceOver sees "Visible", "Hidden", or
"Always-Hidden" instead of "Thaw.ControlItem.*"; after updating, verify with
VoiceOver that the button announces the localized label.
In `@Thaw/MenuBar/LayoutBar/LayoutBarItemView.swift`:
- Around line 220-222: The NSPopover's contentSize (popover.contentSize =
NSSize(width: 280, height: 240)) does not match the SwiftUI IconPickerView frame
(.frame(height: 300)), causing clipping; fix by making these sizes consistent —
either update popover.contentSize height to 300 to match IconPickerView, or
change IconPickerView's .frame(height:) to 240 (and adjust width if needed), and
ensure any padding/margins in IconPickerView won't exceed the chosen height so
the SF Symbol search field and Reset button remain fully visible.
In `@Thaw/MenuBar/MenuBarItems/MenuBarIconProvider.swift`:
- Around line 27-56: The icon(for:scale:) method is marked `@MainActor` but calls
AssetCatalogReader.findBestIcon() (which can synchronously call
discoverAllIcons() -> extractAssetNames() that spawns a blocking Process),
causing main-thread stalls; move asset-catalog work off the main actor by (a)
making icon(for:scale:) async or adding a non-mainActor helper (e.g.,
loadIconOffMain(for:scale:) or AssetCatalogReader.findBestIconAsync) that runs
discoverAllIcons()/extractAssetNames() on a background queue, (b) add a memoized
cache for extractAssetNames()/discoverAllIcons() results keyed by bundle URL to
avoid repeated xcrun invocations, and (c) ensure final image rendering and any
UI-bound updates re-enter MainActor before returning the
MenuBarItemImageCache.CapturedImage so UI stays responsive.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift`:
- Around line 466-480: The loop inside updateCacheWithoutChecks on
MenuBarItemImageCache calls MenuBarIconProvider.icon(for:scale:) on the
MainActor which can synchronously spawn blocking subprocesses; move icon
resolution off the main thread by collecting sectionItems, performing
icon(for:scale:) calls concurrently on a background task (e.g., Task.detached or
a global DispatchQueue) and gather results into a temporary dictionary, then
switch back to the MainActor to merge those results into newImages and log the
catalogCount; ensure you reference MenuBarIconProvider.icon(for:scale:),
updateCacheWithoutChecks, itemManager.itemCache.managedItems(for:), and
newImages when implementing the change.
In `@Thaw/MenuBar/MenuBarManager.swift`:
- Line 328: The empty updateAverageColorInfo() implementation prevents
publishing wallpaper/appearance changes so cached icons never invalidate;
restore or implement updateAverageColorInfo() to recompute and publish the new
averageColorInfo (or send on its Passthrough/CurrentValueSubject) whenever
appearance/wallpaper changes are detected in configureCancellables(), so
MenuBarItemImageCache’s $averageColorInfo subscribers receive updates and
invalidate appropriately; reference the updateAverageColorInfo() function,
configureCancellables(), and the $averageColorInfo publisher used by
MenuBarItemImageCache to locate where to re-enable publishing.
In `@Thaw/MenuBar/Search/MenuBarSearchModel.swift`:
- Line 71: The updateAverageColorInfo(for:) method is empty which leaves
model.averageColorInfo permanently nil while configureCancellables(with:) and
MenuBarSearchPanel.swift still depend on it; either implement a permissionless
fallback to populate averageColorInfo from the given NSScreen (e.g., sample the
screen or use screen.background/colorSpace/appearance heuristics) inside
updateAverageColorInfo(for:) so the panel can reacquire the active menu bar
color, or if the feature is intentionally removed, delete the publisher/consumer
plumbing (calls in configureCancellables(with:), model.averageColorInfo usage in
MenuBarSearchPanel) to fully remove the dead path; locate the relevant symbols
updateAverageColorInfo(for:), configureCancellables(with:), and
model.averageColorInfo to apply the change.
In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 15069-15071: The "No icons found" localization entry is an empty
object so translators lack context; update the entry for the "No icons found"
key by adding minimal metadata (e.g., a "comment" describing where/when this
message appears and a "value"/"translation" field preserving the source text) so
the catalog keeps context for translators—locate the "No icons found" key in
Localizable.xcstrings and populate its object with a concise comment and the
existing string as the value.
- Around line 18602-18604: The localization entry for the key "Right-click any
icon to choose an alternative." is empty; add the required metadata/comments for
translators and any context by populating the entry's comment/value fields
(e.g., add a translator note and developer comment) so the string is not left
blank—locate the empty entry with the exact key "Right-click any icon to choose
an alternative." and fill its metadata/comment block with concise context
describing when and where this instruction appears and any UI constraints.
In `@Thaw/Utilities/AssetCatalogReader.swift`:
- Around line 103-137: The extractAssetNames(from:) function currently spawns
xcrun synchronously and can block the main actor; change it to an async API (or
accept a completion handler) and run the Process off the main thread, add a
timeout to kill the Process if it hangs, and introduce a simple cache keyed by
carURL (e.g., an in-memory [URL: Set<String>] or NSCache) so repeated calls from
MenuBarIconProvider.icon() return cached names immediately; ensure the async
variant returns cached results first if present, otherwise performs the
background extraction, stores the result in the cache, and calls back on the
main actor.
---
Outside diff comments:
In `@Thaw/Events/HIDEventManager.swift`:
- Around line 818-823: The current branch in HIDEventManager handling hoveredID
exits early when a ControlItem's lazy ControlItem.windowID is still nil, which
leaves tooltipHoveredWindowID set and prevents later retries; update the branch
that checks appState.menuBarManager.sections.contains(where:) to treat a
matching ControlItem with a nil windowID as a pre-resolution match (set
displayName = Constants.displayName) or, if you prefer not to show the fallback
tooltip, explicitly clear tooltipHoveredWindowID (set it to nil) before
returning so the hover token doesn't block subsequent resolution; reference
tooltipHoveredWindowID and ControlItem.windowID in your change inside the
HIDEventManager handling code.
In `@Thaw/MenuBar/Appearance/MenuBarOverlayPanel.swift`:
- Around line 462-466: The current updateDesktopWallpaper(for:with:)
unconditionally clears desktopWallpaper which prevents the wallpaper-backed
overlay from ever being rebuilt; instead, only clear desktopWallpaper when the
overlay should not display wallpaper (e.g., when showsMenuBarBackground is true
or display is inactive), otherwise capture and set desktopWallpaper so draw(_:)
can render it; update the logic in updateDesktopWallpaper(for:with:) to
conditionally set desktopWallpaper (rather than always nil) and ensure callers
like show() and the .desktopWallpaper timer still trigger this method to refresh
the image when needed.
---
Nitpick comments:
In `@Shared/Utilities/AXHelpers.swift`:
- Around line 71-75: resolveTitles(for:) currently wraps resolveTitlesBody(for:)
in queue.sync which redundantly blocks a background caller
(MenuBarItem.getMenuBarItemsLegacyMethod) and adds unnecessary serialization;
remove the outer queue.sync and call resolveTitlesBody(for:) directly from
resolveTitles(for:) so the bulk AX work runs on the caller's background queue,
or alternatively change to queue.async if you need to offload work onto the
serial queue—adjust only resolveTitles(for:) (leave resolveTitlesBody(for:) and
other single-accessors like title(for:) that rely on queue.sync intact) and
ensure MenuBarItem.getMenuBarItemsLegacyMethod still dispatches to a background
queue.
In `@Thaw/MenuBar/IceBar/IceBar.swift`:
- Around line 599-629: The duplicated view-modifier chain around the Image in
the fallback appIcon branch (same modifiers used earlier) should be extracted
into a shared ViewBuilder helper (e.g., itemContainer) so both branches reuse
the same padding, background, contentShape, overlay (IceBarItemClickView with
tooltipDelay, leftClickAction, rightClickAction and onHover updating isHovered),
animation (bound to isHovered), and accessibility modifiers
(accessibilityLabel/action). Implement a private func itemContainer<Content:
View>(`@ViewBuilder` content: () -> Content) -> some View that applies those
modifiers using the existing symbols isLightBackground, isHovered, item,
tooltipDelay, leftClickAction, rightClickAction, and replace the duplicated
chains in both the Image(nsImage: image) and Image(nsImage: appIcon) branches
with calls to itemContainer { /* image view only */ }.
In `@Thaw/MenuBar/MenuBarItems/MenuBarIconProvider.swift`:
- Around line 140-167: The parameter `item` in the static function
renderThawControlItem is unused; either remove it from the function signature or
rename it to `_ item` (underscore-prefixed) to silence the analyzer and clarify
intent; update all callers of renderThawControlItem accordingly (or adjust their
invocations if you only rename the parameter) so the function signature change
is consistent across references such as
renderThawControlItem(_:appState:canvasSize:scale:) in MenuBarIconProvider.
- Around line 169-217: renderClockText currently forces a fixed DateFormatter
template ("EEE d MMM h:mm a") which can mismatch the user's System Preferences
(24‑hour, hide date/day, etc.); update renderClockText to respect the user's
actual menu bar clock settings by reading the com.apple.menuextra.clock defaults
and constructing the DateFormatter format accordingly (or fall back to a
locale-aware DateFormatter if no settings found), using the existing
DateFormatter/formatting logic inside renderClockText and preserving the same
drawing/layout code paths and return type MenuBarItemImageCache.CapturedImage.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift`:
- Around line 496-506: Refactor the nested closure inside the images.filter call
to a small helper function to reduce closure depth: create a local function
(e.g., func isValidImageKey(_ key: MenuItemKey) -> Bool) that returns
key.isSystemItem ? allValidTags.contains(key) : allValidTags.contains(where: {
$0.matchesIgnoringWindowID(key) }), then replace the filter body with images =
images.filter { key, _ in isValidImageKey(key) } and keep the subsequent call to
validateAndCleanupInvalidEntries() unchanged.
In `@Thaw/MenuBar/Search/MenuBarSearchPanel.swift`:
- Around line 512-529: The view currently uses managedItems.isEmpty/hasItems to
decide whether to show the “Loading menu bar items…” placeholder, which cannot
distinguish initial loading from a legitimately empty cache; change the view to
drive its UI from an explicit loading/error state exposed by the model (e.g. add
a Bool or enum like model.isLoading / model.loadingState on the view model that
is updated by the refresh logic) and replace the hasItems check with something
like: if model.isLoading show the loading placeholder, else if
model.displayedItems.isEmpty show the empty-state, else show
SectionedList(selection: $model.selection, items: $model.displayedItems,
isEditing: model.editingItemTag != nil). Ensure the model’s refresh path sets
isLoading true at start and false on completion (and surfaces any error) so
MenuBarSearchPanel’s mainContent renders correctly.
In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 10080-10087: The new source-language strings ("Export to
Clipboard" and "Export your overrides to share or reset back to application
defaults.") were added but not backfilled into non‑English
Localizable.xcstrings, so non‑English locales will fall back to English; add
translated entries for these keys into each locale's Localizable.xcstrings (and
the other new keys referenced by ranges 11817-11820, 17189-17196, 18095-18098,
19616-19619, 23339-23342, 24752-24755) or enqueue them for the next l10n sync so
each locale contains the corresponding localized key/value pairs rather than
falling back to English.
In `@Thaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swift`:
- Around line 153-155: Replace the broad NotificationCenter-based listener used
in the onReceive block (currently listening to
UserDefaults.didChangeNotification) with a targeted UserDefaults publisher for
the specific icon-overrides key so hasOverrides is only updated when that key
changes; specifically, switch the onReceive to use
UserDefaults.standard.publisher(for: ...) for the menuBarItemIconOverrides key
(or a KVO-compliant key path) and keep the body that sets hasOverrides =
!AssetCatalogReader.overrides.isEmpty; if the key isn’t KVO-compliant, make it
so (e.g., add an `@objc` dynamic wrapper or use the raw key path publisher) so the
change notifications are precise.
In `@Thaw/Utilities/AssetCatalogReader.swift`:
- Around line 25-34: The error message in AssetCatalogReader's static property
manifest currently uses print("⚠️ Failed to load AppIcons.json manifest");
replace that print call with the project's structured logger DiagLog (e.g.,
DiagLog.error or appropriate DiagLog API) so the manifest load failure is logged
consistently; update the manifest initialization block in AssetCatalogReader to
call DiagLog with a clear message and include any available error context if
present.
- Around line 155-160: Merge the nested optional-bindings into a single if-let
condition: locate the block that checks `if let mapping = manifest[bundleID] {
if let result = resolveIcon(mapping.defaultIcon, bundle: bundle, forceTemplate:
mapping.forceTemplate == true) { return result } }` and combine the two lets so
the `mapping` and `result` are bound in one `if let` statement (keep the
`resolveIcon` call and the `forceTemplate` check intact) and return `result`
inside the single branch.
- Around line 104-107: The code in extractAssetNames(from:) hardcodes
"/usr/bin/xcrun"; change Process invocation to resolve xcrun dynamically (e.g.
use "/usr/bin/env" as the executable and prepend "xcrun" to the arguments:
process.executableURL = URL(fileURLWithPath: "/usr/bin/env") and
process.arguments = ["xcrun", "--sdk", "macosx", "assetutil", "--info",
carURL.path]) or alternatively resolve the full path by running "which xcrun"
once and caching it; also consider allowing the xcrun path to be
injected/configured for custom toolchains.
🪄 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: a4eb2355-3d56-4b6d-8b82-ee15f4534f45
📒 Files selected for processing (27)
MenuBarItemService/Listener.swiftMenuBarItemService/SourcePIDCache.swiftShared/Services/MenuBarItemService.swiftShared/Utilities/AXHelpers.swiftThaw/Events/HIDEventManager.swiftThaw/Main/AppState.swiftThaw/MenuBar/Appearance/MenuBarOverlayPanel.swiftThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/IceBar/IceBar.swiftThaw/MenuBar/IceBar/IceBarColorManager.swiftThaw/MenuBar/LayoutBar/LayoutBarItemView.swiftThaw/MenuBar/MenuBarItems/MenuBarIconProvider.swiftThaw/MenuBar/MenuBarItems/MenuBarItem.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemServiceConnection.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/Search/MenuBarSearchModel.swiftThaw/MenuBar/Search/MenuBarSearchPanel.swiftThaw/Permissions/AppPermissions.swiftThaw/Permissions/Permission.swiftThaw/Resources/AppIcons.jsonThaw/Resources/Localizable.xcstringsThaw/Settings/SettingsPanes/AdvancedSettingsPane.swiftThaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swiftThaw/Utilities/AssetCatalogReader.swiftThaw/Utilities/ScreenCapture.swift
💤 Files with no reviewable changes (2)
- Thaw/Permissions/Permission.swift
- Thaw/Utilities/ScreenCapture.swift
| let (pid, axTitle) = SourcePIDCache.shared.pidAndTitle(for: window) | ||
| diagLog.debug("Listener: sourcePID response for windowID=\(window.windowID) -> pid=\(pid.map { "\($0)" } ?? "nil"), axTitle=\(axTitle ?? "nil")") | ||
| return .sourcePID(pid, axTitle) |
There was a problem hiding this comment.
Avoid writing raw AX titles into diagnostics.
axTitle can contain user content from third-party menu bar items, and this debug line persists it verbatim when diagnostic logging is enabled. Please redact or omit the title here, or log only whether one was present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MenuBarItemService/Listener.swift` around lines 40 - 42, The current
diagnostic log prints raw AX title from SourcePIDCache.shared.pidAndTitle(for:
window) which may contain user content; change the diagLog.debug call to avoid
emitting the actual axTitle (e.g., log axTitle != nil or replace with a fixed
"<redacted>" or length flag) while leaving the returned value .sourcePID(pid,
axTitle) unchanged; update the debug message at the diagLog.debug site so it
records only presence/absence or a redacted token for axTitle instead of the
verbatim string.
| // Set the accessibility title so that the AX API returns | ||
| // the control item identifier, matching the CG window title | ||
| // (autosaveName). This allows item identification without | ||
| // screen recording permission. | ||
| button.setAccessibilityTitle(controlItem.identifier.rawValue) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the ControlItem.swift file
find . -name "ControlItem.swift" -type fRepository: stonerl/Thaw
Length of output: 100
🏁 Script executed:
# Examine the specific lines 96-100 and surrounding context
cat -n Thaw/MenuBar/ControlItem/ControlItem.swift | sed -n '80,115p'Repository: stonerl/Thaw
Length of output: 2529
🏁 Script executed:
# Search for the ControlItem.identifier enum definition
rg -A 10 "enum.*identifier" Thaw/MenuBar/ControlItem/ControlItem.swiftRepository: stonerl/Thaw
Length of output: 38
🏁 Script executed:
# Check what values are used for identifier in ControlItem
rg "\.identifier" Thaw/MenuBar/ControlItem/ControlItem.swift -B 2 -A 2Repository: stonerl/Thaw
Length of output: 2703
🏁 Script executed:
# Search for setAccessibilityTitle usage in the codebase to see patterns
rg "setAccessibilityTitle" --type swift -B 3 -A 1Repository: stonerl/Thaw
Length of output: 549
🏁 Script executed:
# Find the Identifier enum definition in ControlItem.swift
rg -B 5 "enum Identifier" Thaw/MenuBar/ControlItem/ControlItem.swift -A 20Repository: stonerl/Thaw
Length of output: 1167
🏁 Script executed:
# Look for case definitions in the Identifier enum
rg "case visible|case hidden|case alwaysHidden" Thaw/MenuBar/ControlItem/ControlItem.swift -B 1 -A 1Repository: stonerl/Thaw
Length of output: 438
🏁 Script executed:
# Check the full Identifier type definition
cat -n Thaw/MenuBar/ControlItem/ControlItem.swift | sed -n '1,60p'Repository: stonerl/Thaw
Length of output: 2399
🏁 Script executed:
# Check if there are user-facing labels for these control item sections elsewhere
rg "Visible|Hidden" Thaw/Resources/Localizable.xcstrings -C 2 | head -100Repository: stonerl/Thaw
Length of output: 2260
🏁 Script executed:
# Search for any localization or user-facing strings related to control items
rg -i "control.*item|visible.*section|hidden.*section" Thaw/Resources/ --type strings -B 1 -A 1 | head -80Repository: stonerl/Thaw
Length of output: 87
🏁 Script executed:
# Check if there are accessibility-specific strings defined elsewhere
rg "accessibility" Thaw/MenuBar/ControlItem/ControlItem.swift -B 2 -A 2Repository: stonerl/Thaw
Length of output: 1438
🏁 Script executed:
# Check if there's any alternative approach using user-facing strings for accessibility
rg "setAccessibilityTitle" Thaw/ --type swift -B 5 -A 2Repository: stonerl/Thaw
Length of output: 866
🏁 Script executed:
# Check how the localized strings for sections are typically used
rg '"Visible"|"Hidden"|"Always-Hidden"' Thaw/ --type swift -B 2 -A 2 | head -80Repository: stonerl/Thaw
Length of output: 2725
🏁 Script executed:
# Verify the autosaveName usage - see if it also uses the same internal token
rg "autosaveName.*Identifier|identifier.*autosaveName" Thaw/MenuBar/ControlItem/ControlItem.swift -B 3 -A 3Repository: stonerl/Thaw
Length of output: 410
Use a user-facing label for the accessibility title instead of the internal identifier token.
Setting the accessibility title to controlItem.identifier.rawValue exposes Thaw.ControlItem.Visible, Thaw.ControlItem.Hidden, and Thaw.ControlItem.AlwaysHidden to assistive technology users instead of the proper labels "Visible", "Hidden", and "Always-Hidden" defined in MenuBarSection.displayString. Use the localized section label for accessibility and verify the result with VoiceOver before shipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/MenuBar/ControlItem/ControlItem.swift` around lines 96 - 100, The
accessibility title is being set to the internal token
controlItem.identifier.rawValue which exposes developer identifiers to assistive
tech; change the value passed to button.setAccessibilityTitle to the
user-facing, localized label from MenuBarSection.displayString (use
controlItem.section.displayString or the equivalent accessor on the ControlItem
that returns the localized label) so VoiceOver sees "Visible", "Hidden", or
"Always-Hidden" instead of "Thaw.ControlItem.*"; after updating, verify with
VoiceOver that the button announces the localized label.
| private func retryWindowIDResolution() { | ||
| // On macOS 26, try to get the window from the button (it may | ||
| // have become available after a delay). | ||
| if let win = statusItem.button?.window { | ||
| _resolvedWindowID = CGWindowID(exactly: win.windowNumber) | ||
| if let wid = _resolvedWindowID { | ||
| Self.diagLog.debug("Retry resolved windowID \(wid) for \(self.identifier.rawValue) via button.window") | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // As a last resort, scan all menu bar windows and try to match | ||
| // using CGSGetScreenRectForWindow bounds correlation. The newly | ||
| // created status item should appear in the list. | ||
| let allWindows = Bridging.getMenuBarWindowList(option: .itemsOnly) | ||
| Self.diagLog.debug("Retry window resolution for \(self.identifier.rawValue): \(allWindows.count) menu bar windows in list, _resolvedWindowID still nil") | ||
| } |
There was a problem hiding this comment.
The retry path never repairs a failed snapshot.
When button.window is still nil—the exact macOS 26 case called out in the comment—retryWindowIDResolution() only logs the menu bar window count and returns without assigning _resolvedWindowID. A failed snapshot-diff therefore never recovers, and MenuBarItemManager stays on the weaker fallback matching path for these control items.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/MenuBar/ControlItem/ControlItem.swift` around lines 243 - 259,
retryWindowIDResolution currently bails when statusItem.button?.window is nil
and never attempts to resolve _resolvedWindowID from the menu-bar window list;
update it to iterate the results of Bridging.getMenuBarWindowList(option:
.itemsOnly), compute/obtain each window's screen rect (using the existing
CGSGetScreenRectForWindow or equivalent helper), compare those rects against the
status item/button frame (or the control's known bounds) to find the best match,
set _resolvedWindowID to the matching CGWindowID, and emit the same
Self.diagLog.debug message including the resolved id and
self.identifier.rawValue when found so failed snapshot-diff attempts can
recover.
| updateWindowImage(for: screen) | ||
| updateColorInfo(with: frame, screen: screen) | ||
| } | ||
| func performSetup(with iceBarPanel: IceBarPanel) {} |
There was a problem hiding this comment.
colorInfo now stays nil forever, so Ice Bar always renders as if the background is dark.
Thaw/MenuBar/IceBar/IceBar.swift derives foreground color from colorManager.colorInfo?.color.brightness ?? 0. With performSetup(with:) empty, that never flips on light menu bars, so text/icons stay white and can lose contrast. Please seed colorInfo from a non-capture source here, or remove the color-dependent rendering path.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 14-14: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 14-14: Remove the unused function parameter "iceBarPanel" or name it "_".
| let popover = NSPopover() | ||
| popover.behavior = .transient | ||
| popover.contentSize = NSSize(width: 280, height: 240) |
There was a problem hiding this comment.
Size mismatch between popover contentSize and SwiftUI view frame.
The popover's contentSize is set to height: 240 (line 222), but the SwiftUI IconPickerView has .frame(height: 300) (line 411). This 60pt discrepancy will cause content clipping, particularly affecting the SF Symbol search field and Reset button at the bottom.
Proposed fix: Align the sizes
let popover = NSPopover()
popover.behavior = .transient
-popover.contentSize = NSSize(width: 280, height: 240)
+popover.contentSize = NSSize(width: 280, height: 300)Also applies to: 411-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/MenuBar/LayoutBar/LayoutBarItemView.swift` around lines 220 - 222, The
NSPopover's contentSize (popover.contentSize = NSSize(width: 280, height: 240))
does not match the SwiftUI IconPickerView frame (.frame(height: 300)), causing
clipping; fix by making these sizes consistent — either update
popover.contentSize height to 300 to match IconPickerView, or change
IconPickerView's .frame(height:) to 240 (and adjust width if needed), and ensure
any padding/margins in IconPickerView won't exceed the chosen height so the SF
Symbol search field and Reset button remain fully visible.
| averageColorInfo = info | ||
| } | ||
| } | ||
| func updateAverageColorInfo() {} |
There was a problem hiding this comment.
This disables the only color-change signal the image cache listens to.
configureCancellables() still starts the 60-second refresh and calls updateAverageColorInfo(), while Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swift still invalidates cached images from $averageColorInfo. With an empty implementation, wallpaper/appearance changes never publish, so color-dependent icons can stay stale until some unrelated cache trigger fires.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 328-328: Add a nested comment explaining why this function is empty, or complete the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/MenuBar/MenuBarManager.swift` at line 328, The empty
updateAverageColorInfo() implementation prevents publishing wallpaper/appearance
changes so cached icons never invalidate; restore or implement
updateAverageColorInfo() to recompute and publish the new averageColorInfo (or
send on its Passthrough/CurrentValueSubject) whenever appearance/wallpaper
changes are detected in configureCancellables(), so MenuBarItemImageCache’s
$averageColorInfo subscribers receive updates and invalidate appropriately;
reference the updateAverageColorInfo() function, configureCancellables(), and
the $averageColorInfo publisher used by MenuBarItemImageCache to locate where to
re-enable publishing.
| averageColorInfo = info | ||
| } | ||
| } | ||
| private func updateAverageColorInfo(for screen: NSScreen) {} |
There was a problem hiding this comment.
This leaves search-panel color matching permanently disabled.
configureCancellables(with:) still calls this method, and Thaw/MenuBar/Search/MenuBarSearchPanel.swift still renders with model.averageColorInfo. With an empty body, that state is only ever cleared back to nil, so the panel never reacquires the active menu bar color. If this feature is intentionally retired, remove the dead publisher/consumer path too; otherwise hook this up to a permissionless fallback.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 71-71: Remove the unused function parameter "screen" or name it "_".
[failure] 71-71: Add a nested comment explaining why this function is empty, or complete the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/MenuBar/Search/MenuBarSearchModel.swift` at line 71, The
updateAverageColorInfo(for:) method is empty which leaves model.averageColorInfo
permanently nil while configureCancellables(with:) and MenuBarSearchPanel.swift
still depend on it; either implement a permissionless fallback to populate
averageColorInfo from the given NSScreen (e.g., sample the screen or use
screen.background/colorSpace/appearance heuristics) inside
updateAverageColorInfo(for:) so the panel can reacquire the active menu bar
color, or if the feature is intentionally removed, delete the publisher/consumer
plumbing (calls in configureCancellables(with:), model.averageColorInfo usage in
MenuBarSearchPanel) to fully remove the dead path; locate the relevant symbols
updateAverageColorInfo(for:), configureCancellables(with:), and
model.averageColorInfo to apply the change.
| "No icons found" : { | ||
|
|
||
| }, |
There was a problem hiding this comment.
"No icons found" entry is missing metadata/context.
This key is currently an empty object, so translator context is lost and catalog consistency regresses.
Suggested minimal fix
"No icons found" : {
+ "comment" : "A message shown when the icon search returns no results.",
+ "isCommentAutoGenerated" : true
},📝 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.
| "No icons found" : { | |
| }, | |
| "No icons found" : { | |
| "comment" : "A message shown when the icon search returns no results.", | |
| "isCommentAutoGenerated" : true | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/Resources/Localizable.xcstrings` around lines 15069 - 15071, The "No
icons found" localization entry is an empty object so translators lack context;
update the entry for the "No icons found" key by adding minimal metadata (e.g.,
a "comment" describing where/when this message appears and a
"value"/"translation" field preserving the source text) so the catalog keeps
context for translators—locate the "No icons found" key in Localizable.xcstrings
and populate its object with a concise comment and the existing string as the
value.
| "Right-click any icon to choose an alternative." : { | ||
|
|
||
| }, |
There was a problem hiding this comment.
"Right-click any icon to choose an alternative." entry is also empty.
Same issue here: missing metadata makes this string harder to localize and maintain.
Suggested minimal fix
"Right-click any icon to choose an alternative." : {
+ "comment" : "Helper text explaining how to choose a different icon override.",
+ "isCommentAutoGenerated" : true
},📝 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.
| "Right-click any icon to choose an alternative." : { | |
| }, | |
| "Right-click any icon to choose an alternative." : { | |
| "comment" : "Helper text explaining how to choose a different icon override.", | |
| "isCommentAutoGenerated" : true | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/Resources/Localizable.xcstrings` around lines 18602 - 18604, The
localization entry for the key "Right-click any icon to choose an alternative."
is empty; add the required metadata/comments for translators and any context by
populating the entry's comment/value fields (e.g., add a translator note and
developer comment) so the string is not left blank—locate the empty entry with
the exact key "Right-click any icon to choose an alternative." and fill its
metadata/comment block with concise context describing when and where this
instruction appears and any UI constraints.
| /// Extracts unique asset names from a compiled .car file using assetutil. | ||
| private static func extractAssetNames(from carURL: URL) -> Set<String>? { | ||
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/xcrun") | ||
| process.arguments = ["--sdk", "macosx", "assetutil", "--info", carURL.path] | ||
|
|
||
| let pipe = Pipe() | ||
| process.standardOutput = pipe | ||
| process.standardError = FileHandle.nullDevice | ||
|
|
||
| do { | ||
| try process.run() | ||
| } catch { | ||
| return nil | ||
| } | ||
|
|
||
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | ||
| process.waitUntilExit() | ||
|
|
||
| guard process.terminationStatus == 0, | ||
| let json = try? JSONSerialization.jsonObject(with: data) as? [[String: Any]] | ||
| else { | ||
| return nil | ||
| } | ||
|
|
||
| var names = Set<String>() | ||
| for entry in json { | ||
| if let name = entry["Name"] as? String, !name.isEmpty, | ||
| !name.hasPrefix("ZZZZ"), !name.hasPrefix("AppIcon") | ||
| { | ||
| names.insert(name) | ||
| } | ||
| } | ||
| return names | ||
| } |
There was a problem hiding this comment.
Performance concern: Synchronous subprocess blocks the calling thread.
extractAssetNames(from:) runs xcrun assetutil synchronously with process.run() and waitUntilExit(). When called from the main actor via MenuBarIconProvider.icon(), this blocks the main thread.
Consider:
- Caching the extracted names per bundle URL to avoid repeated subprocess calls
- Running the extraction asynchronously and providing a completion handler or async API
- Adding a timeout to prevent indefinite blocking if
assetutilhangs
♻️ Suggested caching addition
+ /// Cache of extracted asset names per bundle URL to avoid repeated subprocess calls.
+ private static var assetNameCache = [URL: Set<String>]()
+ private static let assetNameCacheLock = NSLock()
+
/// Extracts unique asset names from a compiled .car file using assetutil.
private static func extractAssetNames(from carURL: URL) -> Set<String>? {
+ // Check cache first
+ if let cached = assetNameCacheLock.withLock({ assetNameCache[carURL] }) {
+ return cached
+ }
+
let process = Process()
// ... existing implementation ...
+
+ // Cache the result
+ assetNameCacheLock.withLock {
+ assetNameCache[carURL] = names
+ }
return names
}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 106-106: Refactor your code to get this URI from a customizable parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/Utilities/AssetCatalogReader.swift` around lines 103 - 137, The
extractAssetNames(from:) function currently spawns xcrun synchronously and can
block the main actor; change it to an async API (or accept a completion handler)
and run the Process off the main thread, add a timeout to kill the Process if it
hangs, and introduce a simple cache keyed by carURL (e.g., an in-memory [URL:
Set<String>] or NSCache) so repeated calls from MenuBarIconProvider.icon()
return cached names immediately; ensure the async variant returns cached results
first if present, otherwise performs the background extraction, stores the
result in the cache, and calls back on the main actor.
2 participants
What does this PR do?
This PR introduces a robust mechanism to resolve menu bar icons without relying primarily on Screen Capture APIs. It accomplishes this by reading and extracting icons directly from app bundles (asset catalogs) and mapping system items to standard SF Symbols. This ensures icons are visible even when screen recording permissions are not granted. Additionally, it refactors the image cache for better thread safety and performance.
PR Type
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path:
What is the current behavior?
The application relies heavily on the macOS Screen Capture APIs to render menu bar icons. If Screen Recording permissions are denied or if the capture fails transiently, icons appear blank or missing. Additionally,
MenuBarItemImageCachehad potential data-race vulnerabilities around the tracking of failed captures.Issue Number: N/A
What is the new behavior?
AssetCatalogReaderto directly extract, evaluate, and score the best matching icons from third-party app bundles (Assets.carand raw layout resources).MenuBarIconProviderto reliably map known system menu bar items (e.g., WiFi, Bluetooth, Battery) to their corresponding dynamic SF Symbols.failedCapturesLock(NSLock) inMenuBarItemImageCacheto make dictionary accesses thread-safe. Optimized memory allocations by using.reserveCapacity()where appropriate.PR Checklist
Other information
This is a significant improvement for the onboarding experience, as users will now see recognizable icons immediately before having to grant any broad OS-level permissions like Screen Recording.
Notes for reviewers
AssetCatalogReader(specifically the regex weighting, size penalties, and keyword bonuses) to see if the weighting makes sense.failedCapturesdictionary inMenuBarItemImageCache.Summary by CodeRabbit
Release Notes
New Features
Improvements