fix(web): scope file search recents by revision#1392
Open
DivyamTalwar wants to merge 4 commits into
Open
Conversation
Store file-search recents per repository revision and migrate legacy repo-scoped recents only into the default revision context.\n\nConstraint: Recently opened files are stored in browser localStorage and can point at paths missing on other revisions.\nRejected: Change the key without migration | existing users would silently lose default-branch recents.\nConfidence: high\nScope-risk: narrow\nDirective: Keep persisted browse state scoped to the navigation context that can safely resolve it.\nTested: node .yarn/releases/yarn-4.7.0.cjs workspace @sourcebot/web test src/app/(app)/browse/components/fileSearchRecents.test.ts; node .yarn/releases/yarn-4.7.0.cjs workspace @sourcebot/web exec eslint src/app/(app)/browse/components/fileSearchCommandDialog.tsx src/app/(app)/browse/components/fileSearchRecents.ts src/app/(app)/browse/components/fileSearchRecents.test.ts\nNot-tested: Browser E2E over live revision switching
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds revision-scoped recently opened file storage keys, tests the key and migration helpers, and updates the file-search dialog to migrate legacy repo-scoped recents when appropriate. ChangesRevision-scoped recently opened files
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 |
Document the file-search recents revision scoping fix in the Unreleased changelog after the PR number was assigned.\n\nConstraint: Sourcebot requires every PR to include a changelog entry with the PR link.\nConfidence: high\nScope-risk: narrow\nDirective: Keep changelog entries as follow-up commits once PR numbers exist.\nTested: not run; changelog-only change\nNot-tested: Runtime behavior
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/web/src/app/(app)/browse/components/fileSearchRecents.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport this sentinel as a camelCase constant.
DEFAULT_REVISION_NAMEbreaks the TS naming rule here, and keeping"HEAD"private forces other files to duplicate the same contract. Renaming/exporting it would fix both problems at once.Suggested cleanup
-const DEFAULT_REVISION_NAME = 'HEAD'; +export const defaultRevisionName = 'HEAD'; @@ - const encodedRevisionName = encodeURIComponent(revisionName ?? DEFAULT_REVISION_NAME); + const encodedRevisionName = encodeURIComponent(revisionName ?? defaultRevisionName); @@ - return revisionName === undefined || revisionName === null || revisionName === DEFAULT_REVISION_NAME; + return revisionName === undefined || revisionName === null || revisionName === defaultRevisionName;As per coding guidelines,
**/*.{ts,tsx,js,jsx}files should use camelCase starting with a lowercase letter.🤖 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/web/src/app/`(app)/browse/components/fileSearchRecents.ts at line 1, The sentinel in fileSearchRecents should be exposed as a camelCase export instead of a private SCREAMING_SNAKE_CASE constant. Rename DEFAULT_REVISION_NAME to a lowercase camelCase identifier and export it so other modules can reuse the same HEAD contract without duplicating the literal, while keeping the existing behavior in fileSearchRecents intact.Source: Coding guidelines
packages/web/src/app/(app)/browse/components/fileSearchRecents.test.ts (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing
nullmigration assertion.
shouldMigrateLegacyRecentlyOpenedFilestreatsnullas part of the default-revision path, but this suite never locks that branch down.Suggested assertion
test('only migrates legacy recents into the default revision context', () => { expect(shouldMigrateLegacyRecentlyOpenedFiles({ revisionName: undefined })).toBe(true); + expect(shouldMigrateLegacyRecentlyOpenedFiles({ revisionName: null })).toBe(true); expect(shouldMigrateLegacyRecentlyOpenedFiles({ revisionName: 'HEAD' })).toBe(true); expect(shouldMigrateLegacyRecentlyOpenedFiles({ revisionName: 'main' })).toBe(false); expect(shouldMigrateLegacyRecentlyOpenedFiles({ revisionName: 'feature/file-search' })).toBe(false); });🤖 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/web/src/app/`(app)/browse/components/fileSearchRecents.test.ts around lines 42 - 46, Add the missing null-case assertion in the shouldMigrateLegacyRecentlyOpenedFiles test so the default-revision migration behavior is locked down for both undefined and null. Update the fileSearchRecents.test suite alongside the existing shouldMigrateLegacyRecentlyOpenedFiles checks to include an expectation that revisionName: null returns true, matching the current legacy-recents migration path.
🤖 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/web/src/app/`(app)/browse/components/fileSearchCommandDialog.tsx:
- Around line 48-66: The legacy recently-opened migration in
fileSearchCommandDialog should clear the old repo-scoped localStorage entry
after successfully copying its data into the new state. Update the useEffect
that reads getLegacyRecentlyOpenedFilesStorageKey and calls setRecentlyOpened so
it removes the legacy key once a valid parsed array is applied, preventing the
one-time migration from replaying whenever the HEAD-scoped list becomes empty.
---
Nitpick comments:
In `@packages/web/src/app/`(app)/browse/components/fileSearchRecents.test.ts:
- Around line 42-46: Add the missing null-case assertion in the
shouldMigrateLegacyRecentlyOpenedFiles test so the default-revision migration
behavior is locked down for both undefined and null. Update the
fileSearchRecents.test suite alongside the existing
shouldMigrateLegacyRecentlyOpenedFiles checks to include an expectation that
revisionName: null returns true, matching the current legacy-recents migration
path.
In `@packages/web/src/app/`(app)/browse/components/fileSearchRecents.ts:
- Line 1: The sentinel in fileSearchRecents should be exposed as a camelCase
export instead of a private SCREAMING_SNAKE_CASE constant. Rename
DEFAULT_REVISION_NAME to a lowercase camelCase identifier and export it so other
modules can reuse the same HEAD contract without duplicating the literal, while
keeping the existing behavior in fileSearchRecents 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3babb738-ef24-40bd-9dee-963dbff77d89
📒 Files selected for processing (3)
packages/web/src/app/(app)/browse/components/fileSearchCommandDialog.tsxpackages/web/src/app/(app)/browse/components/fileSearchRecents.test.tspackages/web/src/app/(app)/browse/components/fileSearchRecents.ts
Apply CodeRabbit feedback by clearing migrated legacy recents, exporting the default revision sentinel, and covering null migration input.\n\nConstraint: Review feedback identified one replay risk and two small coverage/style gaps in the recents migration.\nConfidence: high\nScope-risk: narrow\nDirective: Keep legacy localStorage migrations one-time and explicitly covered.\nTested: node .yarn/releases/yarn-4.7.0.cjs workspace @sourcebot/web test src/app/(app)/browse/components/fileSearchRecents.test.ts; node .yarn/releases/yarn-4.7.0.cjs workspace @sourcebot/web exec eslint src/app/(app)/browse/components/fileSearchCommandDialog.tsx src/app/(app)/browse/components/fileSearchRecents.ts src/app/(app)/browse/components/fileSearchRecents.test.ts\nNot-tested: Browser E2E over live revision switching
…h-recent-revisions # Conflicts: # CHANGELOG.md
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.
Fixes #1387
Problem
Browse file-search recents were stored per repository, so switching branches or revisions could show stale file paths from another revision.
Root cause
The localStorage key used only
repoName, while selected recents navigate using the currentrevisionName.Solution
Scope the recents key by repository and revision, encode key segments to avoid delimiter collisions, and migrate legacy repo-scoped recents only into the default/
HEADrevision context so existing default-branch recents are preserved.Tests
node .yarn/releases/yarn-4.7.0.cjs workspace @sourcebot/web test src/app/(app)/browse/components/fileSearchRecents.test.tsnode .yarn/releases/yarn-4.7.0.cjs workspace @sourcebot/web exec eslint src/app/(app)/browse/components/fileSearchCommandDialog.tsx src/app/(app)/browse/components/fileSearchRecents.ts src/app/(app)/browse/components/fileSearchRecents.test.tsRisk
Low to moderate. Recents become revision-scoped; legacy repo-scoped recents are migrated only for the default revision to avoid reintroducing stale branch-specific paths.
Summary by CodeRabbit