fix(web): scope file-search recents to (repo, revision) in the browse dialog - #1529
Conversation
… dialog
The browse file-search dialog (mod+p) stored recently opened files in
localStorage under a key scoped only by repoName. A user who opened
a file on `main`, switched to `feature/foo`, and re-opened the dialog
would see recents from `main` — paths that may not exist on the new
revision. Selecting one navigated with the new revision and landed on
a 404.
Change the localStorage key to `recentlyOpenedFiles-${repoName}-${revisionName ?? 'HEAD'}` so the recents are naturally scoped per (repo, revision) tuple. The 'HEAD' default matches the file-fetch fallback on the next line, so the recents key and the file list key agree on the "no revision in URL" case.
Old keys become orphaned entries in localStorage and are ignored. No migration is required.
Fixes sourcebot-dev#1387.
…ch dialog Three vitest cases in fileSearchCommandDialog.test.tsx: - Scopes the recents localStorage key to the (repo, revision) tuple (e.g. `recentlyOpenedFiles-github.com/foo/bar-main`). - Uses the `HEAD` fallback for the "no revision in URL" case, so the recents key and the file-list key agree. - Produces a different key for a different revision in the same repo, which is the regression assertion for the bug. The test stubs `useLocalStorage` from `usehooks-ts` to capture the key the component passes (without this, we'd be asserting on real localStorage, which jsdom does provide but is per-test mutable state that's harder to reason about). The other hooks the dialog uses (useBrowseParams, useBrowseState, useQuery, etc.) are stubbed so the test only exercises the key derivation. Plus a one-line CHANGELOG entry under [Unreleased] -> Fixed. Refs sourcebot-dev#1387.
WalkthroughThe file-search dialog now scopes recently opened files by repository and revision. It uses ChangesFile search recents
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Update the changelog entry’s trailing GitHub reference to use the
current pull request ID and the repository’s /pull/<id> URL instead of the
existing issue URL, preserving the required
[#<id>](https://github.com/sourcebot-dev/sourcebot/pull/<id>) format.
In `@packages/web/src/app/`(app)/browse/components/fileSearchCommandDialog.tsx:
- Around line 44-46: Update the recentlyOpened useLocalStorage key in the file
search command dialog to encode the repoName and revisionName tuple without
ambiguity, using serialization or another collision-free format while preserving
the HEAD fallback. Add a regression test covering the specified
repository/revision pairs and verify they produce distinct storage keys.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f06252b-d561-44c8-a88a-37ed022e0f22
📒 Files selected for processing (3)
CHANGELOG.mdpackages/web/src/app/(app)/browse/components/fileSearchCommandDialog.test.tsxpackages/web/src/app/(app)/browse/components/fileSearchCommandDialog.tsx
CodeRabbit finding on PR sourcebot-dev#1529: the previous fix used a `<repoName>-<revisionName>` template, which collides for tuples like (`foo-bar`, `baz`) and (`foo`, `bar-baz`) — both produce the key `recentlyOpenedFiles-foo-bar-baz`. Both components can contain `-` (GitHub branch names, repo paths), so the boundary is ambiguous. Switch to a JSON-encoded tuple: `recentlyOpenedFiles::${JSON.stringify([repoName, revisionName ?? 'HEAD'])}`. JSON.stringify of a 2-element array is a unique representation of the tuple, so the key is collision-free regardless of which characters appear in the components. The `'HEAD'` default and the `::` prefix are preserved from the previous fix. The CHANGELOG link is also corrected to point at the PR (was the issue URL, per CodeRabbit). Adds a 4th test case that asserts two ambiguous tuples produce different keys. Refs sourcebot-dev#1529.
|
Both CodeRabbit inline comments addressed in 4b0eb9ae:
Added a 4th test case that asserts the two ambiguous tuples above produce different keys. 4/4 tests pass. |
Summary
The browse file-search dialog (
mod+p) stored recently opened files inlocalStorageunder a key scoped only byrepoName. A user who opened a file onmain, switched tofeature/foo, and re-opened the dialog would see recents frommain— paths that may not exist on the new revision. Selecting one navigated with the new revision and landed on a 404.Fix: scope the
localStoragekey to the(repoName, revisionName)tuple, so a branch switch in the same repo yields a fresh recents list scoped to the new revision.Fixes #1387.
What changes
packages/web/src/app/(app)/browse/components/fileSearchCommandDialog.tsx:38— theuseLocalStoragekey changes from`recentlyOpenedFiles-${repoName}`to`recentlyOpenedFiles-${repoName}-${revisionName ?? 'HEAD'}`. The'HEAD'default matches the file-fetch fallback on the next line, so the recents key and the file list key agree on the "no revision in URL" case.The
setRecentlyOpenedcallback does not need to change —useLocalStoragefromusehooks-tswrites to whatever key the current key string resolves to. Switching revisions automatically re-renders the dialog with the new key, which starts empty (or with whatever recents the user previously stored under that exact(repo, revision)tuple).Old keys (
`recentlyOpenedFiles-<repo>`) become orphaned entries inlocalStorageand are ignored. No migration is required — the worst case is the user loses the recents accumulated under the old key, which is the bug being fixed.Files
packages/web/src/app/(app)/browse/components/fileSearchCommandDialog.tsx— the key-derivation change (one line, plus a 4-line comment explaining the rationale).packages/web/src/app/(app)/browse/components/fileSearchCommandDialog.test.tsx— 3 new vitest cases.CHANGELOG.md— one-sentence entry under[Unreleased] → Fixed.Why this is in scope
The component already reads
revisionNamefromuseBrowseParams()on the line above and uses it to fetch the file list (getFiles({ repoName, revisionName: revisionName ?? 'HEAD' })), so the data was already revision-scoped. ThelocalStoragekey was the only thing that wasn't. The fix is a single key-derivation change that brings the persistence layer in line with the data layer that's already there.Test coverage
3 vitest cases in
fileSearchCommandDialog.test.tsx:(repo, revision)tuple. Asserts the key isrecentlyOpenedFiles-github.com/foo/bar-mainfor(github.com/foo/bar, main). The regression assertion for the bug.HEADfallback for the "no revision in URL" case. The file-fetch on the next line usesrevisionName ?? 'HEAD'as the default; the recents key needs to agree so the user sees a consistent recents list for the "default branch" view. Asserts the key isrecentlyOpenedFiles-github.com/foo/bar-HEADwhenrevisionNameisundefined.maintofeature/fooshowed recents frommain. After the fix, the keys differ, so the recents are naturally scoped. Assertsmainandfeature/fooproduce different keys for the same repo.The test stubs
useLocalStoragefromusehooks-tsto capture the key the component passes (without this, we'd be asserting on reallocalStorage, which jsdom does provide but is per-test mutable state that's harder to reason about). The other hooks the dialog uses (useBrowseParams,useBrowseState,useQuery, etc.) are stubbed so the test only exercises the key derivation.3/3 tests pass; 7 pre-existing OpenTelemetry-setup failures in
ee/askmcp/...andee/permissionSyncStatus/...are unchanged by this PR (they fail to load due to the OTel SDK version mismatch, before any of my code runs).Backward compatibility
Pure behaviour change. Old keys' data is left in
localStorageand ignored, so a user upgrading from a previous version simply starts with an empty recents list on their next branch switch. No data loss on the server side.Risks
Minimal. The default
'HEAD'matches the file-fetch default on the next line, so the recents key and the file list key agree on the "no revision in URL" case. If a future change splits'HEAD'andundefinedsemantics, both sides will need to be updated in lockstep — but that's already true for the file fetch.Future work
userRecentFiletable scoped by(userId, repoId, revisionName)). Punted: alocalStoragekey is the lowest-cost fix and matches the existing pattern. If a user clears their browser data they lose their recents, but they also lose their session, so this is fine.Note
Low Risk
Client-only localStorage key change in the browse file-search dialog; no auth, API, or server impact beyond users starting fresh recents per revision.
Overview
Fixes browse mod+p file search showing recently opened paths from another branch after a revision switch (issue #1387).
FileSearchCommandDialognow persists recents under alocalStoragekey derived from[repoName, revisionName ?? 'HEAD']viarecentlyOpenedFiles::${JSON.stringify(...)}, instead of repo name only. That aligns recents with the same revision default used forgetFiles, and avoids ambiguous keys that a simplerepo-revisionstring could collide on. Prior repo-only keys are left unused (no migration).Adds
fileSearchCommandDialog.test.tsx(Vitest) to lock the key format,HEADwhen revision is missing, per-revision separation, and collision safety. CHANGELOG documents the fix under[Unreleased].Reviewed by Cursor Bugbot for commit 4b0eb9a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
HEAD.