Skip to content

feat: settings profiles#331

Open
nightah wants to merge 20 commits intostonerl:developmentfrom
nightah:feature/profiles
Open

feat: settings profiles#331
nightah wants to merge 20 commits intostonerl:developmentfrom
nightah:feature/profiles

Conversation

@nightah
Copy link
Copy Markdown
Collaborator

@nightah nightah commented Mar 25, 2026

What does this PR do?

Introduces a complete settings profiles system for Thaw, allowing users to save, restore, and automatically switch between different menu bar configurations.

  • Profile management — save, rename, duplicate, delete, import, and export full app configuration snapshots including menu bar item layout, section assignments, appearance, general/advanced settings, hotkeys, and display configurations
  • Display auto-switch — assign profiles to displays; automatically applied when the primary menu bar display changes, with support for showing disconnected display names
  • Focus Filter integration — profiles can be tied to macOS Focus modes via System Settings using the AppIntents SetFocusFilterIntent API, with automatic restore when Focus deactivates
  • Profile hotkeys — assign global keyboard shortcuts to profiles for instant switching
  • Context menu — switch profiles from the right-click menu on the menu bar
  • Layout restore — LCS-based ordering algorithm that computes the minimum set of item moves, with bidirectional anchor scanning and cursor hiding during apply

PR Type

  • Bugfix
  • CI/CD related changes
  • Code style update (formatting, renaming)
  • Documentation
  • Feature
  • i18n/l10n (Translation/Localization)
  • Refactor
  • Performance improvement
  • Test addition or update
  • Other (please describe):

Does this PR introduce a breaking change?

  • Yes
  • No

What is the current behavior?

Thaw manages menu bar item visibility and ordering but has no way to save or restore different configurations. Users who switch between displays, Focus modes, or workflows must manually rearrange their menu bar each time.

Issue Number: N/A, though this does somewhat address #21.

What is the new behavior?

Users can save their full app configuration as named profiles and switch between them via:

  • Display auto-switch — profiles assigned to specific displays are applied automatically when the primary menu bar display changes
  • Focus Filters — profiles are applied when a macOS Focus mode activates (configured in System Settings → Focus → [Mode] → Focus Filters → Thaw) and the display profile is restored when Focus deactivates
  • Hotkeys — global keyboard shortcuts for instant profile switching (configured in Settings → Hotkeys → Profiles)
  • Context menu — right-click the menu bar to access a Profiles sub-menu
  • Settings — Apply button in the Profiles settings pane

The menu bar layout restore uses an LCS-based algorithm to minimise the number of item moves, hides the cursor during transitions, and guards against concurrent apply operations.

PR Checklist

  • I've built and run the app locally
  • I've checked for console errors or crashes
  • I've run relevant tests and they pass
  • I've added or updated tests (if applicable)
  • I've updated documentation as needed
  • I've verified localized strings work correctly (if i18n/l10n changes)

Other information

Design decisions & rationale

Focus Mode Integration: AppIntents Focus Filter vs Private APIs

Three approaches were explored to integrate with macOS Focus modes:

  1. Private DND framework (DNDStateService, DNDModeConfigurationService) — The classes exist and load from DoNotDisturb.framework, but all XPC calls fail with BSServiceConnectionErrorDomain Code=3 without the restricted com.apple.developer.focus-interaction entitlement, which requires Apple approval.

  2. INFocusStatusCenter — Returns isFocused: false even when a Focus mode is active, again due to the missing entitlement.

  3. SetFocusFilterIntent (AppIntents framework) — The public, supported API. The system calls perform() on both activation and deactivation (with default/nil parameters). ThawFocusFilter.current allows querying the active filter configuration at startup.

Option 3 was chosen. The tradeoff is that Focus-to-profile mapping is configured in System Settings → Focus → Focus Filters rather than in Thaw's own settings pane, but this is the only approach that works without restricted entitlements. The settings pane includes a footer note guiding users to System Settings.

Startup detection: ThawFocusFilter.current is queried on launch to re-apply the Focus profile if a mode is already active — the system does not re-call perform() on app launch.

Deactivation detection: On macOS Tahoe, DistributedNotificationCenter does not deliver com.apple.donotdisturbd.stateChanged. Instead, the system calls perform() with nil parameters when Focus deactivates, which posts a focusFilterDeactivated notification to ProfileManager.

Menu Bar Layout Restore: LCS-Based Ordering

The layout restore algorithm computes the Longest Common Subsequence between the current and desired item orders, then moves only the items not in the LCS. This minimises the number of macOS menu bar drag operations needed.

Anchor direction fix: The original algorithm scanned backward from desiredIdx-1 and used .leftOfItem, but .leftOfItem places the moved item to the left of the anchor (lower X). The fix: scan forward first for a stable anchor and use .leftOfItem (place before it), scan backward as fallback and use .rightOfItem (place after it). Only items in the LCS or already successfully moved serve as anchors,
preventing drift from unstable positions.

Transient Control Center Items

Items like FocusModes, ScreenMirroring, Display, Sound, and NowPlaying appear and disappear based on system state (active Focus, AirPlay session, audio changes, etc.). These are excluded from profile layout save and restore via a static set check in isProfileItem() to prevent spurious moves when system state differs between save and restore.

Cursor Hiding During Profile Apply

Individual move() calls already hide the cursor, but the 1-second safety watchdog timer force-shows it between moves. The outer profile apply now hides the cursor once with a 30-second watchdog, restoring the original cursor position after all moves complete. This eliminates visible cursor jumping during layout transitions.

Profile Hotkeys: Working Within the Existing Hotkey System

The existing Hotkey class ties actions to HotkeyAction enum cases via a Listener that calls action.perform(). Profile hotkeys are dynamic (one per profile), so a .profileApply case was added that acts as a no-op in perform(). The Listener intercepts .profileApply and looks up the target profile from ProfileManager.hotkeyProfileMap (keyed by ObjectIdentifier). This avoids modifying Listener's
private internals while keeping HotkeyRecorder UI functional — it requires isEnabled to be true via a live Listener to display the recorded key combination.

Concurrent Apply Protection

Profile hotkeys, context menus, and the Apply button all guard against layoutTask != nil to prevent concurrent layout operations that could cause freezes or cascading moves. The settings Apply button additionally awaits layoutTask?.value to stay disabled until the layout completes.

Notes for reviewers

Key areas to review:

  • FocusFilterIntent.swift — the AppIntents integration; verify ThawFocusFilter.current behaviour on different macOS versions
  • MenuBarItemManager.swift applyProfileLayout — the LCS algorithm and bidirectional anchor scanning logic
  • Hotkey.swift Listener init — the .profileApply interception; this is the only modification to the existing hotkey infrastructure
  • ProfileManager.swift rebuildProfileHotkeys — Combine observer lifecycle for dynamic hotkey management

Previews

image image image image
Screen.Recording.2026-03-25.at.3.43.30.pm.mov

