Repository navigation
Add workspace and tab notification mute menus - #6658
lawrencecchen wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds timed notification muting at workspace and terminal-surface granularity. Introduces ChangesNotification Muting Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant ContextMenu
participant TerminalNotificationStore
participant NotificationDelivery
rect rgba(100, 149, 237, 0.5)
Note over User,ContextMenu: Mute action
User->>ContextMenu: select "Mute Notifications" with duration
ContextMenu->>TerminalNotificationStore: muteNotifications(forTabIds/surfaceId, until: expiration)
TerminalNotificationStore-->>ContextMenu: updated notificationMuteExpirations
end
rect rgba(255, 165, 0, 0.5)
Note over TerminalNotificationStore,NotificationDelivery: Notification arrives while muted
TerminalNotificationStore->>TerminalNotificationStore: applyNotification → activeNotificationMuteExpiration != nil
TerminalNotificationStore->>TerminalNotificationStore: shouldMuteNotificationSideEffects = true
TerminalNotificationStore->>TerminalNotificationStore: recordNotification (unread recorded)
TerminalNotificationStore-->>NotificationDelivery: early-return, no sound/banner/reorder
end
rect rgba(60, 179, 113, 0.5)
Note over User,ContextMenu: Unmute action
User->>ContextMenu: select "Unmute Notifications"
ContextMenu->>TerminalNotificationStore: unmuteNotifications(forTabIds/forSurfaceId)
TerminalNotificationStore-->>ContextMenu: mute expiration cleared
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "contextMenu.muteWorkspaceNotifications": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Mute Workspace Notifications" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ワークスペース通知をミュート" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "contextMenu.muteWorkspacesNotifications": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Mute Workspaces Notifications" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ワークスペース通知をミュート" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "contextMenu.unmuteWorkspaceNotifications": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Unmute Workspace Notifications" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ワークスペース通知のミュートを解除" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "contextMenu.unmuteWorkspacesNotifications": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Unmute Workspaces Notifications" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ワークスペース通知のミュートを解除" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "notificationMute.duration.eightHours": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "8 Hours" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "8時間" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "notificationMute.duration.fifteenMinutes": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "15 Minutes" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "15分" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "notificationMute.duration.fourHours": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "4 Hours" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "4時間" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "notificationMute.duration.oneHour": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "1 Hour" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "1時間" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "terminalContextMenu.muteTabNotifications": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Mute Tab Notifications" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "タブ通知をミュート" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "terminalContextMenu.unmuteTabNotifications": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Unmute Tab Notifications" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "タブ通知のミュートを解除" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "command.auth.signIn.title": { |
There was a problem hiding this comment.
All ten new mute/unmute string keys (contextMenu.muteWorkspaceNotifications, contextMenu.muteWorkspacesNotifications, contextMenu.unmuteWorkspaceNotifications, contextMenu.unmuteWorkspacesNotifications, notificationMute.duration.*, terminalContextMenu.*) only carry en and ja entries. Existing catalog strings already include zh-Hans, zh-Hant, ko, de, es, fr, it, da, pl, ru, bs, ar, and potentially others (visible starting at line 241). Users in those locales will see raw English fallback text in the mute menus and submenu items.
Rule Used: Flag production user-facing text that is not fully... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| func clearNotificationMutesForTesting() { | ||
| notificationMuteExpirationsByWorkspaceId = [:] | ||
| notificationMuteExpirationsBySurfaceId = [:] | ||
| } |
There was a problem hiding this comment.
…ForTesting name added to production source
clearNotificationMutesForTesting follows the banned …ForTesting naming convention and is a new seam introduced in this PR — it is not covered by the "existing seams not worsened" pass condition. Both notificationMuteExpirationsByWorkspaceId and notificationMuteExpirationsBySurfaceId are declared @Published private(set), so the test target can already read them via @testable import. To reset them from a test without a production-source seam, widen the setter visibility to internal (or add a dedicated non-ForTesting-named production reset path) so the test can assign directly after @testable import, following the pattern established in PR 6452.
Rule Used: Flag Swift files under a production Sources path (... (source)
| in: tabManager, | ||
| target: contextMenuPinTarget | ||
| ) | ||
| let contextMenuWorkspaceMuteActive = notificationStore.hasActiveWorkspaceNotificationMute( |
There was a problem hiding this comment.
contextMenuWorkspaceMuteActive is computed once at render time by snapshotting Date(). Because notificationMuteExpirationsByWorkspaceId never changes automatically when a mute timer fires, @Published never emits a new value, and SwiftUI will not re-render. The "Unmute Workspace Notifications" button therefore stays visible past the expiry time until some unrelated state change triggers a fresh render. Notifications themselves suppress correctly (the activeNotificationMuteExpiration check evaluates a live Date() on every call), but the context menu label remains stale. A Timer-driven objectWillChange send or an explicit expiry observation would keep the menu in sync.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalNotificationStore.swift (1)
1248-1266: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t consume cooldown reservations for muted notifications.
Both paths commit the reservation before returning for
shouldMuteNotificationSideEffects, so a muted notification can advance delivery cooldowns and throttle the first notification after unmute/expiry even though no side effect was delivered.🐛 Proposed fix
if effects.reorderWorkspace, !shouldMuteNotificationSideEffects, UserDefaultsSettingsClient(defaults: .standard).value(for: SettingCatalog().app.reorderOnNotification) { AppDelegate.shared?.tabManagerFor(tabId: notification.tabId)? .moveTabToTopForNotification(notification.tabId) } - if hasAnyNotificationEffect(effects) { + if hasAnyNotificationEffect(effects), !shouldMuteNotificationSideEffects { commitCooldownReservation(cooldownReservation, at: now) } else { restoreCooldownReservation(cooldownReservation) } guard !shouldMuteNotificationSideEffects else { @@ updated.insert(notification, at: 0) setWorkspaceManualUnread(false, forTabId: notification.tabId) notifications = updated - commitCooldownReservation(cooldownReservation, at: now) `#if` DEBUG cmuxDebugLog( "notification.store.record workspace=\(notification.tabId.uuidString.prefix(8)) surface=\(notification.surfaceId?.uuidString.prefix(8) ?? "nil") removed=\(idsToClear.count) unread=\(!notification.isRead ? 1 : 0) paneFlash=\(notification.paneFlash ? 1 : 0) suppressExternal=\(shouldSuppressExternalDelivery ? 1 : 0) total=\(notifications.count)" ) @@ guard !shouldMuteNotificationSideEffects else { + restoreCooldownReservation(cooldownReservation) `#if` DEBUG cmuxDebugLog( "notification.store.sideEffects.skip workspace=\(notification.tabId.uuidString.prefix(8)) surface=\(notification.surfaceId?.uuidString.prefix(8) ?? "nil") reason=muted" ) `#endif` return } + commitCooldownReservation(cooldownReservation, at: now) deliverNotificationSideEffects( notification, shouldSuppressExternalDelivery: shouldSuppressExternalDelivery, effects: effects )Also applies to: 1306-1368
🤖 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 `@Sources/TerminalNotificationStore.swift` around lines 1248 - 1266, The cooldown reservation is being committed or restored before checking if the notification should be muted. Move the cooldown reservation logic that checks hasAnyNotificationEffect and calls either commitCooldownReservation or restoreCooldownReservation to occur after the shouldMuteNotificationSideEffects guard statement. This ensures that muted notifications do not consume the cooldown reservation and do not throttle future notifications after unmute or expiry.
🤖 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 `@Sources/TerminalNotificationStore.swift`:
- Around line 503-504: The notificationMuteExpirationsByWorkspaceId and
notificationMuteExpirationsBySurfaceId properties are marked with `@Published`,
which will invalidate all observers on each mute/unmute operation due to
dictionary mutations. Remove the `@Published` decorator from these properties and
make them private storage instead. Then expose focused query methods or
value-snapshot APIs that return only the specific data needed by UI components,
following the repo's pattern of using `@Observable` with `@State` or value snapshots
rather than ObservableObject with `@Published` for new state.
- Around line 2279-2282: Remove the clearNotificationMutesForTesting method from
TerminalNotificationStore since production source files must not contain
test-only members with ForTesting naming conventions. Instead, use the existing
unmuteNotifications method to clean up notification mute state in tests, or if
direct property access is necessary, adjust the visibility of
notificationMuteExpirationsByWorkspaceId and
notificationMuteExpirationsBySurfaceId properties to be testable and then use
`@testable` import in test files to access them directly.
---
Outside diff comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 1248-1266: The cooldown reservation is being committed or restored
before checking if the notification should be muted. Move the cooldown
reservation logic that checks hasAnyNotificationEffect and calls either
commitCooldownReservation or restoreCooldownReservation to occur after the
shouldMuteNotificationSideEffects guard statement. This ensures that muted
notifications do not consume the cooldown reservation and do not throttle future
notifications after unmute or expiry.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cc7f1ef-d51f-437c-a2cd-e6d4b6000b3f
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyNSView+MoveTabToNewWorkspace.swiftSources/GhosttyTerminalView.swiftSources/TerminalNotificationStore.swiftcmuxTests/TerminalNotificationClearAllTests.swift
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
Sources/TerminalNotificationStore+Mute.swift (1)
83-90: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove
clearNotificationMutesForTestingfrom productionSources/code.This is a test/debug seam (
#if DEBUG+...ForTesting) in a productionSources/**/*.swiftfile, which is disallowed by repo policy.🧹 Proposed fix
-#if DEBUG -extension TerminalNotificationStore { - func clearNotificationMutesForTesting() { - notificationMuteExpirationsByWorkspaceId = [:] - notificationMuteExpirationsBySurfaceId = [:] - } -} -#endifAs per path instructions, production
Sources/**/*.swiftmust not add test/debug-only seams (including#if DEBUGmembers and...ForTestingnaming).🤖 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 `@Sources/TerminalNotificationStore`+Mute.swift around lines 83 - 90, Remove the entire `#if DEBUG` extension block from TerminalNotificationStore+Mute.swift that contains the `clearNotificationMutesForTesting()` method. This debug seam violates the repository policy that prohibits test/debug-only code in production `Sources/` directories. Delete the complete extension definition starting with `#if DEBUG` through the closing `#endif`, including the `clearNotificationMutesForTesting()` method and its implementation that directly modifies `notificationMuteExpirationsByWorkspaceId` and `notificationMuteExpirationsBySurfaceId`.Source: Path instructions
Sources/TerminalNotificationStore.swift (1)
437-438: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid introducing new
@Publishedmute maps.Publishing whole mute dictionaries invalidates broad observers on every mute/unmute and violates the repo’s new-state policy for cmux-owned SwiftUI state. Keep this as non-published internal state (or migrate to Observation/value snapshots) and expose focused read APIs.
♻️ Suggested direction
- `@Published` var notificationMuteExpirationsByWorkspaceId: [UUID: Date] = [:] - `@Published` var notificationMuteExpirationsBySurfaceId: [UUID: Date] = [:] + private(set) var notificationMuteExpirationsByWorkspaceId: [UUID: Date] = [:] + private(set) var notificationMuteExpirationsBySurfaceId: [UUID: Date] = [:]As per coding guidelines, “Do not use
ObservableObject,@Published,@StateObject, or@EnvironmentObjectfor new cmux-owned SwiftUI state,” and as per path instructions forSources/**/*.swift, SwiftUI state-layout violations in production changes should fail.🤖 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 `@Sources/TerminalNotificationStore.swift` around lines 437 - 438, The properties notificationMuteExpirationsByWorkspaceId and notificationMuteExpirationsBySurfaceId should not be marked with `@Published` as this violates the repo's policy for cmux-owned SwiftUI state and causes broad observer invalidation on every mute/unmute operation. Remove the `@Published` attribute from both properties, keep them as non-published internal state, and instead create focused read APIs (getter methods) that expose only the specific information needed by consumers rather than publishing the entire mute dictionaries.Sources: Coding guidelines, Path instructions
🤖 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 `@cmuxTests/CmuxNotificationMuteConfigTests.swift`:
- Around line 20-22: The test fixture contains a mixed-unit duration entry
combining both "hours" and "minutes" fields, which violates the schema contract
requiring exactly one of seconds, minutes, or hours per entry. Fix the
CmuxNotificationMuteDurationDefinition.init(from:) initializer to enforce one-of
validation and reject entries with multiple unit types by checking that exactly
one of the three duration unit fields is present. Update the fixture to replace
the mixed-unit entry (the "2.5 Hours" entry with both hours and minutes) with a
single-unit equivalent such as "minutes": 150, and add a separate test case that
explicitly validates the rejection of multi-unit entries to ensure the
validation logic works correctly.
In `@docs/notifications.md`:
- Line 130: The constraint documentation in the notifications.md file
incorrectly states "at least one positive" for the `seconds`, `minutes`, or
`hours` fields, but the schema contract requires exactly one of these fields to
be present. Update the constraint wording on this line to clearly state that
each muteDurations entry must include exactly one (not at least one) of the
`seconds`, `minutes`, or `hours` fields to accurately reflect the schema
definition referenced in web/data/cmux.schema.json.
In `@Sources/CmuxConfig.swift`:
- Line 1809: The notificationMuteMenuOptions property in CmuxConfigStore is
using the deprecated `@Published` decorator which violates the coding guidelines
requiring modern Observation framework for new state. Remove the `@Published`
annotation from the notificationMuteMenuOptions declaration and refactor
CmuxConfigStore to use `@Observable` macro instead of ObservableObject, or
alternatively implement this property using value snapshots and async/await
patterns as per the modern state management guidelines specified in the repo
rules.
In `@Sources/CmuxNotificationConfigDefinitions.swift`:
- Around line 39-52: The decoder currently accepts and sums combinations of
seconds, minutes, and hours fields, but the schema requires exactly one of these
fields to be present. Modify the validation logic in the decode initialization
to first check that exactly one of the three fields (seconds, minutes, hours)
from CodingKeys is non-nil before processing. If more than one or zero fields
are provided, throw a DecodingError.dataCorruptedError with an appropriate
debugDescription indicating that exactly one duration unit must be specified.
Then decode and use only the single provided field instead of summing all three.
In `@Sources/TabItemView`+NotificationMuteContextMenu.swift:
- Around line 8-12: The remoteContextMenuWorkspaces() function performs an
O(m×n) scan by iterating through each workspace ID and calling first(where:) on
tabManager.tabs for every ID. To fix this, build a dictionary lookup from
tabManager.tabs once (mapping workspace IDs to Workspace objects) before
iterating through remoteContextMenuWorkspaceIds, then use direct dictionary
lookups instead of repeated full array scans. This changes the complexity to O(n
+ m) where n is the number of tabs and m is the number of workspace IDs, making
the function scalable for large workspace sets.
In `@web/data/cmux.schema.json`:
- Around line 658-668: In the mute duration constraint section of the schema,
replace the `anyOf` keyword with `oneOf` to enforce mutual exclusivity among the
time unit fields (seconds, minutes, and hours). The current `anyOf` allows
multiple time unit properties to coexist in a single entry, which creates
ambiguous configurations. Using `oneOf` ensures that exactly one of these three
time unit options must be present and no combination of multiple time units is
permitted.
- Around line 635-652: The default mute duration labels in the schema are
hardcoded English strings that bypass localization. Replace the literal label
values ("15 Minutes", "1 Hour", "4 Hours", "8 Hours") with localization keys
that correspond to the existing entries in Resources/Localizable.xcstrings (such
as notificationMute.duration.fifteenMinutes, notificationMute.duration.oneHour,
etc.), and update the NotificationMuteMenuOption.options(configuredDurations:)
method to perform localization lookup on these keys when displaying menu
options. Alternatively, if keeping literal strings in the schema is required,
modify the implementation to map known label patterns to their corresponding
localization keys before displaying them.
---
Duplicate comments:
In `@Sources/TerminalNotificationStore.swift`:
- Around line 437-438: The properties notificationMuteExpirationsByWorkspaceId
and notificationMuteExpirationsBySurfaceId should not be marked with `@Published`
as this violates the repo's policy for cmux-owned SwiftUI state and causes broad
observer invalidation on every mute/unmute operation. Remove the `@Published`
attribute from both properties, keep them as non-published internal state, and
instead create focused read APIs (getter methods) that expose only the specific
information needed by consumers rather than publishing the entire mute
dictionaries.
In `@Sources/TerminalNotificationStore`+Mute.swift:
- Around line 83-90: Remove the entire `#if DEBUG` extension block from
TerminalNotificationStore+Mute.swift that contains the
`clearNotificationMutesForTesting()` method. This debug seam violates the
repository policy that prohibits test/debug-only code in production `Sources/`
directories. Delete the complete extension definition starting with `#if DEBUG`
through the closing `#endif`, including the `clearNotificationMutesForTesting()`
method and its implementation that directly modifies
`notificationMuteExpirationsByWorkspaceId` and
`notificationMuteExpirationsBySurfaceId`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4963ec8-2c5e-4a4f-ac90-f22a7479718a
📒 Files selected for processing (18)
Resources/Localizable.xcstringsSources/CmuxConfig.swiftSources/CmuxNotificationConfigDefinitions.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/GhosttyNSView+MoveTabToNewWorkspace.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/NotificationDisplaySettings.swiftSources/NotificationMuteMenuOption.swiftSources/TabItemView+NotificationMuteContextMenu.swiftSources/TerminalNotificationStore+Mute.swiftSources/TerminalNotificationStore.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxNotificationMuteConfigTests.swiftcmuxTests/TerminalNotificationMuteTests.swiftdocs/notifications.mdweb/data/cmux.schema.json
💤 Files with no reviewable changes (1)
- Sources/GhosttyTerminalView.swift
| { "label": "30 Minutes", "minutes": 30 }, | ||
| { "label": "2.5 Hours", "hours": 2, "minutes": 30 } | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mixed-unit duration fixture conflicts with the schema contract.
Line 21 asserts that a single entry may combine "hours" and "minutes", but the config schema contract requires exactly one of seconds, minutes, or hours per item. This test currently codifies behavior that diverges from the schema and from user-facing config validation.
Please align both sides: enforce one-of semantics in CmuxNotificationMuteDurationDefinition.init(from:), and make this fixture single-unit (plus add a reject-multiple-units test).
Proposed test-side alignment
@@
- { "label": "2.5 Hours", "hours": 2, "minutes": 30 }
+ { "label": "150 Minutes", "minutes": 150 }As per path instructions, web/data/cmux.schema.json requires each notifications.muteDurations entry to include exactly one of seconds, minutes, or hours.
📝 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.
| { "label": "30 Minutes", "minutes": 30 }, | |
| { "label": "2.5 Hours", "hours": 2, "minutes": 30 } | |
| ] | |
| { "label": "30 Minutes", "minutes": 30 }, | |
| { "label": "150 Minutes", "minutes": 150 } | |
| ] |
🤖 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 `@cmuxTests/CmuxNotificationMuteConfigTests.swift` around lines 20 - 22, The
test fixture contains a mixed-unit duration entry combining both "hours" and
"minutes" fields, which violates the schema contract requiring exactly one of
seconds, minutes, or hours per entry. Fix the
CmuxNotificationMuteDurationDefinition.init(from:) initializer to enforce one-of
validation and reject entries with multiple unit types by checking that exactly
one of the three duration unit fields is present. Update the fixture to replace
the mixed-unit entry (the "2.5 Hours" entry with both hours and minutes) with a
single-unit equivalent such as "minutes": 150, and add a separate test case that
explicitly validates the rejection of multi-unit entries to ensure the
validation logic works correctly.
Source: Path instructions
| } | ||
| ``` | ||
|
|
||
| Each item needs a non-empty `label` and at least one positive `seconds`, `minutes`, or `hours` value. Project `cmux.json` values replace the global timed choices for that project. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the mute-duration unit constraint wording.
This line says “at least one” unit field, but the schema contract requires exactly one of seconds, minutes, or hours.
📝 Proposed doc fix
-Each item needs a non-empty `label` and at least one positive `seconds`, `minutes`, or `hours` value. Project `cmux.json` values replace the global timed choices for that project.
+Each item needs a non-empty `label` and exactly one positive `seconds`, `minutes`, or `hours` value. Project `cmux.json` values replace the global timed choices for that project.As per path instructions (referenced web/data/cmux.schema.json), each muteDurations entry must include exactly one of seconds, minutes, or hours.
📝 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.
| Each item needs a non-empty `label` and at least one positive `seconds`, `minutes`, or `hours` value. Project `cmux.json` values replace the global timed choices for that project. | |
| Each item needs a non-empty `label` and exactly one positive `seconds`, `minutes`, or `hours` value. Project `cmux.json` values replace the global timed choices for that project. |
🤖 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 `@docs/notifications.md` at line 130, The constraint documentation in the
notifications.md file incorrectly states "at least one positive" for the
`seconds`, `minutes`, or `hours` fields, but the schema contract requires
exactly one of these fields to be present. Update the constraint wording on this
line to clearly state that each muteDurations entry must include exactly one
(not at least one) of the `seconds`, `minutes`, or `hours` fields to accurately
reflect the schema definition referenced in web/data/cmux.schema.json.
Source: Path instructions
| @Published private(set) var workspaceGroupConfigs: [CmuxResolvedWorkspaceGroupConfig] = [] | ||
| @Published private(set) var surfaceTabBarButtons: [CmuxSurfaceTabBarButton] = CmuxSurfaceTabBarButton.defaults | ||
| @Published private(set) var notificationHooks: [CmuxResolvedNotificationHook] = [] | ||
| @Published private(set) var notificationMuteMenuOptions: [NotificationMuteMenuOption] = NotificationMuteMenuOption.defaultOptions |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Avoid adding new @Published state in cmux-owned production Swift.
This adds new Combine-based state (@Published) to CmuxConfigStore. The repo rules require new state to use modern Observation (@Observable + snapshots) rather than expanding ObservableObject/@Published usage.
As per coding guidelines, “Do not use ObservableObject, @Published, @StateObject, or @EnvironmentObject for new cmux-owned SwiftUI state; use @Observable plus @State or value snapshots instead,” and “Flag new Combine usage … when Observation and async/await are available.”
🤖 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 `@Sources/CmuxConfig.swift` at line 1809, The notificationMuteMenuOptions
property in CmuxConfigStore is using the deprecated `@Published` decorator which
violates the coding guidelines requiring modern Observation framework for new
state. Remove the `@Published` annotation from the notificationMuteMenuOptions
declaration and refactor CmuxConfigStore to use `@Observable` macro instead of
ObservableObject, or alternatively implement this property using value snapshots
and async/await patterns as per the modern state management guidelines specified
in the repo rules.
Source: Coding guidelines
| let seconds = try container.decodeIfPresent(TimeInterval.self, forKey: .seconds) | ||
| let minutes = try container.decodeIfPresent(TimeInterval.self, forKey: .minutes) | ||
| let hours = try container.decodeIfPresent(TimeInterval.self, forKey: .hours) | ||
| for (key, value) in [(CodingKeys.seconds, seconds), (.minutes, minutes), (.hours, hours)] { | ||
| if let value, (!value.isFinite || value <= 0) { | ||
| throw DecodingError.dataCorruptedError( | ||
| forKey: key, | ||
| in: container, | ||
| debugDescription: "\(key.stringValue) must be greater than 0" | ||
| ) | ||
| } | ||
| } | ||
| let decodedInterval = (seconds ?? 0) + ((minutes ?? 0) * 60) + ((hours ?? 0) * 60 * 60) | ||
| if decodedInterval <= 0 { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce a single duration unit to match the schema contract.
The decoder currently accepts combinations of seconds, minutes, and hours by summing all provided fields. That diverges from the notifications.muteDurations schema contract and can make app parsing disagree with schema-based validation.
Suggested fix
let seconds = try container.decodeIfPresent(TimeInterval.self, forKey: .seconds)
let minutes = try container.decodeIfPresent(TimeInterval.self, forKey: .minutes)
let hours = try container.decodeIfPresent(TimeInterval.self, forKey: .hours)
+ let presentCount = [seconds, minutes, hours].compactMap { $0 }.count
+ guard presentCount == 1 else {
+ throw DecodingError.dataCorruptedError(
+ forKey: .seconds,
+ in: container,
+ debugDescription: "exactly one of seconds, minutes, or hours must be provided"
+ )
+ }
for (key, value) in [(CodingKeys.seconds, seconds), (.minutes, minutes), (.hours, hours)] {
if let value, (!value.isFinite || value <= 0) {
throw DecodingError.dataCorruptedError(
forKey: key,
in: container,
debugDescription: "\(key.stringValue) must be greater than 0"
)
}
}As per path instructions, “Each entry must include exactly one of seconds, minutes, or hours.”
📝 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.
| let seconds = try container.decodeIfPresent(TimeInterval.self, forKey: .seconds) | |
| let minutes = try container.decodeIfPresent(TimeInterval.self, forKey: .minutes) | |
| let hours = try container.decodeIfPresent(TimeInterval.self, forKey: .hours) | |
| for (key, value) in [(CodingKeys.seconds, seconds), (.minutes, minutes), (.hours, hours)] { | |
| if let value, (!value.isFinite || value <= 0) { | |
| throw DecodingError.dataCorruptedError( | |
| forKey: key, | |
| in: container, | |
| debugDescription: "\(key.stringValue) must be greater than 0" | |
| ) | |
| } | |
| } | |
| let decodedInterval = (seconds ?? 0) + ((minutes ?? 0) * 60) + ((hours ?? 0) * 60 * 60) | |
| if decodedInterval <= 0 { | |
| let seconds = try container.decodeIfPresent(TimeInterval.self, forKey: .seconds) | |
| let minutes = try container.decodeIfPresent(TimeInterval.self, forKey: .minutes) | |
| let hours = try container.decodeIfPresent(TimeInterval.self, forKey: .hours) | |
| let presentCount = [seconds, minutes, hours].compactMap { $0 }.count | |
| guard presentCount == 1 else { | |
| throw DecodingError.dataCorruptedError( | |
| forKey: .seconds, | |
| in: container, | |
| debugDescription: "exactly one of seconds, minutes, or hours must be provided" | |
| ) | |
| } | |
| for (key, value) in [(CodingKeys.seconds, seconds), (.minutes, minutes), (.hours, hours)] { | |
| if let value, (!value.isFinite || value <= 0) { | |
| throw DecodingError.dataCorruptedError( | |
| forKey: key, | |
| in: container, | |
| debugDescription: "\(key.stringValue) must be greater than 0" | |
| ) | |
| } | |
| } | |
| let decodedInterval = (seconds ?? 0) + ((minutes ?? 0) * 60) + ((hours ?? 0) * 60 * 60) | |
| if decodedInterval <= 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 `@Sources/CmuxNotificationConfigDefinitions.swift` around lines 39 - 52, The
decoder currently accepts and sums combinations of seconds, minutes, and hours
fields, but the schema requires exactly one of these fields to be present.
Modify the validation logic in the decode initialization to first check that
exactly one of the three fields (seconds, minutes, hours) from CodingKeys is
non-nil before processing. If more than one or zero fields are provided, throw a
DecodingError.dataCorruptedError with an appropriate debugDescription indicating
that exactly one duration unit must be specified. Then decode and use only the
single provided field instead of summing all three.
Source: Path instructions
| func remoteContextMenuWorkspaces() -> [Workspace] { | ||
| guard !remoteContextMenuWorkspaceIds.isEmpty else { return [] } | ||
| return remoteContextMenuWorkspaceIds.compactMap { workspaceId in | ||
| tabManager.tabs.first(where: { $0.id == workspaceId }) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid per-target rescans in remoteContextMenuWorkspaces().
Line 10 iterates each selected workspace id, and Line 11 does a full tabManager.tabs.first(where:) scan per id. This becomes O(m×n) on large workspace sets.
Suggested fix
func remoteContextMenuWorkspaces() -> [Workspace] {
guard !remoteContextMenuWorkspaceIds.isEmpty else { return [] }
- return remoteContextMenuWorkspaceIds.compactMap { workspaceId in
- tabManager.tabs.first(where: { $0.id == workspaceId })
- }
+ let tabsById = Dictionary(uniqueKeysWithValues: tabManager.tabs.map { ($0.id, $0) })
+ return remoteContextMenuWorkspaceIds.compactMap { tabsById[$0] }
}As per path instructions, apply .github/review-bot-rules/algorithmic-complexity.md: avoid repeated full scans per target and prefer Set/dictionary lookups for scalable collections.
🤖 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 `@Sources/TabItemView`+NotificationMuteContextMenu.swift around lines 8 - 12,
The remoteContextMenuWorkspaces() function performs an O(m×n) scan by iterating
through each workspace ID and calling first(where:) on tabManager.tabs for every
ID. To fix this, build a dictionary lookup from tabManager.tabs once (mapping
workspace IDs to Workspace objects) before iterating through
remoteContextMenuWorkspaceIds, then use direct dictionary lookups instead of
repeated full array scans. This changes the complexity to O(n + m) where n is
the number of tabs and m is the number of workspace IDs, making the function
scalable for large workspace sets.
Source: Path instructions
| "default": [ | ||
| { | ||
| "label": "15 Minutes", | ||
| "minutes": 15 | ||
| }, | ||
| { | ||
| "label": "1 Hour", | ||
| "hours": 1 | ||
| }, | ||
| { | ||
| "label": "4 Hours", | ||
| "hours": 4 | ||
| }, | ||
| { | ||
| "label": "8 Hours", | ||
| "hours": 8 | ||
| } | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify mute duration label localization handling
# Check how NotificationMuteMenuOption uses labels
echo "=== NotificationMuteMenuOption label usage ==="
rg -nP -A5 -B5 'struct NotificationMuteMenuOption' --type=swift
# Check if default labels are referenced or localized
echo -e "\n=== Search for default label strings in Swift ==="
rg -nP '(15 Minutes|1 Hour|4 Hours|8 Hours)' --type=swift
# Check localization file for mute duration keys
echo -e "\n=== Check Localizable.xcstrings for mute duration translations ==="
cat Resources/Localizable.xcstrings | jq '.strings | to_entries | map(select(.key | contains("mute") or contains("duration"))) | from_entries' 2>/dev/null || echo "Run jq to inspect xcstrings"
# Check template generation for muteDurations
echo -e "\n=== Template generation for muteDurations ==="
rg -nP -C5 'muteDurations' Sources/KeyboardShortcutSettingsFileStore+Template.swift --type=swiftRepository: manaflow-ai/cmux
Length of output: 201
🏁 Script executed:
# Find files related to notification and mute functionality
fd -type f -name "*[Nn]otif*" | head -20Repository: manaflow-ai/cmux
Length of output: 325
🏁 Script executed:
# Search for muteDurations usage across the codebase
rg "muteDurations" --type-list | head -20
rg "muteDurations" -lRepository: manaflow-ai/cmux
Length of output: 544
🏁 Script executed:
# Check web directory structure for notification handling
find web -type f -name "*.ts" -o -name "*.tsx" -o -name "*.json" | grep -iE "(notif|mute)" | head -20Repository: manaflow-ai/cmux
Length of output: 396
🏁 Script executed:
# Look for where default schema is used or referenced
rg "cmux.schema.json\|NotificationMute\|mute.*duration" --type=ts --type=tsx -lRepository: manaflow-ai/cmux
Length of output: 182
🏁 Script executed:
# Check web/messages localization files for mute-related strings
ls -la web/messages/
cat web/messages/en.json | jq 'keys | map(select(contains("mute") or contains("notif")))' 2>/dev/null || echo "Check structure"Repository: manaflow-ai/cmux
Length of output: 1486
🏁 Script executed:
# Search for muteDurations across the entire codebase
rg "muteDurations" -lRepository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check the schema file itself to understand the structure
cat web/data/cmux.schema.json | jq '.properties.notifications.properties.muteDurations'Repository: manaflow-ai/cmux
Length of output: 1500
🏁 Script executed:
# Look for notification-related messages in the localization files
rg "mute|notif" web/messages/en.json | head -20Repository: manaflow-ai/cmux
Length of output: 4029
🏁 Script executed:
# Check how the config is used in the backend - look for notification handlers
rg -l "mute|duration" web --type=js | head -10Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for where these default values might be used or displayed
rg "15 Minutes|1 Hour|4 Hours|8 Hours" -lRepository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check if there's any backend code that processes the schema defaults
find web -type f \( -name "*.ts" -o -name "*.js" \) -exec grep -l "schema\|cmux.json\|config" {} \; | head -15Repository: manaflow-ai/cmux
Length of output: 1443
🏁 Script executed:
# Look for notification or mute-related backend logic
rg "notification|mute" web/src web/api --type=js | head -30Repository: manaflow-ai/cmux
Length of output: 250
🏁 Script executed:
# Check if there are any test files that reference the defaults
find . -type f -name "*.test.ts" -o -name "*.test.js" | xargs grep -l "mute\|duration" 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 242
🏁 Script executed:
# Check the schema description more carefully and look for related schema sections
cat web/data/cmux.schema.json | jq '.properties.notifications'Repository: manaflow-ai/cmux
Length of output: 4724
🏁 Script executed:
# Look for where user config gets loaded and used
rg "cmux\.json\|loadConfig\|getConfig" --type=js | head -20Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search the entire codebase for any reference to muteDurations or mute menu
rg -i "mutedurations\|mute.*menu\|notification.*mute" --type-list | head
rg -r "mutedurations\|mute.*menu\|notification.*mute"Repository: manaflow-ai/cmux
Length of output: 941
🏁 Script executed:
# Look for where config files are loaded in the app
rg "cmux\.json" -A5 -B5 | head -50Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check if there's any iOS/macOS code handling notifications and mute options
find . -name "*.swift" -exec grep -l "mute\|notification" {} \; 2>/dev/null | head -10Repository: manaflow-ai/cmux
Length of output: 727
🏁 Script executed:
# Look at the recent commit history to understand the context of this schema addition
git log --oneline web/data/cmux.schema.json 2>/dev/null | head -5Repository: manaflow-ai/cmux
Length of output: 206
🏁 Script executed:
# Check if there are any pending implementation TODOs related to this
rg "TODO|FIXME" web/data/cmux.schema.json -B5 -A5Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look at the specific commit that added this schema entry
git show d3719e2c --statRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
# Check the full diff of that commit
git show d3719e2cRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
# Look at any related PRs or issue context
git log --grep="mute\|notification" --oneline | head -20Repository: manaflow-ai/cmux
Length of output: 206
🏁 Script executed:
# Check if there are TypeScript/JavaScript types generated from the schema
find . -type f \( -name "*.ts" -o -name "*.d.ts" \) -exec grep -l "muteDurations\|NotificationMute" {} \; 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look for any documentation about mute durations
find . -type f \( -name "*.md" -o -name "*.tsx" \) -exec grep -l "mute.*duration\|15 Minutes" {} \; 2>/dev/null | head -10Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look at NotificationMuteMenuOption.swift to see how labels are used
fd -type f -name "*NotificationMute*"Repository: manaflow-ai/cmux
Length of output: 325
🏁 Script executed:
# Search for where cmux.json config is loaded and used in Swift
rg "CmuxConfig\|config\.notifications" Sources --type=swift | head -20Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look at the coding guidelines reference - check if there's a localization audit for notifications
rg "notification.*mute\|mute.*menu" Resources/Localizable.xcstrings -A3 -B3Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check how config defaults are applied in the app
rg "default.*mute\|muteDuration" Sources --type=swift -B3 -A3 | head -30Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Find the notification mute menu option file
find Sources -name "*NotificationMute*" -o -name "*Mute*" | grep -i notificationRepository: manaflow-ai/cmux
Length of output: 294
🏁 Script executed:
# Search for where the mute durations from config are actually used
rg "muteDurations" Sources -B5 -A5Repository: manaflow-ai/cmux
Length of output: 4161
🏁 Script executed:
# Check if there's any dynamic localization happening
rg "localized.*label\|label.*locali" Sources -B2 -A2Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look at CmuxConfig to understand how config is structured
cat Sources/CmuxConfig.swift | head -100Repository: manaflow-ai/cmux
Length of output: 4301
🏁 Script executed:
# Look at how NotificationMuteMenuOption uses the labels
cat Sources/NotificationMuteMenuOption.swiftRepository: manaflow-ai/cmux
Length of output: 2741
🏁 Script executed:
# Check TabItemView to see how the mute menu is displayed
cat Sources/TabItemView+NotificationMuteContextMenu.swift | head -80Repository: manaflow-ai/cmux
Length of output: 2097
🏁 Script executed:
# Check if the schema defaults are actually loaded into the app or if the app always uses its hardcoded defaults
rg "defaultTimedOptions\|defaultOptions" Sources -B3 -A3Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Verify localization keys exist in the xcstrings for the default mute options
rg "notificationMute.duration" Resources/Localizable.xcstrings -B2 -A2Repository: manaflow-ai/cmux
Length of output: 773
🏁 Script executed:
# Check how schema defaults are actually used - are they written to config or just documentation?
rg "default.*muteDurations\|schema.*default" Sources CmuxConfig.swift -B5 -A5Repository: manaflow-ai/cmux
Length of output: 211
🏁 Script executed:
# Verify that the app doesn't load schema defaults into the config file
find . -name "*.swift" -exec grep -l "cmux\.schema\|schema.*notification" {} \; 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 382
Default mute duration labels require localization for every supported locale or schema structure change.
The schema defaults are user-facing strings that appear in notification mute context menus when users customize cmux.json. While the app's hardcoded default options are properly localized via String(localized:) with keys in Resources/Localizable.xcstrings, users copying the schema-provided defaults will see English labels in non-English app instances.
The notificationMute.duration.* keys exist in Resources/Localizable.xcstrings with Japanese translations, but the schema default labels ("15 Minutes", "1 Hour", "4 Hours", "8 Hours") are literal strings, not localization keys. When a user adds these to their cmux.json, the NotificationMuteMenuOption.options(configuredDurations:) method passes the label directly as the menu item title without localization lookup.
Either update the schema defaults to be localization keys (and document the mapping), or add every supported locale's label variants to the default array, or update the implementation to map known label patterns to localized strings.
🤖 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 `@web/data/cmux.schema.json` around lines 635 - 652, The default mute duration
labels in the schema are hardcoded English strings that bypass localization.
Replace the literal label values ("15 Minutes", "1 Hour", "4 Hours", "8 Hours")
with localization keys that correspond to the existing entries in
Resources/Localizable.xcstrings (such as
notificationMute.duration.fifteenMinutes, notificationMute.duration.oneHour,
etc.), and update the NotificationMuteMenuOption.options(configuredDurations:)
method to perform localization lookup on these keys when displaying menu
options. Alternatively, if keeping literal strings in the schema is required,
modify the implementation to map known label patterns to their corresponding
localization keys before displaying them.
Source: Path instructions
| "anyOf": [ | ||
| { | ||
| "required": ["seconds"] | ||
| }, | ||
| { | ||
| "required": ["minutes"] | ||
| }, | ||
| { | ||
| "required": ["hours"] | ||
| } | ||
| ], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use oneOf instead of anyOf to enforce mutual exclusivity.
The current anyOf constraint allows an entry to provide multiple time unit fields simultaneously (e.g., both "minutes": 15 and "hours": 1). While additionalProperties: false prevents unknown fields, it doesn't prevent multiple known time-unit fields from coexisting. Semantically, each mute duration entry should represent exactly one time span.
Replace anyOf with oneOf to ensure exactly one of seconds, minutes, or hours is present per entry, rejecting ambiguous configurations.
🛡️ Proposed fix to enforce mutual exclusivity
"required": ["label"],
- "anyOf": [
+ "oneOf": [
{
"required": ["seconds"]🤖 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 `@web/data/cmux.schema.json` around lines 658 - 668, In the mute duration
constraint section of the schema, replace the `anyOf` keyword with `oneOf` to
enforce mutual exclusivity among the time unit fields (seconds, minutes, and
hours). The current `anyOf` allows multiple time unit properties to coexist in a
single entry, which creates ambiguous configurations. Using `oneOf` ensures that
exactly one of these three time unit options must be present and no combination
of multiple time units is permitted.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 805c30c. Configure here.
| workspaceGroups: workspaceGroups, | ||
| workspaceGroupById: workspaceGroupById, | ||
| workspaceGroupMenuSnapshot: workspaceGroupMenuSnapshot, | ||
| notificationMuteMenuOptions: cmuxConfigStore.notificationMuteMenuOptions, |
There was a problem hiding this comment.
Wrong workspace mute menu config
Medium Severity
Workspace context mute submenus always use cmuxConfigStore.notificationMuteMenuOptions, which is resolved from the selected workspace’s local config path. Right-clicking a different workspace (or multi-select spanning projects) can show another project’s notifications.muteDurations labels and intervals while muting the wrong targets.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 805c30c. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
Issues
Note
Medium Risk
Changes the core notification apply path in
TerminalNotificationStore, but behavior is gated on explicit mute state and covered by new unit tests.Overview
Adds notification mute from the sidebar workspace context menu (including multi-select) and the terminal tab context menu, each with a duration submenu plus Unmute when a mute is active.
Mute state is stored on
TerminalNotificationStoreper workspace and per terminal surface. While muted, notifications can still be recorded as unread, but side effects are skipped (sounds, hooks/commands, native banners, pane flash, workspace reorder, and related external delivery). Workspace-level and surface-level mutes are evaluated separately so muting one tab does not silence siblings.Timed choices come from
notifications.muteDurationsincmux.json(local overrides global), always prefixed with Until Unmuted. Notification config decoding moves intoCmuxNotificationConfigDefinitions.swift; defaults and schema/docs/localizations are updated accordingly.Reviewed by Cursor Bugbot for commit 805c30c. Bugbot is set up for automated code reviews on this repo. Configure here.