Repository navigation
Add table of contents and search to settings window - #2575
Closed
lawrencecchen wants to merge 8 commits into
Closed
lawrencecchen wants to merge 8 commits into
lawrencecchen wants to merge 8 commits into
Conversation
Settings window now has a 160px sidebar with: - Search field to filter sections by keyword - Section list with icons for quick navigation - Active section tracking based on scroll position The sidebar search matches against section titles and individual setting labels within each section. Non-matching sections are hidden from both the sidebar list and the main content area. Window width increased from 640 to 800 to accommodate the sidebar.
|
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:
📝 WalkthroughWalkthroughIntroduces a searchable, sectioned Settings UI implemented as a NavigationSplitView with TOC sidebar and scroll-synced selection; adds a SettingsSection enum and per-section offset reporting for navigation; increases Settings window width from 640 to 840; adds Changes
Sequence DiagramssequenceDiagram
participant User
participant SearchBar as "Search Input"
participant Sidebar as "TOC Sidebar"
participant SettingsView
participant ScrollView
User->>SearchBar: type query
SearchBar->>Sidebar: update settingsSearchText
Sidebar->>Sidebar: filter sections (SettingsSection.matches)
User->>Sidebar: select section
Sidebar->>SettingsView: set selectedSection
SettingsView->>ScrollView: scrollTo(section)
ScrollView->>SettingsView: report header offsets via preference
SettingsView->>Sidebar: update selectedSection highlight
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR adds a 160px sidebar to the settings window containing a search field and section navigation, widening the window from 640→800px. The implementation introduces a Key changes:
Confidence Score: 3/5Mostly safe to merge with the navigation-while-searching bug fixed first; the rest of the implementation is well-structured. Score lowered from 5 due to the P1 logic bug where deep-link navigation into settings (e.g. from the browser import banner) silently fails if the user left a search query active. The scenario is realistic and reproducible. The two P2 style issues (English-only keyword search, no empty state) are UX gaps but don't cause data loss or crashes. Sources/cmuxApp.swift — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User types in search field] --> B{filteredSections empty?}
B -- No --> C[Sidebar shows matching sections]
B -- Yes --> D[Sidebar and content are blank - No empty-state message]
C --> E[User clicks section in sidebar]
E --> F[proxy.scrollTo section anchor .top]
F --> G[ScrollView scrolls to section]
G --> H[SettingsSectionHeader reports offset via PreferenceKey]
H --> I[onPreferenceChange updates activeSection]
I --> J[Sidebar highlights active section]
K[External SettingsNavigationRequest] --> L{settingsSearchText empty?}
L -- Yes --> M[proxy.scrollTo target works correctly]
L -- No --> N[Target section hidden by filter - proxy.scrollTo is a silent no-op]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2670
to
+2719
| var searchableTerms: [String] { | ||
| switch self { | ||
| case .app: | ||
| return [ | ||
| "language", "theme", "app icon", "workspace placement", "minimal mode", | ||
| "keep workspace open", "focus pane", "preferred editor", "reorder notification", | ||
| "dock badge", "menu bar", "unread pane ring", "pane flash", | ||
| "desktop notifications", "notification sound", "notification command", | ||
| "telemetry", "warn before quit", "rename selects", "command palette", | ||
| "sidebar details", "branch layout", "notification message", | ||
| "branch directory", "pull requests", "ssh", "listening ports", | ||
| "latest log", "progress", "custom metadata", | ||
| ] | ||
| case .workspaceColors: | ||
| return [ | ||
| "color indicator", "selection highlight", "notification badge color", | ||
| "tab color palette", "reset palette", | ||
| ] | ||
| case .sidebarAppearance: | ||
| return [ | ||
| "match terminal background", "light mode tint", "dark mode tint", | ||
| "tint opacity", "reset sidebar tint", | ||
| ] | ||
| case .automation: | ||
| return [ | ||
| "socket control", "password", "claude code", "claude binary path", | ||
| "port base", "port range", | ||
| ] | ||
| case .customCommands: | ||
| return ["trusted directories"] | ||
| case .browser: | ||
| return [ | ||
| "search engine", "search suggestions", "browser theme", | ||
| "terminal links", "intercept open", "embedded browser", | ||
| "external urls", "http hosts", "import browser data", | ||
| "react grab", "browsing history", | ||
| ] | ||
| case .keyboardShortcuts: | ||
| return ["shortcut chords", "shortcut hints", "command hold"] | ||
| case .reset: | ||
| return ["reset all settings"] | ||
| } | ||
| } | ||
|
|
||
| func matches(_ query: String) -> Bool { | ||
| guard !query.isEmpty else { return true } | ||
| let q = query.lowercased() | ||
| if title.lowercased().contains(q) { return true } | ||
| return searchableTerms.contains { $0.lowercased().contains(q) } | ||
| } |
Contributor
There was a problem hiding this comment.
searchableTerms are English-only
The searchableTerms arrays are hardcoded English strings. When a user runs the app in Japanese (or any other supported locale), typing in their own language against these terms will always miss. The section title match (line 2717) works correctly because title uses String(localized:), but the keyword fallback only covers English.
Consider either:
- Keeping
searchableTermsas the English index (acceptable for ASCII-first search), but also matching against the localized section title in all locales, or - Sourcing the terms from the localized display strings already rendered in each section's
SettingsCardRowtitles (harder but more complete).
At minimum this is worth a code comment explaining the English-only design decision so future maintainers understand why non-English keyword search doesn't work.
Comment on lines
+2727
to
+2730
| private var filteredSections: [SettingsSection] { | ||
| guard !searchText.isEmpty else { return Array(SettingsSection.allCases) } | ||
| return SettingsSection.allCases.filter { $0.matches(searchText) } | ||
| } |
Contributor
There was a problem hiding this comment.
When filteredSections is empty (query matches no section), the sidebar shows nothing and the main content VStack renders with only its padding — a blank white area with no user feedback. Consider adding a "No results" label in both the sidebar and/or the main content area:
private var filteredSections: [SettingsSection] {
guard !searchText.isEmpty else { return Array(SettingsSection.allCases) }
return SettingsSection.allCases.filter { $0.matches(searchText) }
}
var body: some View {
VStack(alignment: .leading, spacing: 0) {
// ... search field ...
if filteredSections.isEmpty {
Text(String(localized: "settings.search.noResults", defaultValue: "No results"))
.font(.system(size: 12))
.foregroundStyle(.tertiary)
.padding(.horizontal, 16)
.padding(.top, 8)
} else {
ScrollView { /* ... */ }
}
}
}![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/cmuxApp.swift (1)
6024-6035:⚠️ Potential issue | 🟠 MajorClear stale filters before honoring navigation requests.
SettingsWindowControllerkeeps this view alive, sosettingsSearchTextsurvives closing the window. If that old filter hides Browser, Keyboard Shortcuts, or Browser Import,scrollTohere becomes a no-op and the existing deep links regress.Suggested fix
.onReceive(NotificationCenter.default.publisher(for: SettingsNavigationRequest.notificationName)) { notification in guard let target = SettingsNavigationRequest.target(from: notification) else { return } + settingsSearchText = "" DispatchQueue.main.async { withAnimation(.easeInOut(duration: 0.2)) { switch target { case .browser: proxy.scrollTo(SettingsSection.browser, anchor: .top)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6024 - 6035, The navigation handler must clear any stale search/filter state before attempting to scroll so proxy.scrollTo is not a no-op; before calling proxy.scrollTo in the .onReceive block (the handler invoked via SettingsNavigationRequest.notificationName and SettingsNavigationRequest.target(from:)), reset the settings search/filter state (e.g. set settingsSearchText = "" or call the existing clearFilters() helper if present) and then perform the withAnimation { switch target { ... proxy.scrollTo(...) } } so the target sections (SettingsSection.browser, SettingsSection.keyboardShortcuts, SettingsNavigationTarget.browserImport) are visible and scrollTo will succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 2744-2753: The clear-search Button that appears when searchText is
non-empty uses an image-only control and lacks an accessibility label; add a
localized accessibility label to the Button or Image using String(localized:
"accessibility.clearSearch", defaultValue: "Clear search") so VoiceOver
announces it (update the Button/Image in the same conditional where searchText
is checked to call .accessibilityLabel(...) with the localized string).
- Around line 2670-2718: The current searchableTerms hardcodes aliases and
misses many localized setting labels; update searchableTerms (and matches(_:))
to build its list from the same localized strings used to render each section’s
rows (e.g., map the section’s row models / row.title / localizedLabel collection
or the function that returns setting rows) so searches include actual labels
like “Open Files With” and keyboard shortcut action names; replace the static
arrays in the searchableTerms computed property with a derived array that
iterates the section’s row items (and any subsection titles) and collects their
localized titles/labels for matching.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 6024-6035: The navigation handler must clear any stale
search/filter state before attempting to scroll so proxy.scrollTo is not a
no-op; before calling proxy.scrollTo in the .onReceive block (the handler
invoked via SettingsNavigationRequest.notificationName and
SettingsNavigationRequest.target(from:)), reset the settings search/filter state
(e.g. set settingsSearchText = "" or call the existing clearFilters() helper if
present) and then perform the withAnimation { switch target { ...
proxy.scrollTo(...) } } so the target sections (SettingsSection.browser,
SettingsSection.keyboardShortcuts, SettingsNavigationTarget.browserImport) are
visible and scrollTo will succeed.
🪄 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: 274612e7-2a15-49c3-8656-f6c21a464169
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/cmuxApp.swift
Comment on lines
+2670
to
+2718
| var searchableTerms: [String] { | ||
| switch self { | ||
| case .app: | ||
| return [ | ||
| "language", "theme", "app icon", "workspace placement", "minimal mode", | ||
| "keep workspace open", "focus pane", "preferred editor", "reorder notification", | ||
| "dock badge", "menu bar", "unread pane ring", "pane flash", | ||
| "desktop notifications", "notification sound", "notification command", | ||
| "telemetry", "warn before quit", "rename selects", "command palette", | ||
| "sidebar details", "branch layout", "notification message", | ||
| "branch directory", "pull requests", "ssh", "listening ports", | ||
| "latest log", "progress", "custom metadata", | ||
| ] | ||
| case .workspaceColors: | ||
| return [ | ||
| "color indicator", "selection highlight", "notification badge color", | ||
| "tab color palette", "reset palette", | ||
| ] | ||
| case .sidebarAppearance: | ||
| return [ | ||
| "match terminal background", "light mode tint", "dark mode tint", | ||
| "tint opacity", "reset sidebar tint", | ||
| ] | ||
| case .automation: | ||
| return [ | ||
| "socket control", "password", "claude code", "claude binary path", | ||
| "port base", "port range", | ||
| ] | ||
| case .customCommands: | ||
| return ["trusted directories"] | ||
| case .browser: | ||
| return [ | ||
| "search engine", "search suggestions", "browser theme", | ||
| "terminal links", "intercept open", "embedded browser", | ||
| "external urls", "http hosts", "import browser data", | ||
| "react grab", "browsing history", | ||
| ] | ||
| case .keyboardShortcuts: | ||
| return ["shortcut chords", "shortcut hints", "command hold"] | ||
| case .reset: | ||
| return ["reset all settings"] | ||
| } | ||
| } | ||
|
|
||
| func matches(_ query: String) -> Bool { | ||
| guard !query.isEmpty else { return true } | ||
| let q = query.lowercased() | ||
| if title.lowercased().contains(q) { return true } | ||
| return searchableTerms.contains { $0.lowercased().contains(q) } |
There was a problem hiding this comment.
Build search terms from the actual localized settings labels.
This hand-maintained list already misses valid queries like “Open Files With” (Line 4776) and any keyboard shortcut action label (Lines 5866-5874). Since matches(_:) only checks the section title plus these aliases, the new search does not actually match arbitrary setting labels as described. Please derive each section’s index from the same localized strings the rows render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 2670 - 2718, The current searchableTerms
hardcodes aliases and misses many localized setting labels; update
searchableTerms (and matches(_:)) to build its list from the same localized
strings used to render each section’s rows (e.g., map the section’s row models /
row.title / localizedLabel collection or the function that returns setting rows)
so searches include actual labels like “Open Files With” and keyboard shortcut
action names; replace the static arrays in the searchableTerms computed property
with a derived array that iterates the section’s row items (and any subsection
titles) and collects their localized titles/labels for matching.
Contributor
Author
There was a problem hiding this comment.
Valid point. The searchableTerms are supplemental section-level keywords for v1. Deriving from actual row labels is a good follow-up but requires a larger refactor to extract label strings from the view hierarchy. Filed as a follow-up.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen Thanks — acknowledged. Since the full label-derived index is scoped to a follow-up, here’s a tiny, low-risk improvement we can land in this PR to make v1 search more useful right away:
- Append all KeyboardShortcutSettings.Action labels to the Keyboard Shortcuts section’s index (covers every action name).
- Add “Open Files With” to the App section’s supplemental keywords.
- Make matching case/diacritic-insensitive.
Swift patch (in SettingsSection):
enum SettingsSection: String, CaseIterable, Identifiable {
// …
var searchableTerms: [String] {
switch self {
case .app:
return [
"language", "theme", "app icon", "workspace placement", "minimal mode",
"keep workspace open", "focus pane", "preferred editor", "open files with",
"reorder notification", "dock badge", "menu bar", "unread pane ring", "pane flash",
"desktop notifications", "notification sound", "notification command",
"telemetry", "warn before quit", "rename selects", "command palette",
"sidebar details", "branch layout", "notification message",
"branch directory", "pull requests", "ssh", "listening ports",
"latest log", "progress", "custom metadata",
]
// … other sections unchanged …
case .keyboardShortcuts:
var terms = ["shortcut chords", "shortcut hints", "command hold"]
// Dynamically include all action labels shown in the UI.
terms.append(contentsOf: KeyboardShortcutSettings.Action.allCases.map { $0.label })
return terms
case .reset:
return ["reset all settings"]
}
}
func matches(_ query: String) -> Bool {
guard !query.isEmpty else { return true }
let norm = { (s: String) in s.folding(options: [.diacriticInsensitive, .caseInsensitive], locale: .current) }
let q = norm(query)
if norm(title).contains(q) { return true }
return searchableTerms.contains { norm($0).contains(q) }
}
}If you’re good with this scoped tweak, I can prep it for this PR. Also, could you drop a link to the follow-up issue here so we can cross-reference?
🧠 Learnings used
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/ContentView.swift:6055-6057
Timestamp: 2026-03-09T02:09:08.742Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), within CommandPaletteSwitcherSearchIndexer.metadataKeywordsForSearch(_:detail:), when tags are present, only add "tag" and "topic" to contextKeywords; do not append "claude" unconditionally to avoid false-positive switcher matches.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: Znboston
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-26T19:00:12.434Z
Learning: Repo: manaflow-ai/cmux — Sources/GroupHeaderView.swift and Sources/ContentView.swift — Palette color entry labels (`Text(entry.name)`) use dynamic palette data loaded at runtime and cannot be statically keyed with `String(localized:)`. This matches the existing workspace color picker pattern in ContentView.swift. Palette name localization would require restructuring the color palette system and is intentionally deferred; do not flag `entry.name` as a missing localization in this codebase.
Learnt from: debgotwired
Repo: manaflow-ai/cmux PR: 1149
File: Sources/ContentView.swift:3977-3978
Timestamp: 2026-03-10T09:33:37.952Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) is shared command‑palette infrastructure across all submenus. Do not change its sync‑seeding behavior within feature‑scoped PRs; treat brief initial flashes as consistent with existing submenus. Any UX improvement (e.g., synchronous seeding on forced corpus refresh) should be implemented and tested globally in a dedicated follow‑up PR.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 318
File: Sources/Panels/BrowserPanel.swift:5511-5521
Timestamp: 2026-03-17T05:34:52.853Z
Learning: In manaflow-ai/cmux, import-flow UI strings in Sources/Panels/BrowserPanel.swift must use String(localized:..., defaultValue:...) with browser.import.* keys, while brand/engine names (e.g., Google, DuckDuckGo) remain unlocalized per project convention.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:37:25.743Z
Learning: Repo: manaflow-ai/cmux — In Sources/KeyboardShortcutSettings.swift, KeyboardShortcutSettings.setShortcut(_:, for:) is a no‑op when the action is managed by settings.json (isManagedBySettingsFile(action) == true), preventing UserDefaults backfill for file‑managed shortcuts.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/AppDelegate.swift:2196-2200
Timestamp: 2026-04-03T03:36:45.112Z
Learning: Repo: manaflow-ai/cmux — In Sources/AppDelegate.swift, when KeyboardShortcutSettings.didChangeNotification fires, AppDelegate must clear configured-chord caches (pendingConfiguredShortcutChord and activeConfiguredShortcutChordPrefixForCurrentEvent) via clearConfiguredShortcutChordState() before refreshing tooltips/UI. Also clear chord state on applicationWillResignActive to avoid cross-activity leakage. Verified by cmuxTests/AppDelegateShortcutRoutingTests.swift::testShortcutChangeClearsPendingConfiguredChord.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 2505
File: Sources/AppDelegate.swift:4910-4918
Timestamp: 2026-04-06T02:02:42.414Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — Keyboard repair pattern: In AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:), determine whether to repair focus by asking the focused terminal’s hosted view if the current first responder already matches that panel’s preferred keyboard target. Implemented via hostedView.responderMatchesPreferredKeyboardFocus(responder) inside responderNeedsFocusedTerminalKeyRepair(_:in:hostedView:). This covers same-window drift to a different Ghostty surface without comparing workspace/panel IDs. Verified by test cmuxTests/AppDelegateShortcutRoutingTests.swift::testWindowSendEventRepairsVisibleSameWindowResponderDriftForFocusedTerminalTyping.
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceAction(params:) -> case "set_color": palette names are resolved via WorkspaceTabColorSettings.defaultPaletteWithOverrides(), whose entries are always valid hex (validated by the UI). Therefore, additional normalization of entry.hex is unnecessary.
Learnt from: HamptonMakes
Repo: manaflow-ai/cmux PR: 2007
File: Sources/cmuxApp.swift:3798-3799
Timestamp: 2026-03-23T16:36:55.259Z
Learning: Repo: manaflow-ai/cmux — In Sources/cmuxApp.swift, SettingsView.resetAllSettings() must reset newly added AppStorage toggles to defaults. Specifically, ensure ampHooksEnabled is set to AmpIntegrationSettings.defaultHooksEnabled so the Amp integration toggle resets correctly.
Learnt from: lucasward
Repo: manaflow-ai/cmux PR: 1903
File: Sources/ContentView.swift:13069-13113
Timestamp: 2026-03-21T06:23:38.764Z
Learning: Repo: manaflow-ai/cmux — In Sources/ContentView.swift, DraggableFolderNSView.updateIcon() intentionally applies the "sidebarMonochromeIcons" setting only on view creation (no live observer). Do not add defaults observers/AppStorage for live refresh in feature-scoped PRs; a live-refresh can be considered in a separate follow-up.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:16.241Z
Learning: For manaflow-ai/cmux PR `#819` (Japanese i18n), keep scope limited to localization changes; UX enhancements like preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels should be handled in a separate follow-up issue.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:35:54.082Z
Learning: In this repo’s keyboard shortcut implementation, ensure `KeyboardShortcutSettings.setShortcut(...)` does nothing (no-op) when `KeyboardShortcutSettings.isManagedBySettingsFile(action)` returns `true` (i.e., the shortcut is managed via `settings.json`). This prevents writing back overrides into `UserDefaults` and keeps `settings.json` as the source of truth.
Learnt from: wonbywondev
Repo: manaflow-ai/cmux PR: 1795
File: Sources/ContentView.swift:1392-1395
Timestamp: 2026-03-19T09:21:24.369Z
Learning: Repo manaflow-ai/cmux — Notifications UI: Production notifications are shown via NSPopover (toggleNotificationsPopover in Sources/Update/UpdateTitlebarAccessory.swift). The SidebarSelectionState.selection == .notifications path in Sources/ContentView.swift is test/scaffolding-only (set in AppDelegate for tests) and isn’t used by user actions; .tabs remains active in normal flows. Therefore, folder-drop handling doesn’t need to flip selection.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 318
File: Sources/Panels/BrowserPanel.swift:0-0
Timestamp: 2026-03-17T05:34:44.905Z
Learning: In manaflow-ai/cmux (PR `#318`), the browser import wizard in Sources/Panels/BrowserPanel.swift now exposes an Additional data checkbox and passes its state into BrowserImportScope.fromSelection so users can select .everything (including when only additional-data is chosen). Unit and UI tests cover the additional‑data‑only mapping.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 2545
File: Sources/GhosttyTerminalView.swift:3891-3903
Timestamp: 2026-04-02T21:37:21.463Z
Learning: In Swift source files like Sources/GhosttyTerminalView.swift, avoid logging raw startup commands or initialInput even in DEBUG (to prevent leaking sensitive paths/tokens and multiline content). If you need to diagnose startup/input, log only non-sensitive metadata such as (1) presence flags (e.g., hasStartupCommand/hasInitialInput), (2) byte counts, and (3) the relevant surface id (so issues can be correlated without exposing the underlying strings).
![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 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:2674">
P2: Settings search keywords are English-only, so label-based search will not work correctly for localized languages.</violation>
<violation number="2" location="Sources/cmuxApp.swift:2769">
P2: Missing empty state when search yields zero results. When `filteredSections` is empty, the sidebar shows a blank `ScrollView` and the main content area renders an empty `VStack` — no feedback to the user. Add a "No results" label when `filteredSections.isEmpty`.</violation>
<violation number="3" location="Sources/cmuxApp.swift:6028">
P1: Navigation requests can fail when a persisted search filter hides the destination section, because the scroll handler does not clear the filter before `scrollTo`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| switch self { | ||
| case .app: | ||
| return [ | ||
| "language", "theme", "app icon", "workspace placement", "minimal mode", |
There was a problem hiding this comment.
P2: Settings search keywords are English-only, so label-based search will not work correctly for localized languages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/cmuxApp.swift, line 2674:
<comment>Settings search keywords are English-only, so label-based search will not work correctly for localized languages.</comment>
<file context>
@@ -2629,6 +2629,182 @@ enum SettingsNavigationRequest {
+ switch self {
+ case .app:
+ return [
+ "language", "theme", "app icon", "workspace placement", "minimal mode",
+ "keep workspace open", "focus pane", "preferred editor", "reorder notification",
+ "dock badge", "menu bar", "unread pane ring", "pane flash",
</file context>
Contributor
Author
There was a problem hiding this comment.
Valid concern. The section titles are already localized via String(localized:), so searching by section name works in all languages. The supplemental keywords are English-only in v1. Deriving localized per-setting keywords is a follow-up.
— Claude Code
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
This was referenced Apr 5, 2026
Switch from custom VStack+Button layout to List with .listStyle(.sidebar) for the settings TOC, matching the native macOS System Settings pattern. Sidebar width increased from 160 to 200, window width to 840.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (2)
2670-2718:⚠️ Potential issue | 🟠 MajorIndex the real localized setting labels, not a hand-maintained alias list.
matches(_:)only checkstitleplus these English literals, so searches still miss real labels like “Open Files With” and the per-action labels rendered byShortcutSettingRow. In non-English UI, these aliases are also unsearchable. Build each section’s search terms from the same localized labels the rows render, then layer any deliberate synonyms on top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2670 - 2718, The current searchableTerms computed property returns hard-coded English aliases and causes matches(_:) to miss actual localized row labels (and per-action labels like ShortcutSettingRow). Replace the static arrays in searchableTerms with a dynamic collection built from the actual localized titles/labels used by each settings row (e.g., pull the displayed label strings from the same data source or view models that power title and ShortcutSettingRow), then merge any intentional synonyms on top; keep matches(_:) logic but use these localized terms plus title.lowercased() so searches work in other locales and include per-action shortcut labels.
2746-2755:⚠️ Potential issue | 🟡 MinorLabel the clear-search button for VoiceOver.
This is an image-only control, so it currently has no meaningful spoken name.
As per coding guidelines, all user-facing strings must be localized using `String(localized: "key.name", defaultValue: "English text")` for every string shown in the UI.♿ Minimal fix
if !searchText.isEmpty { Button { searchText = "" } label: { Image(systemName: "xmark.circle.fill") .font(.system(size: 11)) .foregroundStyle(.tertiary) } .buttonStyle(.plain) + .accessibilityLabel( + String(localized: "settings.search.clear", defaultValue: "Clear search") + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2746 - 2755, The clear-search Button (the Image with systemName "xmark.circle.fill" that clears searchText) is image-only and lacks an accessibility label; add an accessibility label using .accessibilityLabel(...) with a localized string (using String(localized: "clear.search", defaultValue: "Clear search")) so VoiceOver announces it, and ensure the Button remains .buttonStyle(.plain); target the Button containing the Image and apply the accessibility modifier there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 2783-2791: The selection is being updated both by user taps and by
activeSection changes, causing onSelect to be re-triggered during scroll; add a
transient guard flag (e.g., isUpdatingSelectionFromActiveSection or
suppressOnSelect) tracked in the view/state, set it true immediately before you
programmatically assign selection inside the .onChange(of: activeSection)
handler (the block that currently does `selection = newValue`), and set it back
to false after the update (use DispatchQueue.main.async { ... } if needed to
clear on the next runloop). Then modify the .onChange(of: selection) handler to
call onSelect(section) only when the guard flag is false, so programmatic sync
from activeSection does not invoke onSelect and interrupt the scroll.
- Around line 6021-6028: scrollTo calls can fail when a settings filter is
active because the target sections (e.g., SettingsSection.browser,
SettingsSection.keyboardShortcuts, SettingsNavigationTarget.browserImport) are
filtered out by sectionVisible() when settingsSearchText is non-empty; fix by
clearing settingsSearchText before invoking proxy.scrollTo so the destination
anchors exist. Locate the switch handling target navigation (the proxy.scrollTo
calls) and set settingsSearchText = "" (or call the existing method that clears
the filter) immediately before the switch (or right before each proxy.scrollTo)
to ensure the section is visible, then perform the proxy.scrollTo for the
matching cases.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 2670-2718: The current searchableTerms computed property returns
hard-coded English aliases and causes matches(_:) to miss actual localized row
labels (and per-action labels like ShortcutSettingRow). Replace the static
arrays in searchableTerms with a dynamic collection built from the actual
localized titles/labels used by each settings row (e.g., pull the displayed
label strings from the same data source or view models that power title and
ShortcutSettingRow), then merge any intentional synonyms on top; keep
matches(_:) logic but use these localized terms plus title.lowercased() so
searches work in other locales and include per-action shortcut labels.
- Around line 2746-2755: The clear-search Button (the Image with systemName
"xmark.circle.fill" that clears searchText) is image-only and lacks an
accessibility label; add an accessibility label using .accessibilityLabel(...)
with a localized string (using String(localized: "clear.search", defaultValue:
"Clear search")) so VoiceOver announces it, and ensure the Button remains
.buttonStyle(.plain); target the Button containing the Image and apply the
accessibility modifier there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Fix sidebar click not scrolling: add debounce flag to prevent scroll-position tracking from fighting user navigation clicks - Replace custom TextField search with native NSSearchField for proper macOS search field appearance (magnifying glass, clear button)
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
You’re at about 97% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:2799">
P2: `selection` changes from scroll-sync are incorrectly treated as user navigation, which suppresses active-section updates for 0.5s and can desync sidebar highlight while scrolling.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Replace manual HStack+Divider layout with NavigationSplitView, which gives the native macOS source list sidebar appearance: translucent background, proper blue selection highlight, and built-in divider. Search uses .searchable(placement: .sidebar) for native integration. Removed SettingsTOCSidebar, SettingsSearchField, and SettingsTitleLeadingInsetReader (no longer needed).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (2)
5926-5939:⚠️ Potential issue | 🟠 MajorNavigation requests can silently fail when filtering hides the destination section.
At Line 5929,
scrollToruns without clearingsettingsSearchText. If the search filter hides.browser,.keyboardShortcuts, or.browserImport(via Line 4561), the target anchor is absent and navigation no-ops.♻️ Minimal fix
.onReceive(NotificationCenter.default.publisher(for: SettingsNavigationRequest.notificationName)) { notification in guard let target = SettingsNavigationRequest.target(from: notification) else { return } DispatchQueue.main.async { + settingsSearchText = "" withAnimation(.easeInOut(duration: 0.2)) { switch target { case .browser: proxy.scrollTo(SettingsSection.browser, anchor: .top)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 5926 - 5939, When handling SettingsNavigationRequest in the onReceive block (the closure that calls proxy.scrollTo for SettingsSection.browser, SettingsSection.keyboardShortcuts, and SettingsNavigationTarget.browserImport), clear the settingsSearchText filter first so the destination anchor is not filtered out; update the handler that processes NotificationCenter.default.publisher(for: SettingsNavigationRequest.notificationName) to set settingsSearchText = "" (on main thread) before calling withAnimation and proxy.scrollTo for the relevant targets.
2670-2719:⚠️ Potential issue | 🟠 MajorSearch matching still depends on hand-maintained aliases, not actual setting labels.
Lines 2670-2719 still use a static alias list, so section filtering can miss real row labels and dynamically generated labels (for example, shortcut action labels rendered in Line 5791). This means label-based search coverage is incomplete and brittle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2670 - 2719, The current searchableTerms array in the SettingsSection enum is a brittle, hand-maintained alias list; update searchableTerms (or remove it) and make matches(_:) derive searchable labels dynamically from the actual settings rows for the section (e.g., iterate the model/rows accessor used to render the section — look up the same source that renders rows such as the function or property that produces row view models for this SettingsSection), collecting each row’s displayed title, subtitle, localized strings and any dynamic labels (like shortcut action labels or editor names) and lowercasing them, then check containment against q; ensure matches(_:) continues checking title.lowercased() but also queries those live row labels instead of only searchableTerms so dynamically generated labels are included.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 5926-5939: When handling SettingsNavigationRequest in the
onReceive block (the closure that calls proxy.scrollTo for
SettingsSection.browser, SettingsSection.keyboardShortcuts, and
SettingsNavigationTarget.browserImport), clear the settingsSearchText filter
first so the destination anchor is not filtered out; update the handler that
processes NotificationCenter.default.publisher(for:
SettingsNavigationRequest.notificationName) to set settingsSearchText = "" (on
main thread) before calling withAnimation and proxy.scrollTo for the relevant
targets.
- Around line 2670-2719: The current searchableTerms array in the
SettingsSection enum is a brittle, hand-maintained alias list; update
searchableTerms (or remove it) and make matches(_:) derive searchable labels
dynamically from the actual settings rows for the section (e.g., iterate the
model/rows accessor used to render the section — look up the same source that
renders rows such as the function or property that produces row view models for
this SettingsSection), collecting each row’s displayed title, subtitle,
localized strings and any dynamic labels (like shortcut action labels or editor
names) and lowercasing them, then check containment against q; ensure
matches(_:) continues checking title.lowercased() but also queries those live
row labels instead of only searchableTerms so dynamically generated labels are
included.
The old floating header (Settings title + blur + Open settings.json button) was layering on top of NavigationSplitView's own toolbar. Removed it entirely and moved the settings.json button to .toolbar(). Also cleaned up unused blur tracking state and helper views.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
You’re at about 97% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:5916">
P1: `selectedSection` is now used for both active-scroll tracking and sidebar navigation, which creates a feedback loop that can snap the scroll position while users manually scroll.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The old settings window used fullSizeContentView + transparent titlebar
for a custom floating header. With NavigationSplitView this caused two
titles to layer: "App" (from sidebar selection auto-title) and
"Settings" (from window title bleeding through transparent titlebar).
Fix: disable fullSizeContentView and titlebarAppearsTransparent in the
settings window defaults, make titleVisibility visible, and add
.navigationTitle("") on the detail to suppress the sidebar auto-title.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
Sources/cmuxApp.swift (3)
5853-5867:⚠️ Potential issue | 🟠 MajorClear the active filter before deep-link scrolling.
If
settingsSearchTextis hiding.browser,.keyboardShortcuts, or the.browserImportanchor, thesescrollTocalls have no mounted destination and silently do nothing. That regresses existing callers likeSources/TerminalController.swift:6886-6904,Sources/ContentView.swift:11888-11895, andSources/Panels/BrowserPanelView.swift:1595-1605, which expect those targets to always resolve.♻️ Minimal fix
.onReceive(NotificationCenter.default.publisher(for: SettingsNavigationRequest.notificationName)) { notification in guard let target = SettingsNavigationRequest.target(from: notification) else { return } DispatchQueue.main.async { + settingsSearchText = "" withAnimation(.easeInOut(duration: 0.2)) { switch target {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 5853 - 5867, The deep-link scrolling can fail when settingsSearchText filters out the target anchors; before calling proxy.scrollTo in the NotificationCenter handler (for SettingsNavigationRequest.notificationName), clear the active filter by setting settingsSearchText = "" on the main thread so the anchors are mounted, then perform the existing withAnimation { switch ... proxy.scrollTo(...) } calls; update the handler that uses proxy.scrollTo for SettingsSection.browser, SettingsSection.keyboardShortcuts, and SettingsNavigationTarget.browserImport to reset settingsSearchText first.
2670-2718:⚠️ Potential issue | 🟠 MajorIndex search from the rendered labels, not a hand-maintained alias list.
matches(_:)only checkstitleplussearchableTerms, so the new search still misses real setting labels. For example, Line 4689 renders Open Files With, but this section only indexes"preferred editor", and the keyboard shortcuts section ignores the per-action labels rendered fromKeyboardShortcutSettings.Action.allCasesat Line 5779. That means the feature does not actually match individual setting labels, and localized labels stay unsearchable. Build each section’s search corpus from the same localized strings the rows render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2670 - 2718, The search currently uses the hard-coded searchableTerms array and title in the matches(_:) function, which misses many rendered/localized labels; modify matches(_:) to build its search corpus from the same localized strings the UI renders (instead of or in addition to searchableTerms). Specifically, for each Settings section (use the section enum and its cases, the searchableTerms var as fallback), gather the actual localized row labels used by the view builders (e.g., the strings produced for KeyboardShortcutSettings.Action.allCases and the "Open Files With"/preferred editor label) and check those against the lowercased query; keep title matching but replace the static list with a dynamic collection derived from the same label sources the rows use so localization and per-item labels are searchable.
5821-5852:⚠️ Potential issue | 🟠 MajorDon’t feed scroll-tracked section changes back into
scrollTo.
onPreferenceChangeupdatesselectedSection, and then Line 5843 animatesscrollTofor everyselectedSectionchange. Once the active section changes while the user is scrolling, the view can snap to that section header instead of just updating the sidebar highlight. Keep a separateactiveSection, or suppress thescrollTobranch when the selection change originated from the offset preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 5821 - 5852, The problem is that onPreferenceChange(SettingsSectionOffsetsPreferenceKey.self) mutates selectedSection which triggers the onChange(of: selectedSection) handler and causes proxy.scrollTo to run, snapping the view while the user is scrolling; fix this by distinguishing user-driven/programmatic selection from scroll-tracked updates: when you set selectedSection inside onPreferenceChange, set a short-lived flag (e.g., isScrollTrackedUpdate) or use a separate property (e.g., activeSection) for the sidebar highlight, and then in onChange(of: selectedSection) ignore the change if isScrollTrackedUpdate is true (or only call proxy.scrollTo when the change is a user-initiated navigation using isUserNavigating). Ensure you clear the flag after the update (or update activeSection instead of selectedSection) so programmatic scrolling still works when intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 5853-5867: The deep-link scrolling can fail when
settingsSearchText filters out the target anchors; before calling proxy.scrollTo
in the NotificationCenter handler (for
SettingsNavigationRequest.notificationName), clear the active filter by setting
settingsSearchText = "" on the main thread so the anchors are mounted, then
perform the existing withAnimation { switch ... proxy.scrollTo(...) } calls;
update the handler that uses proxy.scrollTo for SettingsSection.browser,
SettingsSection.keyboardShortcuts, and SettingsNavigationTarget.browserImport to
reset settingsSearchText first.
- Around line 2670-2718: The search currently uses the hard-coded
searchableTerms array and title in the matches(_:) function, which misses many
rendered/localized labels; modify matches(_:) to build its search corpus from
the same localized strings the UI renders (instead of or in addition to
searchableTerms). Specifically, for each Settings section (use the section enum
and its cases, the searchableTerms var as fallback), gather the actual localized
row labels used by the view builders (e.g., the strings produced for
KeyboardShortcutSettings.Action.allCases and the "Open Files With"/preferred
editor label) and check those against the lowercased query; keep title matching
but replace the static list with a dynamic collection derived from the same
label sources the rows use so localization and per-item labels are searchable.
- Around line 5821-5852: The problem is that
onPreferenceChange(SettingsSectionOffsetsPreferenceKey.self) mutates
selectedSection which triggers the onChange(of: selectedSection) handler and
causes proxy.scrollTo to run, snapping the view while the user is scrolling; fix
this by distinguishing user-driven/programmatic selection from scroll-tracked
updates: when you set selectedSection inside onPreferenceChange, set a
short-lived flag (e.g., isScrollTrackedUpdate) or use a separate property (e.g.,
activeSection) for the sidebar highlight, and then in onChange(of:
selectedSection) ignore the change if isScrollTrackedUpdate is true (or only
call proxy.scrollTo when the change is a user-initiated navigation using
isUserNavigating). Ensure you clear the flag after the update (or update
activeSection instead of selectedSection) so programmatic scrolling still works
when intended.
… filter - Add "No Results" empty state when search yields zero matches - Break scroll tracking feedback loop by distinguishing scroll-tracked vs sidebar-clicked selection changes - Clear search filter before navigating to target sections so scrollTo doesn't fail on hidden sections
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
Testing
Related
Summary by cubic
Adds a native table of contents sidebar and search to the Settings window for faster navigation, fulfilling the Linear task to add TOC + search. Uses a macOS
NavigationSplitViewand widens the window to 840px to fit a 200px sidebar.New Features
NavigationSplitView+ List; click to jump to a section.NSSearchFieldfilters by section titles and setting labels; non‑matching sections are hidden in both sidebar and content, with a "No Results" empty state.Bug Fixes
Written for commit 830271d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization