Repository navigation
Keep repository detection off the main registration thread - #26760
Conversation
Share repository discovery decisions across synchronous compatibility callers and the existing asynchronous Git runner. Await discovery in project registration, nested imports, and folder upgrades, and recheck folder ownership after discovery completes.
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to The registration test does not confirm that the project appears in the sidebar. Add that assertion; no production failure is established. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 18.52% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 21 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly summarizes the main change: moving repository detection off the main registration thread through asynchronous execution. Description check The description is detailed and covers the required explanation, changes, rationale, issue reference, testing, review notes, compatibility considerations, and checklist. The visual-proof section does …
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
-
-
-
-
📝 Generate docstrings 💡
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
…pc-audit-local-1008
…to nwparker/win-issues-ipc-audit-local-1008
nwparker
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (7)
src/main/ipc/repos/local-repo-registration.ts (1)
27-27: Concurrent registrations of the same path can both pass the dedupe check.The code now awaits
inspectGitRepoForRegistrationAsyncandgetGitRepoRootAsyncbefore thestore.getRepos()dedupe checks. Two concurrent calls for the same path both yield at these awaits. The existingraceWinnercheck afterdetectRepoIconAndUpstreamruns synchronously up tostore.addRepo. This keeps the race window closed, so no duplicate is added.No change is required.
Also applies to: 59-61
src/main/git/repo-detection.ts (1)
240-266: LGTM!src/main/git/repo.ts (1)
14-18: LGTM!tests/e2e/repo-registration-async.spec.ts (1)
1-180: LGTM!config/scripts/run-ssh-docker-e2e.mjs (1)
57-57: LGTM!src/main/git/repo-detection-async.test.ts (1)
76-76: 🎯 Functional CorrectnessThe fallback test and
runGitRepoCommandsuse the same./runnermodule export. The proposed module-resolution mismatch does not exist.src/main/ipc/repos-add-linked-worktree.test.ts (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Make the async mocks return Promises.
The real
isGitRepoAsync,inspectGitRepoForRegistrationAsync, andgetGitRepoRootAsyncreturn Promises. The mocks return plain values.awaithides the mismatch. Code that calls.thenon the result would break only in production.Use
mockResolvedValue-style wrappers to preserve the real signature.Based on learnings: mock functions must preserve the sync/async signature of the real implementation they replace.
Proposed change
- isGitRepoAsync: isGitRepoMock, - inspectGitRepoForRegistrationAsync: vi.fn((path: string) => ({ + isGitRepoAsync: vi.fn(async (path: string) => isGitRepoMock(path)), + inspectGitRepoForRegistrationAsync: vi.fn(async (path: string) => ({ isRepo: isGitRepoMock(path), rootPath: getGitRepoRootMock(path), mainRepoPath: getLinkedWorktreeMainRepoRootMock(path) })), getGitRepoRoot: getGitRepoRootMock, - getGitRepoRootAsync: getGitRepoRootMock, + getGitRepoRootAsync: vi.fn(async (path: string) => getGitRepoRootMock(path)),Source: Learnings
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
464512fd-42c2-481a-a69f-0411cbfb85c9
📒 Files selected for processing (7)
config/scripts/run-ssh-docker-e2e.mjssrc/main/git/repo-detection-async.test.tssrc/main/git/repo-detection.tssrc/main/git/repo.tssrc/main/ipc/repos-add-linked-worktree.test.tssrc/main/ipc/repos/local-repo-registration.tstests/e2e/repo-registration-async.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
…pc-audit-local-1008
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
529a6a3f-3b99-48b5-988f-5444f60c8a79
📒 Files selected for processing (4)
src/main/git/repo-detection-async.test.tssrc/main/git/repo-detection-command.test.tssrc/main/git/repo-detection-failure-parity.test.tstests/e2e/repo-registration-async.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| expect( | ||
| await orcaPage.evaluate(async () => (await window.api.repos.list()).map((repo) => repo.id)) | ||
| ).toEqual(result.persistedIds) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,205p' tests/e2e/repo-registration-async.spec.ts
rg -n 'E2E Assertions Must Target|thing a spec finally|project row|sidebar' tests/e2e/AGENTS.md tests/e2e/*registration* tests/e2e/helpers | head -95Repository: stablyai/orca
Length of output: 10453
🏁 Script executed:
set -e
printf '%s\n' '--- sidebar/project DOM references ---'
rg -n -F --glob '*.{ts,tsx}' -- 'Worktrees' src tests/e2e || test "$?" -eq 1
rg -n --glob '*.{ts,tsx}' 'role="(option|listbox|treeitem)"|getByRole\(.{0,80}(option|listbox|treeitem)|data-(project|worktree|sidebar)' src tests/e2e || test "$?" -eq 1
printf '%s\n' '--- comparable registration/project DOM assertions ---'
rg -n --glob 'tests/e2e/**/*.ts' 'repos\.add|addRemote|project.*row|sidebar.*project|getByRole\(' tests/e2e || test "$?" -eq 1
printf '%s\n' '--- relevant package/source file names ---'
rg --files src tests/e2e | rg '(sidebar|project|worktree|repo-registration)' | head -120Repository: stablyai/orca
Length of output: 45665
🏁 Script executed:
set -e
printf '%s\n' '--- exact sidebar selectors in renderer ---'
rg -n -F -- 'data-worktree-sidebar' src/renderer/src tests/e2e || test "$?" -eq 1
rg -n -F -- 'data-project-group-header-id' src/renderer/src tests/e2e || test "$?" -eq 1
printf '%s\n' '--- project-group assertion context ---'
sed -n '1,215p' tests/e2e/project-group-manual-sort.spec.ts
printf '%s\n' '--- candidate sidebar component declarations ---'
rg -n --glob '*.tsx' 'ProjectGroup|project group|worktree-sidebar|role=.option.|aria-label' src/renderer/src/components src/renderer/src | head -180Repository: stablyai/orca
Length of output: 41215
🏁 Script executed:
set -e
printf '%s\n' '--- catalog settlement and registration test references ---'
rg -n -F -- 'awaitLocalRepoCatalogSettlement' src tests/e2e
rg -n -F -- 'fetchRepos' src/renderer/src/store src/renderer/src/components/sidebar | head -80
printf '%s\n' '--- sidebar row declarations ---'
sed -n '1,280p' src/renderer/src/components/sidebar/worktree-list/rows/SectionHeader.tsx
sed -n '280,390p' src/renderer/src/components/sidebar/worktree-list/viewport/VirtualizedWorktreeViewport.tsx
printf '%s\n' '--- repo-header and option rendering ---'
rg -n --glob '*.tsx' 'data-repo-header-id|role="option"|data-worktree-id' src/renderer/src/components/sidebar/worktree-listRepository: stablyai/orca
Length of output: 28281
🏁 Script executed:
set -e
printf '%s\n' '--- default sidebar grouping ---'
rg -n --glob '*.{ts,tsx}' 'groupBy\s*:\s*['"'"'"'"'"']repo|groupBy\s*=\s*['"'"'"'"'"']repo|default.*groupBy|groupBy.*default|setGroupBy' src/renderer/src/store src/renderer/src/app-shell src/renderer/src/components/sidebar | head -140
printf '%s\n' '--- catalog settlement implementation ---'
sed -n '70,185p' src/renderer/src/store/repos/repo-catalog-actions.ts
printf '%s\n' '--- comparable visibility test setup/assertion ---'
sed -n '1,155p' tests/e2e/project-group-creation-visibility.spec.tsRepository: stablyai/orca
Length of output: 17368
Assert the registered project in the sidebar.
The E2E rule requires the final expect() to target user-visible DOM. The registration test currently ends with window.api.repos.list() and has no DOM assertion. No exception applies.
Keep the API checks as supporting assertions, then assert the single repository header in the sidebar. Use the repository-header selector instead of counting generic role="option" elements, which represent worktree rows.
🐛 Suggested fix
--- "a/tests/e2e/repo-registration-async.spec.ts"
+++ "b/tests/e2e/repo-registration-async.spec.ts"
@@ -77,10 +77,11 @@
join(root, 'missing')
)
expect('error' in missing).toBe(true)
expect(
await orcaPage.evaluate(async () => (await window.api.repos.list()).map((repo) => repo.id))
).toEqual(result.persistedIds)
+ await expect(orcaPage.locator('[data-worktree-sidebar] [data-repo-header-id]')).toHaveCount(1)
})
test('SSH registration reads the repository on its execution host', async ({
orcaPage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect( | |
| await orcaPage.evaluate(async () => (await window.api.repos.list()).map((repo) => repo.id)) | |
| ).toEqual(result.persistedIds) | |
| expect( | |
| await orcaPage.evaluate(async () => (await window.api.repos.list()).map((repo) => repo.id)) | |
| ).toEqual(result.persistedIds) | |
| await expect(orcaPage.locator('[data-worktree-sidebar] [data-repo-header-id]')).toHaveCount(1) |
1 participant
ELI5
Adding a Git project lets unrelated Orca requests continue while Git identifies the repository.
What Changed
Local and paired-runtime registration await the existing Git runner. A single decision tree serves synchronous compatibility callers and async callers, preserving repository roots, linked worktree identity, bare repositories and metadata fallback. The existing Docker SSH lane includes registration coverage. Nested import and folder upgrades remain separate PRs.
Why
Sharing the classifier avoids maintaining parallel detection rules. The runner retains host environment preparation, WSL routing, admission and the same 15-second per-command limit. Earlier Windows timings (39.6–48.7 ms blocked IPC versus 2.5–5.8 ms response) belong to the original pre-split source; they are not final-head measurements.
Linked Issue
Related to #20497. Merge this before #26790 and #26791; the registration dedup fix #26789 is already on main.
Visual Proof
No layout change. Hidden-renderer captures below explain the previous CI failure: one populated host suppresses headers; the upstream fixture correction adds a real local project and retains the managed header assertion.
Fresh hidden Windows combined-stack run: actual WSL registration retains the literal dollar sign and space. Source-manifest and lockfile qualifications are recorded below.
Testing
Final head
1fdc56bf558cd5bb9156124ae02e9e536f31f814, refreshed onto mainb3aa719a57ca.pnpm tcand changed-code quality pass. Fresh Windows combined validation merge18159658119f1000e339200feaa8b3ad49f028e4matches the expected tracked-file manifest; fresh install, CLI/Electron builds and four shipped hidden tests pass: native/concurrent registration, real WSL literal-dollar/space paths, and folder promotion. Two auxiliary checks also pass: native path/classifier parity with actual unavailable-Git metadata fallback, and linked routing with a WSL hint. Docker SSH skips on Windows. The banner capture utility is separate from production login-shell fallback.Review
Low incremental code merge risk after independent review and path/failure parity checks. Metadata fallback still performs synchronous filesystem reads. The paired-runtime icon-discovery duplicate-registration race predates this change and remains separate. Real WSL path registration and the captured-shell utility test are distinct; production login-shell fallback has mocked runner coverage, not a claimed real-host fallback run. Final-head CI passes, including Windows packaging. The auxiliary Windows fixture later reported lockfile drift; its diff was unavailable, so exact post-install dependency reproducibility is unverified.
Agent skill upstream boundary
Checklist