Add menu bar item triggers#735
Open
alvst wants to merge 4 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
github-actions
Bot
added
bug
Collaborator
|
Issues with the linter @alvst |
Author
|
SwiftLint fixed in b120197. Verified locally. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/aw/actions-lock.json (1)
8-17:⚠️ Potential issue | 🟡 MinorRemove the unused old action entry from the lock file.
The lock file contains two distinct
setupaction entries with different repo paths:
- New:
github/gh-aw-actions/setup@v0.79.6(line 8)- Old:
github/gh-aw/actions/setup@v0.76.1(line 13)The old action path is not referenced anywhere in the repository and should be removed to keep the lock file clean and avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/aw/actions-lock.json around lines 8 - 17, Remove the outdated and unused setup action entry from the actions-lock.json file. The old entry with repo path github/gh-aw/actions/setup at version v0.76.1 (including its associated sha) is no longer referenced anywhere in the repository and should be deleted entirely. Keep only the current entry for github/gh-aw-actions/setup at v0.79.6.Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (3)
6836-6838:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompare saved entries against
uniqueIdentifier.
savedSectionOrderstoresuniqueIdentifiervalues, butcurrentTagsstrips any:instanceIndexsuffix. If the saved layout contains only indexed entries, this guard can incorrectly skip restore even though the item is present.Proposed fix
- let currentTags = Set(items.map { "\($0.tag.namespace):\($0.tag.title)" }) + let currentTags = Set(items.map(\.uniqueIdentifier)) let savedTags = Set(savedSectionOrder.values.flatMap(\.self))Based on learnings, Control Center singleton
instanceIndex == 0identifiers usenamespace:title, while onlyinstanceIndex > 0usesnamespace:title:instanceIndex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 6836 - 6838, The currentTags variable is constructed using only namespace and title (stripping any instanceIndex suffix), but savedSectionOrder stores full uniqueIdentifier values that may include the instanceIndex suffix. This mismatch causes the guard condition to incorrectly skip restoration when saved layouts contain indexed entries. Replace the currentTags construction to use the actual uniqueIdentifier property from each item instead of manually building a string from namespace and title, ensuring the comparison against savedTags accurately reflects all stored identifiers including those with instanceIndex suffixes.Source: Learnings
1231-1263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart relaunch settling before debouncing launch notifications.
Because the publisher is debounced before this sink, only the last launch in a burst is inspected. If a tracked menu-bar app relaunches and an untracked helper/app launches within the 1s debounce window,
startSettlingPeriod(expectedBundleIDs:)is never armed and restore can run against the transient layout this block is meant to avoid. Split this into a non-debounced settling subscriber and keep the debounced cache refresh separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 1231 - 1263, The debounced publisher only processes the last launch notification in a burst, which can cause startSettlingPeriod to not be called when a tracked app relaunches alongside untracked apps within the debounce window. Split the current sink subscriber into two separate subscribers: create a non-debounced subscriber on NSWorkspace.shared.notificationCenter.publisher for didLaunchApplicationNotification that immediately checks if the launched app is tracked (using MenuBarItemManager.tracksMenuBarItem) and calls startSettlingPeriod with the appropriate bundle ID, while keeping the debounced subscriber separate to handle only the cache refresh logic. This ensures settling periods are always armed for tracked app relaunches regardless of concurrent launches.
2407-2428:⚠️ Potential issue | 🟠 MajorRemove the unstructured Task wrapper to honor caller cancellation immediately.
The
Task { ... }wrapping at line 2411 creates an unstructured task whose cancellation is decoupled from the caller's task context. When a move or click operation is cancelled, the pause polling loop continues untilhasUserPausedInput()returns true or the optional timeout fires, then throwsEventError.cannotComplete—delaying the observable cancellation. Inline the polling logic directly in the caller task's scope soTask.checkCancellation()is checked in the correct context.Proposed fix
private nonisolated func waitForUserToPauseInput( for duration: Duration = .milliseconds(50), timeout: Duration? = nil ) async throws { - let waitTask = Task { - let start = ContinuousClock.now + let start = ContinuousClock.now + do { while true { try Task.checkCancellation() if let timeout, start.duration(to: .now) >= timeout { throw EventError.cannotComplete } if hasUserPausedInput(for: duration) { - break + return } try await Task.sleep(for: .milliseconds(50)) } - } - do { - try await waitTask.value + } catch let error as EventError { + throw error } catch { throw EventError.cannotComplete } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 2407 - 2428, The unstructured Task wrapper in the waitForUserToPauseInput function creates a decoupled cancellation context that delays observable cancellation when move or click operations are cancelled. Remove the Task { ... } wrapper that starts at the beginning of the function body and replace it with the inlined polling logic directly in the caller's scope. This means the while loop with hasUserPausedInput(), Task.checkCancellation(), and Task.sleep() should execute directly in the function body instead of being wrapped in a nested Task, and remove the corresponding do-catch block that awaits waitTask.value, replacing it with the direct execution of the loop logic.
🧹 Nitpick comments (3)
Thaw/Permissions/PermissionsView.swift (1)
301-316: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using mock permissions to avoid real system checks in previews.
The
MockPermissionsManagerinstantiates realAccessibilityPermission()andScreenRecordingPermission()objects, which start polling timers and may attempt actual permission checks during preview rendering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Thaw/Permissions/PermissionsView.swift` around lines 301 - 316, The allPermissions array in MockPermissionsManager is initializing real Permission objects (AccessibilityPermission and ScreenRecordingPermission) which trigger actual system permission checks and polling timers during preview rendering. Replace these real Permission instances with mock implementations that are lightweight stubs without any system interaction, polling timers, or permission checks. Create mock versions of AccessibilityPermission and ScreenRecordingPermission that conform to the Permission protocol but don't perform any actual system calls.Thaw.xcodeproj/project.pbxproj (1)
582-586: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor: Marketing version mismatch between targets.
Thaw is at
2.0.0-beta.15while MenuBarItemService is at2.0.0-beta.16. If these should be kept in sync, consider aligning them.Also applies to: 647-647
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Thaw.xcodeproj/project.pbxproj` around lines 582 - 586, The MARKETING_VERSION for the Thaw target is set to 2.0.0-beta.15 but the MenuBarItemService target has 2.0.0-beta.16, causing a version mismatch. Update the MARKETING_VERSION value in the Thaw target's build settings to match the MenuBarItemService target version of 2.0.0-beta.16. This applies to both instances of the MARKETING_VERSION setting for the Thaw target to ensure all targets are properly aligned.ThawTests/PlanNotchOverflowTests.swift (1)
348-395: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen invalid-budget tests with state-preservation assertions.
For consistency with the negative-budget lock, also assert
updatedDesiredFilteredandupdatedSectionMapare unchanged in the zero and non-finite cases; otherwise a silent mutation can slip through whileoverflowUIDsstays empty.Suggested patch
@@ XCTAssertEqual(result.overflowUIDs, [], "must not eject items on a zero budget") + XCTAssertEqual(result.updatedDesiredFiltered, desired) + XCTAssertEqual(result.updatedSectionMap, sectionMap) @@ XCTAssertEqual(result.overflowUIDs, [], "must not eject items on a non-finite budget (\(badBudget))") + XCTAssertEqual(result.updatedDesiredFiltered, desired) + XCTAssertEqual(result.updatedSectionMap, sectionMap) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ThawTests/PlanNotchOverflowTests.swift` around lines 348 - 395, In both testZeroAvailableWidthYieldsNoOverflow and testNonFiniteAvailableWidthYieldsNoOverflow, add assertions to verify that the result's updatedDesiredFiltered equals the original desired sequence and updatedSectionMap equals the original sectionMap, ensuring no silent mutations occur on invalid budgets. The current tests only assert that overflowUIDs is empty, but should also confirm the state preservation to catch potential bugs where input data gets modified without triggering an overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/remove.yml:
- Around line 15-40: The pagination call to github.paginate() is not wrapped in
error handling, so API failures cause silent script failure. Wrap the
github.paginate() call in a try/catch block to handle potential API errors.
Additionally, the deletion loop lacks per-run logging and rate limiting. Add
console logging for each individual run.id when successfully deleted (in the try
block) and when failures occur (in the catch block for each run). Finally, add a
small delay between deletion attempts using await new Promise(r => setTimeout(r,
100)) after each successful deletion to avoid triggering GitHub API rate limits
during batch operations.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 5845-5851: The system clone filter is only applied once during the
initial profile-layout fetch, but subsequent calls to getMenuBarItems(...) in
the apply path and moveItem(trigger) operate on unfiltered arrays, allowing
clones to be treated as successful move operations. Add the same
removeAll(where: \.isSystemClone) filter after every fresh getMenuBarItems(...)
call before the items are used to resolve move targets, ensuring that clones are
consistently filtered out across all live fetches used for move operations. This
prevents clones from incorrectly incrementing movedCount or returning success
without actually moving the real item.
- Around line 2306-2315: The cachedCloneWindowIDs can persist after their
corresponding clone windows are no longer present in rawWindowIDs. If
WindowServer recycles those IDs for real menu items, those items get filtered
out incorrectly. Update the logic to expire clone IDs that are no longer in
rawWindowIDs by filtering cachedCloneWindowIDs to only keep IDs that still exist
in rawWindowIDs, ensuring that recycled window IDs representing real menu items
are not removed by the subsequent filter operation.
In `@Thaw/Settings/Models/MenuBarItemTriggersManager.swift`:
- Around line 275-303: The debounce mechanism currently blocks rescheduling when
a pending task exists for a triggerID, causing rapid state flips to be ignored
rather than restarting the debounce window. Modify the guard statement that
checks pendingApplyTasks[triggerID] == nil to cancel any existing pending task
before creating a new one, rather than returning early. This allows the settle
duration to be properly restarted from the latest state change instead of
applying based on a stale debounce timer from the first flip.
In `@Thaw/Settings/SettingsPanes/HotkeysSettingsPane.swift`:
- Line 163: The code on line 163 is using direct dictionary access with
imageCache.images[$0.tag]?.nsImage instead of calling the
MenuBarItemImageCache.image(for:) helper method. Replace the direct dictionary
access pattern with a call to the image(for:) method, passing the tag value from
row.item. This ensures proper windowID-insensitive cache lookups and prevents
rows from displaying placeholder icons when a cached image is available.
In `@Thaw/UserNotifications/UserNotificationManager.swift`:
- Around line 83-86: The code in the .triggerFired case unconditionally opens
the Settings window regardless of which action was tapped on the notification.
Add a guard condition that checks the action identifier before setting the
navigation state and calling appState.openWindow(.settings). Only perform the
navigation when the action identifier matches the default action (typically
.default), so that dismiss and other non-default actions do not trigger
navigation to Settings.
In `@Thaw/Utilities/MouseHelpers.swift`:
- Around line 64-67: The guard condition comparing autoShowDeadline and deadline
uses the wrong comparison operator, preventing earlier deadlines from being
scheduled. Change the comparison operator from >= to <= in the
autoShowDeadline.uptimeNanoseconds check at lines 64-67 and the duplicate
condition at line 227, so that shorter retry deadlines (like the 250ms retry
after CGDisplayShowCursor failure) can be scheduled instead of being blocked by
later existing timeouts.
In `@Thaw/Utilities/PowerSourceMonitor.swift`:
- Around line 63-104: The PowerSourceMonitor class is missing a deinit method to
clean up resources when the object is deallocated. Add a deinit method to the
PowerSourceMonitor class that calls the existing stop() method. This ensures
that when the instance is deallocated, the run-loop source is properly removed
and the safety timer is invalidated, preventing the IOPowerSourceCallbackType
callback from firing on a dangling pointer after deallocation.
In `@Thaw/Utilities/SystemStateMonitor.swift`:
- Around line 779-784: In the locationManagerDidChangeAuthorization method, the
authorization status check is incomplete and excludes the .authorizedWhenInUse
status. Update the conditional logic that checks the manager.authorizationStatus
to include .authorizedWhenInUse alongside the existing checks for .authorized
and .authorizedAlways. This will ensure that startUpdatingLocation() is called
when the user grants "When In Use" authorization through
requestWhenInUseAuthorization().
In `@Thaw/Utilities/TriggerScriptRunner.swift`:
- Around line 69-77: The timeout handling in TriggerScriptRunner can deadlock
because readDataToEndOfFile() and waitUntilExit() are called sequentially after
terminate(), but if the script process fills the pipe buffer before exiting, a
deadlock occurs. After calling terminate() when the timeout triggers, add a
forced kill step (using killProcess() if the process is still running after a
brief interval), and critically, drain the pipe output before calling
waitUntilExit() rather than after, to prevent the pipe buffer from blocking the
process termination. This ensures the trigger script cannot stall trigger
evaluation even if it ignores SIGTERM or fills the buffer.
In `@ThawTests/ControlCenterHostedMatchLogReplayTests.swift`:
- Around line 195-202: The regex patterns used in the parsing logic are too
restrictive and don't account for spaces in titles and application labels.
Update the regex pattern for extracting the title value to allow spaces instead
of just non-whitespace characters. Similarly, update the regex pattern for
cgOwner extraction to accommodate spaces in the label. Additionally, update the
candidate label extraction regex pattern to allow spaces in application
identifiers instead of restricting to just alphanumeric characters, dots,
underscores, and hyphens. These changes will prevent silent parsing failures
when actual diagnostic lines contain spaces in these fields.
In `@ThawTests/MenuBarItemTriggerTests.swift`:
- Around line 534-562: The test testDecodingWithoutInvertDefaultsFalse builds a
json string to simulate legacy data without the invert field, but then never
decodes it. Instead, it constructs a new MenuBarItemTrigger object, encodes it,
and decodes the newly encoded data, which doesn't verify backward compatibility
with the actual legacy format. To fix this, remove the constructed object
creation and the encode/decode cycle, and instead directly decode the json
string to MenuBarItemTrigger using JSONDecoder, then assert that the decoded
object has invert defaulting to false. This properly tests that the missing
invert field in legacy payloads defaults correctly.
---
Outside diff comments:
In @.github/aw/actions-lock.json:
- Around line 8-17: Remove the outdated and unused setup action entry from the
actions-lock.json file. The old entry with repo path github/gh-aw/actions/setup
at version v0.76.1 (including its associated sha) is no longer referenced
anywhere in the repository and should be deleted entirely. Keep only the current
entry for github/gh-aw-actions/setup at v0.79.6.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 6836-6838: The currentTags variable is constructed using only
namespace and title (stripping any instanceIndex suffix), but savedSectionOrder
stores full uniqueIdentifier values that may include the instanceIndex suffix.
This mismatch causes the guard condition to incorrectly skip restoration when
saved layouts contain indexed entries. Replace the currentTags construction to
use the actual uniqueIdentifier property from each item instead of manually
building a string from namespace and title, ensuring the comparison against
savedTags accurately reflects all stored identifiers including those with
instanceIndex suffixes.
- Around line 1231-1263: The debounced publisher only processes the last launch
notification in a burst, which can cause startSettlingPeriod to not be called
when a tracked app relaunches alongside untracked apps within the debounce
window. Split the current sink subscriber into two separate subscribers: create
a non-debounced subscriber on NSWorkspace.shared.notificationCenter.publisher
for didLaunchApplicationNotification that immediately checks if the launched app
is tracked (using MenuBarItemManager.tracksMenuBarItem) and calls
startSettlingPeriod with the appropriate bundle ID, while keeping the debounced
subscriber separate to handle only the cache refresh logic. This ensures
settling periods are always armed for tracked app relaunches regardless of
concurrent launches.
- Around line 2407-2428: The unstructured Task wrapper in the
waitForUserToPauseInput function creates a decoupled cancellation context that
delays observable cancellation when move or click operations are cancelled.
Remove the Task { ... } wrapper that starts at the beginning of the function
body and replace it with the inlined polling logic directly in the caller's
scope. This means the while loop with hasUserPausedInput(),
Task.checkCancellation(), and Task.sleep() should execute directly in the
function body instead of being wrapped in a nested Task, and remove the
corresponding do-catch block that awaits waitTask.value, replacing it with the
direct execution of the loop logic.
---
Nitpick comments:
In `@Thaw.xcodeproj/project.pbxproj`:
- Around line 582-586: The MARKETING_VERSION for the Thaw target is set to
2.0.0-beta.15 but the MenuBarItemService target has 2.0.0-beta.16, causing a
version mismatch. Update the MARKETING_VERSION value in the Thaw target's build
settings to match the MenuBarItemService target version of 2.0.0-beta.16. This
applies to both instances of the MARKETING_VERSION setting for the Thaw target
to ensure all targets are properly aligned.
In `@Thaw/Permissions/PermissionsView.swift`:
- Around line 301-316: The allPermissions array in MockPermissionsManager is
initializing real Permission objects (AccessibilityPermission and
ScreenRecordingPermission) which trigger actual system permission checks and
polling timers during preview rendering. Replace these real Permission instances
with mock implementations that are lightweight stubs without any system
interaction, polling timers, or permission checks. Create mock versions of
AccessibilityPermission and ScreenRecordingPermission that conform to the
Permission protocol but don't perform any actual system calls.
In `@ThawTests/PlanNotchOverflowTests.swift`:
- Around line 348-395: In both testZeroAvailableWidthYieldsNoOverflow and
testNonFiniteAvailableWidthYieldsNoOverflow, add assertions to verify that the
result's updatedDesiredFiltered equals the original desired sequence and
updatedSectionMap equals the original sectionMap, ensuring no silent mutations
occur on invalid budgets. The current tests only assert that overflowUIDs is
empty, but should also confirm the state preservation to catch potential bugs
where input data gets modified without triggering an overflow.
🪄 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: 4871c4ad-99de-4c5a-a7ef-d656cd27ca81
⛔ Files ignored due to path filters (9)
Thaw/Resources/AppIcon.icon/Assets/Background.pngis excluded by!**/*.pngThaw/Resources/AppIcon.icon/Assets/Cube Dark.pngis excluded by!**/*.pngThaw/Resources/AppIcon.icon/Assets/Cube.pngis excluded by!**/*.pngThaw/Resources/AppIcon.icon/Assets/Shiny Dark.pngis excluded by!**/*.pngThaw/Resources/AppIcon.icon/Assets/Vector 1.svgis excluded by!**/*.svgThaw/Resources/AppIcon.icon/Assets/Vector 2.svgis excluded by!**/*.svgThaw/Resources/AppIcon.icon/Assets/Vector 3.svgis excluded by!**/*.svgThaw/Resources/AppIcon.icon/Assets/cube.svgis excluded by!**/*.svgThaw/Resources/AppIcon.icon/Assets/innards.svgis excluded by!**/*.svg
📒 Files selected for processing (93)
.github/ISSUE_TEMPLATE/bug_report.yml.github/aw/actions-lock.json.github/workflows/issue-triage.lock.yml.github/workflows/issue-triage.md.github/workflows/remove.ymlMenuBarItemService/SourcePIDCache.swiftREADME.mdShared/Bridging/Bridging.swiftShared/Utilities/AXHelpers.swiftShared/Utilities/MarkerPairResolver.swiftThaw.xcodeproj/project.pbxprojThaw/Hotkeys/Hotkey.swiftThaw/Hotkeys/HotkeyAction.swiftThaw/Main/AppDelegate.swiftThaw/Main/AppState.swiftThaw/Main/Navigation/NavigationIdentifiers/SettingsNavigationIdentifier.swiftThaw/MenuBar/Appearance/MenuBarAppearanceManager.swiftThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemTag.swiftThaw/MenuBar/MenuBarItems/PendingLedger.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/Search/MenuBarSearchPanel.swiftThaw/MenuBar/Spacing/MenuBarItemSpacingManager.swiftThaw/Onboarding/OnboardingMockups.swiftThaw/Onboarding/OnboardingPageIndicator.swiftThaw/Onboarding/OnboardingSheet.swiftThaw/Onboarding/OnboardingSlide.swiftThaw/Permissions/AppPermissions.swiftThaw/Permissions/Permission.swiftThaw/Permissions/PermissionsView.swiftThaw/Permissions/PermissionsWindow.swiftThaw/Resources/AppIcon.icon/icon.jsonThaw/Resources/Assets.xcassets/WarningColor.colorset/Contents.jsonThaw/Resources/Info.plistThaw/Resources/Localizable.xcstringsThaw/Settings/Models/AppSettings.swiftThaw/Settings/Models/DisplayIceBarConfiguration.swiftThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/MenuBarItemTrigger.swiftThaw/Settings/Models/MenuBarItemTriggersManager.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Settings/Models/SettingsResetter.swiftThaw/Settings/Models/ThawFocusFilter.swiftThaw/Settings/Models/TriggerFeatureFlags.swiftThaw/Settings/SettingsPanes/AboutSettingsPane.swiftThaw/Settings/SettingsPanes/AdvancedSettingsPane.swiftThaw/Settings/SettingsPanes/AutomationSettingsPane.swiftThaw/Settings/SettingsPanes/DeveloperSettingsPane.swiftThaw/Settings/SettingsPanes/DisplaySettingsPane.swiftThaw/Settings/SettingsPanes/HotkeysSettingsPane.swiftThaw/Settings/SettingsPanes/ProfileSettingsPane.swiftThaw/Settings/SettingsPanes/TriggersSettingsPane.swiftThaw/Settings/SettingsView.swiftThaw/Settings/SettingsWindow.swiftThaw/Thaw-Bridging-Header.hThaw/UserNotifications/UserNotificationIdentifier.swiftThaw/UserNotifications/UserNotificationManager.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/Extensions.swiftThaw/Utilities/ImageHashing.swiftThaw/Utilities/MouseHelpers.swiftThaw/Utilities/PowerSourceMonitor.swiftThaw/Utilities/SettingsURIHandler.swiftThaw/Utilities/SystemStateMonitor.swiftThaw/Utilities/TriggerScriptRunner.swiftThaw/VirtualDisplay/ThawVirtualDisplay.hThaw/VirtualDisplay/ThawVirtualDisplay.mThaw/VirtualDisplay/VirtualDisplay.swiftThaw/VirtualDisplay/VirtualDisplayProvoker.swiftThawTests/ControlCenterHostedMatchLogReplayTests.swiftThawTests/Fixtures/ControlCenterHostedResolutionLog.swiftThawTests/Fixtures/LittleSnitchOrphanLog.swiftThawTests/FlattenCurrentSectionsTests.swiftThawTests/HotkeyActionTests.swiftThawTests/LayoutReconcilerTests.swiftThawTests/MarkerPairResolverTests.swiftThawTests/MenuBarItemManagerRearmTests.swiftThawTests/MenuBarItemTagTests.swiftThawTests/MenuBarItemTriggerTests.swiftThawTests/OnboardingMockupsTests.swiftThawTests/OnboardingSlideTests.swiftThawTests/PartitionUnmanagedUIDsTests.swiftThawTests/PlanFullSortSequenceTests.swiftThawTests/PlanNotchOverflowTests.swiftThawTests/ProfileLayoutLogReplayTests.swiftThawTests/ProfileManagerRearmGateTests.swiftThawTests/ProfileManagerUpdateRearmIntegrationTests.swiftThawTests/ProfileTests.swiftThawTests/WindowIDsChangedGateTests.swift
Comment on lines
+15
to
+40
| const runs = await github.paginate( | ||
| github.rest.actions.listWorkflowRunsForRepo, | ||
| { | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| status: "skipped", | ||
| per_page: 100, | ||
| }, | ||
| ); | ||
|
|
||
| let deletedCount = 0; | ||
| let failedCount = 0; | ||
|
|
||
| for (const run of runs) { | ||
| try { | ||
| await github.rest.actions.deleteWorkflowRun({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| run_id: run.id, | ||
| }); | ||
| deletedCount++; | ||
| } catch (error) { | ||
| console.error(`Error deleting workflow run: ${error.message}`); | ||
| failedCount++; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for pagination failures and improve logging.
The workflow has two error handling gaps:
-
Pagination failures not caught (line 15): If
github.paginate()throws an error (e.g., API rate limit, network timeout), the entire script fails silently. Wrap it in try/catch. -
Limited deletion auditing (lines 28-40): Only counts are logged; individual run IDs are not recorded. If something goes wrong, there's no way to verify which runs were deleted or retry failures.
-
No rate limiting (line 28): Attempting to delete many runs in quick succession could trigger GitHub API throttling.
Consider adding per-deletion delays (await new Promise(r => setTimeout(r, 100))) and logging individual run IDs for audit trails.
💡 Suggested improvements
script: |
+ let paginationError = false;
const runs = await github.paginate(
github.rest.actions.listWorkflowRunsForRepo,
{
owner: context.repo.owner,
repo: context.repo.repo,
status: "skipped",
per_page: 100,
},
+ ).catch(err => {
+ console.error(`Pagination error: ${err.message}`);
+ paginationError = true;
+ return [];
+ });
+ if (paginationError) {
+ core.setFailed('Failed to fetch skipped workflow runs');
+ return;
+ }
+
let deletedCount = 0;
let failedCount = 0;
+ const deletedRunIds = [];
+ const failedRunIds = [];
for (const run of runs) {
+ // Rate limiting: 100ms delay between deletions
+ await new Promise(r => setTimeout(r, 100));
try {
await github.rest.actions.deleteWorkflowRun({
owner: context.repo.owner,
repo: context.repo.repo,
run_id: run.id,
});
deletedCount++;
+ deletedRunIds.push(run.id);
} catch (error) {
console.error(`Error deleting run ${run.id}: ${error.message}`);
failedCount++;
+ failedRunIds.push(run.id);
}
}
console.log(`Deleted skipped workflow runs: ${deletedCount}`);
+ console.log(`Deleted run IDs: ${deletedRunIds.join(', ')}`);
console.log(`Failed to delete workflow runs: ${failedCount}`);
+ if (failedCount > 0) {
+ console.log(`Failed run IDs: ${failedRunIds.join(', ')}`);
+ }📝 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.
Suggested change
| const runs = await github.paginate( | |
| github.rest.actions.listWorkflowRunsForRepo, | |
| { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| status: "skipped", | |
| per_page: 100, | |
| }, | |
| ); | |
| let deletedCount = 0; | |
| let failedCount = 0; | |
| for (const run of runs) { | |
| try { | |
| await github.rest.actions.deleteWorkflowRun({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| run_id: run.id, | |
| }); | |
| deletedCount++; | |
| } catch (error) { | |
| console.error(`Error deleting workflow run: ${error.message}`); | |
| failedCount++; | |
| } | |
| } | |
| let paginationError = false; | |
| const runs = await github.paginate( | |
| github.rest.actions.listWorkflowRunsForRepo, | |
| { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| status: "skipped", | |
| per_page: 100, | |
| }, | |
| ).catch(err => { | |
| console.error(`Pagination error: ${err.message}`); | |
| paginationError = true; | |
| return []; | |
| }); | |
| if (paginationError) { | |
| core.setFailed('Failed to fetch skipped workflow runs'); | |
| return; | |
| } | |
| let deletedCount = 0; | |
| let failedCount = 0; | |
| const deletedRunIds = []; | |
| const failedRunIds = []; | |
| for (const run of runs) { | |
| // Rate limiting: 100ms delay between deletions | |
| await new Promise(r => setTimeout(r, 100)); | |
| try { | |
| await github.rest.actions.deleteWorkflowRun({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| run_id: run.id, | |
| }); | |
| deletedCount++; | |
| deletedRunIds.push(run.id); | |
| } catch (error) { | |
| console.error(`Error deleting run ${run.id}: ${error.message}`); | |
| failedCount++; | |
| failedRunIds.push(run.id); | |
| } | |
| } | |
| console.log(`Deleted skipped workflow runs: ${deletedCount}`); | |
| console.log(`Deleted run IDs: ${deletedRunIds.join(', ')}`); | |
| console.log(`Failed to delete workflow runs: ${failedCount}`); | |
| if (failedCount > 0) { | |
| console.log(`Failed run IDs: ${failedRunIds.join(', ')}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/remove.yml around lines 15 - 40, The pagination call to
github.paginate() is not wrapped in error handling, so API failures cause silent
script failure. Wrap the github.paginate() call in a try/catch block to handle
potential API errors. Additionally, the deletion loop lacks per-run logging and
rate limiting. Add console logging for each individual run.id when successfully
deleted (in the try block) and when failures occur (in the catch block for each
run). Finally, add a small delay between deletion attempts using await new
Promise(r => setTimeout(r, 100)) after each successful deletion to avoid
triggering GitHub API rate limits during batch operations.
Comment on lines
+2306
to
+2315
| let rawWindowIDs = Bridging.getMenuBarWindowList(option: [.itemsOnly, .activeSpace]) | ||
| // Exclude windowIDs already known to be system clones so their | ||
| // churn doesn't read as a layout change. A brand-new clone whose | ||
| // windowID hasn't been learned yet still triggers one recache, | ||
| // which resolves it, records it, and drops it; from then on its | ||
| // presence and removal are ignored. | ||
| let cloneIDs = cacheActor.cachedCloneWindowIDs | ||
| let itemWindowIDs = cloneIDs.isEmpty | ||
| ? rawWindowIDs | ||
| : rawWindowIDs.filter { !cloneIDs.contains($0) } |
Contributor
There was a problem hiding this comment.
Expire clone IDs once they leave the raw window list.
cachedCloneWindowIDs can outlive the transient clone. If WindowServer later recycles that ID for a real menu item, this filter removes the real item before the comparison, so cacheItemsIfNeeded() may never recache it.
Proposed fix
let rawWindowIDs = Bridging.getMenuBarWindowList(option: [.itemsOnly, .activeSpace])
// Exclude windowIDs already known to be system clones so their
// churn doesn't read as a layout change. A brand-new clone whose
// windowID hasn't been learned yet still triggers one recache,
// which resolves it, records it, and drops it; from then on its
// presence and removal are ignored.
let cloneIDs = cacheActor.cachedCloneWindowIDs
- let itemWindowIDs = cloneIDs.isEmpty
+ let activeCloneIDs = cloneIDs.intersection(rawWindowIDs)
+ if activeCloneIDs.count != cloneIDs.count {
+ cacheActor.updateCachedCloneWindowIDs(activeCloneIDs)
+ }
+ let itemWindowIDs = activeCloneIDs.isEmpty
? rawWindowIDs
- : rawWindowIDs.filter { !cloneIDs.contains($0) }
+ : rawWindowIDs.filter { !activeCloneIDs.contains($0) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 2306 - 2315,
The cachedCloneWindowIDs can persist after their corresponding clone windows are
no longer present in rawWindowIDs. If WindowServer recycles those IDs for real
menu items, those items get filtered out incorrectly. Update the logic to expire
clone IDs that are no longer in rawWindowIDs by filtering cachedCloneWindowIDs
to only keep IDs that still exist in rawWindowIDs, ensuring that recycled window
IDs representing real menu items are not removed by the subsequent filter
operation.
Comment on lines
5845
to
+5851
| var items = await MenuBarItem.getMenuBarItems(option: .activeSpace) | ||
| // Drop transient System Status Item Clone windows before planning. | ||
| // partitionUnmanagedUIDs would otherwise classify a clone as an | ||
| // unmanaged item and anchor it into a section, dragging a phantom | ||
| // and reshuffling the bar. This fetch is independent of the cache | ||
| // path, so it needs its own filter. | ||
| items.removeAll(where: \.isSystemClone) |
Contributor
There was a problem hiding this comment.
Filter system clones on every live fetch used for moves.
The initial profile-layout fetch drops clones, but later getMenuBarItems(...) calls in the same apply path and moveItem(trigger) still search unfiltered arrays. Since move(item:) treats clones as a no-op success, selecting a clone can increment movedCount or return true without moving the real item. Use a shared “non-clone active-space items” helper or apply removeAll(where: \.isSystemClone) after each fresh fetch before resolving targets.
Also applies to: 6219-6239, 6347-6379, 6403-6462, 6506-6529, 6571-6602, 6629-6638, 6898-6903
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 5845 - 5851,
The system clone filter is only applied once during the initial profile-layout
fetch, but subsequent calls to getMenuBarItems(...) in the apply path and
moveItem(trigger) operate on unfiltered arrays, allowing clones to be treated as
successful move operations. Add the same removeAll(where: \.isSystemClone)
filter after every fresh getMenuBarItems(...) call before the items are used to
resolve move targets, ensuring that clones are consistently filtered out across
all live fetches used for move operations. This prevents clones from incorrectly
incrementing movedCount or returning success without actually moving the real
item.
Comment on lines
+275
to
+303
| guard pendingApplyTasks[triggerID] == nil else { return } | ||
| // Use the most conservative (longest) settle across all conditions so | ||
| // a jittery source (e.g. battery) still absorbs flapping. | ||
| let settle: Duration = { | ||
| guard let trigger = triggers.first(where: { $0.id == triggerID }) else { return flipDebounce } | ||
| if let override = trigger.settleSecondsOverride, override > 0 { | ||
| return .seconds(override) | ||
| } | ||
| return trigger.allConditions.map(\.kind.settleInterval).max() ?? flipDebounce | ||
| }() | ||
| pendingApplyTasks[triggerID] = Task { @MainActor [weak self] in | ||
| try? await Task.sleep(for: settle) | ||
| guard !Task.isCancelled, let self else { return } | ||
| self.pendingApplyTasks[triggerID] = nil | ||
|
|
||
| guard | ||
| let trigger = self.triggers.first(where: { $0.id == triggerID }), | ||
| trigger.isEnabled, | ||
| !trigger.allItemIdentifiers.isEmpty, | ||
| self.isAvailable(trigger) | ||
| else { | ||
| return | ||
| } | ||
| let state = self.effectiveState(for: trigger, base: self.evaluationState) | ||
| let reveal = trigger.shouldReveal(state: state) | ||
| guard self.lastAppliedReveal[triggerID] != reveal else { return } | ||
| self.apply(trigger, reveal: reveal) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Debounce window is not restarted after subsequent flips.
A pending task blocks rescheduling, so rapid state flips can still apply based on a state that has not stayed stable for the full settle interval.
💡 Proposed fix
private func scheduleDebouncedApply(for triggerID: UUID) {
- guard pendingApplyTasks[triggerID] == nil else { return }
+ pendingApplyTasks[triggerID]?.cancel()
// Use the most conservative (longest) settle across all conditions so
// a jittery source (e.g. battery) still absorbs flapping.
let settle: Duration = {
guard let trigger = triggers.first(where: { $0.id == triggerID }) else { return flipDebounce }
if let override = trigger.settleSecondsOverride, override > 0 {
return .seconds(override)
}
return trigger.allConditions.map(\.kind.settleInterval).max() ?? flipDebounce
}()
pendingApplyTasks[triggerID] = Task { `@MainActor` [weak self] in
try? await Task.sleep(for: settle)
guard !Task.isCancelled, let self else { return }
self.pendingApplyTasks[triggerID] = nil
...
}
}📝 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.
Suggested change
| guard pendingApplyTasks[triggerID] == nil else { return } | |
| // Use the most conservative (longest) settle across all conditions so | |
| // a jittery source (e.g. battery) still absorbs flapping. | |
| let settle: Duration = { | |
| guard let trigger = triggers.first(where: { $0.id == triggerID }) else { return flipDebounce } | |
| if let override = trigger.settleSecondsOverride, override > 0 { | |
| return .seconds(override) | |
| } | |
| return trigger.allConditions.map(\.kind.settleInterval).max() ?? flipDebounce | |
| }() | |
| pendingApplyTasks[triggerID] = Task { @MainActor [weak self] in | |
| try? await Task.sleep(for: settle) | |
| guard !Task.isCancelled, let self else { return } | |
| self.pendingApplyTasks[triggerID] = nil | |
| guard | |
| let trigger = self.triggers.first(where: { $0.id == triggerID }), | |
| trigger.isEnabled, | |
| !trigger.allItemIdentifiers.isEmpty, | |
| self.isAvailable(trigger) | |
| else { | |
| return | |
| } | |
| let state = self.effectiveState(for: trigger, base: self.evaluationState) | |
| let reveal = trigger.shouldReveal(state: state) | |
| guard self.lastAppliedReveal[triggerID] != reveal else { return } | |
| self.apply(trigger, reveal: reveal) | |
| } | |
| } | |
| pendingApplyTasks[triggerID]?.cancel() | |
| // Use the most conservative (longest) settle across all conditions so | |
| // a jittery source (e.g. battery) still absorbs flapping. | |
| let settle: Duration = { | |
| guard let trigger = triggers.first(where: { $0.id == triggerID }) else { return flipDebounce } | |
| if let override = trigger.settleSecondsOverride, override > 0 { | |
| return .seconds(override) | |
| } | |
| return trigger.allConditions.map(\.kind.settleInterval).max() ?? flipDebounce | |
| }() | |
| pendingApplyTasks[triggerID] = Task { `@MainActor` [weak self] in | |
| try? await Task.sleep(for: settle) | |
| guard !Task.isCancelled, let self else { return } | |
| self.pendingApplyTasks[triggerID] = nil | |
| guard | |
| let trigger = self.triggers.first(where: { $0.id == triggerID }), | |
| trigger.isEnabled, | |
| !trigger.allItemIdentifiers.isEmpty, | |
| self.isAvailable(trigger) | |
| else { | |
| return | |
| } | |
| let state = self.effectiveState(for: trigger, base: self.evaluationState) | |
| let reveal = trigger.shouldReveal(state: state) | |
| guard self.lastAppliedReveal[triggerID] != reveal else { return } | |
| self.apply(trigger, reveal: reveal) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/Settings/Models/MenuBarItemTriggersManager.swift` around lines 275 -
303, The debounce mechanism currently blocks rescheduling when a pending task
exists for a triggerID, causing rapid state flips to be ignored rather than
restarting the debounce window. Modify the guard statement that checks
pendingApplyTasks[triggerID] == nil to cancel any existing pending task before
creating a new one, rather than returning early. This allows the settle duration
to be properly restarted from the latest state change instead of applying based
on a stale debounce timer from the first flip.
|
|
||
| @ViewBuilder | ||
| private func iconView(for row: Row) -> some View { | ||
| if let image = row.item.flatMap({ imageCache.images[$0.tag]?.nsImage }) { |
Contributor
There was a problem hiding this comment.
Use the cache lookup helper instead of direct dictionary access.
Line 163 bypasses MenuBarItemImageCache.image(for:), so windowID-insensitive cache hits are missed and rows can show placeholders even when a cached icon exists.
Suggested fix
- if let image = row.item.flatMap({ imageCache.images[$0.tag]?.nsImage }) {
+ if let image = row.item.flatMap({ imageCache.image(for: $0.tag)?.nsImage }) {📝 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.
Suggested change
| if let image = row.item.flatMap({ imageCache.images[$0.tag]?.nsImage }) { | |
| if let image = row.item.flatMap({ imageCache.image(for: $0.tag)?.nsImage }) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/Settings/SettingsPanes/HotkeysSettingsPane.swift` at line 163, The code
on line 163 is using direct dictionary access with
imageCache.images[$0.tag]?.nsImage instead of calling the
MenuBarItemImageCache.image(for:) helper method. Replace the direct dictionary
access pattern with a call to the image(for:) method, passing the tag value from
row.item. This ensures proper windowID-insensitive cache lookups and prevents
rows from displaying placeholder icons when a cached image is available.
Comment on lines
+63
to
+104
| func start() { | ||
| guard runLoopSource == nil else { return } | ||
|
|
||
| let context = Unmanaged.passUnretained(self).toOpaque() | ||
| let callback: IOPowerSourceCallbackType = { context in | ||
| guard let context else { return } | ||
| let monitor = Unmanaged<PowerSourceMonitor>.fromOpaque(context).takeUnretainedValue() | ||
| // The callback is invoked on the run loop it was registered | ||
| // on (the main run loop), but hop explicitly to keep the | ||
| // @MainActor isolation contract clear. | ||
| Task { @MainActor in | ||
| monitor.refresh() | ||
| } | ||
| } | ||
|
|
||
| if let source = IOPSNotificationCreateRunLoopSource(callback, context)?.takeRetainedValue() { | ||
| runLoopSource = source | ||
| CFRunLoopAddSource(CFRunLoopGetMain(), source, .defaultMode) | ||
| } else { | ||
| diagLog.warning("Failed to create power source notification run loop source") | ||
| } | ||
|
|
||
| let timer = Timer(timeInterval: 60, repeats: true) { [weak self] _ in | ||
| Task { @MainActor in | ||
| self?.refresh() | ||
| } | ||
| } | ||
| RunLoop.main.add(timer, forMode: .common) | ||
| safetyTimer = timer | ||
|
|
||
| refresh() | ||
| } | ||
|
|
||
| /// Stops observing power source changes. | ||
| func stop() { | ||
| if let runLoopSource { | ||
| CFRunLoopRemoveSource(CFRunLoopGetMain(), runLoopSource, .defaultMode) | ||
| self.runLoopSource = nil | ||
| } | ||
| safetyTimer?.invalidate() | ||
| safetyTimer = nil | ||
| } |
Contributor
There was a problem hiding this comment.
Missing deinit to clean up the run-loop source.
The callback holds a raw pointer to self via Unmanaged.passUnretained. If the PowerSourceMonitor is deallocated while the run-loop source is still active, the callback could fire on a dangling pointer. Adding a deinit that calls stop() ensures cleanup.
🛡️ Proposed fix
private let diagLog = DiagLog(category: "PowerSourceMonitor")
+ deinit {
+ // Must run synchronously on MainActor; safe because dealloc can only
+ // happen when no strong refs remain, so no deadlock risk.
+ if let source = runLoopSource {
+ CFRunLoopRemoveSource(CFRunLoopGetMain(), source, .defaultMode)
+ }
+ safetyTimer?.invalidate()
+ }
+
/// Begins observing power source changes. Safe to call more than once;📝 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.
Suggested change
| func start() { | |
| guard runLoopSource == nil else { return } | |
| let context = Unmanaged.passUnretained(self).toOpaque() | |
| let callback: IOPowerSourceCallbackType = { context in | |
| guard let context else { return } | |
| let monitor = Unmanaged<PowerSourceMonitor>.fromOpaque(context).takeUnretainedValue() | |
| // The callback is invoked on the run loop it was registered | |
| // on (the main run loop), but hop explicitly to keep the | |
| // @MainActor isolation contract clear. | |
| Task { @MainActor in | |
| monitor.refresh() | |
| } | |
| } | |
| if let source = IOPSNotificationCreateRunLoopSource(callback, context)?.takeRetainedValue() { | |
| runLoopSource = source | |
| CFRunLoopAddSource(CFRunLoopGetMain(), source, .defaultMode) | |
| } else { | |
| diagLog.warning("Failed to create power source notification run loop source") | |
| } | |
| let timer = Timer(timeInterval: 60, repeats: true) { [weak self] _ in | |
| Task { @MainActor in | |
| self?.refresh() | |
| } | |
| } | |
| RunLoop.main.add(timer, forMode: .common) | |
| safetyTimer = timer | |
| refresh() | |
| } | |
| /// Stops observing power source changes. | |
| func stop() { | |
| if let runLoopSource { | |
| CFRunLoopRemoveSource(CFRunLoopGetMain(), runLoopSource, .defaultMode) | |
| self.runLoopSource = nil | |
| } | |
| safetyTimer?.invalidate() | |
| safetyTimer = nil | |
| } | |
| deinit { | |
| // Must run synchronously on MainActor; safe because dealloc can only | |
| // happen when no strong refs remain, so no deadlock risk. | |
| if let source = runLoopSource { | |
| CFRunLoopRemoveSource(CFRunLoopGetMain(), source, .defaultMode) | |
| } | |
| safetyTimer?.invalidate() | |
| } | |
| func start() { | |
| guard runLoopSource == nil else { return } | |
| let context = Unmanaged.passUnretained(self).toOpaque() | |
| let callback: IOPowerSourceCallbackType = { context in | |
| guard let context else { return } | |
| let monitor = Unmanaged<PowerSourceMonitor>.fromOpaque(context).takeUnretainedValue() | |
| // The callback is invoked on the run loop it was registered | |
| // on (the main run loop), but hop explicitly to keep the | |
| // `@MainActor` isolation contract clear. | |
| Task { `@MainActor` in | |
| monitor.refresh() | |
| } | |
| } | |
| if let source = IOPSNotificationCreateRunLoopSource(callback, context)?.takeRetainedValue() { | |
| runLoopSource = source | |
| CFRunLoopAddSource(CFRunLoopGetMain(), source, .defaultMode) | |
| } else { | |
| diagLog.warning("Failed to create power source notification run loop source") | |
| } | |
| let timer = Timer(timeInterval: 60, repeats: true) { [weak self] _ in | |
| Task { `@MainActor` in | |
| self?.refresh() | |
| } | |
| } | |
| RunLoop.main.add(timer, forMode: .common) | |
| safetyTimer = timer | |
| refresh() | |
| } | |
| /// Stops observing power source changes. | |
| func stop() { | |
| if let runLoopSource { | |
| CFRunLoopRemoveSource(CFRunLoopGetMain(), runLoopSource, .defaultMode) | |
| self.runLoopSource = nil | |
| } | |
| safetyTimer?.invalidate() | |
| safetyTimer = nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/Utilities/PowerSourceMonitor.swift` around lines 63 - 104, The
PowerSourceMonitor class is missing a deinit method to clean up resources when
the object is deallocated. Add a deinit method to the PowerSourceMonitor class
that calls the existing stop() method. This ensures that when the instance is
deallocated, the run-loop source is properly removed and the safety timer is
invalidated, preventing the IOPowerSourceCallbackType callback from firing on a
dangling pointer after deallocation.
Comment on lines
+779
to
+784
| func locationManagerDidChangeAuthorization(_ manager: CLLocationManager) { | ||
| let status = manager.authorizationStatus | ||
| if isUpdating, status == .authorized || status == .authorizedAlways { | ||
| manager.startUpdatingLocation() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing .authorizedWhenInUse check prevents location updates from starting.
When the user grants "When In Use" authorization (via requestWhenInUseAuthorization() at line 758), the resulting status is .authorizedWhenInUse, not .authorized or .authorizedAlways. The current check fails to match this status, so startUpdatingLocation() is never called after authorization is granted.
🐛 Proposed fix
func locationManagerDidChangeAuthorization(_ manager: CLLocationManager) {
let status = manager.authorizationStatus
- if isUpdating, status == .authorized || status == .authorizedAlways {
+ if isUpdating, status == .authorizedWhenInUse || status == .authorizedAlways {
manager.startUpdatingLocation()
}
}📝 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.
Suggested change
| func locationManagerDidChangeAuthorization(_ manager: CLLocationManager) { | |
| let status = manager.authorizationStatus | |
| if isUpdating, status == .authorized || status == .authorizedAlways { | |
| manager.startUpdatingLocation() | |
| } | |
| } | |
| func locationManagerDidChangeAuthorization(_ manager: CLLocationManager) { | |
| let status = manager.authorizationStatus | |
| if isUpdating, status == .authorizedWhenInUse || status == .authorizedAlways { | |
| manager.startUpdatingLocation() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/Utilities/SystemStateMonitor.swift` around lines 779 - 784, In the
locationManagerDidChangeAuthorization method, the authorization status check is
incomplete and excludes the .authorizedWhenInUse status. Update the conditional
logic that checks the manager.authorizationStatus to include
.authorizedWhenInUse alongside the existing checks for .authorized and
.authorizedAlways. This will ensure that startUpdatingLocation() is called when
the user grants "When In Use" authorization through
requestWhenInUseAuthorization().
Comment on lines
+69
to
+77
| if timedOut, process.isRunning { | ||
| process.terminate() | ||
| diagLog.warning("Trigger script timed out after \(timeout)s: \(trimmed)") | ||
| } | ||
|
|
||
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | ||
| process.waitUntilExit() | ||
| let output = String(data: data, encoding: .utf8) ?? "" | ||
| let exitCode = timedOut ? -1 : process.terminationStatus |
Contributor
There was a problem hiding this comment.
Timeout handling can still hang and stall trigger evaluation.
Line 74 and Line 75 can block indefinitely despite the timeout contract: output is drained only at the end, and terminate() alone is not a hard stop. A script that fills the pipe buffer or ignores SIGTERM can keep this call stuck and block trigger refresh.
Suggested hardening
+import Darwin
import Foundation
@@
- if timedOut, process.isRunning {
- process.terminate()
- diagLog.warning("Trigger script timed out after \(timeout)s: \(trimmed)")
- }
-
- let data = pipe.fileHandleForReading.readDataToEndOfFile()
- process.waitUntilExit()
+ let outputTask = Task { pipe.fileHandleForReading.readDataToEndOfFile() }
+
+ var didTimeout = false
+ if timedOut && process.isRunning {
+ didTimeout = true
+ process.terminate()
+ let deadline = ContinuousClock.now.advanced(by: .milliseconds(500))
+ while process.isRunning, ContinuousClock.now < deadline {
+ try? await Task.sleep(for: .milliseconds(50))
+ }
+ if process.isRunning {
+ kill(process.processIdentifier, SIGKILL)
+ }
+ diagLog.warning("Trigger script timed out after \(timeout)s: \(trimmed)")
+ }
+
+ process.waitUntilExit()
+ let data = await outputTask.value
let output = String(data: data, encoding: .utf8) ?? ""
- let exitCode = timedOut ? -1 : process.terminationStatus
+ let exitCode = didTimeout ? -1 : process.terminationStatus
return ScriptOutcome(exitCode: exitCode, output: output)📝 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.
Suggested change
| if timedOut, process.isRunning { | |
| process.terminate() | |
| diagLog.warning("Trigger script timed out after \(timeout)s: \(trimmed)") | |
| } | |
| let data = pipe.fileHandleForReading.readDataToEndOfFile() | |
| process.waitUntilExit() | |
| let output = String(data: data, encoding: .utf8) ?? "" | |
| let exitCode = timedOut ? -1 : process.terminationStatus | |
| import Darwin | |
| import Foundation | |
| let outputTask = Task { pipe.fileHandleForReading.readDataToEndOfFile() } | |
| var didTimeout = false | |
| if timedOut && process.isRunning { | |
| didTimeout = true | |
| process.terminate() | |
| let deadline = ContinuousClock.now.advanced(by: .milliseconds(500)) | |
| while process.isRunning, ContinuousClock.now < deadline { | |
| try? await Task.sleep(for: .milliseconds(50)) | |
| } | |
| if process.isRunning { | |
| kill(process.processIdentifier, SIGKILL) | |
| } | |
| diagLog.warning("Trigger script timed out after \(timeout)s: \(trimmed)") | |
| } | |
| process.waitUntilExit() | |
| let data = await outputTask.value | |
| let output = String(data: data, encoding: .utf8) ?? "" | |
| let exitCode = didTimeout ? -1 : process.terminationStatus | |
| return ScriptOutcome(exitCode: exitCode, output: output) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Thaw/Utilities/TriggerScriptRunner.swift` around lines 69 - 77, The timeout
handling in TriggerScriptRunner can deadlock because readDataToEndOfFile() and
waitUntilExit() are called sequentially after terminate(), but if the script
process fills the pipe buffer before exiting, a deadlock occurs. After calling
terminate() when the timeout triggers, add a forced kill step (using
killProcess() if the process is still running after a brief interval), and
critically, drain the pipe output before calling waitUntilExit() rather than
after, to prevent the pipe buffer from blocking the process termination. This
ensures the trigger script cannot stall trigger evaluation even if it ignores
SIGTERM or fills the buffer.
Comment on lines
+195
to
+202
| let title = line.firstMatch(of: /title=(\S+)/).map { String($0.output.1) } | ||
| let cgOwner = line.firstMatch(of: /cgOwner=([A-Za-z0-9._-]+):pid=/).map { String($0.output.1) } | ||
|
|
||
| var candidates = [CandidateChild]() | ||
| if let nearest = line.firstMatch(of: /nearest=\[(.*)\]/) { | ||
| for match in String(nearest.output.1) | ||
| .matches(of: /([A-Za-z0-9._-]+)@([0-9.]+)\(enabled=(nil|true|false)\)/) | ||
| { |
Contributor
There was a problem hiding this comment.
Harden parser patterns for space-containing titles and app labels.
The current regexes assume no spaces in title and candidate labels, which can silently mis-parse real diag unresolved lines and skew replay outcomes.
Proposed fix
- let title = line.firstMatch(of: /title=(\S+)/).map { String($0.output.1) }
+ let rawTitle = line.firstMatch(of: /title=(.*?) bounds=/).map { String($0.output.1) }
+ let title = (rawTitle == "nil") ? nil : rawTitle
@@
- for match in String(nearest.output.1)
- .matches(of: /([A-Za-z0-9._-]+)@([0-9.]+)\(enabled=(nil|true|false)\)/)
+ for match in String(nearest.output.1)
+ .matches(of: /([^@,\]]+)@([0-9.]+)\(enabled=(nil|true|false)\)/)
{
let enabled: Bool? = match.output.3 == "nil" ? nil : (match.output.3 == "true")
candidates.append(CandidateChild(
- appBundleID: String(match.output.1),
+ appBundleID: String(match.output.1).trimmingCharacters(in: .whitespaces),
distance: Double(match.output.2).map { CGFloat($0) } ?? .greatestFiniteMagnitude,
enabled: enabled
))
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ThawTests/ControlCenterHostedMatchLogReplayTests.swift` around lines 195 -
202, The regex patterns used in the parsing logic are too restrictive and don't
account for spaces in titles and application labels. Update the regex pattern
for extracting the title value to allow spaces instead of just non-whitespace
characters. Similarly, update the regex pattern for cgOwner extraction to
accommodate spaces in the label. Additionally, update the candidate label
extraction regex pattern to allow spaces in application identifiers instead of
restricting to just alphanumeric characters, dots, underscores, and hyphens.
These changes will prevent silent parsing failures when actual diagnostic lines
contain spaces in these fields.
Comment on lines
+534
to
+562
| func testDecodingWithoutInvertDefaultsFalse() throws { | ||
| // Simulates a trigger persisted before `invert` existed. | ||
| let json = """ | ||
| { | ||
| "id": "00000000-0000-0000-0000-000000000001", | ||
| "name": "Legacy", | ||
| "isEnabled": true, | ||
| "itemIdentifier": "x", | ||
| "itemDisplayName": "X", | ||
| "revealSection": "visible", | ||
| "hideSection": "hidden", | ||
| "condition": { "onACPower": {} } | ||
| } | ||
| """ | ||
| // Encode/decode the condition shape via the real encoder to avoid | ||
| // hand-writing its representation; fall back to a constructed value | ||
| // if the enum encoding differs. | ||
| let constructed = try MenuBarItemTrigger( | ||
| id: XCTUnwrap(UUID(uuidString: "00000000-0000-0000-0000-000000000001")), | ||
| name: "Legacy", | ||
| itemIdentifier: "x", | ||
| itemDisplayName: "X", | ||
| condition: .onACPower | ||
| ) | ||
| let data = try JSONEncoder().encode(constructed) | ||
| let decoded = try JSONDecoder().decode(MenuBarItemTrigger.self, from: data) | ||
| XCTAssertFalse(decoded.invert) | ||
| _ = json | ||
| } |
Contributor
There was a problem hiding this comment.
Legacy decode test does not exercise legacy payload.
The test builds json for old data but never decodes it, so backward-compatibility for missing invert is not actually verified.
💡 Proposed fix
func testDecodingWithoutInvertDefaultsFalse() throws {
- // Simulates a trigger persisted before `invert` existed.
- let json = """
- {
- "id": "00000000-0000-0000-0000-000000000001",
- "name": "Legacy",
- "isEnabled": true,
- "itemIdentifier": "x",
- "itemDisplayName": "X",
- "revealSection": "visible",
- "hideSection": "hidden",
- "condition": { "onACPower": {} }
- }
- """
- // Encode/decode the condition shape via the real encoder to avoid
- // hand-writing its representation; fall back to a constructed value
- // if the enum encoding differs.
let constructed = try MenuBarItemTrigger(
id: XCTUnwrap(UUID(uuidString: "00000000-0000-0000-0000-000000000001")),
name: "Legacy",
itemIdentifier: "x",
itemDisplayName: "X",
condition: .onACPower
)
- let data = try JSONEncoder().encode(constructed)
- let decoded = try JSONDecoder().decode(MenuBarItemTrigger.self, from: data)
+ let currentData = try JSONEncoder().encode(constructed)
+ var jsonObject = try XCTUnwrap(JSONSerialization.jsonObject(with: currentData) as? [String: Any])
+ jsonObject.removeValue(forKey: "invert")
+ let legacyData = try JSONSerialization.data(withJSONObject: jsonObject)
+ let decoded = try JSONDecoder().decode(MenuBarItemTrigger.self, from: legacyData)
XCTAssertFalse(decoded.invert)
- _ = json
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ThawTests/MenuBarItemTriggerTests.swift` around lines 534 - 562, The test
testDecodingWithoutInvertDefaultsFalse builds a json string to simulate legacy
data without the invert field, but then never decodes it. Instead, it constructs
a new MenuBarItemTrigger object, encodes it, and decodes the newly encoded data,
which doesn't verify backward compatibility with the actual legacy format. To
fix this, remove the constructed object creation and the encode/decode cycle,
and instead directly decode the json string to MenuBarItemTrigger using
JSONDecoder, then assert that the decoded object has invert defaulting to false.
This properly tests that the missing invert field in legacy payloads defaults
correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds trigger-based menu bar item visibility so items can appear only when specific system, app, power, or network conditions are met, including the trigger behavior requested in #422.
Closes: #422
PR Type
Does this PR introduce a breaking change?
What is the new behavior?
This adds trigger infrastructure, a Triggers settings surface, and fully implemented battery/power-based conditions for moving menu bar items based on state. It also adds a new Developer settings pane that gates additional trigger sources individually. All trigger options are implemented and work, while the non-battery trigger sources remain under active reliability optimization.
The trigger system can move a target item between configured sections when a condition becomes true or false. Supported condition types include battery/power state, frontmost/running app, network connectivity, VPN, Wi-Fi SSID, Bluetooth/audio/display state, time windows, Focus, location, Low Power Mode, thermal pressure, camera/microphone use, script results, and menu bar icon image changes.
While testing the frontmost-app trigger, this PR also hardens trigger-driven cursor/menu-bar movement behavior so stale or cancelled trigger checks do not keep posting synthetic cursor movement after the trigger condition is no longer valid.
PR Checklist
swiftformat .to keep the code style consistent.Other information
Note for reviewers: this PR adds a new Developer settings pane for gating trigger sources individually while they are tested and optimized. All listed trigger options are implemented and functional, but the non-battery trigger sources are still being optimized for reliability and edge cases. The fully implemented and fully tested behavior in this PR is the battery/power-based trigger set, which is available without a developer feature flag.
The README roadmap was updated to mark battery/power trigger conditions as implemented while keeping optimization of the additional trigger sources tracked separately.
Validated with:
Both validation commands succeeded.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests