Repository navigation
fix(github): send a paired server its own repo id when an SSH row shares the path - #33257
Conversation
When a repo path is registered on both SSH and a paired runtime, path-only lookup could send the SSH-side repo ID to the runtime. Prefer the row owned by the target runtime, then retain the original first-path fallback. AI-Assisted: OpenAI Codex Signed-off-by: ethereal49 <hongyeluo49@gmail.com>
…r's repo id getRuntimeRepoTarget now takes the caller's repo identity and resolves the row through the repo lookup index, preferring the row on the target runtime with that id, then the row on that runtime for the path. This keeps the refresh sweep's repo reads linear (a per-call list scan broke that ratchet). Legacy rows with no host stamp still fall back to id-or-path lookup. Removes the test that pinned the off-host fallback; it asserted sending another host's repo id to the server.
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Linked Issues check |  |
For #32997, the reviewed changes address the GitHub path. getRuntimeRepoTarget now receives the caller repo ID and path, uses findOnHost, and routes PR refresh and work-item requests to the runtim… |
Provide reviewable evidence that the duplicate-path case in #32997 already works for Gitea, including the relevant routing implementation and regression coverage. Without that evidence, Gitea compliance cannot be determined. |
-
Check name Status Explanation Description check The description is complete and follows the repository template. It explains the problem, user-visible behavior, implementation, rationale, linked issue, testing, known test exception, untested enviro… Title check The title is concise, specific, and accurately describes the primary fix: sending the paired server its own repository ID when SSH and runtime registrations share a path. Out of Scope Changes check The changed production files update GitHub repository routing and lookup. The added tests cover PR refresh and work-item routing for the reported duplicate-path case. These changes support #32997and …Full details: Description check
-
Explanation
The description is complete and follows the repository template. It explains the problem, user-visible behavior, implementation, rationale, linked issue, testing, known test exception, untested environments, and checklist status.
Full details: Out of Scope Changes check
-
Explanation
The changed production files update GitHub repository routing and lookup. The added tests cover PR refresh and work-item routing for the reported duplicate-path case. These changes support
#32997and no unrelated change is demonstrated.
-
Full details: Linked Issues check
-
Explanation
For
#32997, the reviewed changes address the GitHub path.getRuntimeRepoTargetnow receives the caller repo ID and path, usesfindOnHost, and routes PR refresh and work-item requests to the runtime repo ID. The added tests cover both request types.#32997also states that the bug affects Gitea. The reviewed changes and reported tests provide no Gitea-specific evidence. Existing Gitea behavior is not established by the available evidence.Resolution
Provide reviewable evidence that the duplicate-path case in
#32997already works for Gitea, including the relevant routing implementation and regression coverage. Without that evidence, Gitea compliance cannot be determined.
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.
ELI5
One folder can be known to Orca twice: once as an SSH repo and once as a repo on a paired Orca server. Each registration has its own id. When Orca asked the server for that repo's PR status, it could send the SSH registration's id, which the server has never seen. The request failed quietly and PR status never appeared. Now Orca sends the server's own id.
What Changed
Mechanism:
getRuntimeRepoTarget(src/renderer/src/store/github/repository-routing.ts) used to look the repo up by path alone, where the first registration wins. It now takes the caller's repo id as a required argument. It picks the row that lives on the target server: first the row with that id and path, then any row with that path on that server. A newfindOnHostin the existing repo lookup index (github-repo-lookup-index.ts) does this lookup, so the refresh sweep's repo reads stay linear. Rows with no host stamp (legacy) still fall back to the id-or-path lookup.This builds on @Ethereal49's #33194; their commit is kept with their authorship. Added on top:
Why
GitLab, hosted reviews and the other providers already resolve the repo row first and then route by that row's host, sending its id. GitHub was the outlier: it chose the server from the row, then looked the repo up again by path alone. Making the repo identity a required parameter at this one choke point fixes every GitHub caller (PR refresh, work items, counts, checks, comments, review threads). A per-call-site patch would have missed some.
Linked Issue
Fixes #32997
Thanks to the reporter for the precise root-cause analysis in #32997, and to @Ethereal49 for #33194.
Related, not fixed here: #33219 (GitLab tab
listMRsfails for paired-server repos) is already fixed on main by #27247. That change merged after the v1.4.224 cut the reporter was running, and ships in v1.4.235 and later. The existing test inuse-task-page-gitlab-loading.test.tsxcovers it.Visual Proof
N/A: this changes only which repo id is sent to the paired server. The visible effect, PR status appearing, is checked in a live run (see Testing).
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Unit tests: two regression tests (from fix(github): route runtime repo paths by execution host #33194), one for PR refresh and one for work items. With an SSH row and a runtime row sharing one path, they assert the runtime RPC receives the runtime repo id. Both fail on origin/main, where the SSH id is sent, and pass with this change.
All test files in
src/renderer/src/store/slices/github*,src/renderer/src/store/github/andhosted-review*pass, run single-worker (51 files, 596 tests).Live run on macOS (local only, not checked in). A headless paired runtime host from the e2e harness, with an isolated profile and
ORCA_BACKGROUND_LAUNCH=1, plus a stubghon the server's PATH (no real GitHub, no tokens). A paired desktop client with an SSH row injected for the same path, listed first. ThenfetchPRForBranchwith a unique probe branch.ghfor the probe branch (the request was rejected for the unknown SSH id).gh api repos/acme/repo/pulls?head=acme:repro-32997-probe.pnpm tc:weband the full local static job pass. The one exception isorcad-terminal-smoke, which ran only because the change-scope check couldn't read the dependency graph here, and then failed because it needs anout/mobile-webbuild. This PR touches only renderer store files.Not tested: Windows or WSL hosts live, or a real SSH host. The SSH row was injected into the store.
Review
Focus:
findOnHostingithub-repo-lookup-index.ts, and the fallback order ingetRuntimeRepoTarget.Agent skill upstream boundary
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)