feat(repo): add bounded lazy git repository discovery with workspace … - #1034
feat(repo): add bounded lazy git repository discovery with workspace …#1034shauryagangrade wants to merge 22 commits into
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:
📝 WalkthroughWalkthroughRepository discovery scans workspace directories for Git repositories, tracks selection through a React hook, wires that state through ChangesRepository discovery and selection
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/app/components/WorkspaceSurface.tsx (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline repo shape duplicates the exported
GitRepotype.
Array<{ repoRoot: string; name: string; type: "root" | "submodule" | "nested" }>re-declaresGitRepofromsrc/modules/repo/discover.ts(already re-exported viasrc/modules/repo/index.ts). Importing it keeps this prop type in sync automatically if the shape evolves.♻️ Proposed fix
+import type { GitRepo } from "`@/modules/repo`"; ... - detectedRepos?: Array<{ repoRoot: string; name: string; type: "root" | "submodule" | "nested" }>; + detectedRepos?: GitRepo[];🤖 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 `@src/app/components/WorkspaceSurface.tsx` around lines 43 - 46, Update the detectedRepos prop in WorkspaceSurface to use the exported GitRepo type from the repo module instead of an inline object shape, importing the type through the existing repo index export and preserving the current optional prop behavior.src/modules/repo/discover.test.ts (1)
86-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertions don't verify
repoRoot/name, which would have caught the.git-path bug.
"respects maxResults option"only checksrepos.length, and"sorts results with root first, then alphabetically"only checkstypeand a self-referential name sort — since bothzebraandalphabranches currently resolve to the same (buggy).gitname, the alphabetical assertion is vacuously true and wouldn't fail even if names were wrong. Strengthening these to assert concreterepoRoot/namevalues would surface regressions like the one flagged indiscover.ts.Also applies to: 159-172
🤖 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 `@src/modules/repo/discover.test.ts` around lines 86 - 97, The tests in “respects maxResults option” and “sorts results with root first, then alphabetically” need concrete assertions for each returned repository’s repoRoot and name. Update the expectations to verify the exact discovered paths and names, including the workspace root and alphabetically ordered repositories, while retaining the maxResults limit assertion.
🤖 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 `@src/modules/repo/discover.ts`:
- Around line 53-56: Replace the four symlink-containment prefix checks in the
discovery logic, including those around the seen-path checks, with the existing
isWithinRoot(realPath, realWorkspaceRoot) helper. Apply the same boundary-aware
validation to normalizedRoot and realPath at all referenced sites, preserving
the existing seen-path and traversal behavior.
- Around line 109-167: Update both repository-registration branches in the
directory traversal around the .git directory and .git file handling to set
repoRoot to the canonicalized parent working-tree directory dir, not the .git
entry or resolved gitdir storage path. Use that working-tree path for
getRepoName and seenRealPaths checks so the workspace root is not duplicated and
submodules report their checkout directories.
In `@src/modules/repo/useRepoDiscovery.ts`:
- Around line 25-61: Add dedicated tests for the hook containing the discovery
effect, covering stale-result suppression when a newer workspace discovery
completes first, cancellation after unmount, and preservation of the existing
currentRepoRoot when it remains in the refreshed repository list. Also verify
selection falls back appropriately when it no longer exists, while keeping
discoverRepositories itself covered by its existing tests.
---
Nitpick comments:
In `@src/app/components/WorkspaceSurface.tsx`:
- Around line 43-46: Update the detectedRepos prop in WorkspaceSurface to use
the exported GitRepo type from the repo module instead of an inline object
shape, importing the type through the existing repo index export and preserving
the current optional prop behavior.
In `@src/modules/repo/discover.test.ts`:
- Around line 86-97: The tests in “respects maxResults option” and “sorts
results with root first, then alphabetically” need concrete assertions for each
returned repository’s repoRoot and name. Update the expectations to verify the
exact discovered paths and names, including the workspace root and
alphabetically ordered repositories, while retaining the maxResults limit
assertion.
🪄 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: CHILL
Plan: Pro Plus
Run ID: ec6ecbca-51ff-4f16-82c2-21accb2992f4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
src/app/App.tsxsrc/app/components/WorkspaceSurface.tsxsrc/modules/repo/discover.test.tssrc/modules/repo/discover.tssrc/modules/repo/index.tssrc/modules/repo/useRepoDiscovery.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/modules/repo/useRepoDiscovery.test.ts`:
- Around line 4-7: Replace the current discovery-call-only tests with tests that
import and mount useRepoDiscovery, using controlled deferred
discoverRepositories promises to exercise the real effect. Assert state
transitions for stale-result handling, cleanup, null-root reset, and selection
preservation, including the relevant terminal/workspace discovery invariant, so
the tests fail when the hook implementation regresses.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c03ea180-5fce-40b0-ab62-f59b03341244
📒 Files selected for processing (4)
src/app/components/WorkspaceSurface.tsxsrc/modules/repo/discover.test.tssrc/modules/repo/discover.tssrc/modules/repo/useRepoDiscovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/components/WorkspaceSurface.tsx
- src/modules/repo/discover.test.ts
- src/modules/repo/discover.ts
…picker - Add src/modules/repo module with discoverRepositories() that recursively finds git repos (root, submodule, nested) bounded by depth, count, and wall-clock time limits - Handle .git as both directory and file (submodule pointer) with proper working-tree repoRoot resolution - Enforce symlink/workspace containment via canonicalized path checks - Add useRepoDiscovery hook with race-condition protection and selection - Wire up repo picker dropdown in WorkspaceSurface (shown for 2+ repos) - Add tests covering discovery logic and hook state transitions - Add happy-dom as devDependency for hook tests with real DOM
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 `@src/modules/repo/useRepoDiscovery.test.ts`:
- Line 34: Remove the em dash characters from the comments in useRepoDiscovery
tests, including the comments near the test harness and the other referenced
locations. Replace them with guideline-compliant punctuation or wording while
preserving the comments’ meaning.
- Around line 234-253: Remove the “does not update state after unmount” test
because its harness cannot observe the hook’s cancellation guard and only
verifies React’s built-in no-op behavior. Keep the existing stale-result test
around the hook’s sequence/cancellation logic as the test that locks the
relevant invariant.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 0a53e861-edbf-4535-90d1-0c65acaaba9d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.jsonsrc/modules/repo/useRepoDiscovery.test.ts
…test - Replace em dash characters with guideline-compliant punctuation in test comments - Remove "does not update state after unmount" test that only verifies React's built-in no-op behavior; the stale-result test already covers the hook's cancellation invariant
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/App.tsx (1)
1270-1272: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not expose a no-op repository picker.
onRepoChangeonly updates the hook state, while Git consumers still useexplorerRoot; selecting a nested repository changes the picker label but not the Git context. ThreadcurrentRepoRootthrough the Git consumers, or hide the picker until selection has an effect.🤖 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 `@src/app/App.tsx` around lines 1270 - 1272, Update the repository-selection flow around onRepoChange so choosing a detected repository changes the Git context used by downstream Git consumers, threading currentRepoRoot through the relevant consumers and replacing their explorerRoot usage where appropriate. If that integration cannot be completed, hide or disable the repository picker instead of exposing a selection that only changes its label.
🤖 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.
Outside diff comments:
In `@src/app/App.tsx`:
- Around line 1270-1272: Update the repository-selection flow around
onRepoChange so choosing a detected repository changes the Git context used by
downstream Git consumers, threading currentRepoRoot through the relevant
consumers and replacing their explorerRoot usage where appropriate. If that
integration cannot be completed, hide or disable the repository picker instead
of exposing a selection that only changes its label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f502d5e-83bc-49f5-996a-4f500506cc71
📒 Files selected for processing (1)
src/app/App.tsx
|
Thanks @shauryagangrade for working on this, the direction makes sense! A few blockers before we can merge:
And please remove the accidental package-lock.json since the project uses pnpm :) The hook structure and tests are a good start. Once these core issues are addressed, I’d be happy |
Move git repository discovery from frontend TypeScript (multiple IPC calls via native.readDir with JS traversal) to a single Rust command (git_discover_repos) that: - Reads all directory entries including dot-prefixed (.git visibility) - Uses Path::starts_with for safe path-component boundary checks - Skips symlinks to prevent workspace escape - Authorizes discovered repos for subsequent git operations - Handles submodules (.git file with gitdir pointer) - Enforces cooperative timeout (150ms default) Fixes: multiple sequential IPC calls, unsafe startsWith() path checks, frontend traversal following symlinks outside workspace, .git entries hidden by native.readDir(). The picker's selectedRepoRoot was already wired through useSourceControlContext to Source Control and Git History.
|
@crynta Hello! I appreciate you taking out the time to review my PR. I've implemented the changes you mentioned. Have a look at them, and tell me your thoughts! |
- App.tsx: use optional chaining instead of null check - AiChat.tsx: fix hook ordering, remove non-null assertion, stable keys - AiComposerInput.tsx: fix dependency arrays, add biome-ignore - AiStatusBarControls.tsx: memoize hasKeyFor with useCallback - LocalAgentNotificationsBridge.tsx: extract liveStatus to variable - PlanDiffReview.tsx: stable keys, add biome-ignore - useWhisperRecording.ts: forEach to for-of, memoize sttOptions, useCallback teardownStream - composer.tsx: remove ref from deps, wrap attachFileByPath in useCallback - security.ts: use template literals - security.test.ts: fix template curly, add biome-ignore comments - ShortcutsSection.tsx: fix key usage
- Fix useLiteralKeys in stt.ts - Add biome-ignore for noAssignInExpressions in edit.ts, colorSwatches.ts, markdownExtras.ts - Fix useExhaustiveDependencies in EditorPane.tsx, GitDiffPane.tsx, useDocument.ts, FileExplorer.tsx - Use template literals in inlineExtension.ts and prompt.ts - Rename unused catch variable in inlineExtension.ts - Add role="tree" and remove tabIndex in FileExplorer.tsx for a11y compliance - Use optional chaining in FileExplorer.tsx - Use stable key in ShortcutsSection.tsx - Add biome-ignore for noAccumulatingSpread in fileIcons.ts and folderIcons.ts
Extract doc.size into a docSize variable to fix two biome lint warnings about React hook dependencies and a TypeScript error. The original code used a conditional expression in the dependency array that was both incomplete (missing doc.size) and unnecessary. The fix extracts the conditional access into a variable and uses it consistently in both the dependency array and the effect body.
2 participants
…picker
What
Adds a new
src/modules/repomodule that recursively discovers git repositories (root, submodule, nested) within a workspace, capped by depth, result count, and wall-clock time budgets. A dropdown picker inWorkspaceSurfacelets users switch between detected repos when multiple are found.Closes #952
Why
Workspaces often contain multiple git repositories (monorepos, submodules, nested clones). Without discovery, the app only knows about the workspace root and can't distinguish between repos. This feature is the foundation for scoping terminals, editors, and source-control views to a specific repository.
How
Bounded lazy walk —
discoverRepositories()runs only when the workspace root changes (not at startup, not on a timer). It recurses up tomaxDepth: 3directories, returns at mostmaxResults: 20repos, and hard-aborts aftertimeoutSecs: 150msviaperformance.now()checks before every directory read..gitas directory vs file —.gitdirectories are recognized and added without descending into them..gitfiles (submodule pointers) are read and parsed forgitdir: <path>, with relative paths resolved via@tauri-apps/api/path.join.Symlink containment — Both the workspace root and every candidate repo root are canonicalized via
native.canonicalize(). AstartsWith(realWorkspaceRoot)check ensures only repos within the workspace boundary are included. AseenRealPathsSet deduplicates repos reachable through multiple symlink paths.Repo picker UI — A
DropdownMenuinWorkspaceSurface(top-right) appears only when 2+ repos are detected. Shows repo name, type badge for submodules/nested, and a checkmark for the active selection.React hook —
useRepoDiscoverywraps the async call with sequence-number deduplication (stale-result suppression), cancellation on unmount, and selection preservation across re-discoveries.Zero new dependencies — Uses only the existing
nativeTauri IPC bridge and@tauri-apps/api/path(both already in the project). Nofs.watch, no polling, no background threads.Testing
pnpm lintcleanpnpm check-typescleanpnpm testclean (486/486 tests pass, including 10 new tests fordiscoverRepositories)pnpm tauri devUnit tests (
src/modules/repo/discover.test.ts)10 tests covering:
[]maxResultscapmaxDepthcap.gitfile (submodule pointer) parsing.hidden→ skipped,.git→ processed)canonicalizefailure fallback to manual normalizationScreenshots / GIFs
(TODO: add screenshot of the repo picker dropdown in a multi-repo workspace)
Notes for reviewer
DiscoverOptions.getRepoNameextracts the basename of the repo root path. For repos with identical directory names (e.g. twolib/folders), the name alone won't disambiguate — a follow-up could append the relative parent path.currentRepoRootstate. Wiring it to scope terminal/editor/git-history views to the selected repo is left for a follow-up to keep this PR focused.@tauri-apps/api/path.joinreturnsPromise<string>(async), which is why everyjoin()call is awaited. This is consistent with Tauri v2's webview API.src-tauri/changes in this PR.Summary by CodeRabbit
Summary by CodeRabbit
.gitfiles), skips inaccessible folders, prevents duplicates, enforces timeout/depth/max results, and keeps consistent ordering.happy-domto dev dependencies for test support.