Summary by CodeRabbit

  • New Features

    • Full profile management: save, load, apply, duplicate, rename, delete, import/export profiles
    • Apply profiles via menu bar menus, per-profile hotkeys, or Focus Filter intents; display-based auto-switching included
    • Settings UI: Profiles pane, per-profile hotkey recorders, and profiles section in Hotkeys settings
    • Profile-aware menu-bar layout restore that reinstates ordering, pinned/hidden items, and custom names
  • Localization

    • Added localized strings and UI text for profile management features

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 25, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a profiles subsystem: codable Profile models, a ProfileManager with save/load/apply/import/export and Focus-filter wiring, UI and menu integrations (settings pane, menus, hotkeys), per-profile hotkeys, and a menu-bar layout restoration routine using an LCS-based reordering algorithm.

Changes

Cohort / File(s) Summary
Core profile models
Thaw/Settings/Models/Profile.swift
New codable profile model types: metadata, general/advanced snapshots, menu-bar layout snapshot, Profile container, export/import bundle, capture/apply helpers, and forward-compatible decoding.
Profile manager & runtime integration
Thaw/Settings/Models/ProfileManager.swift, Thaw/Main/AppState.swift, Thaw/Utilities/Defaults.swift
New ProfileManager (published profiles, activeProfileID, profileHotkeys, hotkeyProfileMap, layoutTask); save/load/apply/delete/rename/duplicate/import/export, display associations, hotkey rebuilds, Focus-filter and startup wiring; AppState now holds profileManager; added Defaults.Key.profileHotkeys.
Hotkey action & behavior
Thaw/Hotkeys/HotkeyAction.swift, Thaw/Hotkeys/Hotkey.swift, Thaw/Settings/Models/HotkeysSettings.swift, Thaw/Settings/SettingsPanes/HotkeysSettingsPane.swift
Added .profileApply HotkeyAction (excluded from settings list); hotkey callback special-cases profileApply to load/apply mapped profile asynchronously (aborts if layoutTask busy); hotkeys settings use filtered actions and settings pane renders per-profile hotkey recorders.
Menu-bar UI & menus
Thaw/MenuBar/ControlItem/ControlItem.swift, Thaw/MenuBar/MenuBarManager.swift
Added conditional “Profiles” submenu to control-item and context menus; per-profile menu items invoke applyProfileFromMenu(_:) which loads and applies profiles asynchronously with concurrent-layout guards.
Menu-bar layout restore algorithm
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift
Added applyProfileLayout(...) to restore pinned/section order using an LCS-based reordering algorithm, batching moves and using helpers longestCommonSubsequence and sectionBoundaryDestination.
Settings UI: profiles pane & navigation
Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift, Thaw/Main/Navigation/NavigationIdentifiers/SettingsNavigationIdentifier.swift, Thaw/Settings/SettingsView.swift
New ProfileSettingsPane SwiftUI view (create/import/export/update/rename/duplicate/delete, per-display auto-switch). Added .profiles navigation identifier and integrated pane into Settings view.
Focus Filter / App Intents & localization
Thaw/Settings/Models/FocusFilterIntent.swift, Thaw/Resources/Localizable.xcstrings
Added ProfileEntity, ProfileEntityQuery, and ThawFocusFilter AppIntent for Focus-driven profile activation; added localization keys for profiles UI and actions.
Minor whitespace
Shared/Bridging/Bridging.swift
Trailing newline added at EOF (whitespace-only).

Sequence Diagram

sequenceDiagram
    actor User
    participant UI
    participant ProfileManager
    participant FileSystem as "File System"
    participant AppState
    participant Hotkeys as HotkeySystem
    participant MenuBar as MenuBarItemManager

    User->>UI: trigger apply (menu / settings / hotkey / focus)
    UI->>ProfileManager: request load/applyProfile(id)
    ProfileManager->>FileSystem: read profile JSON
    FileSystem-->>ProfileManager: profile data
    ProfileManager->>AppState: applyProfile(profile)
    AppState->>AppState: apply settings & appearance
    ProfileManager->>Hotkeys: rebuildProfileHotkeys()
    ProfileManager->>MenuBar: applyProfileLayout(...) (async)
    MenuBar->>MenuBar: compute moves (LCS), perform batched moves
    MenuBar-->>ProfileManager: layout applied
    ProfileManager-->>UI: completion / update activeProfileID
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐰 I hopped through profiles, keys, and names,

JSON carrots, layout games.
Menus shuffled into place—
A gentle hop, a tidy space.
Rabbit cheers for states made true.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: settings profiles' is a short, clear description that accurately summarizes the main change—a new settings profiles feature for Thaw.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering all required sections including what the PR does, type, breaking changes, current/new behavior, a detailed checklist, and extensive design rationale.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Add a profiles system that captures and restores the full app configuration including menu bar item layout, appearance, general and advanced settings, hotkeys, and display configuration.
Profiles can be triggered automatically via display detection, macOS Focus Filters (AppIntents), or manually via hotkeys and the right-click context menu.
The menu bar layout restore uses an LCS-based algorithm to minimise item moves, with bidirectional anchor scanning and cursor hiding for a smooth visual experience.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (3)
Thaw/Settings/Models/Profile.swift (1)

80-82: Silent fallback when decoding invalid enum raw values.

Both GeneralSettingsSnapshot.apply and AdvancedSettingsSnapshot.apply silently skip updating the strategy/style when the raw value doesn't map to a valid enum case. Consider logging a diagnostic warning when this occurs, as it could indicate profile data corruption or version mismatch.

🔧 Suggested improvement
 `@MainActor`
 func apply(to settings: GeneralSettings) {
     // ... other assignments ...
     if let strategy = RehideStrategy(rawValue: rehideStrategyRawValue) {
         settings.rehideStrategy = strategy
+    } else {
+        // Log warning about invalid raw value
     }
     settings.rehideInterval = rehideInterval
 }

