Conversation
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 1
🧹 Nitpick comments (1)
src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the test comments.
Replace each multi-line comment with one concise line, or remove it when the test or helper name states the intent.
src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts#L1-L6: Reduce the file header to one concise comment.src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.ts#L61-L62: Reduce the queue-drain explanation to one concise comment.As per coding guidelines, comments must be concise, limited to non-obvious information, and preferably one line.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42953ec4-8ac9-48b9-ae8b-79c9043d89e0
📒 Files selected for processing (2)
src/main/runtime/orca-runtime-fork-upstream-ssh-backfill.test.tssrc/main/runtime/orca-runtime.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/ssh-docker-fork-upstream-backfill.spec.ts (1)
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the file header comment.
The header repeats issue history and the test flow. Keep only the non-obvious fixture constraint, or move issue context to test metadata.
As per coding guidelines, “Comments must be concise, limited to non-obvious information, and preferably one line.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10da00d2-6152-4f10-b90d-566ad33a4426
📒 Files selected for processing (3)
config/scripts/run-ssh-docker-fork-upstream-backfill-e2e.mjspackage.jsontests/e2e/ssh-docker-fork-upstream-backfill.spec.ts
A fork on an SSH host kept `upstream === undefined` until someone opened its settings page, so its GitHub Project rows stayed hidden and its fork badge never rendered. Run the one-shot backfill for a connection's repos when that connection reports `connected` — the first moment the probe can succeed — instead of skipping SSH repos outright. All passes now share one chain, so a burst of connects queues behind the startup pass rather than fanning out `gh repo view` past its semaphore. Fixes #12967
Drives the actual #12967 sequence end to end: a legacy repo whose persisted `upstream` was stripped, an app relaunch, then an SSH connect that must backfill it and render the fork badge with no settings visit. Deterministic and offline — getRepoUpstream short-circuits on the `upstream` git remote before it reaches `gh`, so the container's two remotes decide the answer with no GitHub auth or network. Verified by revert: with the fix backed out, upstream stays unresolved for the full 90s after connect.
fd6573d to
665ca63
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This branch has not been deployed
1 participant
Summary
Legacy fork repos on SSH hosts could remain
upstream === undefinedforever. Startup backfill skipped remote repos because their hosts were not connected yet, and no later hook retried them. Sincerepo.upstreamdrives GitHub Project matching, the fork badge, and auto-detected avatar migration, affected repos showed an empty Projects board and no fork indicator until the user happened to open that repo's Settings page.This PR runs the existing fork-upstream migration when an SSH connection becomes ready. It stays off the connection critical path and persists only a positively identified upstream for SSH repos.
Fixes #12967.
Behavior
connectedschedules a fire-and-forget pass for unresolved repos belonging to that SSH connection.nullis not persisted because the existing API cannot distinguish a confirmed non-fork from a transient Git/ghfailure. Positive upstreams are persisted; ambiguous repos retry on the next app launch or explicit Settings lookup.Reproduction
The regression invariant is: after a ready SSH provider publishes
connected, a legacy repo withupstream === undefinedmust be probed and persisted without opening Settings.The focused runtime test passes with this PR. Temporarily removing only the
connected -> backfillForkUpstreamsForConnectionedge makes the same test fail with zero upstream probes; restoring the edge makes it pass again.The Docker E2E recreates the real user sequence with an
sshdcontainer and deployed relay: add a remote fork, strip its persisted upstream to model legacy state, restart Electron, verify the badge is absent, connect, then verify the upstream is persisted andFork of stablyai/orcaappears. It is deterministic and offline because the configured Git remotes determine the answer before any GitHub API call.Testing
pnpm typecheckpnpm lint(only nine pre-existingWorktreeJumpPalette.tsxhook warnings frommain)pnpm run check:code-quality:changed— 0 findingspnpm test:e2e:ssh-docker-fork-upstream-backfill— 1 passed against Docker SSH + Electrongit diff --checkReview follow-ups
CodeRabbit's two valid findings were fixed:
shell: true; it invokes pnpm's JS CLI through Node and tests whitespace-bearing forwarded arguments.Three internal review/fix rounds found no remaining in-scope correctness, performance, compatibility, or regression issues.
Known limitations
getRepoUpstreamstill lacks a strictfound / confirmed-not-fork / unavailableresult. SSH therefore leaves ambiguousnullunresolved instead of persisting a potentially false negative.orca servedoes not run the notifier-owned startup migration. That trigger-plumbing gap should be a separate PR.