[codex] fix workspace search regressions#2979
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors desktop window zoom persistence and startup sequencing; removes keyword-search UI and its hotkey; maps Changes
Sequence Diagram(s)mermaid Main->>Store: read persisted window state (bounds + zoomLevel) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/workspace-fs/src/search.ts (1)
167-188: Potential collision initemsByCompactRelativePathmap.The
compactRelativePathnormalization strips all separators, so files likefoo/bar.ts,foo-bar.ts, andfoo_bar.tswould all normalize tofoobarts, and only the last-indexed entry would be stored. This could cause some exact matches to be missed.Consider using a multi-value map (like
itemsByCompactName) for consistency:♻️ Proposed change to use multi-value maps for path lookups
function createFileSearchIndex(items: SearchIndexEntry[]): FileSearchIndex { const itemsByLowerName = new Map<string, SearchIndexEntry[]>(); const itemsByCompactName = new Map<string, SearchIndexEntry[]>(); - const itemsByLowerRelativePath = new Map<string, SearchIndexEntry>(); - const itemsByCompactRelativePath = new Map<string, SearchIndexEntry>(); + const itemsByLowerRelativePath = new Map<string, SearchIndexEntry[]>(); + const itemsByCompactRelativePath = new Map<string, SearchIndexEntry[]>(); for (const item of items) { addSearchIndexMapEntry(itemsByLowerName, item.lowerName, item); addSearchIndexMapEntry(itemsByCompactName, item.compactName, item); - itemsByLowerRelativePath.set(item.lowerRelativePath, item); - itemsByCompactRelativePath.set(item.compactRelativePath, item); + addSearchIndexMapEntry(itemsByLowerRelativePath, item.lowerRelativePath, item); + addSearchIndexMapEntry(itemsByCompactRelativePath, item.compactRelativePath, item); }This would also require updating the
FileSearchIndexinterface andcollectExactFileSearchMatchesto handle arrays for these maps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workspace-fs/src/search.ts` around lines 167 - 188, The itemsByCompactRelativePath map in createFileSearchIndex currently stores a single SearchIndexEntry per compactRelativePath which causes collisions (e.g., foo/bar.ts, foo-bar.ts → same key) and loses matches; change itemsByCompactRelativePath (and itemsByLowerRelativePath for consistency) to map string → SearchIndexEntry[] (multi-value map) by using addSearchIndexMapEntry like itemsByCompactName does, update the FileSearchIndex type to reflect arrays for these maps, and then update collectExactFileSearchMatches (and any other consumers) to handle arrays returned from itemsByCompactRelativePath/itemsByLowerRelativePath, flattening/concatenating entries and preserving deduplication/ordering as before.apps/desktop/src/main/windows/main.ts (1)
246-249: Optional: read zoom level once indebouncedSave().Line 246 and Line 248 call
getZoomLevel()twice back-to-back. Cache once to keep the saved value and in-memory value perfectly aligned.Small cleanup
- saveWindowState({ + const zoomLevel = window.webContents.getZoomLevel(); + saveWindowState({ x: bounds.x, y: bounds.y, width: bounds.width, height: bounds.height, isMaximized, - zoomLevel: window.webContents.getZoomLevel(), + zoomLevel, }); - persistedZoomLevel = window.webContents.getZoomLevel(); + persistedZoomLevel = zoomLevel;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/windows/main.ts` around lines 246 - 249, In debouncedSave, avoid calling window.webContents.getZoomLevel() twice; call it once (e.g., const zoom = window.webContents.getZoomLevel()) and use that single value for both the saved payload (zoomLevel) and persistedZoomLevel so the in-memory and persisted values are identical; update the code paths that set zoomLevel and persistedZoomLevel inside debouncedSave to use the cached zoom variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/windows/main.ts`:
- Around line 253-256: The zoom-changed handler reads a stale value because
getZoomLevel() runs before the change is applied; update the handler registered
on window.webContents.on("zoom-changed") to defer reading
window.webContents.getZoomLevel() by one tick (e.g., use setTimeout(..., 0) or
process.nextTick) before assigning persistedZoomLevel and calling
debouncedSave(), so the stored zoom reflects the new applied level.
---
Nitpick comments:
In `@apps/desktop/src/main/windows/main.ts`:
- Around line 246-249: In debouncedSave, avoid calling
window.webContents.getZoomLevel() twice; call it once (e.g., const zoom =
window.webContents.getZoomLevel()) and use that single value for both the saved
payload (zoomLevel) and persistedZoomLevel so the in-memory and persisted values
are identical; update the code paths that set zoomLevel and persistedZoomLevel
inside debouncedSave to use the cached zoom variable.
In `@packages/workspace-fs/src/search.ts`:
- Around line 167-188: The itemsByCompactRelativePath map in
createFileSearchIndex currently stores a single SearchIndexEntry per
compactRelativePath which causes collisions (e.g., foo/bar.ts, foo-bar.ts → same
key) and loses matches; change itemsByCompactRelativePath (and
itemsByLowerRelativePath for consistency) to map string → SearchIndexEntry[]
(multi-value map) by using addSearchIndexMapEntry like itemsByCompactName does,
update the FileSearchIndex type to reflect arrays for these maps, and then
update collectExactFileSearchMatches (and any other consumers) to handle arrays
returned from itemsByCompactRelativePath/itemsByLowerRelativePath,
flattening/concatenating entries and preserving deduplication/ordering as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5075d8a-a2e2-47c9-97b4-63b48490c98a
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/shared/detect-language.tsapps/desktop/src/shared/hotkeys.tspackages/workspace-fs/src/search.test.tspackages/workspace-fs/src/search.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/shared/hotkeys.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/workspace-fs/src/search.ts">
<violation number="1" location="packages/workspace-fs/src/search.ts:47">
P1: Maps for path-based exact matching incorrectly store single items instead of arrays, causing valid files to be dropped when path normalizations collide.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workspace-fs/src/search.ts`:
- Around line 451-453: The exact-path query is only replacing backslashes and
isn't run through the same text normalizer, causing misses for inputs like
"./src/file.ts"; change the normalizedPathQuery to apply the same
normalizeSearchText pipeline after normalizing separators (e.g., compute
normalizedPathQuery = normalizeSearchText(lowerQuery.replace(/\\/g, "/"))) and
use that value for the itemsByLowerRelativePath lookup so exact relative-path
lookups use the identical normalization as compactQuery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82a515c5-48da-4364-a948-79ad866b6b77
📒 Files selected for processing (2)
packages/workspace-fs/src/search.test.tspackages/workspace-fs/src/search.ts
✅ Files skipped from review due to trivial changes (1)
- packages/workspace-fs/src/search.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/windows/main.ts (1)
314-314: Redundant assignment on window close.The assignment
persistedZoomLevel = zoomLevelhas no effect here since the window is closing and the variable goes out of scope. Consider removing it for clarity.Optional cleanup
zoomLevel, }); - persistedZoomLevel = zoomLevel; browserManager.unregisterAll();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/windows/main.ts` at line 314, The assignment persistedZoomLevel = zoomLevel inside the window close handler is redundant because persistedZoomLevel is not used after the window is closed; remove the line to avoid dead code. Locate the close event callback in main.ts where persistedZoomLevel and zoomLevel are referenced and delete the assignment persistedZoomLevel = zoomLevel, leaving any other cleanup/close logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workspace-fs/src/search.ts`:
- Around line 137-149: The helper is storing raw absolutePath and
lowerRelativePath which breaks deletes/lookups that expect canonicalized keys;
before returning, compute canonicalAbsolutePath =
normalizeAbsolutePath(absolutePath) and canonicalLowerRelativePath =
normalizePathForGlob(relativePath).toLowerCase() (and use normalizePathForGlob
for compactRelativePath as well) then replace the stored absolutePath and
lowerRelativePath with those canonical values so the index (itemsByPath) and
exact-path queries use the same normalized forms.
---
Nitpick comments:
In `@apps/desktop/src/main/windows/main.ts`:
- Line 314: The assignment persistedZoomLevel = zoomLevel inside the window
close handler is redundant because persistedZoomLevel is not used after the
window is closed; remove the line to avoid dead code. Locate the close event
callback in main.ts where persistedZoomLevel and zoomLevel are referenced and
delete the assignment persistedZoomLevel = zoomLevel, leaving any other
cleanup/close logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 405e6dc8-43ce-4282-bd97-8104084321aa
📒 Files selected for processing (3)
apps/desktop/src/main/windows/main.tspackages/workspace-fs/src/search.test.tspackages/workspace-fs/src/search.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workspace-fs/src/search.test.ts
1 participant
Summary
This PR fixes the recent workspace search regression and a few adjacent desktop-editor issues reported by users.
What changed
.astrolanguage detection so Astro files get HTML highlightingCmd+Rdoes not temporarily forget font sizeRoot cause
The file search path had regressed in two ways:
The initial fix corrected search quality but introduced an avoidable hot-path scan on every query. The final implementation keeps the correctness fix while using indexed exact-match maps and only falling back to Fuse when there is no exact hit.
User impact
Validation
bun test packages/workspace-fs/src/search.test.tsbunx biome check packages/workspace-fs/src/search.ts packages/workspace-fs/src/search.test.tsbun test apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.tsbunx biome check apps/desktop/src/main/windows/main.ts apps/desktop/src/shared/detect-language.ts apps/desktop/src/lib/trpc/routers/changes/utils/parse-status.test.ts apps/desktop/src/shared/hotkeys.ts 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx' packages/workspace-fs/src/search.ts packages/workspace-fs/src/search.test.tsBenchmark notes
Local microbenchmarks on the search backend show the final implementation is much faster for the main user complaint, exact filename/path searches, while index construction is somewhat slower and separator-free compact fuzzy queries remain slower:
search.ts:38.1ms -> 0.022mspackages/workspace-fs/src/search.ts:273.3ms -> 0.045mspage.tsx:37.4ms -> 0.080msworkspacefssearchts:73.6ms -> 142.8ms3.7ms -> 6.8msThese were backend function timings, not end-to-end renderer profiling.
Summary by cubic
Fixes workspace file search by prioritizing exact filename/path matches via indexed lookups, canonicalizing index and query paths, returning all compact collisions, and rebuilding indexes after directory renames. Removes the keyword search surface, adds
.astrodetection, and makes desktop zoom persist and restore across reloads.Bug Fixes
packages/workspace-fs).apps/desktop).Refactors
meta+shift+fhotkey (apps/desktop)..astro→ HTML language detection.Written for commit 1484cf2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Removed
Tests