chore(web): Scope code nav to current repository by default #647
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces cross-repository search scoping to the code navigation system. It adds a toggle UI in the explore menu to enable/disable cross-repository search, refactors the SymbolHoverPopup component from callback-based navigation to data-driven props (fileName, repoName, source), threads a repoName parameter through code navigation APIs for search filtering, and adds telemetry events for code navigation interactions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/web/src/features/chat/tools.ts (2)
24-55: Repository scoping for references tool is wired correctlyOptional
repositoryinput and forwarding it asrepoNameinto the references API cleanly enable per-repo scoping while preserving the previous behavior when omitted. Only minor nit: you defineconst revision = "HEAD";but still inline"HEAD"in the request; could reuserevisionfor consistency, but not required.
63-95: Definitions tool mirrors references tool behavior appropriatelyAdding the optional
repositoryfield and passing it through asrepoNameto the definitions API keeps the two tools symmetric and supports repo-scoped lookups without breaking existing callers.packages/web/src/app/[domain]/browse/[...path]/components/pureCodePreviewPanel.tsx (1)
110-132: Nice improvement to scroll behavior.The viewport check to determine scroll strategy (
nearestvscenter) prevents jarring scroll jumps when the highlight is already visible. This is a good UX consideration.Minor nit: Line 128 uses optional chaining (
editorRef.view?.dispatch) but the guard at line 110 already ensureseditorRef.viewis defined.- editorRef.view?.dispatch({ + editorRef.view.dispatch({packages/web/src/ee/features/codeNav/components/symbolHoverPopup/index.tsx (1)
122-127: Consider adding error handling for audit actions.The fire-and-forget pattern is appropriate for audit/telemetry to avoid blocking user interactions. However, errors are currently silently swallowed. Consider adding a
.catch()handler to log failures for observability:- createAuditAction({ + createAuditAction({ action: "user.performed_goto_definition", metadata: { message: symbolInfo.symbolName, }, - }); + }).catch((error) => console.error('Audit action failed:', error));The same applies to the
onFindReferencescallback at lines 175-180.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)docs/docs/features/code-navigation.mdx(1 hunks)packages/web/src/app/[domain]/browse/[...path]/components/pureCodePreviewPanel.tsx(3 hunks)packages/web/src/app/[domain]/browse/layout.tsx(2 hunks)packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx(2 hunks)packages/web/src/app/components/keyboardShortcutHint.tsx(1 hunks)packages/web/src/components/ui/toggle.tsx(2 hunks)packages/web/src/ee/features/codeNav/components/exploreMenu/index.tsx(4 hunks)packages/web/src/ee/features/codeNav/components/symbolHoverPopup/index.tsx(6 hunks)packages/web/src/ee/features/codeNav/components/symbolHoverPopup/useHoveredOverSymbolInfo.ts(7 hunks)packages/web/src/features/chat/components/chatThread/referencedFileSourceListItem.tsx(2 hunks)packages/web/src/features/chat/tools.ts(2 hunks)packages/web/src/features/codeNav/api.ts(5 hunks)packages/web/src/features/codeNav/types.ts(1 hunks)packages/web/src/features/search/fileSourceApi.ts(2 hunks)packages/web/src/lib/posthogEvents.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/web/src/app/components/keyboardShortcutHint.tsx (1)
packages/web/src/lib/utils.ts (1)
cn(24-26)
packages/web/src/ee/features/codeNav/components/symbolHoverPopup/useHoveredOverSymbolInfo.ts (4)
packages/web/src/features/search/types.ts (1)
SourceRange(16-16)packages/web/src/lib/utils.ts (2)
measure(511-529)unwrapServiceError(539-546)packages/web/src/features/codeNav/api.ts (1)
findSearchBasedSymbolDefinitions(71-127)packages/web/src/app/api/(client)/client.ts (1)
findSearchBasedSymbolDefinitions(82-88)
packages/web/src/ee/features/codeNav/components/symbolHoverPopup/index.tsx (4)
packages/web/src/app/[domain]/browse/hooks/useBrowseNavigation.ts (1)
useBrowseNavigation(8-36)packages/web/src/ee/features/codeNav/components/symbolHoverPopup/useHoveredOverSymbolInfo.ts (1)
useHoveredOverSymbolInfo(39-203)packages/web/src/ee/features/audit/actions.ts (1)
createAuditAction(17-26)packages/web/src/ee/features/codeNav/components/symbolHoverPopup/symbolDefinitionPreview.tsx (1)
SymbolDefinitionPreview(17-57)
packages/web/src/features/chat/tools.ts (2)
packages/web/src/features/codeNav/api.ts (2)
findSearchBasedSymbolReferences(16-68)findSearchBasedSymbolDefinitions(71-127)packages/web/src/app/api/(client)/client.ts (2)
findSearchBasedSymbolReferences(74-80)findSearchBasedSymbolDefinitions(82-88)
🔇 Additional comments (19)
CHANGELOG.md (1)
12-21: Changelog entry accurately documents new default and UI toggleThe new “Added” and “Changed” entries clearly describe the explore menu toggle and repo-scoped default for code nav, consistent with the PR intent. No changes needed.
packages/web/src/features/search/fileSourceApi.ts (1)
9-23: Escaping repository in repo filter regex is a solid hardeningAnchoring the repo filter and escaping the repository string before interpolating into the regexp is correct and consistent with the code nav API usage; it avoids partial matches and issues with special characters in repo names.
packages/web/src/lib/posthogEvents.ts (1)
296-316: New code‑nav telemetry events are well‑structuredThe added event payloads (
source,durationMs,isGlobalSearchEnabled) give good observability into code‑nav usage and the explore menu toggle. As long as all emitters are updated to provide these fields, this looks solid.docs/docs/features/code-navigation.mdx (1)
24-42: Docs row clearly explains cross‑repo toggle and default scopeThe “Cross-repository navigation” entry accurately captures the new globe toggle and default repo‑scoped behavior for references/definitions. Reads clearly and aligns with the implementation.
packages/web/src/app/[domain]/browse/layout.tsx (1)
13-35: Default browse search now safely scopes to the current repoEscaping the repo name and anchoring it in
repo:^…$for the default SearchBar query correctly constrains searches to the active repository and avoids regex issues with special characters. The optionalrev:${revisionName}suffix keeps existing revision scoping behavior.packages/web/src/features/codeNav/api.ts (1)
11-52: Repository filter in code‑nav queries is correctly parameterized and escapedAdding optional
repoNameto the request and conditionally appending an anchored, escaped repo regexp to the IR query cleanly enables repository‑scoped references/definitions. Existing callers that don’t passrepoNameretain the previous cross‑repo behavior, which is ideal for a gradual rollout.Also applies to: 71-111
packages/web/src/app/components/keyboardShortcutHint.tsx (1)
1-13: className support makes the shortcut hint more reusableAllowing an optional
classNameand merging it with the base styles viacnimproves composability while preserving existing layout and aria-label behavior. Looks good.packages/web/src/features/codeNav/types.ts (1)
7-16: LGTM!The JSDoc comments clearly document the optional behavior, and the schema correctly models the repository-scoped vs. global search distinction. The
repoNamefield being optional aligns well with the PR objective.packages/web/src/ee/features/codeNav/components/exploreMenu/index.tsx (2)
49-66: LGTM!The query implementation correctly:
- Includes
isGlobalSearchEnabledin the query key for proper cache invalidation- Conditionally passes
repoNamebased on the toggle state- Wraps the fetch with measurement and captures telemetry events
161-179: LGTM!The global search toggle UI is well-implemented with proper accessibility via the tooltip and keyboard shortcut hint. The Toggle state binding is correct.
packages/web/src/ee/features/codeNav/components/symbolHoverPopup/useHoveredOverSymbolInfo.ts (2)
59-76: LGTM!The query implementation correctly threads
repoNamefor repository-scoped search, includes it in the query key, and captures telemetry with duration measurement.
142-186: Solid implementation for highlight range extraction.The coordinate-to-position conversion handles null cases correctly and the column computation properly converts to 1-based indexing. The dependency array is appropriate.
One consideration: using
rect.top + rect.height / 2for the y-coordinate works well for single-line symbols. If multi-line symbols are possible, the end position calculation might not capture the full range accurately.packages/web/src/app/[domain]/browse/[...path]/components/pureCodePreviewPanel.tsx (1)
156-165: LGTM!The migration from callback-based navigation (
onFindReferences/onGotoDefinition) to data-driven props (fileName,repoName,source) is cleaner and aligns with the PR's approach of threading repository context through the code navigation system.packages/web/src/features/chat/components/chatThread/referencedFileSourceListItem.tsx (1)
259-267: LGTM!The updated
SymbolHoverPopupusage correctly passes the new data-driven props (source,repoName,fileName) instead of the removed callback props. This aligns well with the broader refactor to centralize navigation logic within theSymbolHoverPopupcomponent itself.packages/web/src/app/[domain]/search/components/codePreviewPanel/codePreview.tsx (1)
207-216: LGTM!The
SymbolHoverPopupintegration is well-structured:
- Properly gated by
hasCodeNavEntitlement- Correctly passes
source="preview"to identify this context- Uses
file.filepathforfileNamewhich is consistent with the file's content modelpackages/web/src/ee/features/codeNav/components/symbolHoverPopup/index.tsx (4)
88-107: Well-designed definition prioritization.The logic to prefer definitions within the current file/repo context before falling back to the first available definition is sound and aligns with the PR objective of scoping navigation to the current repository by default.
166-199: LGTM!The
onFindReferencesimplementation correctly uses the current file context (repoName,fileName,revisionName,languagefrom props) for navigation and browse state, which ensures references are searched from the user's current position.
215-221: LGTM!The hotkey handler correctly calls
onFindReferences()without parameters since the callback now obtainssymbolInfovia closure from theuseHoveredOverSymbolInfohook.
261-284: LGTM!The UI consistently uses
previewedSymbolDefinitionfor rendering the preview, disabling the button, and displaying appropriate labels. The state transitions are handled correctly.
We were seeing that on large deployments, code navigation queries were sometimes taking several seconds to resolve. This was because we were searching across all repositories to resolve references / definitions.
This PR changes the default behaviour to only search in the current repository where the user is. I've also added a toggle button to allow searching across all repositories as well:
The rationale / intuition here is that, most of the time, symbol are only referenced within the repository they are defined in. There are exceptions of course (e.g., methods exported from a shared library), but most of the time this will be the case. Besides speed, the other benefit of scoping to the current repository as it reduces the false-positive rate inherent with search-based code navigation (e.g., if function
doSomethingis declared in multiple function).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.