Repository navigation
Conversation
|
@psh4607 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge — the resolver logic is straightforward, well-tested, and consistent with the existing Claude binary path pattern; no changes to search result correctness or data handling. The change is well-scoped: a new resolver with injected dependencies (easily unit-tested), clean delegation at two call sites, and settings plumbing that mirrors a proven pattern. The one nit — Sources/RipgrepResolver.swift — Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["ripgrepMatchingPaths() / ripgrepExecutable()"] --> B["RipgrepResolver.resolve()"]
B --> C{"automation.ripgrepBinaryPath\nconfigured?"}
C -- Yes --> D{"isExecutableRegularFile?"}
D -- Yes --> E["return custom path"]
D -- No --> F["log warning once\n(dedup tracker)"]
F --> G["check commonPaths list"]
C -- No --> G
G -- found --> H["return first match\n(Homebrew → MacPorts → system → nix-darwin)"]
G -- none found --> I{"PATH env lookup"}
I -- found --> J["return PATH match"]
I -- none found --> K["return nil\n(fall back to Foundation scan)"]
Reviews (4): Last reviewed commit: "review: nonisolated file-scope state + r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds ripgrep ( ChangesRipgrep Binary Path Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses ripgrep (rg) discovery failures on Nix-managed macOS installs by introducing a user-configurable automation.ripgrepBinaryPath setting and centralizing rg path resolution (including nix-darwin fallback locations) for Find/search features.
Changes:
- Add
automation.ripgrepBinaryPathto settings UI/config/schema and implement a sharedRipgrepResolverwith nix-darwin fallback paths. - Update Find/search call sites (
FileExplorerSearchController,SessionIndexStore) to use the shared resolver (and re-resolve after settings changes). - Includes additional unrelated changes: a new “Inherit Working Directory in New Workspaces” setting and a markdown cmd-click routing behavior change.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/data/cmux.schema.json | Adds JSON schema entry for automation.ripgrepBinaryPath. |
| Sources/SettingsSearchAliases.swift | Adds search alias for the new ripgrep setting (and an app setting). |
| Sources/SettingsNavigation.swift | Adds settings-path anchor IDs (including ripgrep) and an app setting entry. |
| Sources/SessionIndexStore.swift | Switches rg path resolution to the shared resolver and re-resolves per call. |
| Sources/RipgrepResolver.swift | New shared resolver honoring custom path, common paths (incl. nix), then $PATH. |
| Sources/KeyboardShortcutSettingsFileStore.swift | Adds parsing/template support for automation.ripgrepBinaryPath. |
| Sources/GhosttyTerminalView.swift | Adjusts cmd-click markdown URL routing and defers split creation asynchronously. |
| Sources/FileExplorerSearchController.swift | Delegates ripgrep executable discovery to RipgrepResolver. |
| Sources/cmuxApp.swift | Adds Settings UI for ripgrep binary path (and also a workspace CWD inherit toggle). |
| Resources/Localizable.xcstrings | Adds localized strings for new settings UI (and an app setting). |
| GhosttyTabs.xcodeproj/project.pbxproj | Wires new source/test files into Xcode project build phases. |
| cmuxTests/RipgrepResolverTests.swift | Adds unit tests covering resolver precedence and nix path inclusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/SettingsNavigation.swift`:
- Line 420: The mapping "automation.ripgrepBinaryPath" references settingID(for:
.automation, idSuffix: "ripgrep-path") but there is no corresponding entry in
the settingEntries collection, so the new setting won't be discoverable by
search or navigation; fix it by adding a settingEntries entry with idSuffix:
"ripgrep-path" (matching the same anchor/title used for the UI/anchor for
ripgrep binary path) so the SettingsSearchIndex/anchor and the mapping stay
consistent — update the settingEntries array/object where other automation
entries live to include the ripgrep-path entry using the same identifier strings
referenced by settingID(for:idSuffix:).
🪄 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: 34f7fc3c-4734-41ce-b55a-4d0f6ef78244
📒 Files selected for processing (12)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/FileExplorerSearchController.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/RipgrepResolver.swiftSources/SessionIndexStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/RipgrepResolverTests.swiftweb/data/cmux.schema.json
…w-ai#3657) Find ('cmd-F') was failing on nix-darwin macOS installs because rg lives in a nix profile path that wasn't in cmux's hardcoded fallback list, and Dock-launched GUI apps don't reliably inherit launchctl setenv PATH. Two complementary changes: 1. New automation.ripgrepBinaryPath setting (mirrors automation.claudeBinaryPath). Lets users on Nix, asdf, or other non-standard installs point cmux at their rg directly. Editable in Settings > Automation and via cmux.json. 2. Extended the fallback list to include common nix-darwin profile paths so single-install Nix users work out of the box: /etc/profiles/per-user/<user>/bin/rg, /run/current-system/sw/bin/rg, /nix/var/nix/profiles/default/bin/rg. Resolution order is preserved: existing Homebrew/MacPorts/system precedence is kept, then nix paths, then $PATH lookup. The shared RipgrepResolver re-evaluates on each call so a settings change takes effect without an app restart.
psh4607
force-pushed
the
fix/ripgrep-binary-path-3657
branch
from
May 7, 2026 14:01
e627a82 to
d4566c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/SettingsNavigation.swift (1)
423-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
settingEntriesitem forripgrep-path.
automation.ripgrepBinaryPathnow maps tosetting:automation:ripgrep-path, but there is still no matchingsetting(.automation, "ripgrep-path", ...)entry insettingEntries. This keeps the new setting non-discoverable/inconsistent in settings search/navigation metadata.Proposed fix
setting(.automation, "claude-code", String(localized: "settings.automation.claudeCode", defaultValue: "Claude Code Integration"), "agent hooks notifications"), setting(.automation, "claude-path", String(localized: "settings.automation.claudeCode.customPath", defaultValue: "Claude Binary Path"), "custom claude executable"), + setting(.automation, "ripgrep-path", String(localized: "settings.automation.ripgrep.customPath", defaultValue: "Ripgrep Binary Path"), "custom ripgrep rg executable nix"), setting(.automation, "cursor", String(localized: "settings.automation.cursor", defaultValue: "Cursor Integration"), "agent hooks notifications"),Based on learnings: “when adding Settings navigation/search wiring, keep anchor and SettingsSearchIndex entries consistent to avoid broken jump-to behavior.”
🤖 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/SettingsNavigation.swift` at line 423, The new mapping "automation.ripgrepBinaryPath" was added but there is no corresponding entry in settingEntries, so add a setting entry for ripgrep-path to the settingEntries collection: create a setting(.automation, "ripgrep-path", ...) (matching the same label/anchor used by settingID(for: .automation, idSuffix: "ripgrep-path")) so the setting becomes discoverable and consistent with SettingsSearchIndex/anchor logic; ensure the entry's identifier, display text, and anchor match the existing pattern used by other automation entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/RipgrepResolver.swift`:
- Around line 53-55: The resolver currently accepts a candidate when
FileManager.isExecutableFile(atPath: customPath) is true, but that can be a
directory; update the validation to also check that customPath is not a
directory before returning it. Use FileManager.fileExists(atPath:isDirectory:)
(or URL.resourceValues with .isDirectoryKey) to detect directories and only
return customPath from the RipgrepResolver (the branch that calls
fileManager.isExecutableFile(atPath:)) when it is executable AND !isDirectory;
otherwise fall through to the existing fallback logic.
---
Duplicate comments:
In `@Sources/SettingsNavigation.swift`:
- Line 423: The new mapping "automation.ripgrepBinaryPath" was added but there
is no corresponding entry in settingEntries, so add a setting entry for
ripgrep-path to the settingEntries collection: create a setting(.automation,
"ripgrep-path", ...) (matching the same label/anchor used by settingID(for:
.automation, idSuffix: "ripgrep-path")) so the setting becomes discoverable and
consistent with SettingsSearchIndex/anchor logic; ensure the entry's identifier,
display text, and anchor match the existing pattern used by other automation
entries.
🪄 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: 061c8d4a-e627-48c6-b2b9-59baec44057c
📒 Files selected for processing (11)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/FileExplorerSearchController.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/RipgrepResolver.swiftSources/SessionIndexStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/RipgrepResolverTests.swiftweb/data/cmux.schema.json
Splits applied to Sisyphus's manaflow-ai#3703 review pass: - SettingsNavigation: add missing settingEntries row for ripgrep-path so the new setting is discoverable via Settings search/navigation, not just the path-anchor lookup (CodeRabbit, Copilot). - Localizable.xcstrings: add settings.search.alias.setting.automation.ripgrep-path with en/ja/ko translations so non-English locales get the localized search tokens (Copilot). - RipgrepResolver: switch NSLog -> os.Logger (Greptile P1; project standard for diagnostic destinations) and dedupe the invalid-path warning per process so a stuck-misconfigured path can't spam the unified log on every search keystroke (Copilot). - SessionIndexStore: rename cachedRipgrepPath -> resolvedRipgrepPath since it's now a re-resolving computed property, not a cache (Copilot). - SettingsSearchIndexTests: add coverage for both the ripgrep alias-search path ('ripgrep nix', 'rg binary') and the path-anchor lookup so this discoverability gap can't silently regress.
Contributor
Author
|
Pushed
PR is now clean: 11 files, 1 fix commit + 1 review-fix commit, scoped to ripgrep only. |
…flow-ai#3657) Addresses Greptile P1 + CodeRabbit feedback on the second review pass: - nonisolated private let on the file-scoped Logger and InvalidPathLogTracker (project rule .github/review-bot-rules/swift-logging.md). Without it, RipgrepResolver.resolve() — a nonisolated synchronous function — would cross the MainActor boundary on every read once strict concurrency is fully enabled. - Reject directory matches in the resolution path. FileManager.isExecutableFile(atPath:) is access(X_OK), so it returns true for any directory with the search/execute bit set. Filter via fileExists(atPath:isDirectory:) so a misconfigured automation.ripgrepBinaryPath pointing at a directory falls through to the real binary instead of being accepted. - Regression test (testResolveRejectsDirectoryWithExecuteBit) locks in the directory-rejection behavior and documents the underlying Foundation quirk so it can't silently regress. Note on subsystem string: Greptile suggested Logging.subsystem, but that constant doesn't exist in the codebase yet — every Logger in cmux today uses a hardcoded subsystem string. Matching the existing CmuxEventLogWriter pattern ("com.cmuxterm.app") to avoid scope creep; introducing a Logging enum is a separate refactor.
Contributor
Author
|
Second review-fix pass pushed as
Note on Resolved 10 review threads from this and the previous pass. PR is now 3 commits, all addressed:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/RipgrepResolver.swift (1)
71-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard executable checks against directory paths.
FileManager.isExecutableFile(atPath:)may accept directories, so the resolver can return a directory asrg. That leads to launch failures and skips valid fallback binaries.Suggested patch
enum RipgrepResolver { + private static func isExecutableFilePath(_ path: String, fileManager: FileManager) -> Bool { + var isDirectory: ObjCBool = false + guard fileManager.fileExists(atPath: path, isDirectory: &isDirectory), + !isDirectory.boolValue else { + return false + } + return fileManager.isExecutableFile(atPath: path) + } + static func resolve( customPath: String? = RipgrepIntegrationSettings.customRipgrepPath(), commonPaths: [String] = RipgrepResolver.defaultCommonPaths(), environment: [String: String] = ProcessInfo.processInfo.environment, fileManager: FileManager = .default ) -> String? { if let customPath { - if fileManager.isExecutableFile(atPath: customPath) { + if isExecutableFilePath(customPath, fileManager: fileManager) { return customPath } // Configured-but-not-executable falls through to the common paths // so a stale/typo'd setting doesn't completely disable Find when a // valid binary still exists in a default location. @@ - for path in commonPaths where fileManager.isExecutableFile(atPath: path) { + for path in commonPaths where isExecutableFilePath(path, fileManager: fileManager) { return path } @@ - if fileManager.isExecutableFile(atPath: candidate) { + if isExecutableFilePath(candidate, fileManager: fileManager) { return candidate } } } return nilIn Apple Foundation, can FileManager.isExecutableFile(atPath:) return true for directories, and is pairing it with fileExists(atPath:isDirectory:) the recommended way to validate an executable file path?Also applies to: 84-85
🤖 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/RipgrepResolver.swift` around lines 71 - 74, The current check using fileManager.isExecutableFile(atPath: customPath) can return true for directories; update the customPath and fallback checks to also call fileManager.fileExists(atPath:isDirectory:) (using an UnsafeMutablePointer<ObjCBool> or inout Bool) to ensure the path exists AND isDirectory is false before returning it—i.e., require both isExecutableFile(atPath:) == true and isDirectory == false for customPath and the other executable-path checks in RipgrepResolver (the blocks around customPath and the fallback checks at the lines referenced).
🤖 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.
Duplicate comments:
In `@Sources/RipgrepResolver.swift`:
- Around line 71-74: The current check using
fileManager.isExecutableFile(atPath: customPath) can return true for
directories; update the customPath and fallback checks to also call
fileManager.fileExists(atPath:isDirectory:) (using an
UnsafeMutablePointer<ObjCBool> or inout Bool) to ensure the path exists AND
isDirectory is false before returning it—i.e., require both
isExecutableFile(atPath:) == true and isDirectory == false for customPath and
the other executable-path checks in RipgrepResolver (the blocks around
customPath and the fallback checks at the lines referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60ec2599-2c89-45e1-a1a2-26826d603b1f
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/RipgrepResolver.swiftSources/SessionIndexStore.swiftSources/SettingsNavigation.swiftcmuxTests/SettingsSearchIndexTests.swift
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
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.
Closes #3657.
Summary
cmux's project-search ("Find") was failing on Nix-managed macOS installs because
rglives in a nix profile path that wasn't in cmux's hardcoded fallback list, and Dock-launched macOS apps don't reliably inheritlaunchctl setenv PATH. Picked up after the reporter mentioned PR intent.Changes
Two complementary fixes per the issue:
New
automation.ripgrepBinaryPathsetting — mirrors the existingautomation.claudeBinaryPath. Editable fromSettings > Automationand from~/.config/cmux/cmux.json. Highest-leverage fix for non-standard installs (Nix, asdf, custom locations).Extended hardcoded fallback list with common nix-darwin profile paths so single-install Nix users work out of the box without touching settings:
/etc/profiles/per-user/$USER/bin/rg/run/current-system/sw/bin/rg/nix/var/nix/profiles/default/bin/rgResolution order
RipgrepResolver.resolve()(new shared helper used by bothFileExplorerSearchControllerandSessionIndexStore) checks in order:automation.ripgrepBinaryPathsetting, if configured and executable\$PATHlookup (best-effort)Existing Homebrew/MacPorts/system precedence is preserved so users with multiple installs aren't silently switched to a different
rg. A configured-but-not-executable custom path logs and falls through to the fallback list rather than hard-failing — a stale typo'd setting shouldn't completely break Find when a valid binary still exists in a default location.The resolver re-evaluates on every call (replacing
SessionIndexStore.cachedRipgrepPathwhich was astatic letthat locked to first resolution), so a settings change takes effect immediately without an app restart.Files
Sources/RipgrepResolver.swift— shared resolver +RipgrepIntegrationSettingsaccessorscmuxTests/RipgrepResolverTests.swift— 8 unit tests covering custom path / fallback / env / nix path inclusion / Homebrew precedence / setting trim semanticsSources/FileExplorerSearchController.swift,Sources/SessionIndexStore.swift— both call sites now delegate toRipgrepResolver.resolve()Sources/cmuxApp.swift— newSettingsCardrow,@AppStorage, reset bindingSources/KeyboardShortcutSettingsFileStore.swift— JSON parser, supported settings paths, default templateSources/SettingsNavigation.swift,Sources/SettingsSearchAliases.swift— settings discovery / searchweb/data/cmux.schema.json— schema entryResources/Localizable.xcstrings— en/ja/ko translations for the 3 new UI stringsVerification
xcodebuild -scheme cmux-unit -only-testing:cmuxTests/RipgrepResolverTests— 8/8 passing locally (covers custom path priority, fallthrough on bad path, common-paths fallback, $PATH fallback, nil terminus, nix path inclusion, Homebrew precedence preservation, setting trim/empty handling)./scripts/reload.sh --tag fix-ripgrep-binary-path-3657— Debug app builds cleanNotes for reviewers
customPath/commonPaths/environment/fileManageroverrides.automation.ripgrepBinaryPath).Summary by cubic
Adds a new
automation.ripgrepBinaryPathsetting and a sharedRipgrepResolverwith nix-darwin fallback paths so Find works on Nix-managed macOS (fixes #3657). Tightens resolution to accept only executable files, dedupes invalid-path logging, and re-resolves on each call.New Features
automation.ripgrepBinaryPath(Settings > Automation and~/.config/cmux/cmux.json), searchable via Settings (en/ja/ko aliases).RipgrepResolverused by Find: setting → common paths (Homebrew/MacPorts/system → nix-darwin) →$PATH, re-evaluated on each call.Bug Fixes
os.Loggeronce per process and falls back to common locations/$PATHinstead of failing.Written for commit ffe8842. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior
Localization
Tests