feat: add Quick Open file search (Cmd+Shift+O) to command palette#5982
Open
tctony wants to merge 1 commit into
Open
feat: add Quick Open file search (Cmd+Shift+O) to command palette#5982tctony wants to merge 1 commit into
tctony wants to merge 1 commit into
Conversation
|
@tctony is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to 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:
📝 WalkthroughWalkthroughAdds a Quick Open file-search mode to the command palette, triggered by Cmd+Shift+O and the ChangesQuick Open File Search
Sequence Diagram(s)sequenceDiagram
participant User
participant AppDelegate
participant NotificationCenter
participant ContentView
participant CommandPaletteQuickOpenFileSearch
participant NSWorkspace
rect rgba(100, 149, 237, 0.5)
note over User, AppDelegate: Trigger
User->>AppDelegate: Cmd+Shift+O keystroke
AppDelegate->>NotificationCenter: post commandPaletteFileSearchRequested<br/>(markPending: true)
end
rect rgba(144, 238, 144, 0.5)
note over NotificationCenter, ContentView: Palette open
NotificationCenter->>ContentView: commandPaletteFileSearchRequested
ContentView->>ContentView: openCommandPaletteFileSearch()<br/>→ handleCommandPaletteListRequest(scope: .fileSearch)<br/>→ set query to "@"
end
rect rgba(255, 200, 100, 0.5)
note over ContentView, CommandPaletteQuickOpenFileSearch: Search execution
ContentView->>CommandPaletteQuickOpenFileSearch: resolve(matchingQuery, workspaceRoot)
CommandPaletteQuickOpenFileSearch-->>ContentView: ResolvedPath (currentDir, searchTerm, isPathMode)
alt Path mode
ContentView->>CommandPaletteQuickOpenFileSearch: listFiles(inDirectory:, maxCount:)
CommandPaletteQuickOpenFileSearch-->>ContentView: [URL] directory listing
else Cross-directory mode
ContentView->>CommandPaletteQuickOpenFileSearch: searchCrossDirectory(query, rootDir)
CommandPaletteQuickOpenFileSearch-->>ContentView: [ScoredFile] ranked
end
end
rect rgba(220, 130, 220, 0.5)
note over ContentView, NSWorkspace: File open
ContentView->>CommandPaletteQuickOpenFileSearch: openAction(for: url)
CommandPaletteQuickOpenFileSearch-->>ContentView: QuickOpenFileOpenAction
ContentView->>NSWorkspace: open / reveal / openWith textEditor
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (14 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 |
Contributor
Greptile SummaryThis PR implements a Quick Open file search (Cmd+Shift+O) for the command palette, activated via the
Confidence Score: 3/5Not safe to merge as-is: the iOS Package.resolved was accidentally replaced with a completely different lockfile that adds 11 unrelated SDKs, and all new user-facing strings are missing translations for 16 of 19 supported locales. Two issues must be resolved before merging. The iOS lockfile replacement is a concrete build-graph corruption: it adds posthog-ios, sentry-cocoa, sparkle, and other large SDKs that have no connection to this feature, and downgrades swift-asn1 from 1.7.0 to 1.6.0. The localization gap means Quick Open UI text will appear in English for users of 16 supported languages the moment this ships. ios/cmuxPackage/Package.resolved (needs full revert to pre-PR state) and Resources/Localizable.xcstrings (needs translations for zh-Hant, ko, de, es, fr, it, da, pl, ru, bs, ar, nb, pt-BR, th, tr, uk). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Cmd+Shift+O / Menu"] --> B["NSNotification: commandPaletteFileSearchRequested"]
B --> C["ContentView.openCommandPaletteFileSearch()"]
C --> D["handleCommandPaletteListRequest(scope: .fileSearch)"]
D --> E["commandPaletteQuery = '@'"]
E --> F["commandPaletteListScope(for: query) == .fileSearch"]
F --> G["commandPaletteFileSearchResolve()"]
G --> H{isPathMode?}
H -->|"@/, @~, @./"| I["commandPaletteFileSearchPathEntries()\nSync: FileManager.contentsOfDirectory\n@MainActor"]
H -->|"@query"| J["Cross-directory BFS\nTask(priority: .userInitiated)\nwithTaskGroup"]
J --> K["searchCrossDirectoryBranch()\nBFS + fuzzyMatch\nnonisolated async"]
K --> L["Top-30 scored results\nMainActor.run → update UI"]
I --> M["CommandPaletteCommand list\nNucleo fuzzy scoring"]
M --> N["User selects file"]
L --> N
N --> O["openFileInDefaultEditor(url)\nTask → openAction(for:)\nisTextFile / UTType check"]
O --> P{Action}
P -->|script| Q["Open in text editor"]
P -->|binary| R["Reveal in Finder"]
P -->|regular| S["NSWorkspace.shared.open()"]
Reviews (8): Last reviewed commit: "feat: add Quick Open file search to comm..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+5630
to
+5636
| let sq = Self.commandPaletteFileSearchMatchingTerm(capturedMatchingQuery) | ||
| let rootDir = capturedFSRoot ?? NSHomeDirectory() | ||
| Self.fileSearchGeneration &+= 1 | ||
| let gen = Self.fileSearchGeneration | ||
| let scored = Self.searchCrossDirectory(query: sq, rootDir: rootDir, shouldCancel: { Self.fileSearchGeneration != gen }) | ||
| await MainActor.run { | ||
| guard commandPaletteSearchRequestID == requestID, |
Contributor
There was a problem hiding this comment.
fileSearchGeneration static var
fileSearchGeneration is a private static var on a @MainActor-bound View struct. It is written (&+= 1) and read inside a Task.detached block, which inherits no actor isolation and runs on the cooperative thread pool. The shouldCancel closure that captures it is then passed into DispatchQueue.concurrentPerform, where multiple concurrent threads read the same value without any synchronization. In Swift 6 strict-concurrency mode, accessing a @MainActor-isolated static from a non-isolated detached task is a data-isolation violation, and at runtime it is a plain data race.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
| case .newBrowserWorkspace: | ||
| return String(localized: "shortcut.newBrowserWorkspace.label", defaultValue: "New Browser Workspace") | ||
| case .openFolder: return "Open Folder" | ||
| case .quickOpenFile: return "Quick Open…" |
Contributor
There was a problem hiding this comment.
quickOpenFile is a hardcoded string literal instead of routing through String(localized:defaultValue:) like the surrounding cases. The string catalog (shortcut.quickOpenFile.label) with zh-Hans translation exists in Localizable.xcstrings but is never referenced by this call site, so the label will always appear in English.
Suggested change
| case .quickOpenFile: return "Quick Open…" | |
| case .quickOpenFile: return String(localized: "shortcut.quickOpenFile.label", defaultValue: "Quick Open…") |
Rule Used: Flag production user-facing text that is not fully... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+6168
to
+6174
| let sorted = contents.prefix(maxCount).sorted { a, b in | ||
| let aIsDir = (try? a.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false | ||
| let bIsDir = (try? b.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false | ||
| if aIsDir != bIsDir { return aIsDir } | ||
| return a.lastPathComponent.localizedCaseInsensitiveCompare(b.lastPathComponent) == .orderedAscending | ||
| } | ||
| return Array(sorted) |
Contributor
There was a problem hiding this comment.
contents.prefix(maxCount) is applied before the sort, so for directories with more than 1 000 entries the function takes an arbitrary slice of up to 1 000 URLs (in filesystem-enumeration order) and only then sorts them alphabetically. Entries whose names start later in the alphabet can be silently excluded even if they should appear in the sorted top-1 000. Sorting the full set first, then capping, gives deterministic, alphabetically-correct results.
Suggested change
| let sorted = contents.prefix(maxCount).sorted { a, b in | |
| let aIsDir = (try? a.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false | |
| let bIsDir = (try? b.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false | |
| if aIsDir != bIsDir { return aIsDir } | |
| return a.lastPathComponent.localizedCaseInsensitiveCompare(b.lastPathComponent) == .orderedAscending | |
| } | |
| return Array(sorted) | |
| let sorted = contents.sorted { a, b in | |
| let aIsDir = (try? a.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false | |
| let bIsDir = (try? b.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false | |
| if aIsDir != bIsDir { return aIsDir } | |
| return a.lastPathComponent.localizedCaseInsensitiveCompare(b.lastPathComponent) == .orderedAscending | |
| } | |
| return Array(sorted.prefix(maxCount)) |
Rule Used: Flag production code that adds nested full-collect... (source)
Comment on lines
+6212
to
+6220
| enumerator.skipDescendants() | ||
| continue | ||
| } | ||
| results.append(url) | ||
| } | ||
| return results | ||
| } | ||
|
|
||
| static func shouldSkipDirectoryForQuickOpen(_ name: String) -> Bool { |
Contributor
There was a problem hiding this comment.
isTextFile(at:) opens a FileHandle and reads up to 4 KB synchronously. It is called from openFileInDefaultEditor, which — as a static func on a @MainActor View — runs on the main thread when the user activates a result. On a slow or network-mounted filesystem, this blocks the main actor and stalls the UI. The read should be performed asynchronously (e.g., via Task { await ... }) before the open call, consistent with the cmux rule against expensive synchronous disk loads on interactive paths.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
Comment on lines
+5557
to
+5570
| ) { | ||
| let matches = CommandPaletteSearchOrchestrator.resolvedSearchMatches( | ||
| searchIndex: searchIndex, | ||
| searchCorpus: searchCorpus, | ||
| searchCorpusByID: searchCorpusByID, | ||
| query: matchingQuery, | ||
| query: nucleoQuery, | ||
| usageHistory: usageHistory, | ||
| queryIsEmpty: queryIsEmpty, | ||
| historyTimestamp: historyTimestamp, | ||
| additionalScoreBoost: additionalScoreBoost | ||
| additionalScoreBoost: additionalScoreBoost, | ||
| resultLimit: scope == .fileSearch ? 30 : nil | ||
| ) | ||
| #if DEBUG | ||
| if scope == .fileSearch { |
Contributor
There was a problem hiding this comment.
#if DEBUG blocks with no-op loop bodies
There are two #if DEBUG blocks that iterate over search matches but do nothing with the captured value (let entry = searchCorpusByID[m.commandID] is unused, and the second block has an entirely empty if body). These appear to be unfinished debug tracing that was accidentally left in. They should either be completed with a proper cmuxDebugLog call or removed to avoid compiler warnings and reader confusion.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@cmuxTests/CommandPaletteQuickOpenFileSearchTests.swift`:
- Around line 13-466: The test file CommandPaletteQuickOpenFileSearchTests.swift
(containing the QuickOpenFileSearchScopeTests XCTestCase and related tests like
testCommandsScopeWithPrefix, testFileSearchScopeWithPrefix, etc.) is incorrectly
added to the cmuxUITests target; move it into the cmuxTests unit-test target by
updating the Xcode project: add or correct the PBXFileReference for
CommandPaletteQuickOpenFileSearchTests.swift under the project, and ensure it is
listed in the PBXSourcesBuildPhase of the cmuxTests target (remove it from
cmuxUITests sources if present) so the tests compile and run as part of
cmuxTests.
In `@docs/quick-open-file.md`:
- Around line 241-265: The test count in the header is wrong: reconcile the
"单元测试（45 个）" summary with the actual listed tests in
QuickOpenFileSearchScopeTests by either adding the two missing entries in the
"Query 提取" subsection and the one missing entry in "Nucleo 搜索词提取" (include their
test names like the other bullets) or change the header count to match the
current total (e.g., 42); update the section title and any other summary counts
to keep them consistent with the enumerated test names (refer to test names such
as testCommandsScopeWithPrefix, testFileSearchScopeWithPrefix,
testFileSearchMatchingQueryEmpty, testMatchingTermEmpty to locate the related
lists).
- Around line 187-193: The dedup fingerprint currently uses
commandPaletteFileSearchDedupFingerprint(query:isCrossDirectory:) which for
cross-directory searches falls back to
commandPaletteFileSearchFingerprint(query:) and omits
resolvedFileSearchWorkspaceRoot (from
tabManager.selectedWorkspace?.currentDirectory), causing identical queries in
different workspaces to be deduplicated incorrectly; update
commandPaletteFileSearchDedupFingerprint to include the workspace root/current
directory (and the isCrossDirectory flag if not already) when computing
fileSearchDedupFingerprint so that searches from different windows/workspaces
produce distinct fingerprints and do not return early in the
BFS/searchCrossDirectory flow.
In `@Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swift`:
- Line 285: The new hard-coded user-facing label for the ShortcutAction enum
case quickOpenFile bypasses localization; replace the literal "Quick Open…" with
a localized string using String(localized: "shortcut.action.quickOpenFile",
defaultValue: "Quick Open…") (or similar key) so the UI uses the strings
catalog; update the return in the ShortcutAction .quickOpenFile branch and add
the corresponding key/value to the Localizable strings files.
In `@Resources/Localizable.xcstrings`:
- Around line 58560-58610: Add Japanese ("ja") localization blocks for each new
key so they match the structure used by "en" and "zh-Hans"; specifically add a
"ja" -> "stringUnit" -> {"state":"translated","value": "<Japanese translation>"}
entry for commandPalette.kind.directory, commandPalette.kind.file,
commandPalette.kind.openInFinder, commandPalette.search.fileSearchEmpty,
commandPalette.search.fileSearchPlaceholder, menu.file.quickOpenFile, and
shortcut.quickOpenFile.label (and repeat the same for the other ranges noted).
Use the English value as the source string and supply the correct Japanese
translation for the "value" field, preserving the exact JSON/xcstrings structure
and "state":"translated" flag.
In `@Sources/ContentView.swift`:
- Around line 5197-5224: In commandPaletteFileSearchMatchingTerm(_:), the
trimmed.hasPrefix("./") branch currently returns term unconditionally which
causes workspace-relative paths that resolve to directories (e.g. @./Sources) to
be treated as fuzzy search terms instead of browsing into that directory; change
the logic in the trimmed.hasPrefix("./") branch to mirror the ~ and
absolute-path handling: expand the ./ path against the workspace/current
directory (reuse the resolver’s remainder/current-dir result or use
FileManager.default.currentDirectoryPath + String(trimmed.dropFirst(2))), check
FileManager.default.fileExists(atPath:isDirectory:) and if it is a directory
return "" (so the UI will browse) otherwise return term. Ensure you reference
commandPaletteFileSearchMatchingTerm and the trimmed.hasPrefix("./") branch when
making the change.
- Around line 5877-6400: The ContentView.swift file contains the entire Quick
Open engine (filesystem traversal, scoring, path resolution, and file-type
dispatch) which should be extracted into a dedicated module file; move the pure
helper functions, types, and constants into a new Swift source (e.g.
QuickOpenEngine.swift) and leave only the palette state/wiring in ContentView.
Specifically, extract: ScoredFile, fastQuit* constants, searchCrossDirectory,
fileSearchCrossDirectoryFuzzyScore, fileSearchCrossDirectoryFuzzyMatch,
quickOpenRelativePath, insertTopK, shouldSkipDirectoryForQuickOpen,
listFilesRecursively, listFilesInDirectory, resolveSymlinkTarget, isSymlink,
isDirectory, isTextFile, openFileInDefaultEditor,
commandPaletteFileSearchPathForDirectory, commandPaletteFileSearchDotEntry,
resolveLongestExistingDirectory, commandPaletteFileSearchResolve,
commandPaletteFileSearchPathEntries, commandPaletteFileSearchFingerprint and
dedupFingerprint; make them internal/public as needed, add required imports
(Foundation/AppKit/UniformTypeIdentifiers), update ContentView to call into the
new file for these symbols, and run tests to ensure access levels and symbol
names remain identical to avoid breaking references.
- Around line 5503-5509: The early-return dedup uses
Self.fileSearchDedupFingerprint and never clears it when leaving cross-directory
path mode; update the logic around
Self.commandPaletteFileSearchDedupFingerprint(effectiveQuery:isCrossDirectory:)
so that if fsIsCrossDirectory is false (we left cross-directory mode) you clear
or reset Self.fileSearchDedupFingerprint before comparing/assigning the new fp
(or detect a mode change and nil the token), ensuring old cross-directory
fingerprints do not short-circuit subsequent non-cross-directory queries.
- Around line 5931-5938: The click handler flips to fuzzy cross-directory mode
when matchingQuery is empty because usePrefix is set to !matchingQuery.isEmpty;
to keep initial @ folder clicks in path mode, always enable the path prefix when
drilling into a directory. Update the action in ContentView so usePrefix is true
(or computed to be true when coming from the initial @ listing) before calling
Self.commandPaletteFileSearchPathForDirectory(url, rootDir: rootDir,
usePathPrefix: usePrefix) and assigning commandPaletteQuery using
Self.commandPaletteFileSearchPrefix + newPath; this preserves path-mode parsing
in commandPaletteFileSearchResolve.
🪄 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: b2e09f48-94da-4c57-a94f-deaf021ee6d4
📒 Files selected for processing (16)
Examples/SampleSidebarExtensionApp/SampleSidebarExtensionApp.xcodeproj/xcshareddata/xcschemes/CMUXExtKitSampleSidebarApp.xcschemeExamples/SampleSidebarExtensionApp/SampleSidebarExtensionApp.xcodeproj/xcshareddata/xcschemes/SampleSidebarExtensionApp.xcschemePackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CommandPalette/CommandPaletteSearchOrchestrator.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmux.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedcmuxTests/CommandPaletteQuickOpenFileSearchTests.swiftdocs/quick-open-file.mdios/cmuxPackage/Package.resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/quick-open-file.md (1)
80-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLabel this pseudo-code fence.
This bare fenced block will keep markdownlint
MD040failing. Add an explicit language tag (for example,text) here; prose/table examples can stay as-is.🤖 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 `@docs/quick-open-file.md` around lines 80 - 103, The fenced pseudo-code block is unlabeled which triggers markdownlint MD040; update the opening fence for the block that begins with the BFS pseudocode (the triple backticks before "调用任务: BFS(root, depth=0) ...") to include an explicit language tag (for example change ``` to ```text) so the block is treated as plain text/pseudocode and MD040 is satisfied.Source: Linters/SAST tools
🤖 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 `@docs/quick-open-file.md`:
- Line 258: Update the example in testFileSearchMatchingQueryEmpty to make the
trailing-space case explicit: instead of the ambiguous backtick span showing `@
`, spell the space visibly (e.g., `@␠` or `@·`) so the pair reads `@`, `@␠` →
`""`, which avoids the MD038 lint warning and clearly distinguishes empty vs
space.
In `@Sources/CommandPaletteQuickOpenFileSearch.swift`:
- Around line 343-350: The BFS can loop on symlinked directories; before
enqueueing a directory in the walk (the branch around isDir and queue.append),
detect and avoid symlink cycles by resolving and deduplicating real paths: when
isDir is true, compute a canonicalPath via url.resolvingSymlinksInPath().path
(or by reading .isSymbolicLinkKey and resolving) and maintain a Set<String>
visitedRealPaths; if canonicalPath is already in visitedRealPaths skip
enqueueing, otherwise insert canonicalPath and append the directory (or its
resolved URL) to queue; this prevents re-entering symlinked subtrees while
preserving existing checks like shouldSkipDirectoryForQuickOpen,
fileSearchCrossDirectoryFuzzyScore, insertTopK and use of localHeap.
- Around line 295-305: The current withTaskGroup launches one
searchCrossDirectoryBranch task per entry in topDirs, which can create unbounded
concurrent BFS scans; change this to a bounded worker pool or chunked submission
so at most N directory searches run concurrently (e.g., create a fixed number of
worker tasks that pull from topDirs or iterate topDirs in chunks) and have each
worker call searchCrossDirectoryBranch(query:rootDir:rootDirectory:threshold:),
forwarding results into the same aggregation; ensure cancellation still
propagates to the workers and choose a reasonable concurrency limit constant (or
configurable parameter) instead of spawning a task for every top-level
directory.
- Around line 9-39: The helper commandPaletteFileSearchMatchingTerm incorrectly
uses FileManager.default.currentDirectoryPath for "./" checks; change it to use
the workspace root used by commandPaletteFileSearchResolve so "./foo" is
resolved workspace-relative. Concretely: update
commandPaletteFileSearchMatchingTerm to accept (or obtain) the same
workspaceRoot string used by commandPaletteFileSearchResolve and replace
FileManager.default.currentDirectoryPath with that workspaceRoot when computing
expanded for the "./" branch; keep the rest of the directory-exists logic the
same so workspace paths like "./Sources" are classified correctly.
---
Outside diff comments:
In `@docs/quick-open-file.md`:
- Around line 80-103: The fenced pseudo-code block is unlabeled which triggers
markdownlint MD040; update the opening fence for the block that begins with the
BFS pseudocode (the triple backticks before "调用任务: BFS(root, depth=0) ...") to
include an explicit language tag (for example change ``` to ```text) so the
block is treated as plain text/pseudocode and MD040 is satisfied.
🪄 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: 56eed6ea-86f4-4bff-84a8-8ee2a75ab9ea
📒 Files selected for processing (7)
Packages/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftResources/Localizable.xcstringsSources/CommandPaletteQuickOpenFileSearch.swiftSources/ContentView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CommandPaletteQuickOpenFileSearchTests.swiftdocs/quick-open-file.md
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 (2)
Sources/CommandPaletteQuickOpenFileSearch.swift (2)
249-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrecompute directory metadata before sorting.
The comparator fetches
.isDirectoryKeyon every comparison. On large folders this becomes many repeated filesystem lookups in an interactive path. ComputeisDirectoryonce per entry, then sort the cached tuples.🔧 Suggested fix
- let sorted = contents.sorted { a, b in - let aIsDir = (try? a.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false - let bIsDir = (try? b.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false - if aIsDir != bIsDir { return aIsDir } - return a.lastPathComponent.localizedCaseInsensitiveCompare(b.lastPathComponent) == .orderedAscending - } - return Array(sorted.prefix(maxCount)) + let entries = contents.map { url in + ( + url: url, + isDirectory: (try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) ?? false + ) + } + let sorted = entries.sorted { a, b in + if a.isDirectory != b.isDirectory { return a.isDirectory } + return a.url.lastPathComponent.localizedCaseInsensitiveCompare(b.url.lastPathComponent) == .orderedAscending + } + return Array(sorted.prefix(maxCount).map(\.url))As per coding guidelines, production code over scalable user data should avoid repeated sort/filter/map work in hot paths and other unbounded hot-path overhead.
🤖 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/CommandPaletteQuickOpenFileSearch.swift` around lines 249 - 255, The comparator in the sorting closure repeatedly performs filesystem lookups for .isDirectoryKey (inside the sorted closure over contents), causing many redundant I/O ops; fix by precomputing the directory flag once per entry (e.g., map contents to an array of tuples (url, isDirectory) using try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory ?? false), perform the sort on that cached tuple array (comparing the cached isDirectory and lastPathComponent), then return the first maxCount URLs from the sorted tuples; update the code around the sorted = contents.sorted { … } block to implement this precompute-and-sort approach using the existing variable names (contents, sorted, maxCount).Source: Coding guidelines
158-197:⚠️ Potential issue | 🟠 MajorFix directory handling in quick-open + avoid repeated resourceValues during directory sort
quickOpenFileOpenAction(for:)evaluates script/binary heuristics before any directory check, so a directory named likeScripts.pycan be misrouted to.textEditorinstead of.open. Add an earlyisDirectoryguard.🔧 Suggested fix
nonisolated static func quickOpenFileOpenAction(for url: URL) async -> QuickOpenFileOpenAction { let resolvedURL = url.resolvingSymlinksInPath()
if isDirectory(resolvedURL) {return .open(resolvedURL)} let utType = UTType(filenameExtension: resolvedURL.pathExtension) let resourceValues = try? resolvedURL.resourceValues( forKeys: [.isExecutableKey, .isSymbolicLinkKey] )</details>
listFilesInDirectory(_:)sorts with a comparator that callsresourceValues(forKeys: [.isDirectoryKey])for every comparison; precomputeisDirectoryonce per URL before sorting to avoid repeated metadata work in the sort comparator.- Add a regression test asserting that directories (including those with script-like extensions) produce
.open.🤖 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/CommandPaletteQuickOpenFileSearch.swift` around lines 158 - 197, quickOpenFileOpenAction(for:) currently applies script/binary heuristics before checking for directories, so files like "Scripts.py" that are actually directories can be misclassified; update quickOpenFileOpenAction(for:) to immediately resolve resourceValues(forKeys: [.isDirectoryKey]) and return .open(resolvedURL) if isDirectory is true (do this before the isScript/isKnownBinary checks), and avoid re-calling resourceValues later. In listFilesInDirectory(_:) stop calling resourceValues(forKeys: [.isDirectoryKey]) inside the sort comparator; instead map the URL list to a temporary array of (url, isDirectory: Bool) by fetching resourceValues once per URL, sort that array using the precomputed isDirectory flags, then extract sorted URLs. Add a regression test asserting directories (including names with script-like extensions) are returned as .open by quickOpenFileOpenAction(for:).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/CommandPaletteQuickOpenFileSearch.swift`:
- Around line 249-255: The comparator in the sorting closure repeatedly performs
filesystem lookups for .isDirectoryKey (inside the sorted closure over
contents), causing many redundant I/O ops; fix by precomputing the directory
flag once per entry (e.g., map contents to an array of tuples (url, isDirectory)
using try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory ?? false),
perform the sort on that cached tuple array (comparing the cached isDirectory
and lastPathComponent), then return the first maxCount URLs from the sorted
tuples; update the code around the sorted = contents.sorted { … } block to
implement this precompute-and-sort approach using the existing variable names
(contents, sorted, maxCount).
- Around line 158-197: quickOpenFileOpenAction(for:) currently applies
script/binary heuristics before checking for directories, so files like
"Scripts.py" that are actually directories can be misclassified; update
quickOpenFileOpenAction(for:) to immediately resolve resourceValues(forKeys:
[.isDirectoryKey]) and return .open(resolvedURL) if isDirectory is true (do this
before the isScript/isKnownBinary checks), and avoid re-calling resourceValues
later. In listFilesInDirectory(_:) stop calling resourceValues(forKeys:
[.isDirectoryKey]) inside the sort comparator; instead map the URL list to a
temporary array of (url, isDirectory: Bool) by fetching resourceValues once per
URL, sort that array using the precomputed isDirectory flags, then extract
sorted URLs. Add a regression test asserting directories (including names with
script-like extensions) are returned as .open by quickOpenFileOpenAction(for:).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: efef77bc-203f-472c-8d95-2779159e919f
📒 Files selected for processing (4)
Sources/CommandPaletteQuickOpenFileSearch.swiftSources/ContentView.swiftcmuxTests/CommandPaletteQuickOpenFileSearchTests.swiftdocs/quick-open-file.md
Comment on lines
5570
to
+5620
| return CommandPaletteSwitcherFingerprintContext.fingerprint(windowContexts: fingerprintContexts) | ||
| } | ||
|
|
||
| // MARK: - File Search (Quick Open) | ||
|
|
||
| private func commandPaletteFileSearchFingerprint() -> Int { | ||
| Self.commandPaletteFileSearchFingerprint(query: commandPaletteQuery) | ||
| } | ||
|
|
||
| /// Path mode: single-directory listing. Called directly from | ||
| /// refreshCommandPaletteSearchCorpus. | ||
| private func commandPaletteFileSearchPathEntries( | ||
| matchingQuery: String, currentDir: String | ||
| ) -> [CommandPaletteCommand] { | ||
| guard let rootDir = resolvedFileSearchWorkspaceRoot, !rootDir.isEmpty else { return [] } | ||
| let resolvedDir = currentDir | ||
| let fileManager = FileManager.default | ||
| guard fileManager.fileExists(atPath: resolvedDir) else { return [] } | ||
|
|
||
| var entries: [CommandPaletteCommand] = [] | ||
| if resolvedDir != rootDir { | ||
| entries.append(Self.commandPaletteFileSearchDotEntry(currentDir: resolvedDir)) | ||
| } | ||
| let nucleoTerm = Self.commandPaletteFileSearchMatchingTerm( | ||
| matchingQuery, | ||
| workspaceRoot: rootDir | ||
| ) | ||
| let fileURLs = Self.listFilesInDirectory(resolvedDir, maxCount: 1000) | ||
| .filter { nucleoTerm.isEmpty ? !$0.lastPathComponent.hasPrefix(".") : true } | ||
| for url in fileURLs { | ||
| let isDir = Self.isDirectory(url) | ||
| let name = url.lastPathComponent | ||
| entries.append(CommandPaletteCommand( | ||
| id: "file.quickopen.\(url.absoluteString.hashValue)", | ||
| rank: isDir ? 0 : 1, | ||
| title: name, subtitle: "", | ||
| shortcutHint: nil, | ||
| kindLabel: isDir | ||
| ? String(localized: "commandPalette.kind.directory", defaultValue: "Directory") | ||
| : String(localized: "commandPalette.kind.file", defaultValue: "File"), | ||
| keywords: [], | ||
| dismissOnRun: !isDir, | ||
| action: { | ||
| if isDir { | ||
| let newPath = Self.commandPaletteFileSearchPathForDirectory( | ||
| url, rootDir: rootDir, usePathPrefix: true | ||
| ) | ||
| self.commandPaletteQuery = Self.commandPaletteFileSearchPrefix + newPath | ||
| } else { | ||
| Self.openFileInDefaultEditor(url) | ||
| } |
Contributor
There was a problem hiding this comment.
@MainActor on every keystroke
commandPaletteFileSearchPathEntries is a regular instance method on ContentView (implicitly @MainActor), and calls Self.listFilesInDirectory(resolvedDir, maxCount: 1000) synchronously. listFilesInDirectory issues a FileManager.contentsOfDirectory syscall followed by up to 1000 resourceValues calls and a sort — all on the main thread. This runs on every keystroke change in path mode. On a network-mounted volume or a slow disk, a single keystroke can stall the main run loop for hundreds of milliseconds.
Similarly, commandPaletteFileSearchResolve — called two-to-four times per keystroke — invokes resolveLongestExistingDirectory, which walks path components with synchronous fileExists(atPath:isDirectory:) calls on the main actor.
The path-listing work should be moved into the existing Task(priority: .userInitiated) search task and deliver results via a MainActor.run callback, consistent with how the cross-directory BFS path is already handled.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteQuickOpenFileSearchTests.swift (1)
331-334:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid process-global CWD mutation in tests (parallel flake risk).
Line 331-334 changes the process working directory globally. That can race with other tests and make failures nondeterministic. Use
workspaceRoot:inmatchingTerminstead so the test is self-contained.Suggested fix
- let previousDirectory = FileManager.default.currentDirectoryPath - XCTAssertTrue(FileManager.default.changeCurrentDirectoryPath(root.path)) - defer { FileManager.default.changeCurrentDirectoryPath(previousDirectory) } - let dotTestDirectory = root.appendingPathComponent(".test", isDirectory: true) let dotGitPath = root.appendingPathComponent(".git", isDirectory: true) try FileManager.default.createDirectory(at: dotTestDirectory, withIntermediateDirectories: true) try FileManager.default.createDirectory(at: dotGitPath, withIntermediateDirectories: true) - XCTAssertEqual(CommandPaletteQuickOpenFileSearch.matchingTerm("./.test"), "") - XCTAssertEqual(CommandPaletteQuickOpenFileSearch.matchingTerm("./.git"), "") + XCTAssertEqual( + CommandPaletteQuickOpenFileSearch.matchingTerm("./.test", workspaceRoot: root.path), + "" + ) + XCTAssertEqual( + CommandPaletteQuickOpenFileSearch.matchingTerm("./.git", workspaceRoot: root.path), + "" + ) try FileManager.default.removeItem(at: dotGitPath) try "gitdir: ../.git/worktrees/example\n".write(to: dotGitPath, atomically: true, encoding: .utf8) - XCTAssertEqual(CommandPaletteQuickOpenFileSearch.matchingTerm("./.git"), ".git") + XCTAssertEqual( + CommandPaletteQuickOpenFileSearch.matchingTerm("./.git", workspaceRoot: root.path), + ".git" + )Also applies to: 340-341, 345-345
🤖 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/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteQuickOpenFileSearchTests.swift` around lines 331 - 334, Remove the process-global current working directory mutation (lines 331-334 and similar occurrences at lines 340-341 and 345) by deleting the previousDirectory assignment, the changeCurrentDirectoryPath calls, and the defer statement. Instead, pass the root path using the workspaceRoot parameter when calling matchingTerm to keep the test self-contained and avoid race conditions during parallel test execution.Sources/CommandPaletteQuickOpenFileSearch.swift (1)
72-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
.textEditorfallback non-executable.At Line 79, falling back to
NSWorkspace.shared.open(url)can execute scripts/binaries depending on system associations. Use a non-executing fallback (e.g., reveal in Finder) to preserve Quick Open’s safe-open guarantees.Suggested patch
case .textEditor(let url): guard let editorURL = NSWorkspace.shared.urlForApplication(toOpen: UTType.plainText) else { - NSWorkspace.shared.open(url) + NSWorkspace.shared.activateFileViewerSelecting([url]) return }🤖 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/CommandPaletteQuickOpenFileSearch.swift` around lines 72 - 81, The .textEditor case in the performQuickOpenFileOpenAction method uses NSWorkspace.shared.open(url) as a fallback when no text editor URL is found, which can execute scripts or binaries based on system associations, violating the safe-open guarantee. Replace this fallback with NSWorkspace.shared.activateFileViewerSelecting([url]) instead to reveal the file in Finder, which is a non-executable operation that preserves the intended safety behavior.
🤖 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/CmuxCommandPalette/Sources/CmuxCommandPalette/QuickOpen/CommandPaletteQuickOpenFileSearch.swift`:
- Around line 208-214: The sorted function in
CommandPaletteQuickOpenFileSearch.swift is calling isDirectory(a) and
isDirectory(b) inside the sort comparator closure, which results in O(n log n)
filesystem metadata reads for hot path Quick Open operations. Precompute the
directory flags for all entries in the contents collection before sorting by
creating a mapping or array of tuples containing each item and its precomputed
isDirectory result, then sort based on these cached flags instead of calling
isDirectory repeatedly during the comparison operations.
---
Outside diff comments:
In
`@Packages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteQuickOpenFileSearchTests.swift`:
- Around line 331-334: Remove the process-global current working directory
mutation (lines 331-334 and similar occurrences at lines 340-341 and 345) by
deleting the previousDirectory assignment, the changeCurrentDirectoryPath calls,
and the defer statement. Instead, pass the root path using the workspaceRoot
parameter when calling matchingTerm to keep the test self-contained and avoid
race conditions during parallel test execution.
In `@Sources/CommandPaletteQuickOpenFileSearch.swift`:
- Around line 72-81: The .textEditor case in the performQuickOpenFileOpenAction
method uses NSWorkspace.shared.open(url) as a fallback when no text editor URL
is found, which can execute scripts or binaries based on system associations,
violating the safe-open guarantee. Replace this fallback with
NSWorkspace.shared.activateFileViewerSelecting([url]) instead to reveal the file
in Finder, which is a non-executable operation that preserves the intended
safety behavior.
🪄 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: 96e97608-30e9-4ffb-b2fc-f563c2f45767
📒 Files selected for processing (6)
Packages/CmuxCommandPalette/Sources/CmuxCommandPalette/QuickOpen/CommandPaletteQuickOpenFileOpenAction.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/QuickOpen/CommandPaletteQuickOpenFileSearch.swiftPackages/CmuxCommandPalette/Sources/CmuxCommandPalette/QuickOpen/CommandPaletteQuickOpenScoredFile.swiftPackages/CmuxCommandPalette/Tests/CmuxCommandPaletteTests/CommandPaletteQuickOpenFileSearchTests.swiftSources/CommandPaletteQuickOpenFileSearch.swiftcmux.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (1)
- cmux.xcodeproj/project.pbxproj
Comment on lines
+208
to
+214
| let sorted = contents.sorted { a, b in | ||
| let aIsDir = isDirectory(a) | ||
| let bIsDir = isDirectory(b) | ||
| if aIsDir != bIsDir { return aIsDir } | ||
| return a.lastPathComponent.localizedCaseInsensitiveCompare(b.lastPathComponent) == .orderedAscending | ||
| } | ||
| return Array(sorted.prefix(maxCount)) |
There was a problem hiding this comment.
Precompute directory metadata before sorting.
At Line 208, isDirectory is called inside the sort comparator, causing repeated filesystem metadata reads (O(n log n) probes) on a hot Quick Open path. Compute directory flags once per entry, then sort on cached flags.
Suggested patch
- let sorted = contents.sorted { a, b in
- let aIsDir = isDirectory(a)
- let bIsDir = isDirectory(b)
- if aIsDir != bIsDir { return aIsDir }
- return a.lastPathComponent.localizedCaseInsensitiveCompare(b.lastPathComponent) == .orderedAscending
- }
- return Array(sorted.prefix(maxCount))
+ let entries = contents.map { ($0, isDirectory($0)) }
+ let sorted = entries.sorted { a, b in
+ if a.1 != b.1 { return a.1 }
+ return a.0.lastPathComponent.localizedCaseInsensitiveCompare(b.0.lastPathComponent) == .orderedAscending
+ }
+ return Array(sorted.prefix(maxCount).map(\.0))As per coding guidelines, production code over scalable user data should avoid repeated sort/filter/map work in hot paths.
🤖 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/CmuxCommandPalette/Sources/CmuxCommandPalette/QuickOpen/CommandPaletteQuickOpenFileSearch.swift`
around lines 208 - 214, The sorted function in
CommandPaletteQuickOpenFileSearch.swift is calling isDirectory(a) and
isDirectory(b) inside the sort comparator closure, which results in O(n log n)
filesystem metadata reads for hot path Quick Open operations. Precompute the
directory flags for all entries in the contents collection before sorting by
creating a mapping or array of tuples containing each item and its precomputed
isDirectory result, then sort based on these cached flags instead of calling
isDirectory repeatedly during the comparison operations.
Source: Coding guidelines
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.
Summary
What changed?: impl feature quick open file like
Go To File (Cmd+P)in vscode.Cmd+Shift+O).@abc/xyzto search files within the workspace with cross-directory fuzzy search.@/,@~,@./.Why?: Users need fast opening and review files that just edited by coding agent!
Testing
Manual testing: workspace root listing, home directory browsing, cross-directory fuzzy search with cancellation, symlink handling, file open safety (scripts → text editor, binaries → Finder**
Demo Video
cmux-quick-open-file.mov
Review Trigger (Copy/Paste as PR comment)
Checklist
docs/quick-open-file.md)Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests