Repository navigation
Fix notification Settings open path - #4456
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughTerminalNotificationStore computes & opens candidate notifications URLs; SettingsView adds lazy-load plumbing, one-time gating flags, and runs browser-import detection in a detached task with a generation guard; UI, tests, localization, and Sendable conformances updated. ChangesSettings and Notification changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (15 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 SummaryThis PR fixes notification Settings deep-linking by switching to the modern app-specific URL (
Confidence Score: 5/5Safe to merge — the notification URL change is narrow and tested, lazy-load deferred state is correctly guarded, and async browser detection uses Task.detached with proper MainActor hand-off. All previously flagged concerns have been resolved in this revision: the DispatchQueue-based detection was replaced with Task.detached, the unreachable fallback chain was removed, missing locale translations were backfilled across all 20 supported locales, and scroll-proximity triggering was added for browser history so it is no longer navigation-only. No new correctness or concurrency issues were introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SettingsView
participant ScrollProxy as ScrollView / GeometryReader
participant LazyMarker as SettingsLazyLoadMarker
participant BrowserHistoryStore
participant InstalledBrowserDetector
User->>SettingsView: Open Settings (any section)
SettingsView->>ScrollProxy: coordinateSpace(.named("SettingsScrollCoordinateSpace"))
Note over LazyMarker: .background(SettingsLazyLoadMarker) on history card and import card
LazyMarker-->>ScrollProxy: preference(frames: [.browserHistory: rect, .browserImport: rect])
ScrollProxy-->>SettingsView: onPreferenceChange fires
SettingsView->>SettingsView: handleSettingsLazyLoadFrames(frames, viewportHeight)
alt Section is near viewport
SettingsView->>BrowserHistoryStore: loadIfNeeded()
BrowserHistoryStore-->>SettingsView: "$entries published → didLoadBrowserHistoryForSettings = true"
SettingsView->>InstalledBrowserDetector: "applicationBundleLookupSnapshot() @MainActor"
SettingsView->>InstalledBrowserDetector: Task.detached detectInstalledBrowsers(snapshot)
InstalledBrowserDetector-->>SettingsView: "MainActor.run { detectedImportBrowsers = result }"
end
User->>SettingsView: Navigate via deep link to .browserImport
SettingsView->>SettingsView: prepareSettingsDestinationIfNeeded(destination)
SettingsView->>InstalledBrowserDetector: refreshDetectedImportBrowsersIfNeeded()
Reviews (7): Last reviewed commit: "Keep Browser settings navigation lazy" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/cmuxApp.swift (1)
5799-5804: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDon’t put
AuthSettingsRow’s@ObservedObjectunder the newLazyVStack.This container change now places
AuthSettingsRow(authManager: authManager)inside a lazy stack even though that row owns@ObservedObject var authManager. In this repo, lazy containers should receive value snapshots and action closures rather thanObservableObjectstores. Either keep this stack eager or refactorAuthSettingsRowto take immutable auth state plus callbacks.Based on learnings: "In this repo’s SwiftUI views, any view placed under LazyVStack, LazyHStack, List, or ForEach must not capture or hold ObservableObject store instances..."
🤖 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/cmuxApp.swift` around lines 5799 - 5804, You moved AuthSettingsRow(authManager: authManager) into a LazyVStack which incorrectly lets that view retain an `@ObservedObject`; either revert to an eager container (put AuthSettingsRow back outside LazyVStack) or refactor AuthSettingsRow to accept immutable auth state plus action callbacks instead of an ObservableObject. Specifically, change AuthSettingsRow to remove its `@ObservedObject` authManager dependency (or create a new init that takes a snapshot model and closures for actions), update call sites (the LazyVStack location using AuthSettingsRow(authManager: authManager)) to pass a plain snapshot struct and closures, or restore the previous non-lazy container so AuthSettingsRow keeps using `@ObservedObject` as before. Ensure references to AuthSettingsRow and the authManager store are the only symbols modified.
🤖 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.
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 5799-5804: You moved AuthSettingsRow(authManager: authManager)
into a LazyVStack which incorrectly lets that view retain an `@ObservedObject`;
either revert to an eager container (put AuthSettingsRow back outside
LazyVStack) or refactor AuthSettingsRow to accept immutable auth state plus
action callbacks instead of an ObservableObject. Specifically, change
AuthSettingsRow to remove its `@ObservedObject` authManager dependency (or create
a new init that takes a snapshot model and closures for actions), update call
sites (the LazyVStack location using AuthSettingsRow(authManager: authManager))
to pass a plain snapshot struct and closures, or restore the previous non-lazy
container so AuthSettingsRow keeps using `@ObservedObject` as before. Ensure
references to AuthSettingsRow and the authManager store are the only symbols
modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c14da5f-3d2a-4cc6-926e-437fb6bcd332
📒 Files selected for processing (1)
Sources/cmuxApp.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 `@Resources/Localizable.xcstrings`:
- Around line 8735-8763: The new localization key
settings.browser.import.detecting in Resources/Localizable.xcstrings only has
en/ja/uk/ko entries; add this key with translated stringUnit values for every
locale present in this catalog (i.e., all existing locale keys used elsewhere in
Resources/Localizable.xcstrings) following the repo’s fallback convention where
a translation is unavailable. Locate the block for
settings.browser.import.detecting and add matching "stringUnit" objects for each
supported locale, setting "state" consistent with other entries and using the
accepted fallback text when a translation isn’t provided.
🪄 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: c5b8b6d2-2745-4005-98d4-923c02ce4c03
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/cmuxApp.swift
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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/cmuxApp.swift (1)
5787-5792:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't mark browser history as loaded before the store is actually loaded.
didLoadBrowserHistoryForSettingsis set totruebeforeBrowserHistoryStore.shared.isLoadedbecomes the source of truth, so the loading subtitle and disabled state can disappear while history is still loading. It also trips the one-shot guard even ifloadIfNeeded()doesn't complete immediately.Proposed fix
private func loadBrowserHistoryForSettingsIfNeeded() { guard !didLoadBrowserHistoryForSettings else { return } - didLoadBrowserHistoryForSettings = true BrowserHistoryStore.shared.loadIfNeeded() - browserHistoryEntryCount = BrowserHistoryStore.shared.entries.count + didLoadBrowserHistoryForSettings = BrowserHistoryStore.shared.isLoaded + if didLoadBrowserHistoryForSettings { + browserHistoryEntryCount = BrowserHistoryStore.shared.entries.count + } }🤖 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/cmuxApp.swift` around lines 5787 - 5792, The guard in loadBrowserHistoryForSettingsIfNeeded() sets didLoadBrowserHistoryForSettings = true before the store actually finishes loading; change the flow so you call BrowserHistoryStore.shared.loadIfNeeded() first and only set didLoadBrowserHistoryForSettings after the store reports it's loaded (e.g., by checking BrowserHistoryStore.shared.isLoaded after loadIfNeeded() returns or by using the store's completion/notification callback), and update browserHistoryEntryCount from BrowserHistoryStore.shared.entries only after the store is confirmed loaded; keep the one-shot guard but move the flag assignment to the post-load path so the subtitle/disabled state don't disappear prematurely.
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 8737-8851: The two new localization keys
settings.browser.import.detecting and settings.browser.history.subtitleLoading
are missing a Khmer (km) entry in Resources/Localizable.xcstrings which breaks
the full-locale requirement; add a km localization block for each key matching
the existing per-locale structure (i.e., "km": { "stringUnit": { "state":
"translated", "value": "…" } }) and supply appropriate Khmer translations (or
approved placeholder translated text) for both keys so they mirror the other
locales' entries and satisfy the catalog's fallback convention.
In `@Sources/cmuxApp.swift`:
- Around line 5006-5043: Move the Settings lazy-load helpers into a new Swift
source file: create a new file (e.g., SettingsLazyLoadHelpers.swift), add import
SwiftUI, and paste the following types and extension with the same names so
callers remain unchanged: SettingsScrollCoordinateSpace,
SettingsLazyLoadTrigger, SettingsLazyLoadFramePreferenceKey,
SettingsLazyLoadMarker, and the View extension settingsLazyLoadTrigger(_:).
Ensure you preserve the existing access levels (internal/fileprivate) and API
surface so references in cmuxApp.swift compile, then remove these definitions
from cmuxApp.swift to reduce file size.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 5787-5792: The guard in loadBrowserHistoryForSettingsIfNeeded()
sets didLoadBrowserHistoryForSettings = true before the store actually finishes
loading; change the flow so you call BrowserHistoryStore.shared.loadIfNeeded()
first and only set didLoadBrowserHistoryForSettings after the store reports it's
loaded (e.g., by checking BrowserHistoryStore.shared.isLoaded after
loadIfNeeded() returns or by using the store's completion/notification
callback), and update browserHistoryEntryCount from
BrowserHistoryStore.shared.entries only after the store is confirmed loaded;
keep the one-shot guard but move the flag assignment to the post-load path so
the subtitle/disabled state don't disappear prematurely.
🪄 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: 9bc37765-fb0c-4076-8940-b87badff557b
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/Panels/BrowserPanel.swiftSources/TerminalNotificationStore.swiftSources/cmuxApp.swiftcmuxTests/NotificationAndMenuBarTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 134ccec. Configure here.
This branch was successfully deployed
1 participant
Summary
Testing
Note
The first focused test command above was run before the review-fix commit that removed fallback-chain testing. The second focused test command covers the current notification and Settings presenter code.
Task
Plain-text task: opening in settings does not work and sometimes is very laggy.
Note
Medium Risk
Touches Settings navigation/rendering and moves browser detection to detached async work; mistakes could cause missing/incorrect UI state or stale results. Notification settings URL behavior changes but is localized to opening System Settings.
Overview
Fixes notification Settings opening by generating a modern, app-specific System Settings URL (with a safe fallback) via
TerminalNotificationStore.notificationSettingsURL, and updates tests to validate the new URL and reset injected hooks.Reduces Settings lag by lazy-loading browser history and installed-browser detection: history now loads only when the Browser History row is near-visible/targeted, and browser import detection runs off the main actor with a loading subtitle, disabled refresh while detecting, and generation-based result de-staling. Adds
SettingsLazyLoadHelpers(preference/coordinate-space markers) and new localized strings for the loading states.Reviewed by Cursor Bugbot for commit 4537321. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests