Repository navigation
Conversation
…nd icon models for Find results Pure model layer for the grouped Find pipeline: basename-relevance tiering (stem match > basename contains > body-only), per-file grouping, match preview slicing with highlight ranges, and cached per-extension result icons. No behavior change to the app target yet. Claude-Session: 027daef1-4981-4bba-922c-0738c4fba193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Codex <noreply@openai.com>
…ne view Replace the flat walk-order results table with a grouped NSOutlineView: files tiered by basename relevance, hits preview-sliced and highlighted, loaded in 100-row pages up to a disclosed 5,000-match hard cap (SIGTERM at cap). Snapshot lifecycle is now accurate on every emit - pure .searching frames update only the status line while early match pages render before ripgrep exits, and content-revision refreshes defer to the terminal frame. Typing debounce drops from 200ms to 50ms with in-flight process cancellation. Keyboard contract, multi-select, drag-out, terminal path insertion, context menu, and configurable open actions carry over to the grouped view. Claude-Session: 027daef1-4981-4bba-922c-0738c4fba193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Codex <noreply@openai.com>
…space Store the Find query and last snapshot in FileExplorerStore behind equality-guarded, non-published setters (streaming frames never invalidate the SwiftUI workspace tree), reseed the search UI from cache on remount with settled lifecycle normalization, and clear state when the workspace changes via beginWorkspace(_:). Claude-Session: 027daef1-4981-4bba-922c-0738c4fba193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Codex <noreply@openai.com>
… pass Expanding N newly inserted groups one call at a time makes NSOutlineView run a full NSTableRowData pass per call (visible-row frame maintenance plus total-height span recomputation over the row-height index sets). Wrapping the expand loops in a dedicated beginUpdates/endUpdates transaction - kept separate from the structural insert batch, which measures ~2.5x worse when merged - coalesces that work into one pass: - 100-result page append at a 5,000-result buffer: 7.3ms -> 2.9ms - query-change re-render with materialized rows: 38ms -> 12ms - initial 5,000-result apply: 451ms -> 187ms Medians from interleaved A/B rounds of a Debug/-Onone apply harness, 900x900 viewport, Apple M5 Pro. All 67 focused unit tests pass unchanged. Claude-Session: 027daef1-4981-4bba-922c-0738c4fba193 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Codex <noreply@openai.com>
|
@heyitaki is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughFile Explorer Find now uses grouped outline results with sticky headers, incremental updates, pagination, hard result limits, configurable case/code filtering, persisted workspace state, and updated context-menu and keyboard integrations. ChangesFile Explorer Find
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FileExplorerContainerView
participant FileSearchController
participant FileExplorerSearchResultsView
User->>FileExplorerContainerView: Enter query and select options
FileExplorerContainerView->>FileSearchController: Start search
FileSearchController-->>FileExplorerContainerView: Deliver snapshots
FileExplorerContainerView->>FileExplorerSearchResultsView: Apply grouped snapshot
User->>FileExplorerSearchResultsView: Scroll near bottom
FileExplorerSearchResultsView->>FileSearchController: Request next page
FileSearchController-->>FileExplorerSearchResultsView: Deliver appended results
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (10 errors, 2 warnings)
✅ Passed checks (13 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 replaces the flat-table ripgrep Find implementation with a grouped, ranked, paginated outline view: results are ranked by basename relevance per page, grouped by file in an
Confidence Score: 3/5The core search logic is well-structured with thorough tests; two rule violations need fixing before merge. The new grouping, ranking, preview, and icon modules are clean, well-tested, and correct. The controller coalescing and pagination design is sound. However, Sources/FileSearchResultIcon.swift (unguarded test method), Sources/FileExplorerSearchController.swift and Sources/FileExplorerSearchResultsView.swift (#if DEBUG test seams), Resources/Localizable.xcstrings (incomplete locale coverage for all five new keys) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as FileExplorerView
participant Ctrl as FileSearchController
participant Pipeline as FileSearchOutputPipeline
participant rg as ripgrep process
participant View as FileExplorerSearchResultsView
UI->>Ctrl: search(query, rootPath, options)
Ctrl->>rg: Process.run() [detached Task]
rg-->>Pipeline: stdout chunks
Pipeline-->>Ctrl: FileSearchPipelineUpdate (early emit)
Ctrl->>Ctrl: enqueuePipelineUpdate (coalescing slot)
Ctrl->>Ctrl: appendNewlyBufferedToDisplay + rank page
Ctrl->>View: onSnapshotChanged(snapshot)
View->>View: apply(snapshot) diff-aware outline update
Note over View: User scrolls near bottom
View->>UI: onLoadMoreRequested
UI->>Ctrl: loadMore()
Ctrl->>Pipeline: requestNextEmission(target)
Pipeline-->>Ctrl: update when buffer reaches target
Ctrl->>View: onSnapshotChanged(snapshot with more rows)
Note over rg,Pipeline: Hard cap 5000 matches
rg-->>Pipeline: stdout 5000th match
Pipeline-->>Ctrl: "shouldStopProcess=true"
Ctrl->>rg: SIGTERM
Ctrl->>View: "onSnapshotChanged isTruncated=true"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as FileExplorerView
participant Ctrl as FileSearchController
participant Pipeline as FileSearchOutputPipeline
participant rg as ripgrep process
participant View as FileExplorerSearchResultsView
UI->>Ctrl: search(query, rootPath, options)
Ctrl->>rg: Process.run() [detached Task]
rg-->>Pipeline: stdout chunks
Pipeline-->>Ctrl: FileSearchPipelineUpdate (early emit)
Ctrl->>Ctrl: enqueuePipelineUpdate (coalescing slot)
Ctrl->>Ctrl: appendNewlyBufferedToDisplay + rank page
Ctrl->>View: onSnapshotChanged(snapshot)
View->>View: apply(snapshot) diff-aware outline update
Note over View: User scrolls near bottom
View->>UI: onLoadMoreRequested
UI->>Ctrl: loadMore()
Ctrl->>Pipeline: requestNextEmission(target)
Pipeline-->>Ctrl: update when buffer reaches target
Ctrl->>View: onSnapshotChanged(snapshot with more rows)
Note over rg,Pipeline: Hard cap 5000 matches
rg-->>Pipeline: stdout 5000th match
Pipeline-->>Ctrl: "shouldStopProcess=true"
Ctrl->>rg: SIGTERM
Ctrl->>View: "onSnapshotChanged isTruncated=true"
Reviews (1): Last reviewed commit: "perf(file-explorer): coalesce Find outli..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+98
to
+104
| /// Test hook, wipes the symbol cache so per-call resolution behavior can | ||
| /// be observed independently between cases. | ||
| static func _resetSymbolCacheForTests() { | ||
| symbolCacheLock.lock() | ||
| symbolCache.removeAll(keepingCapacity: true) | ||
| symbolCacheLock.unlock() | ||
| } |
Contributor
There was a problem hiding this comment.
Three new test seams violate the no-test-debug-seam-in-production-source rule. The worst offender is _resetSymbolCacheForTests() here: it matches the …ForTests naming pattern, is not compiled out by #if DEBUG, and ships verbatim in production builds. FileExplorerSearchController.swift adds debugPipelineDeliveryCount and debugEnqueuePipelineUpdate under #if DEBUG (lines 568–573), and FileExplorerSearchResultsView.swift adds debugAppliedWorkCount and debugEmptyStateLabelHidden under #if DEBUG (lines 68–78). The canonical fix per cmux PR #6452 is to widen the relevant state from private to internal and read it from the test target via @testable import, rather than bridging it through a named test accessor in production source.
Rule Used: Flag Swift files under a production Sources path (... (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!
| @@ -105024,6 +105024,40 @@ | |||
| } | |||
Contributor
There was a problem hiding this comment.
The five added/renamed keys — fileExplorer.search.codeOnly, fileExplorer.search.codeOnly.tooltip, fileExplorer.search.matchCase, fileExplorer.search.matchCase.tooltip, and fileExplorer.search.noResultsFound — carry only en and ja entries. The catalog already contains 17 locales (ar, bs, da, de, en, es, fr, it, ja, km, ko, nb, pl, ru, th, tr, uk), so all three button labels, both tooltips, and the empty-state copy will fall back to English for users in the other 15 locales. Per the full-internationalization rule, every new key must include a translated entry for every locale already present in the touched catalog.
Rule Used: Flag production user-facing text that is not fully... (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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/FileExplorerView.swift (1)
1447-1451: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not expose raw backend failures in the search status.
messagecan containerror.localizedDescriptionor ripgrep diagnostics, including local paths and implementation details. Render localized product-level recovery copy and keep sanitized diagnostics internal.Proposed fix
- case .failed(let message): - return String( - format: String(localized: "fileExplorer.search.failed", defaultValue: "Search failed: %@"), - message - ) + case .failed: + return String( + localized: "fileExplorer.search.failed.generic", + defaultValue: "Search failed. Try again." + )Add the new key to every supported locale in the touched string catalog.
As per coding guidelines, user-facing errors must not expose raw upstream messages and should provide safe product terms and concrete recovery guidance. <coding_guidelines>
🤖 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/FileExplorerView.swift` around lines 1447 - 1451, Update the .failed case in FileExplorerView to stop interpolating the raw message and instead return localized, product-level recovery copy with concrete guidance. Keep backend diagnostics internal, and add the new localization key to every supported locale in the touched string catalog.Source: Coding guidelines
🤖 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/FileExplorerStoreTests.swift`:
- Around line 840-843: Bound the pagination loop around controller.loadMore() to
at most 50 pages, asserting on each iteration that the snapshot result count
increases; use the test’s asynchronous completion signal or a deadline-bounded
predicate check so stalled hasMore state fails rather than hangs.
- Around line 1257-1287: Update the search status formatter and localization
catalog to use explicit plural keys for the matches message, selecting the
`.one` variant for a count of one and `.other` for all other counts. In the
search status test around the existing totalMatchCount assertions, add coverage
for a single match and verify it uses the singular localized text while
preserving the existing plural assertion.
- Around line 871-873: Replace the single Task.yield() in the snapshot assertion
test with a deadline-bounded wait that polls until
snapshots.last?.totalMatchCount == 5, then retain the existing delivery-count
and snapshot assertions after the predicate succeeds.
In `@cmuxTests/FileSearchPreviewSliceTests.swift`:
- Around line 128-143: Remove XCTest.measure wall-clock benchmarks from
FileSearchPreviewSliceTests.swift lines 128-143, FileSearchRankingTests.swift
lines 108-127, and FileSearchResultIconTests.swift lines 99-128. Preserve each
test’s deterministic slicing, ordering/complexity, and cache-identity
assertions, while moving throughput or performance benchmarking to a separate
benchmark target or suite.
In `@Resources/Localizable.xcstrings`:
- Around line 105112-105145: Update the Japanese values for both
fileExplorer.search.matchCase and fileExplorer.search.matchCase.tooltip to use
大文字と小文字を区別, leaving the English strings unchanged.
In `@Sources/FileExplorerSearchController.swift`:
- Around line 678-684: Update the process-launch failure handling in the search
controller to stop using error.localizedDescription for the user-facing .failed
status. Replace it with localized, product-level recovery text that does not
expose executable paths or POSIX details, while keeping the raw error available
only for internal handling.
- Around line 568-574: Remove the DEBUG-only debugPipelineDeliveryCount property
and debugEnqueuePipelineUpdate method from
Sources/FileExplorerSearchController.swift, replacing their test usage with an
internal coalescer/API or observable test snapshots exposed through `@testable`
import. Update cmuxTests/FileExplorerStoreTests.swift lines 848-890 to exercise
that internal API or the real pipeline behavior, without adding production-only
test seams.
- Around line 810-819: Update finish(generation:update:) to preserve the
hard-cap signal from the completed pipeline update: when pipeline.finish
indicates shouldStopProcess is true, set didHitHardCap before emitting the
settled status. Ensure the resulting snapshot reports isTruncated correctly
while leaving normal completion behavior unchanged.
- Around line 759-765: Update the pagination Task in the load-more flow around
requestNextEmission to revalidate that self.pipeline is still identical to the
captured pipeline after await and before applyPipelineUpdate. Return without
applying the update when the pipeline is no longer authoritative, while
preserving the existing generation check in applyPipelineUpdate.
In `@Sources/FileExplorerSearchResultsView.swift`:
- Around line 1200-1209: Replace the static-only SearchResultCellLayout
namespace with a private immutable layout value owned by the search-result-cell
composition, and update all references to use that shared instance. Preserve the
existing chevron, icon, and hit-text metric values without introducing ambient
top-level state.
- Around line 290-316: The updateStickyHeader method currently scans preceding
outline rows to find the owning SearchResultGroupItem on every scroll event.
Replace the backward row loop with direct outline-hierarchy lookup from topRow,
resolving the top item’s parent group without iterating through the collection,
while preserving the existing stickyGroup and visibility behavior when no group
is found.
- Around line 68-78: Remove the DEBUG-only debugAppliedWorkCount and
debugEmptyStateLabelHidden seams from
Sources/FileExplorerSearchResultsView.swift. In
cmuxTests/FileExplorerSearchResultsViewApplyTests.swift lines 20-108, test the
extracted internal short-circuit policy; at lines 142-157, verify empty-state
behavior through observable UI state rather than an accessor; and at lines
264-282, reuse the policy test or use a genuine performance benchmark instead of
the production counter.
- Around line 61-67: Scope search-result caching and pagination deduplication by
the controller’s authoritative generation: add that generation to
FileSearchSnapshot, AppliedIdentity, and the load-more cursor, and require
generation matches when reusing cached results or rejecting duplicate pages.
Replace query/count-based lifecycle checks with this single generation source so
refreshed or option-changed searches reset correctly and can load their first
page.
- Around line 424-455: Update the pagination identity map and lookups in the
append-groups flow to use each group’s relativePath instead of path. Keep
existingByPath (or rename it consistently) keyed by relativePath, and apply the
same key when inserting newly created SearchResultGroupItem instances so
grouping matches the grouper and equality behavior.
In `@Sources/FileSearchResultIcon.swift`:
- Around line 98-104: Remove _resetSymbolCacheForTests from
Sources/FileSearchResultIcon.swift. In
cmuxTests/FileSearchResultIconTests.swift, stop calling it from setUp and remove
or redesign the reset-specific test using test-target observation or an injected
cache owner; update all three listed sites accordingly.
- Around line 58-96: Update cachedSymbol and its cache state to be owned by
`@MainActor`, removing nonisolated(unsafe), NSLock, and manual synchronization.
Keep the public helpers’ off-main callability by routing cache access through
the existing MainActor UI ownership boundary, while ensuring all NSImage
creation and shared image access occur on MainActor.
---
Outside diff comments:
In `@Sources/FileExplorerView.swift`:
- Around line 1447-1451: Update the .failed case in FileExplorerView to stop
interpolating the raw message and instead return localized, product-level
recovery copy with concrete guidance. Keep backend diagnostics internal, and add
the new localization key to every supported locale in the touched string
catalog.
🪄 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: 4090c270-1358-4104-82f7-f83a8ee2cbd5
📒 Files selected for processing (27)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/FileExplorerHeaderView.swiftSources/FileExplorerKeyboardShortcuts.swiftSources/FileExplorerSearchController.swiftSources/FileExplorerSearchResultCellView.swiftSources/FileExplorerSearchResultsTableView.swiftSources/FileExplorerSearchResultsView.swiftSources/FileExplorerState.swiftSources/FileExplorerStore.swiftSources/FileExplorerTerminalPathInsertion.swiftSources/FileExplorerView.swiftSources/FileSearchGrouping.swiftSources/FileSearchPreviewSlice.swiftSources/FileSearchRanking.swiftSources/FileSearchResultIcon.swiftSources/KeyboardShortcutContext.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateFileExplorerShortcutRoutingTests.swiftcmuxTests/FileExplorerSearchResultsViewApplyTests.swiftcmuxTests/FileExplorerShortcutSettingsTests.swiftcmuxTests/FileExplorerStoreTests.swiftcmuxTests/FileSearchGroupingTests.swiftcmuxTests/FileSearchPreviewSliceTests.swiftcmuxTests/FileSearchRankingTests.swiftcmuxTests/FileSearchResultIconTests.swiftcmuxTests/FileSearchRipgrepParserTests.swift
💤 Files with no reviewable changes (2)
- Sources/FileExplorerSearchResultCellView.swift
- Sources/FileExplorerSearchResultsTableView.swift
Comment on lines
+840
to
+843
| while finalSnapshot.hasMore { | ||
| controller.loadMore() | ||
| finalSnapshot = try #require(snapshots.last) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Bound the pagination loop so regressions fail instead of hanging.
If loadMore() leaves hasMore true without making progress, this loop never terminates. Cap it at the 50 possible pages and assert that each iteration increases the result count.
As per coding guidelines, asynchronous tests must use completion signals or deadline-bounded predicate checks.
🤖 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 `@cmuxTests/FileExplorerStoreTests.swift` around lines 840 - 843, Bound the
pagination loop around controller.loadMore() to at most 50 pages, asserting on
each iteration that the snapshot result count increases; use the test’s
asynchronous completion signal or a deadline-bounded predicate check so stalled
hasMore state fails rather than hangs.
Source: Coding guidelines
Comment on lines
+871
to
+873
| await Task.yield() | ||
| #expect(controller.debugPipelineDeliveryCount == 1) | ||
| #expect(snapshots.last?.totalMatchCount == 5) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wait for the coalesced snapshot, not one scheduler yield.
Task.yield() does not guarantee that the MainActor drain has executed. Await a deadline-bounded predicate such as snapshots.last?.totalMatchCount == 5 before asserting.
As per coding guidelines, tests must await real completion signals or deadline-bounded predicates.
🤖 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 `@cmuxTests/FileExplorerStoreTests.swift` around lines 871 - 873, Replace the
single Task.yield() in the snapshot assertion test with a deadline-bounded wait
that polls until snapshots.last?.totalMatchCount == 5, then retain the existing
delivery-count and snapshot assertions after the predicate succeeds.
Source: Coding guidelines
Comment on lines
+1257
to
+1287
| #expect(statusLabel.stringValue == String( | ||
| format: String(localized: "fileExplorer.search.searching", defaultValue: "%d matches, searching"), | ||
| 0 | ||
| )) | ||
| #expect(resultsView.debugAppliedWorkCount == 0) | ||
|
|
||
| let results = [Self.searchResult(relativePath: "first.txt")] | ||
| searchController.publish(FileSearchSnapshot( | ||
| query: "needle", | ||
| results: results, | ||
| status: .matches, | ||
| isSearching: true, | ||
| totalMatchCount: 100 | ||
| )) | ||
| #expect(statusLabel.stringValue == String( | ||
| format: String(localized: "fileExplorer.search.searching", defaultValue: "%d matches, searching"), | ||
| 100 | ||
| )) | ||
| #expect(resultsView.debugAppliedWorkCount == 1) | ||
|
|
||
| searchController.publish(FileSearchSnapshot( | ||
| query: "needle", | ||
| results: results, | ||
| status: .matches, | ||
| isSearching: false, | ||
| totalMatchCount: 650 | ||
| )) | ||
| #expect(statusLabel.stringValue == String( | ||
| format: String(localized: "fileExplorer.search.matches", defaultValue: "%d matches"), | ||
| 650 | ||
| )) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover singular counts with explicit plural localization keys.
These assertions lock the UI to %d matches; a total of one can render as “1 matches.” Update the status formatter/catalog to use .one and .other, and add a one-match assertion.
Based on learnings, cmux pluralized strings should use explicit .one and .other localization keys.
🤖 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 `@cmuxTests/FileExplorerStoreTests.swift` around lines 1257 - 1287, Update the
search status formatter and localization catalog to use explicit plural keys for
the matches message, selecting the `.one` variant for a count of one and
`.other` for all other counts. In the search status test around the existing
totalMatchCount assertions, add coverage for a single match and verify it uses
the singular localized text while preserving the existing plural assertion.
Source: Learnings
Comment on lines
+128
to
+143
| /// Perf guardrail, slicing a single visible hit cell must stay fast even | ||
| /// with a worst-case long-line preview. The Find sidebar's | ||
| /// `refreshVisibleHitCells()` walks every visible row on snapshot apply, | ||
| /// so a regression that reintroduced quadratic trim would show up here as | ||
| /// a 100×+ slowdown. | ||
| func testLongLineSliceIsCheap() { | ||
| let leading = String(repeating: " ", count: 200) | ||
| let body = String(repeating: "foo bar baz ", count: 25) | ||
| let trailing = String(repeating: " ", count: 100) | ||
| let preview = leading + body + "needle" + trailing | ||
| measure { | ||
| for _ in 0..<2000 { | ||
| _ = FileSearchPreviewSlicer.slice(preview: preview, query: "needle") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move wall-clock benchmarks out of correctness tests. These XCTest.measure blocks make the suites dependent on host and CI timing.
cmuxTests/FileSearchPreviewSliceTests.swift#L128-L143: retain deterministic slicing assertions and benchmark separately.cmuxTests/FileSearchRankingTests.swift#L108-L127: retain ordering/complexity assertions and benchmark separately.cmuxTests/FileSearchResultIconTests.swift#L99-L128: retain cache identity assertions and benchmark lookup throughput separately.
As per coding guidelines, tests must avoid measured wall-clock dependencies. <coding_guidelines>
📍 Affects 3 files
cmuxTests/FileSearchPreviewSliceTests.swift#L128-L143(this comment)cmuxTests/FileSearchRankingTests.swift#L108-L127cmuxTests/FileSearchResultIconTests.swift#L99-L128
🤖 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 `@cmuxTests/FileSearchPreviewSliceTests.swift` around lines 128 - 143, Remove
XCTest.measure wall-clock benchmarks from FileSearchPreviewSliceTests.swift
lines 128-143, FileSearchRankingTests.swift lines 108-127, and
FileSearchResultIconTests.swift lines 99-128. Preserve each test’s deterministic
slicing, ordering/complexity, and cache-identity assertions, while moving
throughput or performance benchmarking to a separate benchmark target or suite.
Source: Coding guidelines
Comment on lines
+105112
to
+105145
| "fileExplorer.search.matchCase": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Match case" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "大文字小文字を区別" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "fileExplorer.search.matchCase.tooltip": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Match Case" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "大文字小文字を区別" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target entries with line numbers.
sed -n '105100,105155p' Resources/Localizable.xcstrings | cat -n
printf '\n--- locales in catalog ---\n'
python3 - <<'PY'
import json
from pathlib import Path
p = Path('Resources/Localizable.xcstrings')
data = json.loads(p.read_text(encoding='utf-8'))
print(sorted(data.get('localizations', {}).keys()))
PY
printf '\n--- search for the same Japanese phrase ---\n'
rg -n '"大文字小文字を区別"|"大文字と小文字を区別"' Resources/Localizable.xcstringsRepository: manaflow-ai/cmux
Length of output: 1959
Use 大文字と小文字を区別 for both Japanese strings.
大文字小文字を区別 reads awkwardly here.
🤖 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 `@Resources/Localizable.xcstrings` around lines 105112 - 105145, Update the
Japanese values for both fileExplorer.search.matchCase and
fileExplorer.search.matchCase.tooltip to use 大文字と小文字を区別, leaving the English
strings unchanged.
Comment on lines
+290
to
+316
| private func updateStickyHeader() { | ||
| let scrollY = contentView.documentVisibleRect.minY | ||
| guard scrollY > 0.5 else { | ||
| stickyGroup = nil | ||
| stickyContainer.isHidden = true | ||
| return | ||
| } | ||
|
|
||
| // Find the topmost group whose row sits at-or-above the visible top. | ||
| // We probe a few pixels INTO the visible area so a group header that's | ||
| // exactly at the top still counts as "covered" by the sticky. | ||
| let probeY = scrollY + 1 | ||
| let topRow = outlineView.row(at: NSPoint(x: 0, y: probeY)) | ||
| guard topRow >= 0 else { | ||
| stickyGroup = nil | ||
| stickyContainer.isHidden = true | ||
| return | ||
| } | ||
| var group: SearchResultGroupItem? | ||
| var r = topRow | ||
| while r >= 0 { | ||
| if let item = outlineView.item(atRow: r) as? SearchResultGroupItem { | ||
| group = item | ||
| break | ||
| } | ||
| r -= 1 | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid scanning preceding rows on every scroll event.
With one file containing 5,000 hits, this loop can inspect roughly 5,000 rows for every bounds notification. Resolve the top item’s owning group directly from the outline hierarchy instead.
As per path instructions, production hot paths must avoid repeated full-collection scans around 1,000 records.
🤖 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/FileExplorerSearchResultsView.swift` around lines 290 - 316, The
updateStickyHeader method currently scans preceding outline rows to find the
owning SearchResultGroupItem on every scroll event. Replace the backward row
loop with direct outline-hierarchy lookup from topRow, resolving the top item’s
parent group without iterating through the collection, while preserving the
existing stickyGroup and visibility behavior when no group is found.
Source: Path instructions
Comment on lines
+424
to
+455
| var existingByPath: [String: SearchResultGroupItem] = [:] | ||
| existingByPath.reserveCapacity(groupItems.count) | ||
| for item in groupItems { | ||
| existingByPath[item.group.path] = item | ||
| } | ||
|
|
||
| var newItems: [SearchResultGroupItem] = [] | ||
| outlineView.beginUpdates() | ||
| for group in appendedGroups { | ||
| if let existing = existingByPath[group.path] { | ||
| let oldHitCount = existing.hitItems.count | ||
| let appendedHitItems = group.hits.map { SearchResultHitItem(hit: $0) } | ||
| existing.hitItems.append(contentsOf: appendedHitItems) | ||
| existing.group = FileSearchFileGroup( | ||
| path: existing.group.path, | ||
| relativePath: existing.group.relativePath, | ||
| filename: existing.group.filename, | ||
| directoryDisplay: existing.group.directoryDisplay, | ||
| hits: existing.group.hits + group.hits | ||
| ) | ||
| outlineView.insertItems( | ||
| at: IndexSet(integersIn: oldHitCount..<existing.hitItems.count), | ||
| inParent: existing, | ||
| withAnimation: [] | ||
| ) | ||
| refreshVisibleGroupCell(for: existing) | ||
| continue | ||
| } | ||
|
|
||
| let item = SearchResultGroupItem(group: group) | ||
| groupItems.append(item) | ||
| existingByPath[group.path] = item |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use relativePath consistently as the group identity.
The grouper, incremental diff, and SearchResultGroupItem equality all identify groups by relativePath, but pagination uses absolute path. Differing path representations can split or merge the wrong group.
Proposed fix
- var existingByPath: [String: SearchResultGroupItem] = [:]
- existingByPath.reserveCapacity(groupItems.count)
+ var existingByRelativePath: [String: SearchResultGroupItem] = [:]
+ existingByRelativePath.reserveCapacity(groupItems.count)
for item in groupItems {
- existingByPath[item.group.path] = item
+ existingByRelativePath[item.group.relativePath] = item
}
for group in appendedGroups {
- if let existing = existingByPath[group.path] {
+ if let existing = existingByRelativePath[group.relativePath] {
// ...
}
let item = SearchResultGroupItem(group: group)
groupItems.append(item)
- existingByPath[group.path] = item
+ existingByRelativePath[group.relativePath] = itemAs per path instructions, correctness-critical identity must have one authoritative structured source.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var existingByPath: [String: SearchResultGroupItem] = [:] | |
| existingByPath.reserveCapacity(groupItems.count) | |
| for item in groupItems { | |
| existingByPath[item.group.path] = item | |
| } | |
| var newItems: [SearchResultGroupItem] = [] | |
| outlineView.beginUpdates() | |
| for group in appendedGroups { | |
| if let existing = existingByPath[group.path] { | |
| let oldHitCount = existing.hitItems.count | |
| let appendedHitItems = group.hits.map { SearchResultHitItem(hit: $0) } | |
| existing.hitItems.append(contentsOf: appendedHitItems) | |
| existing.group = FileSearchFileGroup( | |
| path: existing.group.path, | |
| relativePath: existing.group.relativePath, | |
| filename: existing.group.filename, | |
| directoryDisplay: existing.group.directoryDisplay, | |
| hits: existing.group.hits + group.hits | |
| ) | |
| outlineView.insertItems( | |
| at: IndexSet(integersIn: oldHitCount..<existing.hitItems.count), | |
| inParent: existing, | |
| withAnimation: [] | |
| ) | |
| refreshVisibleGroupCell(for: existing) | |
| continue | |
| } | |
| let item = SearchResultGroupItem(group: group) | |
| groupItems.append(item) | |
| existingByPath[group.path] = item | |
| var existingByRelativePath: [String: SearchResultGroupItem] = [:] | |
| existingByRelativePath.reserveCapacity(groupItems.count) | |
| for item in groupItems { | |
| existingByRelativePath[item.group.relativePath] = item | |
| } | |
| var newItems: [SearchResultGroupItem] = [] | |
| outlineView.beginUpdates() | |
| for group in appendedGroups { | |
| if let existing = existingByRelativePath[group.relativePath] { | |
| let oldHitCount = existing.hitItems.count | |
| let appendedHitItems = group.hits.map { SearchResultHitItem(hit: $0) } | |
| existing.hitItems.append(contentsOf: appendedHitItems) | |
| existing.group = FileSearchFileGroup( | |
| path: existing.group.path, | |
| relativePath: existing.group.relativePath, | |
| filename: existing.group.filename, | |
| directoryDisplay: existing.group.directoryDisplay, | |
| hits: existing.group.hits + group.hits | |
| ) | |
| outlineView.insertItems( | |
| at: IndexSet(integersIn: oldHitCount..<existing.hitItems.count), | |
| inParent: existing, | |
| withAnimation: [] | |
| ) | |
| refreshVisibleGroupCell(for: existing) | |
| continue | |
| } | |
| let item = SearchResultGroupItem(group: group) | |
| groupItems.append(item) | |
| existingByRelativePath[group.relativePath] = item |
🤖 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/FileExplorerSearchResultsView.swift` around lines 424 - 455, Update
the pagination identity map and lookups in the append-groups flow to use each
group’s relativePath instead of path. Keep existingByPath (or rename it
consistently) keyed by relativePath, and apply the same key when inserting newly
created SearchResultGroupItem instances so grouping matches the grouper and
equality behavior.
Source: Path instructions
Comment on lines
+1200
to
+1209
| enum SearchResultCellLayout { | ||
| static let chevronX: CGFloat = 8 | ||
| static let chevronWidth: CGFloat = 12 | ||
| static let chevronIconGap: CGFloat = 4 | ||
| static let iconWidth: CGFloat = 16 | ||
| static var iconX: CGFloat { chevronX + chevronWidth + chevronIconGap } | ||
| // hit text sits a couple points to the right of the icon's frame leading so it | ||
| // visually aligns with the *glyph* of the SF Symbol (which is centered in the 16pt frame), | ||
| // not the frame edge. | ||
| static var hitTextX: CGFloat { iconX + 2 } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Avoid adding a top-level static-only layout namespace.
SearchResultCellLayout introduces an internal ambient namespace under Sources. Make these metrics a private immutable value owned and shared by the result-cell composition.
As per coding guidelines, production Swift source must avoid static-only namespaces and ambient global 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/FileExplorerSearchResultsView.swift` around lines 1200 - 1209,
Replace the static-only SearchResultCellLayout namespace with a private
immutable layout value owned by the search-result-cell composition, and update
all references to use that shared instance. Preserve the existing chevron, icon,
and hit-text metric values without introducing ambient top-level state.
Source: Coding guidelines
Comment on lines
+58
to
+96
| // process-wide cache for configured symbol images keyed on SF | ||
| // Symbol name. Capped at a generous bound (~120 entries cover every name | ||
| // in the maps below); evictions are not needed in practice. Reads are | ||
| // dominant, sticky-header updates fire on every scroll tick, so we use | ||
| // an unfair lock for the rare write path rather than serialize behind a | ||
| // queue or actor. | ||
| // A synchronous lock keeps the tiny cache critical section ordered across AppKit callers without async hops. | ||
| private static let symbolCacheLock = NSLock() | ||
| nonisolated(unsafe) private static var symbolCache: [String: NSImage] = [:] | ||
| nonisolated(unsafe) private static let symbolConfiguration = NSImage.SymbolConfiguration( | ||
| pointSize: 12, | ||
| weight: .regular | ||
| ) | ||
|
|
||
| private static func cachedSymbol(named name: String) -> NSImage { | ||
| symbolCacheLock.lock() | ||
| if let cached = symbolCache[name] { | ||
| symbolCacheLock.unlock() | ||
| return cached | ||
| } | ||
| symbolCacheLock.unlock() | ||
|
|
||
| let base = NSImage(systemSymbolName: name, accessibilityDescription: nil) | ||
| ?? NSImage(systemSymbolName: "doc", accessibilityDescription: nil) | ||
| ?? NSImage() | ||
| let configured = base.withSymbolConfiguration(symbolConfiguration) ?? base | ||
| configured.isTemplate = true | ||
|
|
||
| symbolCacheLock.lock() | ||
| // Another thread may have raced ahead; prefer the existing entry so | ||
| // every caller observes the same reference. | ||
| if let existing = symbolCache[name] { | ||
| symbolCacheLock.unlock() | ||
| return existing | ||
| } | ||
| symbolCache[name] = configured | ||
| symbolCacheLock.unlock() | ||
| return configured | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the AppKit image cache under one @MainActor owner.
The public helpers remain callable off-main while nonisolated(unsafe) bypasses isolation and NSLock only protects the dictionary—not subsequent shared NSImage access. These icons are consumed by MainActor AppKit cells, so isolate the cache/helper there instead of adding manual synchronization.
As per coding guidelines, “Keep SwiftUI or AppKit bridge UI lifecycle under one explicit MainActor owner” and do not introduce manual locks where actor ownership can synchronize the state. <coding_guidelines>
Proposed ownership change
-enum FileSearchResultIcon {
+@MainActor
+enum FileSearchResultIcon {
...
- private static let symbolCacheLock = NSLock()
- nonisolated(unsafe) private static var symbolCache: [String: NSImage] = [:]
- nonisolated(unsafe) private static let symbolConfiguration = NSImage.SymbolConfiguration(
+ private static var symbolCache: [String: NSImage] = [:]
+ private static let symbolConfiguration = NSImage.SymbolConfiguration(
...
- symbolCacheLock.lock()
if let cached = symbolCache[name] {
- symbolCacheLock.unlock()
return cached
}
- symbolCacheLock.unlock()
...
- symbolCacheLock.lock()
- if let existing = symbolCache[name] {
- symbolCacheLock.unlock()
- return existing
- }
symbolCache[name] = configured
- symbolCacheLock.unlock()
return configured📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // process-wide cache for configured symbol images keyed on SF | |
| // Symbol name. Capped at a generous bound (~120 entries cover every name | |
| // in the maps below); evictions are not needed in practice. Reads are | |
| // dominant, sticky-header updates fire on every scroll tick, so we use | |
| // an unfair lock for the rare write path rather than serialize behind a | |
| // queue or actor. | |
| // A synchronous lock keeps the tiny cache critical section ordered across AppKit callers without async hops. | |
| private static let symbolCacheLock = NSLock() | |
| nonisolated(unsafe) private static var symbolCache: [String: NSImage] = [:] | |
| nonisolated(unsafe) private static let symbolConfiguration = NSImage.SymbolConfiguration( | |
| pointSize: 12, | |
| weight: .regular | |
| ) | |
| private static func cachedSymbol(named name: String) -> NSImage { | |
| symbolCacheLock.lock() | |
| if let cached = symbolCache[name] { | |
| symbolCacheLock.unlock() | |
| return cached | |
| } | |
| symbolCacheLock.unlock() | |
| let base = NSImage(systemSymbolName: name, accessibilityDescription: nil) | |
| ?? NSImage(systemSymbolName: "doc", accessibilityDescription: nil) | |
| ?? NSImage() | |
| let configured = base.withSymbolConfiguration(symbolConfiguration) ?? base | |
| configured.isTemplate = true | |
| symbolCacheLock.lock() | |
| // Another thread may have raced ahead; prefer the existing entry so | |
| // every caller observes the same reference. | |
| if let existing = symbolCache[name] { | |
| symbolCacheLock.unlock() | |
| return existing | |
| } | |
| symbolCache[name] = configured | |
| symbolCacheLock.unlock() | |
| return configured | |
| } | |
| `@MainActor` | |
| enum FileSearchResultIcon { | |
| // process-wide cache for configured symbol images keyed on SF | |
| // Symbol name. Capped at a generous bound (~120 entries cover every name | |
| // in the maps below); evictions are not needed in practice. Reads are | |
| // dominant, sticky-header updates fire on every scroll tick, so we keep the | |
| // tiny cache on the main actor instead of synchronizing it separately. | |
| private static var symbolCache: [String: NSImage] = [:] | |
| private static let symbolConfiguration = NSImage.SymbolConfiguration( | |
| pointSize: 12, | |
| weight: .regular | |
| ) | |
| private static func cachedSymbol(named name: String) -> NSImage { | |
| if let cached = symbolCache[name] { | |
| return cached | |
| } | |
| let base = NSImage(systemSymbolName: name, accessibilityDescription: nil) | |
| ?? NSImage(systemSymbolName: "doc", accessibilityDescription: nil) | |
| ?? NSImage() | |
| let configured = base.withSymbolConfiguration(symbolConfiguration) ?? base | |
| configured.isTemplate = true | |
| if let existing = symbolCache[name] { | |
| return existing | |
| } | |
| symbolCache[name] = configured | |
| return configured | |
| } | |
| } |
🤖 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/FileSearchResultIcon.swift` around lines 58 - 96, Update cachedSymbol
and its cache state to be owned by `@MainActor`, removing nonisolated(unsafe),
NSLock, and manual synchronization. Keep the public helpers’ off-main
callability by routing cache access through the existing MainActor UI ownership
boundary, while ensuring all NSImage creation and shared image access occur on
MainActor.
Source: Coding guidelines
Comment on lines
+98
to
+104
| /// Test hook, wipes the symbol cache so per-call resolution behavior can | ||
| /// be observed independently between cases. | ||
| static func _resetSymbolCacheForTests() { | ||
| symbolCacheLock.lock() | ||
| symbolCache.removeAll(keepingCapacity: true) | ||
| symbolCacheLock.unlock() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the production cache-reset test seam. The tests currently force a test-only API into runtime source.
Sources/FileSearchResultIcon.swift#L98-L104: remove_resetSymbolCacheForTests.cmuxTests/FileSearchResultIconTests.swift#L15-L18: stop resetting production state duringsetUp.cmuxTests/FileSearchResultIconTests.swift#L92-L96: remove or redesign the reset-specific test using test-target observation or an injected owner.
As per path instructions, production files under Sources/ must not add test-only seams. <path_instructions>
📍 Affects 2 files
Sources/FileSearchResultIcon.swift#L98-L104(this comment)cmuxTests/FileSearchResultIconTests.swift#L15-L18cmuxTests/FileSearchResultIconTests.swift#L92-L96
🤖 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/FileSearchResultIcon.swift` around lines 98 - 104, Remove
_resetSymbolCacheForTests from Sources/FileSearchResultIcon.swift. In
cmuxTests/FileSearchResultIconTests.swift, stop calling it from setUp and remove
or redesign the reset-specific test using test-target observation or an injected
cache owner; update all three listed sites accordingly.
Source: Path instructions
teamleaderleo
added
the
needs a call
This branch has not been 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.
What and why
Find previously streamed ripgrep matches into a flat table in filesystem walk order and stopped at 500 rows. This PR makes a grouped implementation the only Find path: results grouped by file, files ranked by basename relevance, hits preview-sliced and highlighted, loaded in 100-row pages up to a disclosed 5,000-match cap. Existing behavior — status line, keyboard contract, multi-select, drag-out, context menu, configurable open actions — carries over.
Design notes
FileSearchSnapshot.isSearchingnow means the backend is genuinely active: early.matchespages render before ripgrep exits, pure.searchingframes touch only the status line, and content-revision refreshes defer to the terminal frame..limitedstate: 100-row pages, 5,000-match hard cap (SIGTERM at cap), disclosed viatotalMatchCount/isTruncated.Performance
The grouped view does strictly more per frame than the flat table it replaces — grouping, ranking, an outline hierarchy, and a 5,000-match buffer vs the old 500-row cap — so interactive costs were profiled and kept in the low milliseconds (Debug/
-Ononeharness, 5,000-result buffer, Apple M5 Pro): a 100-result page append applies in ~3 ms, a query-change re-render with rows on screen in ~12 ms, and a full 5,000-result cache reseed in ~190 ms (one-time, on Find-tab remount). Ranking and grouping run only at the snapshot boundary, duplicate frames short-circuit at O(1), outline updates are diff-aware, and group expansions coalesce into a single update transaction per apply.Compatibility and testing
Follow-ups
9f52dada4a) in its own PR.🤖 Generated with Claude Code