Restore search-result row highlight in CmuxSettingsUI - #5012
Merged
Merged
Conversation
The SPM settings reimplementation kept the search-to-scroll navigation but dropped the pulse that flashed an accent border on the navigated-to row, so clicking a search hit silently jumped without confirming which row matched. Legacy SettingsView drove this through an environment highlight state plus a per-row TimelineView border; the package's rows were never anchored, so even the scroll-to-center no-oped. Port the legacy affordance: SettingsSearchHighlight adds the highlight state, environment plumbing, and the pulsing-border modifier. Each SettingsCardRow now resolves the cmux.json path it already declares via configurationReview into the sidebar/search anchor id through a new SettingsSearchIndex.anchorID(forSettingsPath:), tags itself with that id, and pulses when it is the active highlight target. The settings root injects the index and arms the highlight on every highlighting navigation request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughRows can declare search anchors (explicit or derived from config paths); SettingsSearchIndex maps dotted paths to curated anchor ids; the window resolves an anchor, scrolls to it, and triggers a timed pulsing highlight on the matching row. ChangesSettings search highlighting
Sequence DiagramsequenceDiagram
participant User
participant SettingsWindowRoot
participant SettingsSearchIndex
participant ScrollView
participant SettingsCardRow
participant SettingsSearchHighlightModifier
User->>SettingsWindowRoot: navigateTo(settings path)
SettingsWindowRoot->>SettingsSearchIndex: anchorID(forSettingsPath:)
SettingsWindowRoot->>ScrollView: scrollTo(anchorID)
SettingsWindowRoot->>SettingsSearchHighlightModifier: set highlight state (anchor, token, startedAt)
SettingsCardRow->>SettingsSearchHighlightModifier: matched anchor -> render pulsing accent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryRestores the search-result row highlight in
Confidence Score: 4/5Safe to merge for the highlight and reopen fixes; the The highlight implementation is solid — the finite
Important Files Changed
Reviews (16): Last reviewed commit: "Remove temporary window-state debug prob..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+103
to
+122
| func body(content: Content) -> some View { | ||
| content | ||
| .background { | ||
| if matches(highlightState) { | ||
| TimelineView(.animation) { context in | ||
| let opacity = highlightOpacity(at: context.date, for: highlightState) | ||
| RoundedRectangle(cornerRadius: 8, style: .continuous) | ||
| .fill(Color.accentColor.opacity(opacity * 0.24)) | ||
| .overlay( | ||
| RoundedRectangle(cornerRadius: 8, style: .continuous) | ||
| .stroke(Color.accentColor.opacity(opacity), lineWidth: 2.5) | ||
| ) | ||
| .shadow(color: Color.accentColor.opacity(opacity * 0.24), radius: 8, x: 0, y: 0) | ||
| } | ||
| // Restart the animation when the user re-navigates to | ||
| // the same anchor: a changing token forces a fresh | ||
| // TimelineView identity so `startedAt` re-seeds. | ||
| .id(highlightState.token) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
TimelineView(.animation) keeps a live display-link after fade-out
Once elapsed >= 5.9 the highlightOpacity function always returns 0, yet highlightedSearchAnchorID is never cleared after the animation completes — only on the next navigation. So matches(highlightState) stays true and the TimelineView(.animation) keeps firing at display-refresh rate (~120fps on ProMotion), calling highlightOpacity every frame and drawing RoundedRectangle shapes with opacity(0) indefinitely. The display link for the targeted row remains live until the user clicks another search result or closes the window. Consider clearing the anchor id after the animation window via a one-shot Task { try? await Task.sleep(for: .seconds(6)); highlightedSearchAnchorID = nil } gated by the same token, or by deriving visibility from elapsed in a TimelineSchedule.explicit schedule that terminates after the last keyframe.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
![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
🤖 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/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchHighlight.swift`:
- Around line 18-28: Add Swift-DocC triple-slash comments for every public
symbol listed: provide a one-sentence summary for the
SettingsSearchHighlightState struct and its public init, and add similar
one-sentence DocC comments for EnvironmentValues.settingsSearchHighlightState,
EnvironmentValues.settingsSearchIndex, View.settingsSearchAnchor(_:) and
View.settingsSearchAnchors(_:); place the comments immediately above each
declaration, follow the example one-sentence summary pattern used elsewhere in
the file, and ensure each public symbol has at least one clear descriptive
sentence.
In
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swift`:
- Around line 73-82: Consolidate the three separate `@State` vars
(highlightedSearchAnchorID, searchHighlightToken, searchHighlightStartedAt) into
a single `@State` value (e.g. highlightState: SettingsSearchHighlightState) and
update all places that read or write them (including SettingsCardRow access via
\.settingsSearchHighlightState) to use highlightState.anchorID, .token, and
.startedAt; replace the three-step update pattern with a single assignment like
highlightState = SettingsSearchHighlightState(anchorID: anchorID, token:
highlightState.token + 1, startedAt: Date()) so updates are atomic and there are
no dangling mutable side channels.
🪄 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: 676a3b57-0ed8-430e-bfb4-da920cce78b4
📒 Files selected for processing (5)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Chrome/SettingsCardRow.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchHighlight.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchIndex.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Scene/SettingsWindowScene.swiftPackages/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsSearchIndexTests.swift
The legacy in-app settings (Sources/SettingsNavigation.swift) still ships in the app target and declares a same-named SettingsSearchHighlightState plus View/EnvironmentValues extensions. The app target imports CmuxSettingsUI, so the package's public copies made every unqualified use (settingsSearchAnchor(s), the highlight-state init) ambiguous and broke the Release/Debug app builds. None of these symbols are consumed outside the package, so make them internal; the public API stays limited to SettingsSearchIndex.anchorID(forSettingsPath:). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchHighlight.swift (1)
30-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove the unneeded explicit initializer.
Swift automatically synthesizes memberwise initializers for structs. Since all properties are
letconstants, the synthesized initializer is identical to the explicit one. Removing it reduces code and resolves the SwiftLint warning.♻️ Proposed fix
let startedAt: Date? - - init(anchorID: String?, token: Int, startedAt: Date?) { - self.anchorID = anchorID - self.token = token - self.startedAt = startedAt - } }🤖 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 `@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchHighlight.swift` around lines 30 - 34, Remove the explicit initializer init(anchorID: String?, token: Int, startedAt: Date?) from the struct (the synthesized memberwise initializer already matches since all properties are let constants); delete the init(...) method so Swift uses the automatically generated memberwise initializer and the SwiftLint warning is resolved.
🤖 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
`@Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchHighlight.swift`:
- Around line 30-34: Remove the explicit initializer init(anchorID: String?,
token: Int, startedAt: Date?) from the struct (the synthesized memberwise
initializer already matches since all properties are let constants); delete the
init(...) method so Swift uses the automatically generated memberwise
initializer and the SwiftLint warning is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7434d90a-97b7-47d3-95fa-cf2b3af8d31d
📒 Files selected for processing (1)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/SettingsSearchHighlight.swift
Greptile flagged that TimelineView(.animation) keeps a live display link firing at refresh rate forever after the pulse fades, since the highlight state isn't cleared until the next navigation. Switch to a finite .explicit schedule covering only the pulse window so the TimelineView stops requesting frames once the fade completes (no Task.sleep, matching the no-sleep-in-app-code rule). A row that scrolls in mid-pulse still renders the correct frame; a row whose pulse already elapsed renders the final invisible frame once and schedules nothing further. Also consolidate the three highlight @State properties into a single SettingsSearchHighlightState value (CodeRabbit nitpick). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed review feedback in e606214:
Note: the |
Two gaps from dogfooding:
1. The "Sidebar Branch Layout" row never highlighted because its row
path (configurationReview .json("sidebar.branchLayout")) didn't match
the curated entry's synonym dotted token (sidebar.branchVerticalLayout,
the internal defaults key), so anchorID(forSettingsPath:) returned nil.
Add sidebar.branchLayout to the synonyms. Add
SettingsRowAnchorResolutionTests, which asserts every configReview
path declared in Sections/*.swift resolves to a real indexed entry —
this is what caught branchLayout and guards the whole bridge.
2. Section headers didn't pulse when their section search hit was
clicked. SettingsSectionHeader now takes its SettingsSectionID and
applies a highlight-only modifier (settingsSearchHighlight, no extra
scroll .id since the enclosing section already owns section:<raw>).
applyScrollNavigation now arms the highlight for section hits too
(anchorID == sectionID), not just deep row hits. The Browser >
Import Browser Data subsection gets the same treatment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two issues from dogfooding: (1) search surfaced raw catalog keys with no UI (e.g. account.welcomeShown); (2) several results didn't highlight because their row didn't resolve to the curated entry id. Root cause was a leaky path bridge plus a catalog back-fill: - The index back-filled every SettingCatalog key as a search hit even when no row existed. Drop the back-fill: search now indexes only sections + curated entries (which mirror detail-pane rows), and anchorID(forSettingsPath:) no longer has a setting:<path> fallback. - Many curated entries' synonyms omitted the row's exact cmux.json path (or used the internal key, e.g. sidebar.branchVerticalLayout vs the row's sidebar.branchLayout), so rows resolved to the wrong/nonexistent id. Add the exact path token to every such entry (~20). - Rows with no single cmux.json key (Theme/App Icon pickers, Account card, Reset All, Open cmux.json, Documentation, Enable Browser, Dock, global hotkey enable/recorder, shortcut chords/reset/list, import block + hint, File Drops) now carry an explicit searchAnchorID / settingsSearchAnchors equal to their curated id. SettingsRowAnchorResolutionTests is now bidirectional and exhaustive: forward (every row configReview path resolves to a real entry) and inverse (every curated search result is reachable from a row anchor), so a future drift in either direction fails CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Diffing the curated table against the legacy SettingsNavigation index (still in the app target) surfaced settings legacy made searchable that the SPM rewrite dropped even though the rows still exist in the detail pane: Desktop Notifications, Terminal Config, Browsing History, and Reset Palette. Re-add curated entries and anchor each row so they search and highlight. (fork-conversation-default stays out: the new UI dropped that row.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Enumerating every search result (per-letter sweep) surfaced 13 entries whose search label differed from the row in the detail pane, e.g. "Disable cmux Browser" vs the row's "Enable cmux Browser", "Workspace Presentation Mode" vs "Minimal Mode", and the global-hotkey recorder showing "Global Hotkey" (duplicating the section row) instead of "Show/Hide All Windows". Align each curated title with its row's label so the search hit reads the same as the setting it scrolls to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…highlight # Conflicts: # Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swift
Clicking a sidebar section while scrolled far away (e.g. jumping to Keyboard Shortcuts from the Browser/Import area) sometimes left the section header mid- or bottom-viewport instead of pinned at the top. The LazyVStack only realizes off-screen sections as the first scroll nears them, so a single scrollTo lands before the target's geometry is known. Re-issue the scroll on the next runloop tick (generation-guarded) so the header reliably settles at the top once realized. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A subagent audit of the search<->row mapping surfaced issues the data tests missed because the test path-list assumed a row existed: - Phantom result: "HTTP Hosts Allowed in Embedded Browser" (setting:browser:http-allowlist) is a custom block, not a SettingsCardRow, so it had no anchor — its search hit highlighted nothing. Tag the block with the explicit anchor; correct the test (it's not a .json row, so move it to the explicit-anchor list). - Integration entries (claude-code, claude-path, ripgrep-path, subagent-notifications, cursor, gemini) were filed under .account in search but their rows live in AutomationSection. Move the curated entries to .automation so the search result's section/icon matches the row's real home. - Five search titles didn't match their row labels: warn-before-closing -tab-x-button, hidden-webview-discard, hidden-webview-discard-delay, badge, import-hint. Align to the row text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second independent audit found two anchor collisions: the agent-hibernation entry's synonyms listed all three sub-paths (enabled/idleSeconds/maxLiveTerminals) and the search-engine entry listed the custom name/url paths, so every one of those always- or conditionally-rendered rows resolved to the SAME anchor id. Multiple rows then carried the same SwiftUI .id, making proxy.scrollTo ambiguous and the highlight land on the wrong row. Keep only the primary dotted path on each entry (agentHibernation.enabled, defaultSearchEngine); the sub-field rows intentionally carry no search anchor (the entry's hit lands on the primary row). Drop those sub-paths from the row-anchor test and add rowAnchorsAreUniqueAcrossRows so a future synonym that reintroduces a collision fails CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 audit: de-colliding agent-hibernation/search-engine by dropping their sub-paths left four real detail rows unsearchable (Hibernate After Idle Seconds, Max Live Agent Terminals, Custom Search Engine Name, Custom Search URL). Give each its own curated entry with a unique anchor so every row is independently searchable + highlightable with no shared .id. Re-add the four paths to the row-anchor test; the uniqueness guard confirms no collision. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 2, 2026
This was referenced Jun 11, 2026
Merged
This was referenced Jun 19, 2026
This was referenced Jun 26, 2026
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
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.
What
When you search in Settings and click a result, the navigated-to row should flash an accent-colored rounded border (the "find me" pulse). The legacy in-app settings window did this; the SPM reimplementation (
CmuxSettingsUI) kept the search-to-scroll path but dropped the highlight, and the rows were never anchored, so even the scroll-to-center quietly no-oped.How
Ports the legacy affordance into the package:
SettingsSearchHighlight.swiftaddsSettingsSearchHighlightState, its environment key, thesettingsSearchAnchor(s)view modifiers, and the pulsing-borderTimelineViewmodifier (same fade curve as legacy).SettingsSearchIndex.anchorID(forSettingsPath:)maps a row's dotted cmux.json path to the sidebar/search anchor id. The map is built automatically from the curated entries' dotted synonym tokens, so there's no second hand-maintained id table.SettingsCardRowresolves the path it already declares viaconfigurationReviewinto that anchor id, tags itself with.id(...), and pulses when it's the active target.SettingsWindowRootinjects the index and arms the highlight (anchor + token + start time) on every highlighting navigation request, mirroring legacyapplySettingsNavigation.No new keyboard shortcuts, no app-target changes; the behavior lives entirely in the settings package.
Test
swift testonCmuxSettingsUIcovers the new resolver: the "Show Branch + Directory in Sidebar" path resolves to its sidebar hit anchor, resolved anchors always correspond to a real indexed entry, and unknown paths resolve tonil.Visual one-to-one parity with the legacy highlight is verified by dogfooding a tagged build (search a query, click a hit, confirm the row scrolls into view and pulses the accent border).
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Restores the search-result highlight in
CmuxSettingsUIso clicking a result scrolls to and pulses the exact row or section. Also fixes the Settings window not reopening after close.Bug Fixes
cmux.jsonpaths to stable anchors; explicit anchors for action/picker/custom rows; fixed the "Sidebar Branch Layout" synonym; restored anchors for Terminal Config, Desktop Notifications, Browsing History, and Reset Palette; titles match row labels.artifacts/.Refactors
settingsSearchAnchor(_:); highlight state/index symbols are internal to avoid app-target clashes.SettingsRowAnchorResolutionTestswith anchor-uniqueness checks; extendedSettingsSearchIndexTests; added DEBUG-only ':all' search sentinel; lazy-loaded the keyboard-shortcuts list for faster open.Written for commit beb34cc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Content