GitDesktop / Tickets / #88 feat(lan,companion,settings): support sharing multiple repositories

Anonymous - 2026-07-19

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 7cb71b3

Status: ✅ Deploy successful!

Preview URL: https://bedf0fa3.gitdesktop.pages.dev

Branch Preview URL: https://feat-lan-companion-slice4.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

I've traced the full data flow across the new multi-repo LAN sharing feature. Verifying the key trust boundaries:

Scoped routes are authenticated. build_router merges the /api/repos/{repoId}/… scoped subtree, the alias subtree, and GET /api/repos into api, which is wrapped in auth::require_auth (per-device bearer) and the outer host_guard. The SSE stream route lives inside this protected subtree.

No path traversal via repoId. resolve_scoped_repo uses repoId purely as a HashMap lookup key against the registry (state.repos…get(&repo_id)); a miss returns 404 noSuchRepo. The filesystem path only ever comes from registry entries installed by the trusted desktop (share_repo/set_active_repo), never constructed from the wire id. An arbitrary/forged id just fails the lookup.

No path leak to the wire. /api/repos emits only {id, name, active}; the test pins the absence of path/repoPath.

Cut scoping preserves the auth invariant. I walked every branch of install_active_repo/unshare_repo/MonitorCut: a registry entry survives only while at least one role (active or shared) holds it, and a repo leaving the registry always fires either Repo(path) (scoped end) or All, while new connections to a removed id 404. There is no state where a phone retains access to a repo after both roles drop it.

Trusted inputs. lan_share_repo/lan_unshare_repo/lan_shared_repos_list are Tauri commands callable only from the desktop webview; the repo_path is the user's own path, not attacker-controlled. The phone-facing companion only ever calls read routes.

Frontend sinks are safe. repoId/streamId are grammar-validated (REPO_ID_RE, STREAM_ID_RE) and encodeURIComponent-encoded before building same-origin relative EventSource/hash URLs; replace() builds a same-origin path — no attacker-controlled host/protocol. React escapes the name/path display; no dangerouslySetInnerHTML.

No genuine security issues in these changes.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

Context for reviewers — slice 4 of the LAN phone-companion epic (epic-branch PR into epic/lan-companion; not master). Deliberate calls on record, so rounds can ground against them:

Registry = shared ∪ active. The repo open on the desktop is always reachable while sharing is on (unchanged invariant from slices 1–3). Consequence, live-validated: unsharing the currently-open repo intentionally does not cut phone access — it stops keeping the repo reachable once the desktop switches away. The unshare toast and settings copy state this.

Wire shapes. Repo ids are opaque 16-hex (first 8 bytes of sha256 over the worktree-stable identity); on-disk paths never reach the wire (/api/repos = {id, name, active}, pinned by a route test asserting the exact keys and the absence of path). Unknown, malformed, and real-but-unshared ids all return the identical 404 noSuchRepo — deliberate anti-enumeration; please don't suggest distinguishing them.

Scoped monitor cuts. MonitorCut::All on disable/rebind/device-revoke stays deliberately coarse; Repo(path) fires only when a repo leaves the registry. A non-matching cut re-arms the SSE forward loop by emitting an inert empty comment frame (a bare : line — valid SSE, ignored by EventSource; covered by the router-level test sse_monitor_survives_a_scoped_cut_on_another_repo and observed live). Lagged degrades to All fail-safe.

Path-vs-id asymmetry (accepted). Desktop "already shared" checks (panel + palette, same compare) match the stored path verbatim; the server dedups by resolved id. The rare same-repo-via-two-worktrees case degrades to a harmless idempotent no-op server-side. The server remains the correctness authority.

Definitive errors beat stale-data preference. Screens keep cached data + a stale banner on transient refetch errors, but noSuchRepo is definitive: RepoGoneState takes precedence even over cached content (found on a real phone mid-E2E, fixed, re-tested), and noSuchRepo is excluded from retries.

Persistence. lan-shared-repos.json stores paths only ({"repos":[{"path"}]}, unknown top-level keys preserved, atomic write + cache invalidation — the lan-devices.json idiom); names/ids are recomputed. Seeding at lan_enable silently skips paths missing on disk; the desktop list still shows them from the store (no "unavailable" affordance this slice, deliberately). Corrupt individual entries are skipped defensively; code-written data can't produce them.

Docs. README + marketing site have zero companion content by standing epic decision (held across #70/#75/#81/#84) — docs-sync lands in the epic→master close PR. The in-app help guide got a multi-repo truth pass and the changelog fragment is amended in this PR.

Palette pair. lan-share-current-repo / lan-unshare-current-repo have mutually exclusive enabled gates; the palette hides disabled actions, so exactly one is visible at a time — a toggle by design, not a missing action.

Companion routing. Legacy hashes (#status, #prs/4) still parse and auto-upgrade via hash-replace once /api/repos loads (bookmark back-compat). Repo ids are validated against /^[0-9a-f]{16}$/ at both entry points (router parse + picker list), making a malformed-id redirect loop unreachable. A repo-less route with a failed repos query renders a retryable error state; 401 still routes centrally to #pair.

Verification on record: cargo 861/861 + clippy -D warnings clean (8 new registry/cut tests incl. the wire-shape pin), full pnpm build chain green, scoped biome, changelog:check. Machine E2E: unauthenticated 401 on all three surfaces, pairing ceremony twice via curl, per-repo scoped fetches (distinct repos return their own branch/status), alias mount byte-identical for the active repo, store shape verified on disk. Real-phone E2E: sharing via settings and palette, picker switching, a live agent watch surviving a desktop repo switch (the slice's marquee behavior), and unshare-mid-browse landing the teaching state within one poll.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I've now confirmed the key behaviors against the actual code. Here's my review.

Summary

This is a large, carefully-built slice that extends the LAN companion from one shared repo to an active ∪ shared registry, with scoped routes, scoped SSE cuts, a persisted store, a repo picker, and desktop share/unshare controls. The Rust registry bookkeeping is thorough and well-tested, and the companion routing/redirect logic is thoughtful. There is one blocker: the device-level useRepos probe now runs on the pairing screen and can self-lock a revoked device out of re-pairing — a regression of a previously-fixed bug.

Correctness

blocker — companion/src/lib/queries.ts (useRepos) + companion/src/App.tsx (the unconditional useRepos() call). The new probe drops the enabled: !route.isPairing gate that useStatus had, and additionally sets refetchOnWindowFocus: true. App() calls useRepos() before the if (route.isPairing) return <Pair /> early-return, so on #pair it issues GET /api/repos. For a device that was paired then revoked, the browser still holds the gd_lan cookie, so that request is a present-but-invalid credential — and require_auth (src-tauri/src/lan/auth.rs:786-791) calls record_failure for exactly that case (only a missing credential is penalty-free, line 772-778). Because a phone "backgrounds/foregrounds constantly" during re-pairing (walking between desktop and phone to read the PIN), refetchOnWindowFocus: true re-fires /api/repos on each foreground, banking a failure each time until rate_check locks the IP out — so the revoked device can't re-pair. This is the precise self-lockout the round-6 PR [#75] fix documented in the queryCache.onError comment ("re-sent its dead cookie every poll … until it rate-limited itself out of RE-pairing"), and the old useStatus(active, !route.isPairing) fired zero authed traffic on #pair to prevent it. Fix: thread an enabled param into useRepos and pass !route.isPairing from App (and don't leave refetchOnWindowFocus firing on the pairing takeover).

Edge cases

should-fix — src-tauri/src/lan/routes/mod.rs (list_repos, the active flag). The registry dedups by opaque id, but active is computed by path (repo_paths_match(&repo.path, active)). These identity bases disagree for a repo with two worktrees where one is shared and the other is active: they collide on one id → one registry entry, and seed_shared (mod.rs, called on every lan_enable) does repos.insert(id, RegisteredRepo{ path: shared_worktree_path, … }), overwriting the entry's path with the shared worktree path. Then repo_paths_match(shared_worktree_path, active_worktree_path) is false, so the open repo is reported active: false on the wire and the picker shows no "Active" badge on the repo you're actually in. The repo_paths_match doc even asserts active/shared/stream/cut identity "must all agree" — but this one branch uses path where everything else uses id. Fix: derive active from id equality against the active repo's resolved id rather than path-matching the (possibly-clobbered) stored path.

Performance

nit — src-tauri/src/lan/routes/mod.rs (list_repos). repo_paths_match can canonicalize (filesystem stat) both paths, and it's invoked for every non-active entry while the repos mutex is held. /api/repos isn't polled (30s staleTime + focus), so it's low-frequency, but snapshotting the paths and doing the matching after dropping the lock would keep the registry mutex off the filesystem.

Readability / UX

nit — companion/src/components/Chrome.tsx (BottomNav) + App.tsx picker. On #repos with an ambiguous set (multiple repos, none active), route.repoId is null, so a bottom-nav tap goes to the legacy #${tab}, which the bootstrap effect resolves back to replace("#repos") — the tab momentarily grabs roving focus and bounces straight back. The tabs are effectively inert on the picker; consider hiding/disabling the bottom nav there rather than rendering self-bouncing links.

The Rust registry logic (active∪shared dedup-by-id, the scoped-cut install_active_repo/unshare_repo paths, the shared_index→repos lock ordering, and the store's unknown-key preservation) is sound and has good test coverage for the coexistence, switch, unshare, and seed cases.

Posted by GitDesktop — AI output, verify before acting on it.

Related

Tickets: ~~#75~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

Round-1 dispositions (AI review + audit; Copilot's three threads answered inline):

Blocker (useRepos on #pair → re-pair self-lockout): accepted, fix in the next push. Verified end-to-end: useRepos() mounts unconditionally in App() with no enabled gate plus refetchOnWindowFocus: true, and require_auth deliberately penalizes a present-but-invalid credential (the PR [#75] lockout-budget design) — so a revoked device's stale cookie banks a rate-limit failure per phone-foreground during the pairing dance. This regresses the exact class [#75] closed, and the review's citation of the queryCache.onError comment is on point. Fix: useRepos(enabled) with !route.isPairing from the shell (the picker's own call is unreachable on #pair); a disabled query neither mount-fetches nor focus-refetches, so zero authed traffic on the pairing screen again. The post-pair race guards (errorUpdatedAt > lastPairedAt(), !isFetching) are untouched.

Should-fix (active computed by path where everything else agrees on id): accepted with the suggested mechanism, fix in the next push. The two-worktree shared+active collision is real — the single id-keyed entry's stored path can be the other worktree's, so the open repo reports active: false. Fix: LanState.active_repo_id (already maintained by the registry bookkeeping) is shared into RouterState, and list_repos flags active by id equality. This deletes repo_paths_match from list_repos entirely.

Perf nit (canonicalize under the repos mutex): resolved as a side effect of [#2] — with the id-equality flag there is no path matching (and no filesystem touch) in list_repos at all.

UX nit (self-bouncing bottom-nav on the picker): accepted, fix in the next push via the same remembered-context mechanism as the Copilot picker thread: BottomNav on #repos gets the last scoped repo's id (tabs become a functional way out of the picker), and is hidden when no context exists.

Also in the next push (Copilot threads): the picker's remembered {repoId, tab} context and the palette twins gating on the shared list having loaded. The docs thread is declined per context-comment call [#7] (epic-close docs-sync). All fixes will re-run the full gates (cargo + clippy, pnpm build, scoped biome) before the push.

Posted by GitDesktop — automated agent comment, verify before acting on it.

Related

Tickets: #2
Tickets: #7
Tickets: ~~#75~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Verified against the code: resolve_scoped_repo (routes/mod.rs:150–171) resolves a {repoId} request by reading repos[id].path verbatim into the ScopedRepo on every request, and repoName is a real store field (ui.ts:163), so the palette-twin fallback is sound.

Summary

This slice extends the LAN companion from a single active repo to an active ∪ shared registry — a persisted store, scoped routes/queries/streams, a repo picker, and desktop share/unshare controls. The Rust registry bookkeeping is thorough and well-tested (coexistence, switch, unshare, seed, scoped-cut cases are all covered), and the companion routing/redirect/teaching-state logic is careful. Overall sound; the two blockers/should-fixes from the previous round are genuinely fixed. One remaining correctness edge (a shared repo can be served under the wrong worktree path) is worth addressing but is niche.

Resolved since last review

Blocker (self-lockout on #pair) — fixed. useRepos now takes an enabled gate; App passes useRepos(!route.isPairing), and Pair early-returns before Shell so the picker's own useRepos(true) can't mount on #pair. A disabled query neither mount-fetches nor focus-refetches, so zero authed traffic hits /api/repos during the pairing dance. (Copilot's related picker-context and share-gating threads — App.tsx:224 / :171 — are also resolved: lastScoped/pickerContext preserve repo+tab into the picker, and both share/unshare twins now gate on sharedLoaded.)

Should-fix (active computed by path) — fixed. RouterState.active_repo_id (the same Arc LanState owns) is now the source of truth, and list_repos flags active by active_id == id rather than repo_paths_match, with a dedicated two-worktree test. This also removes the previous per-entry canonicalize under the repos lock (the old perf nit).

UX nit (self-bouncing bottom nav on the picker) — fixed. The bottom nav is hidden on #repos with no remembered context, and otherwise scoped to chromeContext.repoId.

Correctness

should-fix — src-tauri/src/lan/mod.rs (install_active_repo / share_repo) with routes/mod.rs::resolve_scoped_repo. A repo that is both shared and active under a different worktree path has its single registry entry's stored path clobbered to the active worktree's path, and that path is never restored when the repo stops being active — so scoped routes serve the wrong worktree's data. Concrete path: open worktree C:/R and share it (shared_index[X]=C:/R, repos[X].path=C:/R); open a linked worktree C:/R-feature of the same repo — it resolves to the same id X (identity is the common git dir), and install_active_repo does repos.insert(X, {path: "C:/R-feature"}), overwriting the stored path; then switch the desktop to a third repo — install_active_repo sees still_shared and keeps the entry, leaving repos[X].path == "C:/R-feature". Now a phone browsing repo X hits resolve_scoped_repo, which reads repos[X].path (C:/R-feature) and serves that worktree's Status/branch — not the C:/R the user actually shared. Only reachable with linked worktrees, so it's an edge, and it may overlap the "path-vs-id asymmetry (accepted)" the team noted — but that note was about the desktop-side verbatim already-shared check, not the served path, so it's worth a deliberate call. Fix: when install_active_repo retains an entry because still_shared, reset its path to shared_index[prev_id] (and/or don't let install/share overwrite a shared entry's stored path with a different worktree path), so a shared repo reverts to serving its shared path once it's no longer the active worktree.

The rest of the registry logic holds up: the shared_index→repos lock ordering (async mutex taken first, std::Mutexes only briefly inside, never across an .await), the seed/share/unshare/switch cut-scoping, and the store's unknown-key preservation are all correct and well-covered by the new tests.

Copilot also flagged the missing README/marketing-site update (changelog:4). Not actionable here: the in-app help guide and changelog fragment are updated in this PR, and the README/site sync for the whole companion preview is a standing epic-branch deferral rather than an omission in this change.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

Round-2 disposition (one new should-fix):

Shared-path clobber under linked worktrees: accepted — fix in the next push. The trace is correct and the distinction from the recorded "path-vs-id asymmetry (accepted)" note is fair: that note covered the desktop-side already-shared check; this is the served path, which must not silently follow the active worktree. The fix makes the explicitly shared path authoritative for a shared repo's registry entry: install_active_repo no longer overwrites an entry whose id is in the shared set (it still records the active id, so the /api/repos active flag is unaffected), and sharing a repo whose entry came from the active mirror adopts the shared path. Net semantic, documented at the site: the scoped surface serves the worktree you explicitly shared, even while the desktop is active in a sibling worktree (the alias surface keeps serving the active path); switch-away needs no restore because the entry never lost the shared path. Tests will cover both orders (share-then-activate, activate-then-share) plus the reviewer's exact repro (path stays A while active under B and after switching away).

Round-1 closures and the docs self-disposition are appreciated — nothing further on those.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

Round-2 disposition addendum (the fix as implemented, one step past the recorded plan): the implementer extended the shared-path-authoritative rule to the symmetric edge the disposition didn't name — unsharing a repo that is also the active repo re-points its surviving registry entry from the shared worktree path back to the active worktree path. Rationale: once the explicit share is gone, "active" is the entry's only remaining role, so serving the stale shared path would be the same bug class in the other direction. No cut fires (the repo never leaves the registry). Covered by a dedicated test alongside the reviewer's exact linked-worktree repro (real git worktrees, no mocks); suite is 864/864 with clippy -D warnings clean.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I read the SSE stream handler (reviews.rs), the scoped resolver (routes/mod.rs), and confirmed repoBasename exists — enough to ground the review below.

Summary

This slice extends the LAN companion from a single active repo to an active ∪ shared registry: a persisted store, scoped routes/queries/streams, a repo picker, and desktop share/unshare controls plus command-palette twins. The registry bookkeeping and its lock discipline are careful and unusually well-tested (coexistence, switch, unshare, seed, scoped-cut, and all three shared-path-authoritative worktree cases). Overall sound and mergeable; I found one genuine correctness gap in SSE cut-scoping, niche enough to be a should-fix rather than a blocker.

Resolved since last review

should-fix (shared-path clobber under linked worktrees) — fixed and I can see it in the current diff. install_active_repo now only inserts the active entry if !shared.contains_key(&id), share_repo/seed_shared make the shared path authoritative, and unshare_repo re-points an active entry back to the active worktree. Traced through the reviewer's exact repro (share A → open linked worktree B → switch away): repos[X].path stays A. Backed by shared_worktree_path_survives_active_open_of_a_linked_worktree, active_first_then_share_makes_the_shared_path_authoritative, and unshare_active_repo_repoints_the_entry_to_the_active_worktree.

Correctness

should-fix — src-tauri/src/lan/routes/reviews.rs::forward_stream (the MonitorCut::Repo(p) arm, repo_paths_match(&p, &repo)) together with mod.rs::install_active_repo. Scoped cuts are matched to open streams by path, but this PR makes an entry's served path mutable for a fixed id, so a cut carrying the entry's current path fails to sever a stream that captured the entry's earlier path.

Concrete case (one repo, two worktrees A/B, no sharing even required):

Desktop active on worktree A → repos[X].path = A. A phone opens a live agent-run watch; resolve_scoped_repo reads repos[X].path = A, so the SSE stream captures repo = A (and subscribe_in_for matches because the run was launched on A).

Desktop switches active to worktree B of the same repo X. install_active_repo sees still_new (identical id), so it fires no cut and overwrites repos[X].path = B. The open stream still holds repo = A.

Desktop switches active to an unrelated repo Z. install_active_repo removes X and fires MonitorCut::Repo(B) (repos.remove(prev_id).path, now B). The still-open stream's repo is A, so repo_paths_match("B", "A") is false and the stream is never severed — the phone keeps watching a run whose repo has fully left the registry (its /api/repos/X/… routes now 404), until the run itself ends. The use-review-stream reviews-probe only reclassifies on stream close, so a long-running review/session stays watchable, violating the module's "paired devices never watch a run on a repo that isn't shared" containment.

Before this PR the same worktree switch fired the unconditional send(()) cut, which ended every stream, so this is a regression introduced by the id-based dedup + mutable path. The sse_monitor_survives_a_scoped_cut_on_another_repo test only exercises two distinct string paths, never a single id whose served path changes, so the gap is untested. (The linked-worktree share→active→unshare→switch-away sequence hits the same miss via the unshare_repo re-point.)

Fix: scope cuts by the stable repo id rather than the mutable served path — carry repoId into the stream and into MonitorCut::Repo, comparing ids — or, whenever an entry's served path changes while the entry stays registered (the same-id worktree switch, and the unshare-while-active re-point), fire a MonitorCut::Repo(old_path) so streams on the stale path reconnect and re-resolve to the new one.

Readability

nit — companion/src/screens/Prs.tsx, ActivitySection / ThreadsSection. The comments say a first-seen noSuchRepo here will "take over the whole screen with the teaching state," but each returns <RepoGoneState /> at its own position inside the parent's <article>, so it renders inline (sandwiched under ConversationSection), not full-screen — and if both sub-sections 404 you briefly get two stacked cards. It self-corrects within one usePr poll (or when useRepos refetches and Shell swaps to the picker), so it's cosmetic; either soften the comment or lift the gone-state decision to PrDetail.

The other reviewers' three threads are all addressed in the current diff: the picker context (lastScoped/pickerContext) and the palette share-gating (sharedLoaded) are wired as described, and the README/marketing-site omission is the standing epic-branch deferral (the changelog fragment and in-app help guide are both updated here), not an omission in this change.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

Round-3 dispositions:

Cut-scoping gap (stream captured path vs mutable served path): accepted — fix in the next push, using the review's second suggested mechanism. Verified every link of the trace: streams capture the served path at connect, a same-id worktree switch mutates the entry's path with (correctly) no cut, and the eventual removal cut carries the new path — so the old stream is never severed and containment breaks until the run ends. The fix fires MonitorCut::Repo(<old path>) at the two places a registered entry's served path changes (the same-id active switch overwrite, and the unshare-while-active re-point), which severs exactly the streams that captured the stale path and restores pre-PR semantics for these cases (the unconditional cut also ended such streams; a run registered under the old worktree not appearing in lists scoped to the new one is likewise pre-existing behavior, and will be said at the fire site). Deliberately NOT taking the id-based alternative this round: carrying the stable id into stream registration/matching touches state.rs/agent.rs stream identity and is recorded as the architecturally-cleaner follow-up for the epic-close pass rather than a review-round patch.

Comment-accuracy nit (sub-section gone states "take over the whole screen"): fixed (next push) — both ActivitySection/ThreadsSection comments now describe the real behavior: the teaching state renders in place of the section as a transitional state, and the parent's own poll takes the full detail over within one cycle (the brief two-card race is transitional, not the destination). Comment-only change, applied by the orchestrator and verified with tsc -b + biome.

Round-2's closure re-trace and the cross-reviewer thread verifications are appreciated.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

Round-3 disposition addendum (fix as implemented, one site beyond the recorded plan): the disposition named the two served-path-mutation sites from the finding (install_active_repo same-id switch, unshare_repo re-point). The implemented fix covers a third site with the same invariant — share_repo's active-mirror overwrite (active under worktree B, then sharing path A flips the entry B→A) now also fires MonitorCut::Repo(B), since a stream captured on B would be stranded identically. The invariant as shipped: any change to a registered entry's served path fires a cut carrying the old path. Verified by cut-value assertions at all three sites plus a no-cut guard for identical-path re-activation/re-share; the pre-existing SSE test covers forward_stream honoring a path-carrying cut. Suite 866/866, clippy clean.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I traced the SSE cut path end-to-end: stream captures repo from Extension<ScopedRepo> at connect (line 115/128/147) and passes it into forward_stream; the cut fix carries the old served path at each of the three registry-mutation sites, which is exactly the string a stream on that path captured. Confirmed repoName/repoBasename both exist, so the desktop palette twins are grounded.

Summary

Slice 4 extends the LAN companion registry from a single active repo to active ∪ shared, with a persisted store, scoped routes/queries/streams, a repo picker, and desktop share/unshare controls with command-palette twins. The registry lock discipline and cut-scoping are careful and unusually well-tested. The round-3 correctness gap (streams stranded when an entry's served path mutates under a fixed id) is now fixed and I can verify it in the current diff. Sound and mergeable — I found no blocking or should-fix issues in this revision.

Resolved since last review

should-fix (SSE cut-scoping under a mutable served path) — fixed, and visible in the current diff. A MonitorCut::Repo(<old path>) now fires at each of the three sites where a registered entry's served path changes while the id stays registered:

install_active_repo same-id worktree switch (if existing.path != repo.path before the insert),

share_repo's active-mirror overwrite (repos.get(&id).and_then(|e| (e.path != repo_path).then(...))),

unshare_repo's re-point (if e.path != path { path_cut = Some(...) }).

I re-walked the round-3 repro (active A → switch to same-repo worktree B → switch to unrelated Z): step 2 now fires Repo(A), so the stream that captured A is severed via repo_paths_match(A, A) and the phone re-resolves against B — the stream is no longer stranded. I also checked the two cut-assignment branches in install_active_repo can't clobber each other: the path-change branch only fires when the new id equals prev_id (still_new, removal skipped), and the removal branch only when it differs and the new id has no pre-existing non-shared entry — so at most one served-path change per call, one cut. Lock ordering is consistent (shared_index async guard outermost, std mutexes taken sequentially and never nested, no await under a std guard), so it's deadlock-free. Backed by same_id_active_worktree_switch_cuts_the_old_worktree_path, active_first_then_share_makes_the_shared_path_authoritative, unshare_active_repo_repoints_the_entry_to_the_active_worktree, and the no-cut guard same_id_reactivation_with_identical_path_fires_no_cut.

nit (sub-section gone-state comment) — resolved. The ActivitySection/ThreadsSection comments now accurately describe in-place rendering with the parent taking over within one poll, rather than the earlier "take over the whole screen" claim.

Notes on other reviewers

Copilot's App.tsx:224 (picker can't preserve repo/tab) and App.tsx:171 (palette twin gating during initial load) are both addressed in the current diff — lastScoped/pickerContext carry the last scoped context to the picker and BottomNav, and both twins gate on sharedLoaded = sharedRepos.data !== undefined. Not issues.

Copilot's changelog.d:4 (README/marketing site don't mention the companion) reflects the standing epic-branch deferral — the changelog fragment and in-app help guide are updated here; the README/site docs-sync lands at the epic→master close. Not an omission in this change.

Nothing else stands out — the frontend routing grammar degrades malformed #r/… ids to the picker without looping (isRepoId filter in resolveRepo/sortRepos), the bootstrap redirect is replace-based and self-terminates once repoId != null, and the active flag is correctly id-based (the two-worktree case is pinned by test 1b in list_repos_lists_the_registered_repo_…).

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-19

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

feat(lan,companion,settings): support sharing multiple repositories

Milestone

Searches

Help

#88 feat(lan,companion,settings): support sharing multiple repositories

Settings and desktop controls

Companion repository selection

Scoped companion data and stream handling

Documentation

Discussion

Deploying gitdesktop with Cloudflare Pages

Summary

Correctness

Edge cases

Performance

Readability / UX

Related

Related

Summary

Resolved since last review

Correctness

Summary

Resolved since last review

Correctness

Readability

Summary

Resolved since last review

Notes on other reviewers

Latest commit:	`7cb71b3`
Status:	✅ Deploy successful!
Preview URL:	https://bedf0fa3.gitdesktop.pages.dev
Branch Preview URL:	https://feat-lan-companion-slice4.gitdesktop.pages.dev

feat(lan,companion,settings): support sharing multiple repositories

Milestone

Searches

Help

#88 feat(lan,companion,settings): support sharing multiple repositories

Desktop sharing and LAN API

Settings and desktop controls

Companion repository selection

Scoped companion data and stream handling

Documentation

Discussion

Deploying gitdesktop with Cloudflare Pages

Summary

Correctness

Edge cases

Performance

Readability / UX

Related

Related

Summary

Resolved since last review

Correctness

Summary

Resolved since last review

Correctness

Readability

Summary

Resolved since last review

Notes on other reviewers