Repository navigation
Fix desktop notification permission settings row - #6960
Merged
austinywang merged 30 commits intoJul 28, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDesktop notification authorization now flows from host state into a new macOS settings model and row. Crash-storage symlink handling, file-loading cancellation, and Claude stream character counting are also updated. ChangesDesktop notification settings
Storage and loading fixes
Stream parsing fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (22 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 |
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 26, 2026 23:35
faaf241 to
36b9115
Compare
Contributor
Greptile SummaryThis PR wires the Desktop Notifications settings row to live macOS permission state via a new
Confidence Score: 5/5Safe to merge — the permission model, stream lifecycle, and presentation mapping are all correct, and the secondary fixes are well-scoped. The authorization stream is correctly wired: the SettingReadDriver owns the subscription lifetime, onTermination removes the NotificationCenter observer, and the DesktopNotificationAuthorizationModel refreshes OS state on each row observation without re-subscribing. All six UNAuthorizationStatus cases are mapped and covered by tests. Localization is complete across all 20 supported locales. No actor isolation mistakes, no blocking primitives, no test seams in production source. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant AppSection as AppSection (.task)
participant Model as DesktopNotificationAuthorizationModel
participant Driver as SettingReadDriver
participant Host as HostSettingsActions
participant Store as TerminalNotificationStore
participant OS as UNUserNotificationCenter
AppSection->>Model: startObserving()
Model->>Host: currentStatus()
Host->>Store: authorizationState (sync read)
Store-->>Host: .unknown / current
Host-->>Model: DesktopNotificationAuthorizationState
Model->>Model: "current = state (sync)"
Model->>Driver: activate(makeStream)
Driver->>Host: desktopNotificationAuthorizationStatusUpdates()
Host->>Host: addObserver(authorizationStatusDidChangeNotification)
Host-->>Driver: "AsyncStream<State>"
Driver->>Driver: "Task @MainActor { for await state in stream }"
Host->>Host: drainTask: yield(currentState)
Driver->>Model: "sink(currentState) -> current updated"
Model->>Host: refreshDesktopNotificationAuthorizationStatus()
Host->>Store: refreshAuthorizationStatus()
Store->>OS: getNotificationSettings()
OS-->>Store: UNNotificationSettings (async)
Store->>Store: "authorizationState = mapped (didSet)"
Store->>Store: didSet: post(authorizationStatusDidChangeNotification)
Host->>Host: "observer fires -> signalContinuation.yield(())"
Host->>Host: drainTask picks up signal, yields new state
Driver->>Model: "sink(newState) -> current updated"
Model->>AppSection: "@Observable triggers SwiftUI re-render"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant AppSection as AppSection (.task)
participant Model as DesktopNotificationAuthorizationModel
participant Driver as SettingReadDriver
participant Host as HostSettingsActions
participant Store as TerminalNotificationStore
participant OS as UNUserNotificationCenter
AppSection->>Model: startObserving()
Model->>Host: currentStatus()
Host->>Store: authorizationState (sync read)
Store-->>Host: .unknown / current
Host-->>Model: DesktopNotificationAuthorizationState
Model->>Model: "current = state (sync)"
Model->>Driver: activate(makeStream)
Driver->>Host: desktopNotificationAuthorizationStatusUpdates()
Host->>Host: addObserver(authorizationStatusDidChangeNotification)
Host-->>Driver: "AsyncStream<State>"
Driver->>Driver: "Task @MainActor { for await state in stream }"
Host->>Host: drainTask: yield(currentState)
Driver->>Model: "sink(currentState) -> current updated"
Model->>Host: refreshDesktopNotificationAuthorizationStatus()
Host->>Store: refreshAuthorizationStatus()
Store->>OS: getNotificationSettings()
OS-->>Store: UNNotificationSettings (async)
Store->>Store: "authorizationState = mapped (didSet)"
Store->>Store: didSet: post(authorizationStatusDidChangeNotification)
Host->>Host: "observer fires -> signalContinuation.yield(())"
Host->>Host: drainTask picks up signal, yields new state
Driver->>Model: "sink(newState) -> current updated"
Model->>AppSection: "@Observable triggers SwiftUI re-render"
Reviews (29): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 26, 2026 23:44
36b9115 to
0ef3109
Compare
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 26, 2026 23:50
0ef3109 to
96aff1c
Compare
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 27, 2026 07:30
96aff1c to
d04323d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionPresentation.swift`:
- Around line 13-19: The `.unknown` case in
`DesktopNotificationPermissionPresentation` is incorrectly mapped to the live
`requestAuthorization` action, which lets `DesktopNotificationsSettingsRow`
expose a stale Enable path before the authoritative host state is available.
Update the `.unknown` branch to fail closed by using no primary action or a
disabled/loading presentation instead of `requestAuthorization`, and keep
`sendTestEnabled` false until a real permission state is received.
🪄 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: f21b3b6c-97dd-4f3b-907a-1a552989fb84
📒 Files selected for processing (16)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Bindings/DesktopNotificationAuthorizationModel.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Bindings/SettingObservationStarting.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/SettingsHostActions.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationAuthorizationState.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionAction.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionPresentation.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionStatusLabel.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionSubtitle.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationsSettingsRow.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/CountingMobilePairingHostActions.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/DesktopNotificationAuthorizationModelLifecycleTests.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/DesktopNotificationPermissionPresentationTests.swiftResources/Localizable.xcstringsSources/HostSettingsActions.swiftSources/TerminalNotificationStore.swift
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 27, 2026 07:38
d04323d to
86d7fb5
Compare
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 27, 2026 07:50
86d7fb5 to
9488e48
Compare
austinywang
force-pushed
the
issue-5919-settings-desktop-notifications-row-stuck-on-p
branch
from
June 27, 2026 07:58
9488e48 to
306d4ef
Compare
added 2 commits
June 28, 2026 20:21
…esktop-notifications-row-stuck-on-p # Conflicts: # Resources/Localizable.xcstrings
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Bindings/DesktopNotificationAuthorizationModel.swift`:
- Around line 39-47: The startObserving() method in
DesktopNotificationAuthorizationModel currently exits early after the first
call, so currentStatus() and refreshStatus() are skipped on later observations
and stale notification authorization can persist. Keep the
driver.activate(makeStream) subscription guarded by hasStarted, but move the
current = currentStatus() read and refreshStatus() call so they run on every
startObserving() invocation. Use the existing startObserving(), currentStatus(),
refreshStatus(), and hasStarted symbols to update the model state without
re-subscribing.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionPresentation.swift`:
- Around line 13-18: The `.unknown` branch in
`DesktopNotificationPermissionPresentation` still falls back to the
`.notDetermined` subtitle, which shows stale permission text before the
authoritative host state is available. Update the `.unknown` case to use a
neutral/loading subtitle or no subtitle at all, keeping the UI fail-closed until
the reliable state arrives, and leave the `.notDetermined` copy only for the
real not-determined state.
🪄 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: a02ad097-9cf2-4218-a88e-f1e238b66a93
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (16)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Bindings/DesktopNotificationAuthorizationModel.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Bindings/SettingObservationStarting.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/SettingsHostActions.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationAuthorizationState.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionAction.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionPresentation.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionStatusLabel.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionSubtitle.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationsSettingsRow.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/CountingMobilePairingHostActions.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/DesktopNotificationAuthorizationModelLifecycleTests.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/DesktopNotificationPermissionPresentationTests.swiftResources/Localizable.xcstringsSources/HostSettingsActions.swiftSources/TerminalNotificationStore.swift
💤 Files with no reviewable changes (3)
- Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationAuthorizationState.swift
- Sources/TerminalNotificationStore.swift
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/FileExplorerStore.swift`:
- Line 1050: The early return in the load/task setup path skips cleanup when
Task.isCancelled is already true, leaving child.isLoading and the task/path
state stale. Update the cancellation handling around the provider guard so that
any pre-start cancellation still clears the loading flag and associated
task/path bookkeeping before returning. Keep the fix in the same loading flow
that the auto-expand path uses after setting child.isLoading = true, and ensure
the cleanup happens regardless of whether provider is available.
🪄 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: 190561d8-ec30-4205-8072-ea716074c48f
📒 Files selected for processing (7)
Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Bindings/DesktopNotificationAuthorizationModel.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/DesktopNotificationPermissionPresentation.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/DesktopNotificationAuthorizationModelLifecycleTests.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/DesktopNotificationPermissionPresentationTests.swiftSources/AppDelegate+CmuxSSHURL.swiftSources/FileExplorerStore.swiftSources/SessionPersistencePolicy+CrashStorage.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…esktop-notifications-row-stuck-on-p # Conflicts: # .github/swift-file-length-budget.tsv # cmuxTests/CLIGenericHookPersistenceTests.swift # cmuxTests/CLINotifyProcessTestSupport.swift
The appearance-test stabilization (08b736e) changed testBrowserPanelRefreshesUnderPageBackgroundColorWhenGhosttyBackgroundChanges to call applyWebViewBackgroundForTesting directly, so a regression in the live .ghosttyDefaultBackgroundDidChange publisher/sink wiring would still pass. Inject the panel's broadcast NotificationCenter (defaulting to .default, so production behavior is unchanged) and post the notification through it. The test now exercises the real subscription plus GhosttyBackgroundTheme.color(from:) parsing without posting to the shared NotificationCenter.default that app-host appearance tests rely on. The WithGhosttyOpacity test keeps the direct-helper path as lower-level coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both under-page-background tests now post .ghosttyDefaultBackgroundDidChange through an injected NotificationCenter, exercising the live subscription (one covers the Double opacity payload, the other the NSNumber payload). The #if DEBUG applyWebViewBackgroundForTesting accessor in production source is no longer referenced and is removed per the test/debug-seam policy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…esktop-notifications-row-stuck-on-p # Conflicts: # .github/swift-file-length-budget.tsv
…esktop-notifications-row-stuck-on-p # Conflicts: # .github/swift-file-length-budget.tsv
The file-explorer prefetch path sampled `contentRevision` inside the delayed Task body instead of when the prefetch was requested. If a reload() landed after the 200ms debounce fired (scheduling the Task) but before that Task ran, cancelAllLoads() could no longer cancel it, and the Task then read the post-reload revision — so loadChildren's guard (contentRevision == expectedContentRevision) trivially passed while the captured FileExplorerNode was stale, letting it populate nodesByPath and node.children from an old tree. Capture `revision = contentRevision` before creating the DispatchWorkItem, matching the explicit expand()/reload() paths, so a stale prefetch is rejected by the existing revision guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 28 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
The prefetch revision-capture fix added 5 lines to FileExplorerStore.swift (1317 -> 1322). Regenerate the budget via `python3 scripts/swift_file_length_budget.py --write-budget` so the workflow-guard-tests bare budget check passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…onal/ephemeral Addresses cubic-dev-ai review on #6960: - WorkspaceChromeColorTests.expectedChromeHex: replace the tautological wrapper around WindowAppearanceSnapshot.compositedTerminalColor (the same production call bonsplitChromeHex makes) with an independent source-over composite over the runtime windowBackgroundColor, so a regression in the production blend now diverges the two values instead of passing trivially. - PanelAppearanceBackgroundTests: derive the expected composited channels independently instead of re-calling GhosttyBackgroundTheme.color (the path under test), and assert the flattened result is opaque (alpha == 1). - DesktopNotificationPermissionPresentationTests: add provisional/ephemeral state coverage (deliverQuietly/temporary -> allowed, openSystemSettings, send-test enabled). - Regenerate swift-file-length-budget.tsv for the added test lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…esktop-notifications-row-stuck-on-p # Conflicts: # .github/swift-file-length-budget.tsv
…esktop-notifications-row-stuck-on-p Resolves conflicts from #7129 (per-category agent notification settings): - AppSection.swift: keep BOTH the desktopNotifications permission model (this branch) and the agentPermissionPrompt/agentTurnComplete/ agentIdleReminder rows (#7129) across the @State decls, init, and the startSettingsObservation array; the body auto-merged with both row sets. - .github/swift-file-length-budget.tsv: regenerated via scripts/swift_file_length_budget.py --write-budget (never hand-edited). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…esktop-notifications-row-stuck-on-p # Conflicts: # .github/swift-file-length-budget.tsv
…esktop-notifications-row-stuck-on-p
…esktop-notifications-row-stuck-on-p
…esktop-notifications-row-stuck-on-p
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
1 participant
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.
Fixes #5919\n\n## Summary\n- Wire Settings > App > Desktop Notifications to the host notification authorization state instead of a static unknown baseline\n- Refresh permission state on row observation and route determined states to the app-specific System Settings notifications pane\n- Add regression coverage for granted/denied presentation and row observation refresh\n\n## Validation\n- swift test --package-path Packages/macOS/CmuxSettingsUI\n- python3 -m json.tool Resources/Localizable.xcstrings >/dev/null\n- python3 scripts/check-workspace-package-groups.py --check\n- git diff --check\n\nNo dev build, reload script, or xcodebuild run per request.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes #5919. The Desktop Notifications settings row now reflects live macOS permission, shows the right action (Enable or Open System Settings), and gates “Send Test” correctly (enabled for authorized/provisional/ephemeral/not‑determined; disabled for unknown/denied).
DesktopNotificationAuthorizationModel, a dynamic settings row with provisional/ephemeral states, proper status labels/actions, and expanded localized strings.Written for commit ff63482. Summary will update on new commits.
Summary by CodeRabbit