Also applies to: 122-124

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/Settings/Models/Profile.swift` around lines 80 - 82, Both
GeneralSettingsSnapshot.apply and AdvancedSettingsSnapshot.apply currently
ignore invalid RehideStrategy (and analogously RehideStyle) raw values without
notification; update those apply methods to detect when RehideStrategy(rawValue:
...) (and RehideStyle(rawValue: ...)) returns nil and emit a diagnostic warning
via the app's logging facility (include identifying context such as the profile
id or name and the invalid raw value) before leaving the existing value
unchanged so you can trace profile corruption/version mismatches.
Thaw/MenuBar/ControlItem/ControlItem.swift (1)

761-777: Consider extracting duplicated profile-apply logic to ProfileManager.

This method is nearly identical to MenuBarManager.applyProfileFromMenu(_:) (lines 446-460 in MenuBarManager.swift). If the profile application flow changes (e.g., error handling, pre/post-apply hooks), both locations would need to be updated in sync.

♻️ Suggested approach: move shared logic to ProfileManager

Add a convenience method to ProfileManager:

// In ProfileManager.swift
func applyProfile(withID profileID: UUID, to appState: AppState) async throws {
    guard layoutTask == nil, profileID != activeProfileID else { return }
    let profile = try loadProfile(id: profileID)
    activeProfileID = profileID
    applyProfile(profile, to: appState)
}

Then both call sites simplify to:

 `@objc` private func applyProfileFromMenu(_ menuItem: NSMenuItem) {
     guard
         let profileID = menuItem.representedObject as? UUID,
-        let appState,
-        appState.profileManager.layoutTask == nil,
-        profileID != appState.profileManager.activeProfileID
+        let appState
     else { return }
-    let profileManager = appState.profileManager
     Task {
-        do {
-            let profile = try profileManager.loadProfile(id: profileID)
-            profileManager.activeProfileID = profileID
-            profileManager.applyProfile(profile, to: appState)
-        } catch {}
+        try? await appState.profileManager.applyProfile(withID: profileID, to: appState)
     }
 }
🤖 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 761 - 777, The
applyProfileFromMenu(_:) in ControlItem.swift duplicates logic found in
MenuBarManager.applyProfileFromMenu(_:); extract the shared flow into
ProfileManager by adding a new async throwing convenience method (e.g.,
ProfileManager.applyProfile(withID: UUID, to appState: AppState)) that checks
layoutTask == nil and profileID != activeProfileID, calls loadProfile(id:), sets
activeProfileID, and calls applyProfile(_:to:); then replace the body of both
ControlItem.applyProfileFromMenu(_:) and MenuBarManager.applyProfileFromMenu(_:)
to call that new ProfileManager method inside their Task blocks and
propagate/handle errors consistently.
Thaw/Resources/Localizable.xcstrings (1)

4252-4254: Add translator context metadata for the new display-assignment string.

Line 4252 introduces a new key with an empty object, unlike surrounding entries that provide translator context via comment. Please add metadata for consistency and better localization quality.

Proposed patch
-    "Assign a profile to each display." : {
-
-    },
+    "Assign a profile to each display." : {
+      "comment" : "Description for assigning a saved profile to each connected display.",
+      "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 4252 - 4254, The new
localization entry with key "Assign a profile to each display." is missing
translator context metadata; update the empty object to include a "comment"
string describing intended meaning and usage (e.g., clarify that this instructs
the user to assign a color/profile to each physical display or monitor) so
translators have context—modify the object for the key "Assign a profile to each
display." to include a "comment" field with concise translator guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 3924-3956: The code currently calls section.show() on
appState.menuBarManager.sections before discovering control items and returns
early on the guard failure, leaving sections expanded; fix by either moving the
loop that calls section.show() to after the guard that creates ControlItemPair
(the guard around MenuBarItem.getMenuBarItems and ControlItemPair(...) which
logs MenuBarItemManager.diagLog.error("applyProfileLayout: missing control
items") on failure) or wrap the expansion in a defer that collapses those same
sections on exit so that if the guard fails the sections are collapsed again;
update references to appState.menuBarManager.sections and the
section.show()/collapse behavior accordingly.
- Around line 4026-4028: savedCursorPosition is captured in Cocoa/global
coordinates (NSEvent.mouseLocation) but MouseHelpers.warpCursor(to:) expects
CoreGraphics coordinates; convert the saved Cocoa point into the same CG
coordinate space before warping back. Fix by: get the NSScreen that contains
NSEvent.mouseLocation (e.g. find screen whose frame contains
savedCursorPosition), then compute the CoreGraphics point using that screen's
frame (CGPoint(x: saved.x, y: screen.frame.origin.y + screen.frame.height -
saved.y) or equivalent conversion) and use that when calling
MouseHelpers.warpCursor(to:); apply the same change for both occurrences around
MouseHelpers.hideCursor(watchdogTimeout:) / MouseHelpers.showCursor() so
NSEvent.mouseLocation and MouseHelpers.warpCursor(to:) use the same coordinate
space.

In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 12047-12050: Update the user-facing localization key "Import a
profile from file" to include the missing article so it reads "Import a profile
from a file"; edit the string key/value in Localizable.xcstrings (the entry
containing "Import a profile from file") and also update the associated
"comment" text if it repeats the old phrasing to keep them consistent.

In `@Thaw/Settings/Models/Profile.swift`:
- Around line 170-178: The metadata computed property on Profile is omitting
associatedDisplayUUID and associatedDisplayName from the ProfileMetadata it
returns, creating fragile external mutation via setAssociatedDisplay(); update
the Profile model to store associatedDisplayUUID and associatedDisplayName (add
properties to Profile and update its initializers) and then include those two
fields in the metadata computed property so
ProfileMetadata(id:name:createdAt:modifiedAt:associatedDisplayUUID:associatedDisplayName:)
is constructed with the correct values; also update any code paths that
construct Profile instances (e.g., save/rename/duplicate in ProfileManager) to
populate the new Profile properties.

In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 316-334: The profile apply flow must be serialized end-to-end:
instead of only assigning layoutTask after loading the profile, create/claim a
single long-lived task/lock (e.g., profileApplyTask or an actor-protected serial
entry point for applyProfile(id: ) async) before any async work begins so
concurrent callers (hotkey/menu/display) cannot slip through; if an existing
profileApplyTask exists cancel or await it, then assign the new task
immediately, perform profile loading and the subsequent appState mutations and
call itemManager.applyProfileLayout(...) inside that task, and finally clear
profileApplyTask when complete—this centralizes serialization and prevents
overlapping restores that currently happen with layoutTask alone.
- Around line 418-449: The temp profile created by
saveProfile("__temp_update__") can be left behind if a later step throws; wrap
cleanup in a guaranteed path by capturing tempMeta.id right after verifying
tempMeta and immediately installing a defer block that removes the temp file
(using profileURL(for: tempId)), removes its entry from profiles
(profiles.removeAll { $0.id == tempId }), and calls saveManifest() so the
manifest is consistent; alternatively, avoid writing a temp file at all by
building the updated Profile in memory and writing only the final profile (use
loadProfile(id:) or appState to populate updated before any filesystem write).
Ensure symbols referenced: saveProfile(name:), tempMeta.id, profileURL(for:),
profiles, saveManifest().

In `@Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift`:
- Around line 336-345: The exportAllProfiles() handler currently swallows
failures from profileManager.exportAllProfiles() and from writing the file;
update it to surface errors via the existing errorMessage and showingError state
instead of silently returning. Specifically, when
profileManager.exportAllProfiles() returns nil or throws an error, set
errorMessage to a descriptive string (including any underlying error text if
available) and set showingError = true; similarly, replace the try?
json.write(...) with proper do/catch and on write/encoding failure set
errorMessage and showingError so the UI shows the failure. Use the same
error-reporting pattern used elsewhere in this file (errorMessage /
showingError) and reference the exportAllProfiles() function and
profileManager.exportAllProfiles() call to locate where to add the do/catch and
state updates.

---

Nitpick comments:
In `@Thaw/MenuBar/ControlItem/ControlItem.swift`:
- Around line 761-777: The applyProfileFromMenu(_:) in ControlItem.swift
duplicates logic found in MenuBarManager.applyProfileFromMenu(_:); extract the
shared flow into ProfileManager by adding a new async throwing convenience
method (e.g., ProfileManager.applyProfile(withID: UUID, to appState: AppState))
that checks layoutTask == nil and profileID != activeProfileID, calls
loadProfile(id:), sets activeProfileID, and calls applyProfile(_:to:); then
replace the body of both ControlItem.applyProfileFromMenu(_:) and
MenuBarManager.applyProfileFromMenu(_:) to call that new ProfileManager method
inside their Task blocks and propagate/handle errors consistently.

In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 4252-4254: The new localization entry with key "Assign a profile
to each display." is missing translator context metadata; update the empty
object to include a "comment" string describing intended meaning and usage
(e.g., clarify that this instructs the user to assign a color/profile to each
physical display or monitor) so translators have context—modify the object for
the key "Assign a profile to each display." to include a "comment" field with
concise translator guidance.

In `@Thaw/Settings/Models/Profile.swift`:
- Around line 80-82: Both GeneralSettingsSnapshot.apply and
AdvancedSettingsSnapshot.apply currently ignore invalid RehideStrategy (and
analogously RehideStyle) raw values without notification; update those apply
methods to detect when RehideStrategy(rawValue: ...) (and RehideStyle(rawValue:
...)) returns nil and emit a diagnostic warning via the app's logging facility
(include identifying context such as the profile id or name and the invalid raw
value) before leaving the existing value unchanged so you can trace profile
corruption/version mismatches.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2c95de32-6734-42f0-87a8-1103ed5ba508

📥 Commits

Reviewing files that changed from the base of the PR and between 926d548 and 98ecf88.

📒 Files selected for processing (17)
  • Shared/Bridging/Bridging.swift
  • Thaw/Hotkeys/Hotkey.swift
  • Thaw/Hotkeys/HotkeyAction.swift
  • Thaw/Main/AppState.swift
  • Thaw/Main/Navigation/NavigationIdentifiers/SettingsNavigationIdentifier.swift
  • Thaw/MenuBar/ControlItem/ControlItem.swift
  • Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift
  • Thaw/MenuBar/MenuBarManager.swift
  • Thaw/Resources/Localizable.xcstrings
  • Thaw/Settings/Models/FocusFilterIntent.swift
  • Thaw/Settings/Models/HotkeysSettings.swift
  • Thaw/Settings/Models/Profile.swift
  • Thaw/Settings/Models/ProfileManager.swift
  • Thaw/Settings/SettingsPanes/HotkeysSettingsPane.swift
  • Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift
  • Thaw/Settings/SettingsView.swift
  • Thaw/Utilities/Defaults.swift

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

♻️ Duplicate comments (6)
Thaw/Resources/Localizable.xcstrings (1)

12047-12049: ⚠️ Potential issue | 🟡 Minor

Fix grammar in user-facing import help text.

Line 12047 should read “Import a profile from a file.”

Proposed patch
-    "Import a profile from file" : {
-      "comment" : "A help text for the \"Import a profile from file\" button in the profile settings pane.",
+    "Import a profile from a file" : {
+      "comment" : "A help text for the \"Import a profile from a file\" button in the profile settings pane.",
       "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 12047 - 12049, Update the
user-facing localized string currently keyed/quoted as "Import a profile from
file" so its displayed text reads "Import a profile from a file." Locate the
entry with that key/quote in Localizable.xcstrings (the comment text for the
profile settings pane) and change the string value to include the missing
article "a" so the full phrase becomes "Import a profile from a file."
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (2)

3924-3955: ⚠️ Potential issue | 🟠 Major

Collapse the expanded sections on the early-return path.

Lines 3924-3927 show the hidden sections before control-item discovery, but the guard starting at Line 3947 can still return. When that happens the apply aborts with the UI left expanded. Use a defer for the collapse, or move the expansion until after control items have been found.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 3924 - 3955,
The early return in applyProfileLayout leaves sections expanded because sections
are shown prior to the guard that can exit; wrap the expansion/collapse pair in
a defer or move the calls so collapse always runs on every exit: after you call
for section.show() on appState.menuBarManager.sections where section.name !=
.visible, add a corresponding defer that iterates the same sections and calls
section.hide() (or move the show loop to after the control-item discovery block
that constructs ControlItemPair) so the UI is always collapsed even when the
guard (and the MenuBarItemManager.diagLog.error return) triggers.

4026-4131: ⚠️ Potential issue | 🟠 Major

Capture and restore the cursor in the same coordinate space.

This saves NSEvent.mouseLocation but restores with MouseHelpers.warpCursor(to:), while the rest of this file uses CoreGraphics coordinates from MouseHelpers.locationCoreGraphics / getMouseLocation(). Flipping against NSScreen.main only works on a single zero-origin display; on secondary displays it can warp the pointer to the wrong place.

🐛 Possible fix
-        let savedCursorPosition = NSEvent.mouseLocation
+        let savedCursorPosition = MouseHelpers.locationCoreGraphics
         MouseHelpers.hideCursor(watchdogTimeout: .seconds(30))
         defer { MouseHelpers.showCursor() }
@@
-        let screenHeight = NSScreen.main?.frame.height ?? 0
-        let flippedY = screenHeight - savedCursorPosition.y
-        MouseHelpers.warpCursor(to: CGPoint(x: savedCursorPosition.x, y: flippedY))
+        if let savedCursorPosition {
+            MouseHelpers.warpCursor(to: savedCursorPosition)
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 4026 - 4131,
The code captures NSEvent.mouseLocation but restores via
MouseHelpers.warpCursor(to:) using a flipped NSScreen y, causing wrong placement
on non-primary displays; replace the NSEvent capture and manual flip with the
same coordinate-space helper used elsewhere: call
MouseHelpers.locationCoreGraphics() (or getMouseLocation()) to save the cursor
in CoreGraphics coordinates before hiding, and then restore that point directly
with MouseHelpers.warpCursor(to:) (remove the NSScreen flip logic); keep the
hide/show defer and ensure saved variable name (savedCursorPosition) and the
restore call locations (before the function exits) remain consistent with
MouseHelpers.hideCursor/showCursor usage.
Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift (1)

336-345: ⚠️ Potential issue | 🟠 Major

Export All still hides encode/write failures.

This still turns both the nil-return path and the file-write failure path into a silent no-op instead of using errorMessage / showingError.

🐛 Possible fix
 private func exportAllProfiles() {
-        guard let json = profileManager.exportAllProfiles() else { return }
+        guard let json = profileManager.exportAllProfiles() else {
+            errorMessage = "Failed to encode profiles for export."
+            showingError = true
+            return
+        }
         let panel = NSSavePanel()
         panel.allowedContentTypes = [.json]
         panel.nameFieldStringValue = "Thaw Profiles.json"
         panel.canCreateDirectories = true
 
         guard panel.runModal() == .OK, let url = panel.url else { return }
-        try? json.write(to: url, atomically: true, encoding: .utf8)
+        do {
+            try json.write(to: url, atomically: true, encoding: .utf8)
+        } catch {
+            errorMessage = error.localizedDescription
+            showingError = true
+        }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift` around lines 336 -
345, The exportAllProfiles() function swallows both a nil JSON from
profileManager.exportAllProfiles() and any write errors; update
exportAllProfiles() to handle and surface these failures by checking the
nil-return and using the view/controller's errorMessage and showingError (or the
existing showingError mechanism) to present a descriptive error, and replace the
try? json.write(...) with a do/catch that catches and logs the thrown error and
sets errorMessage/showingError so the UI displays the failure; keep the existing
NSSavePanel flow and reference profileManager.exportAllProfiles(), the panel.url
check, and the errorMessage/showingError properties when implementing the
changes.
Thaw/Settings/Models/ProfileManager.swift (2)

316-334: ⚠️ Potential issue | 🔴 Critical

layoutTask is still an unsafe apply lock.

At Line 325 the guard is claimed only after the profile has already mutated live settings, and the canceled task still unconditionally clears layoutTask at Line 333. Callers like Thaw/Hotkeys/Hotkey.swift, Lines 79-84, only gate on layoutTask == nil, so two applies can still overlap both before the task is assigned and after an older task wipes out a newer handle. This needs one apply token/lock claimed before any work starts, and only that owner should release it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/Settings/Models/ProfileManager.swift` around lines 316 - 334, Assign the
new Task to layoutTask before awaiting work and only clear layoutTask if the
clearing Task is the same owner to prevent races: create a local Task instance
(e.g., let task = Task { ... }), set self?.layoutTask = task immediately, then
perform await appState.itemManager.applyProfileLayout(...) inside that task and
at the end only set self?.layoutTask = nil if self?.layoutTask === task (and
respect cancellation inside the task); update the code references to layoutTask,
applyProfileLayout, and the profile.menuBarLayout-derived inputs accordingly so
the lock/token is held from creation until the owner releases it.

406-410: ⚠️ Potential issue | 🟠 Major

The export format still is not round-trippable.

Both export paths serialize only Profile, while importProfile(from:) only decodes a single Profile. That means “Export All Profiles” cannot be imported back through this UI, and metadata-only fields like display associations still fall out of the backup format.

Also applies to: 452-461, 633-665

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/Settings/Models/ProfileManager.swift` around lines 406 - 410,
exportProfile(id:to:) currently writes only a single Profile which doesn't match
the format importProfile(from:) expects and drops metadata-only fields (e.g.
display associations), so update the exporter to write the same round-trippable
container the importer decodes (the full backup/collection type used by the
import path) and include all metadata fields; specifically change
exportProfile(id:to:) (and the "Export All Profiles" flow) to serialize the same
Codable wrapper/type importProfile(from:) decodes (including display
associations and any manager-level metadata) so imports can fully restore
exported data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Thaw/MenuBar/ControlItem/ControlItem.swift`:
- Around line 763-776: The current gating checks
appState.profileManager.layoutTask before launching async work, which is not
atomic and allows races; add a synchronous helper on ProfileManager (e.g., a
method like beginAtomicApplyProfile(id: UUID, to: AppState) or
performAtomicApplyProfile(id: UUID, applyTo: AppState)) that atomically checks
and marks "apply in progress" (or uses/sets layoutTask) and then performs the
async loadProfile and applyProfile within that helper so only one apply can
proceed; replace the current Task block in ControlItem (which calls loadProfile,
sets activeProfileID, and calls applyProfile) with a single call to this new
ProfileManager helper (and have the helper clear the in-progress marker on
completion or error).

In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 3965-3983: transientControlCenterItems is compared against
item.uniqueIdentifier which includes an instance index, so the lookup fails;
update isProfileItem to compare the transientControlCenterItems set against the
identifier without the instance suffix (the namespace:title component) instead
of the full uniqueIdentifier — derive the base identifier from the MenuBarItem
(e.g. strip the instance/index suffix or call the existing helper that returns
the namespace:title form) and use that when checking
transientControlCenterItems.contains(...), keeping the other tag and property
checks unchanged.

In `@Thaw/MenuBar/MenuBarManager.swift`:
- Around line 453-458: The Task block currently swallows errors from profile
loading, making failures silent; update the catch to log the error via diagLog
(including the profileID and error.localizedDescription or error) so failures
are visible, and ensure activeProfileID assignment and
applyProfile(applyProfile(_:to:)) remain in the do-path only after successful
load; specifically modify the Task around loadProfile(id:) /
appState.profileManager.activeProfileID / appState.profileManager.applyProfile
to catch and call diagLog.error with contextual info instead of an empty catch.

In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 4252-4254: The new localization entry for the string key "Assign a
profile to each display." is missing translator metadata; update its object to
match adjacent entries by adding a translator comment and an autogenerated
marker (the same metadata keys/format used by surrounding entries) so it
contains at least a Comment field and an auto-generated flag to provide context
for translators.

In `@Thaw/Settings/Models/Profile.swift`:
- Around line 27-84: The snapshot omits the custom icon backing state so
profiles lose uploaded assets; update GeneralSettingsSnapshot to include the
lastCustomIceIcon field (same type as GeneralSettings.lastCustomIceIcon), add it
to the initializer produced by capture(from:) and assign it back in apply(to:)
(i.e., add the property to the struct, set GeneralSettingsSnapshot.capture to
read settings.lastCustomIceIcon, and in GeneralSettingsSnapshot.apply assign
settings.lastCustomIceIcon = lastCustomIceIcon).

In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 587-590: The guard in activateFocusFilter (the check using
profileID and activeProfileID around the early return) prevents reapplying the
same Focus profile after it was deactivated; change the condition so
reactivation still applies when the Focus filter is currently inactive (e.g.,
only return early if profileID == activeProfileID AND focusFilterActive is
true), or alternatively track a deactivatedFocusProfileID and allow reapply when
it matches; apply the same fix to the equivalent block around the other
activation path referenced (the block covering the 606-612 logic) so
reactivation correctly reapplies settings after deactivation.
- Around line 483-489: In setAssociatedDisplay(uuid:forDisplayUUID:) ensure you
also clear the cached display name when removing the association: when iterating
profiles and setting profiles[index].associatedDisplayUUID = nil also set
profiles[index].associatedDisplayName = nil so a profile doesn't still show a
disconnected display; keep the saveManifest() call after the loop. Reference the
function name setAssociatedDisplay and the properties associatedDisplayUUID and
associatedDisplayName on the Profile entries.
- Around line 581-585: ProfileManager.applyFocusFilterProfile() fails to read
the saved UUID because FocusFilterIntent currently stores a UUID object (NSData)
under "FocusFilterRequestedProfileID" while applyFocusFilterProfile() expects a
String; update the FocusFilterIntent storage to save the string form of the UUID
(use profile.id.uuidString) when calling UserDefaults.standard.set(..., forKey:
"FocusFilterRequestedProfileID") so ProfileManager.applyFocusFilterProfile() can
successfully retrieve it via UserDefaults.standard.string(forKey:).

In `@Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift`:
- Around line 91-95: The Apply button and applyProfile(id:) call aren't guarding
against a manager-level in-flight apply; update the UI and apply logic to check
a shared manager flag (e.g., profileManager.isApplying or similar) before
enabling or starting an apply. Specifically, disable the Button when either
local isApplying OR profileManager.isApplying is true (in addition to the
existing profile.id == profileManager.activeProfileID check), and inside the
applyProfile(id:) path (the code that currently starts a new layoutTask around
line ~262) early-return if profileManager.isApplying is true or delegate the
work to a single manager method that serializes/ignores concurrent requests so a
running apply cannot be cancelled/replaced by this button/hotkey/menu path.
Ensure you reference and update the same manager-level flag used elsewhere to
coordinate in-flight applies.

---

Duplicate comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 3924-3955: The early return in applyProfileLayout leaves sections
expanded because sections are shown prior to the guard that can exit; wrap the
expansion/collapse pair in a defer or move the calls so collapse always runs on
every exit: after you call for section.show() on
appState.menuBarManager.sections where section.name != .visible, add a
corresponding defer that iterates the same sections and calls section.hide() (or
move the show loop to after the control-item discovery block that constructs
ControlItemPair) so the UI is always collapsed even when the guard (and the
MenuBarItemManager.diagLog.error return) triggers.
- Around line 4026-4131: The code captures NSEvent.mouseLocation but restores
via MouseHelpers.warpCursor(to:) using a flipped NSScreen y, causing wrong
placement on non-primary displays; replace the NSEvent capture and manual flip
with the same coordinate-space helper used elsewhere: call
MouseHelpers.locationCoreGraphics() (or getMouseLocation()) to save the cursor
in CoreGraphics coordinates before hiding, and then restore that point directly
with MouseHelpers.warpCursor(to:) (remove the NSScreen flip logic); keep the
hide/show defer and ensure saved variable name (savedCursorPosition) and the
restore call locations (before the function exits) remain consistent with
MouseHelpers.hideCursor/showCursor usage.

In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 12047-12049: Update the user-facing localized string currently
keyed/quoted as "Import a profile from file" so its displayed text reads "Import
a profile from a file." Locate the entry with that key/quote in
Localizable.xcstrings (the comment text for the profile settings pane) and
change the string value to include the missing article "a" so the full phrase
becomes "Import a profile from a file."

In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 316-334: Assign the new Task to layoutTask before awaiting work
and only clear layoutTask if the clearing Task is the same owner to prevent
races: create a local Task instance (e.g., let task = Task { ... }), set
self?.layoutTask = task immediately, then perform await
appState.itemManager.applyProfileLayout(...) inside that task and at the end
only set self?.layoutTask = nil if self?.layoutTask === task (and respect
cancellation inside the task); update the code references to layoutTask,
applyProfileLayout, and the profile.menuBarLayout-derived inputs accordingly so
the lock/token is held from creation until the owner releases it.
- Around line 406-410: exportProfile(id:to:) currently writes only a single
Profile which doesn't match the format importProfile(from:) expects and drops
metadata-only fields (e.g. display associations), so update the exporter to
write the same round-trippable container the importer decodes (the full
backup/collection type used by the import path) and include all metadata fields;
specifically change exportProfile(id:to:) (and the "Export All Profiles" flow)
to serialize the same Codable wrapper/type importProfile(from:) decodes
(including display associations and any manager-level metadata) so imports can
fully restore exported data.

In `@Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift`:
- Around line 336-345: The exportAllProfiles() function swallows both a nil JSON
from profileManager.exportAllProfiles() and any write errors; update
exportAllProfiles() to handle and surface these failures by checking the
nil-return and using the view/controller's errorMessage and showingError (or the
existing showingError mechanism) to present a descriptive error, and replace the
try? json.write(...) with a do/catch that catches and logs the thrown error and
sets errorMessage/showingError so the UI displays the failure; keep the existing
NSSavePanel flow and reference profileManager.exportAllProfiles(), the panel.url
check, and the errorMessage/showingError properties when implementing the
changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 63628bde-fa74-4f88-adf0-9d94a96cc3d0

📥 Commits

Reviewing files that changed from the base of the PR and between 98ecf88 and 5fd6c1b.

📒 Files selected for processing (17)
  • Shared/Bridging/Bridging.swift
  • Thaw/Hotkeys/Hotkey.swift
  • Thaw/Hotkeys/HotkeyAction.swift
  • Thaw/Main/AppState.swift
  • Thaw/Main/Navigation/NavigationIdentifiers/SettingsNavigationIdentifier.swift
  • Thaw/MenuBar/ControlItem/ControlItem.swift
  • Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift
  • Thaw/MenuBar/MenuBarManager.swift
  • Thaw/Resources/Localizable.xcstrings
  • Thaw/Settings/Models/FocusFilterIntent.swift
  • Thaw/Settings/Models/HotkeysSettings.swift
  • Thaw/Settings/Models/Profile.swift
  • Thaw/Settings/Models/ProfileManager.swift
  • Thaw/Settings/SettingsPanes/HotkeysSettingsPane.swift
  • Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift
  • Thaw/Settings/SettingsView.swift
  • Thaw/Utilities/Defaults.swift
✅ Files skipped from review due to trivial changes (3)
  • Shared/Bridging/Bridging.swift
  • Thaw/Main/Navigation/NavigationIdentifiers/SettingsNavigationIdentifier.swift
  • Thaw/Settings/SettingsView.swift
🚧 Files skipped from review as they are similar to previous changes (5)
  • Thaw/Settings/Models/HotkeysSettings.swift
  • Thaw/Settings/SettingsPanes/HotkeysSettingsPane.swift
  • Thaw/Hotkeys/HotkeyAction.swift
  • Thaw/Hotkeys/Hotkey.swift
  • Thaw/Settings/Models/FocusFilterIntent.swift

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
Thaw/MenuBar/ControlItem/ControlItem.swift (1)

761-775: Silent error handling inconsistent with MenuBarManager.applyProfileFromMenu.

MenuBarManager.applyProfileFromMenu logs errors via diagLog.error, but this implementation uses try? which silently discards load failures. For diagnostic consistency, consider logging the error here as well.

♻️ Proposed fix
     `@objc` private func applyProfileFromMenu(_ menuItem: NSMenuItem) {
         guard
             let profileID = menuItem.representedObject as? UUID,
             let appState,
             appState.profileManager.layoutTask == nil,
             profileID != appState.profileManager.activeProfileID
         else { return }
         let profileManager = appState.profileManager
         Task {
-            guard let profile = try? profileManager.loadProfile(id: profileID) else { return }
-            profileManager.activeProfileID = profileID
-            profileManager.applyProfile(profile, to: appState)
+            do {
+                let profile = try profileManager.loadProfile(id: profileID)
+                profileManager.activeProfileID = profileID
+                profileManager.applyProfile(profile, to: appState)
+            } catch {
+                // Consider adding diagLog here for consistency
+            }
         }
     }
🤖 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 761 - 775, The
method applyProfileFromMenu currently swallows loadProfile errors with try?;
change it to a do/catch around profileManager.loadProfile(id:) inside the Task,
log any thrown error using the same diagnostic logger used by MenuBarManager
(diagLog.error) including contextual info (profileID) and return on failure,
then proceed to set activeProfileID and call applyProfile(profile, to: appState)
on success; reference applyProfileFromMenu, profileManager.loadProfile(id:),
profileManager.activeProfileID, and profileManager.applyProfile(...) when making
the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@Thaw/MenuBar/ControlItem/ControlItem.swift`:
- Around line 761-775: The method applyProfileFromMenu currently swallows
loadProfile errors with try?; change it to a do/catch around
profileManager.loadProfile(id:) inside the Task, log any thrown error using the
same diagnostic logger used by MenuBarManager (diagLog.error) including
contextual info (profileID) and return on failure, then proceed to set
activeProfileID and call applyProfile(profile, to: appState) on success;
reference applyProfileFromMenu, profileManager.loadProfile(id:),
profileManager.activeProfileID, and profileManager.applyProfile(...) when making
the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8a21485d-0cd9-4352-8bb3-c902fbd38eb2

📥 Commits

Reviewing files that changed from the base of the PR and between 5fd6c1b and 198ab09.

📒 Files selected for processing (7)
  • Thaw/MenuBar/ControlItem/ControlItem.swift
  • Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift
  • Thaw/MenuBar/MenuBarManager.swift
  • Thaw/Resources/Localizable.xcstrings
  • Thaw/Settings/Models/Profile.swift
  • Thaw/Settings/Models/ProfileManager.swift
  • Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift
✅ Files skipped from review due to trivial changes (2)
  • Thaw/Settings/Models/ProfileManager.swift
  • Thaw/Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (2)
  • Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift
  • Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Thaw/Settings/Models/Profile.swift`:
- Around line 69-72: The assignment order restores the public iceIcon before its
backing state, causing observers to see the new iceIcon while lastCustomIceIcon
and customIceIconIsTemplate are still stale; reorder the assignments so you set
settings.lastCustomIceIcon and settings.customIceIconIsTemplate before assigning
settings.iceIcon (keeping settings.showIceIcon as before), ensuring the backing
fields are restored first to avoid transient inconsistent state for observers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 486901a3-2c0f-4e6b-bc6e-aa01edcee50e

📥 Commits

Reviewing files that changed from the base of the PR and between 198ab09 and e4d462a.

📒 Files selected for processing (4)
  • Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift
  • Thaw/Settings/Models/Profile.swift
  • Thaw/Settings/Models/ProfileManager.swift
  • Thaw/Settings/SettingsPanes/ProfileSettingsPane.swift
🚧 Files skipped from review as they are similar to previous changes (1)
  • Thaw/Settings/Models/ProfileManager.swift

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (2)
Thaw/Settings/Models/ProfileManager.swift (1)

409-429: ⚠️ Potential issue | 🟠 Major

Guarantee cleanup of the temporary update profile.

Any throw after saveProfile(name: "__temp_update__", ...) succeeds leaves a real __temp_update__ file and manifest entry behind. The cleanup needs to be on a guaranteed path, not only the success path.

🛠️ Minimal fix with `defer`
         let tempName = "__temp_update__"
         try saveProfile(name: tempName, from: appState)
         guard let tempMeta = profiles.last, tempMeta.name == tempName else { return }
+        let tempID = tempMeta.id
+        defer {
+            try? FileManager.default.removeItem(at: profileURL(for: tempID))
+            profiles.removeAll { $0.id == tempID }
+            saveManifest()
+        }
 
         // Load the temp profile and re-save with original identity.
-        var updated = try loadProfile(id: tempMeta.id)
+        var updated = try loadProfile(id: tempID)
         updated = Profile(
             id: id,
             name: old!.name,
             createdAt: old!.createdAt,
             modifiedAt: Date(),
             content: updated.content
         )
@@
-        // Remove temp profile.
-        try? FileManager.default.removeItem(at: profileURL(for: tempMeta.id))
-        profiles.removeAll { $0.id == tempMeta.id }
-
         // Update metadata.
         if let index = profiles.firstIndex(where: { $0.id == id }) {
             profiles[index].modifiedAt = updated.modifiedAt
         }
-        saveManifest()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/Settings/Models/ProfileManager.swift` around lines 409 - 429, A
temporary profile saved with saveProfile(name: "__temp_update__", from:) can be
left on disk or in profiles if any subsequent throw occurs; ensure guaranteed
cleanup by capturing the temp profile identity immediately after save (e.g. let
tempMeta = profiles.last / tempID = tempMeta.id) and install a defer block that
always removes the file at profileURL(for: tempID) and removes the manifest
entry from profiles (profiles.removeAll { $0.id == tempID }) before proceeding
to loadProfile, encoder.encode, write, etc.; this guarantees removal even if
loadProfile, encoding, or write throws and uses the existing symbols
saveProfile, profiles, profileURL(for:), loadProfile, and encoder.encode.
Thaw/Settings/Models/Profile.swift (1)

69-72: ⚠️ Potential issue | 🟡 Minor

Restore the custom-icon backing state before publishing iceIcon.

iceIcon is assigned while lastCustomIceIcon and customIceIconIsTemplate still describe the previous asset, so observers can see a transient mismatched custom-icon state during profile apply.

🛠️ Minimal fix
     func apply(to settings: GeneralSettings) {
         settings.showIceIcon = showIceIcon
-        settings.iceIcon = iceIcon
         settings.lastCustomIceIcon = lastCustomIceIcon
         settings.customIceIconIsTemplate = customIceIconIsTemplate
+        settings.iceIcon = iceIcon
         settings.useIceBar = useIceBar
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Thaw/Settings/Models/Profile.swift` around lines 69 - 72, The assignments
publish a new iceIcon while the backing custom-icon flags still reflect the
previous asset, causing observers to see a transient mismatch; reorder the
updates so you set settings.lastCustomIceIcon and
settings.customIceIconIsTemplate before assigning settings.iceIcon (and keep
settings.showIceIcon updated as needed), i.e. update the custom-icon backing
state first, then publish the new iceIcon to avoid transient inconsistent state
observed by listeners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 641-649: The import currently appends ProfileMetadata directly
which can violate the one-profile-per-display rule; before appending the new
metadata (the imported / entry objects), enforce uniqueness by using the
existing setter logic: call setAssociatedDisplay(uuid:displayName:forProfileID:)
(or otherwise clear any existing owner's associatedDisplayUUID) for the
imported.id with entry.associatedDisplayUUID and entry.associatedDisplayName so
ownership is reconciled the same way as normal assignment; ensure
applyProfileForDisplay(uuid:) will then see a single owner and append to
profiles only after the association has been handled.
- Around line 357-363: The current rename replaces the entire ProfileMetadata at
profiles[index], dropping manifest-only display bindings; instead, in
ProfileManager when updating profiles[index] (where you construct
ProfileMetadata with id, name, createdAt, modifiedAt) preserve any existing
display-binding fields from the old metadata (copy over the display/auto-switch
mapping properties that live on ProfileMetadata) and only change the name and
modifiedAt; i.e., read the existing profiles[index] into a variable and build
the new ProfileMetadata reusing its display-related properties so the
auto-switch/display mapping is not lost.
- Around line 314-331: The current cancel flow lets an older Task still reach
its completion and unconditionally set layoutTask = nil, racing with newer
tasks; fix by capturing the new Task instance into a local constant (e.g. let
current = Task { ... }) and assign layoutTask = current before awaiting
applyProfileLayout, then when clearing set layoutTask = nil only if
self?.layoutTask === current (or the equivalent identity check) so
canceled/older tasks don't clear a newer layoutTask; apply this change around
the Task creation and the final self?.layoutTask = nil in ProfileManager.swift
(the layoutTask Task closure that calls applyProfileLayout).

---

Duplicate comments:
In `@Thaw/Settings/Models/Profile.swift`:
- Around line 69-72: The assignments publish a new iceIcon while the backing
custom-icon flags still reflect the previous asset, causing observers to see a
transient mismatch; reorder the updates so you set settings.lastCustomIceIcon
and settings.customIceIconIsTemplate before assigning settings.iceIcon (and keep
settings.showIceIcon updated as needed), i.e. update the custom-icon backing
state first, then publish the new iceIcon to avoid transient inconsistent state
observed by listeners.

In `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 409-429: A temporary profile saved with saveProfile(name:
"__temp_update__", from:) can be left on disk or in profiles if any subsequent
throw occurs; ensure guaranteed cleanup by capturing the temp profile identity
immediately after save (e.g. let tempMeta = profiles.last / tempID =
tempMeta.id) and install a defer block that always removes the file at
profileURL(for: tempID) and removes the manifest entry from profiles
(profiles.removeAll { $0.id == tempID }) before proceeding to loadProfile,
encoder.encode, write, etc.; this guarantees removal even if loadProfile,
encoding, or write throws and uses the existing symbols saveProfile, profiles,
profileURL(for:), loadProfile, and encoder.encode.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: db2a808a-1f60-4fc8-97df-ea50a2f0dfea

📥 Commits

Reviewing files that changed from the base of the PR and between e4d462a and 76a28fa.

📒 Files selected for processing (2)
  • Thaw/Settings/Models/Profile.swift
  • Thaw/Settings/Models/ProfileManager.swift

@diazdesandi diazdesandi added the feature New capability that did not exist before label Mar 25, 2026
@diazdesandi diazdesandi added this to the 1.3.0 milestone Mar 25, 2026
@nightah
Copy link
Copy Markdown
Collaborator Author

nightah commented Mar 25, 2026

I've addressed the SonarQube, CodeRabbit, and SwiftLint concerns, along with a few other small visual adjustment refactors.

The following image illustrates said changes.

image

@stonerl stonerl changed the base branch from main to development March 28, 2026 12:49
@nightah
Copy link
Copy Markdown
Collaborator Author

nightah commented Mar 28, 2026

Latest changes include some more profile saving options:

image

@nightah
Copy link
Copy Markdown
Collaborator Author

nightah commented Mar 29, 2026

In the last few commits, I also added a visual representation of the notch for devices on the Menu Bar Layout.

image

@nightah
Copy link
Copy Markdown
Collaborator Author

nightah commented Mar 31, 2026

Update — Additional changes since last comment

New item placement (2774580)
New and unsaved menu bar items now always appear to the right of the visible control icon, giving them a predictable default position. This addresses the behaviour reported in #212 and #324 — new items no longer silently land in the hidden section, and the Thaw icon stays pinned to the left of all visible items.

Icon overflow for notched displays (3798c2d)
On MacBooks with a notch, visible items that would overlap the notch area are now handled gracefully with overflow detection. This addresses #150 and #279, where icons disappeared or were rendered incorrectly around the notch.

Re-sort on launch (c156dc5, 97b041e)
When Thaw launches, and a profile's saved layout doesn't match the current menu bar state, items are automatically re-sorted to match the profile. This prevents the gradual drift reported in #303, #327, #314, and #98, where items would migrate between sections across reboots or reconnections. A guard (f137eaf) skips the full sort when the layout already matches, avoiding unnecessary moves on startup.

Profile localization (748e489)
Added localised strings for the profiles feature.

Issues potentially addressed by this branch:

Issue Title How
#21 Menu Bar Layout Backup and Restore Profiles save/restore full layout + settings
#212 New items don't default to visible section New items placed right of control icon
#324 Pin Thaw icon to the left of visible icons New items appear to the right, Thaw icon stays left
#251 Default position for new icons Predictable placement for new items
#303 Hidden items migrating to visible section Re-sort on launch restores saved layout
#327 Claude icon keeps going into hidden Re-sort on launch corrects drift
#314 Song-Rating app disappears after a while Re-sort on launch corrects drift
#98 Stage Manager icon resets after reboot Re-sort on launch restores position
#131 Airpods icon reverts after reconnecting Re-sort corrects position (partial — depends on timing)
#150 Icons not displayed correctly with notch Notch overflow handling
#279 MenuItems disappear with notch Notch overflow handling
#308 Auto re-assign icons by display Display auto-switch profiles

@nightah
Copy link
Copy Markdown
Collaborator Author

nightah commented Apr 1, 2026

Update: Fix expanded control items intercepting clicks on smaller displays

Problem

When sections are hidden, the hidden/always-hidden control items expand to 10,000pt to push menu bar items off-screen. On smaller displays (e.g. built-in MacBook screens), this expanded NSStatusItem window covers the application menu area on the left side of the menu bar. The window intercepts all mouse events — left clicks trigger section toggles (showing the Thaw bar), and right clicks show Thaw's context menu — preventing users from interacting with the frontmost app's menus entirely.

Fix

Set ignoresMouseEvents = true on the control item's window when the section divider enters .hideSection state, and restore it (false) when entering .showSection. This tells the window server to pass clicks straight through to whatever is underneath. The same treatment is applied to spacer items on ultra-wide displays.

Users toggle hidden sections via the visible Thaw icon, hotkeys, hover, or scroll — none of which rely on clicking the expanded divider, so no functionality is lost.

Issues addressed

Issue Title Status
#348 When "show on click" is enabled, some very long app menubars trigger Thaw Fixes
#281 Thaw hijacks the top of the screen on external monitor when MacBook is in clamshell mode Fixes
#237 Context menu shows up even when it shouldn't Fixes
#334 Hidden bar overlaps menu bar Potentially related

@nightah nightah force-pushed the feature/profiles branch from 284dc68 to 5c857f9 Compare April 6, 2026 10:42
@nightah nightah force-pushed the feature/profiles branch from 5c857f9 to 6cd26f7 Compare April 7, 2026 03:52
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 7, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New capability that did not exist before

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants