Skip to content

fix(sidebar): scope the worktree-visibility toggle to its own host - #13211

Closed
manuaudio wants to merge 1 commit into
stablyai:mainfrom
manuaudio:fix/worktree-visibility-host-identity
Closed

manuaudio wants to merge 1 commit into
stablyai:mainfrom
manuaudio:fix/worktree-visibility-host-identity

Conversation

@manuaudio

@manuaudio manuaudio commented Aug 8, 2026 •

Copy link
Copy Markdown
Contributor

Same defect class as #13071 (P1), in a different flow. No separate issue filed — happy to open one if you'd rather track it on its own.

Summary

The worktree-visibility toggle could show you one host's state and write the change to another host's project.

WorktreeVisibilityDialog resolves its repo with a bare id and then mutates without a host:

const repo = repos.find((candidate) => candidate.id === repoId) ?? null
...
await updateRepo(repoId, { externalWorktreeVisibility: showOther ? 'hide' : 'show', ... })

updateRepo's own signature documents why that matters:

options.hostId targets a specific host's row + RPC target when the id exists on multiple hosts; else the focused host is assumed.

So when one repo id exists on two hosts (two clones of a remote, the same repo added twice, local + SSH copies), the dialog reads whichever row matches first and writes to whichever row updateRepo assumes. Those need not be the same row, and neither need be the one the user opened the menu on.

RepositoryPane.tsx:93 already passes { hostId: selectedHostId } for exactly this reason, so the correct pattern is established in-tree.

What changed

The menu that opens the dialog already has the full Repo:

handleOpenWorktreeVisibility(row.repo.id)   // host discarded

Carry it through the modal data into updateRepo, and resolve the row with findRepoForHost so the dialog reads the same row it writes. The host is narrowed with normalizeExecutionHostId rather than cast.

Screenshots

No visual change. The only user-visible difference is that the dialog now reflects the project you opened it on.

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test — scoped to the touched tree, not the full suite (see below)
  • pnpm build
  • Added or updated high-quality tests that would catch regressions

pnpm typecheck and oxlint pass; 2011 tests across src/renderer/src/components/sidebar/ pass. I ran the scoped suites rather than a full pnpm build on this branch — it is renderer-only identity plumbing with no packaging, dependency or main-process surface. Glad to run the full build if you want it on the record.

New worktree-visibility-host-identity.test.tsx renders the real dialog with one repo id on two hosts and clicks the toggle:

  • with a host, updateRepo is called with { hostId: 'ssh:host-a' } and the value derived from host A's state
  • without a host, the dialog still opens and calls updateRepo with no host option

Verified non-blind: reverting the plumbing fails the first test while the second keeps passing — it pins the unchanged host-less path, so the pair distinguishes the fix from its absence rather than just exercising the component.

Note the dialog renders through a Radix portal, so renderToStaticMarkup sees nothing; the test uses @testing-library/react under happy-dom, matching the existing SelectedTextCopyMenu test.

AI Review Report

Reviewed with Claude Opus 5 against the CONTRIBUTING checklist.

  • Cross-platform (macOS/Linux/Windows) — pure renderer identity plumbing. No path construction, shell behaviour, accelerator, shortcut label or Electron platform branch is touched, so there is nothing to guard.
  • SSH/remote/local — this is the SSH-relevant path. Local rows resolve to the local host id and behave as before; SSH rows now resolve to their own ssh:<target>. updateRepo's existing per-host RPC targeting is unchanged, it simply receives the host the user meant. No wire change.
  • Folder workspaces — the dialog is gated on isGitRepoKind, unchanged.
  • Agent/integration/git provider — none touched.
  • Performance — a filter over the same list, on a user-initiated dialog open.
  • UI quality — no layout, token or component change.

What it flagged and I acted on: making the host mandatory looked cleaner, but the review caught that the inbox-recovery caller (onOpenRecovery) only has a bare id, and findRepoForHost returns null for an ambiguous bare id by design — that path would have become a menu item that opens an empty dialog. The resolve now falls back to the previous first-match when no host is supplied. My own test caught the same thing independently, which is why the host-less case is pinned.

Security Audit

Reviewed for input handling, command execution, path handling, auth, secrets, dependency and IPC risk.

  • Mutation targeting — the substance of the change: narrows a persisted mutation to an explicitly identified row instead of an inferred one, removing a path where a user's toggle lands on a project they were not looking at.
  • Input handling — the host id read out of modal data is narrowed with normalizeExecutionHostId, not cast, so a malformed value falls back instead of propagating into an RPC target.
  • Command execution / path handling — none added.
  • IPC — no new surface. updateRepo's existing channel and its already-validated hostId parameter are unchanged; this only populates a parameter that was already there.
  • Auth, secrets, dependencies — untouched, no new dependency.

No follow-up needed.

Notes

This is the third site in the same family. #13117 covers the removal flow from #13071 (three sidebar sites plus the orphan purge in worktrees.ts); this one covers the visibility toggle. A fourth remains — selectProjectGroupRemovalTargets discards the host when building group-removal targets — which I left alone because it changes a shared selector's shape and its callers. Happy to take that one too if it is wanted.

X: @manuaudiomix

The dialog resolved its repo with a bare id and called updateRepo without a
host. When one repo id exists on several hosts that reads whichever row
matches first, so the dialog can show one host's visibility state and write
the toggle to another host's row.

The menu that opens it already has the full Repo in hand, so carry that
host through the modal and into updateRepo — the same identity updateRepo
already documents for ids that exist on multiple hosts.

The host stays optional: the inbox-recovery caller only has a bare id, and
that path keeps its current first-match behaviour rather than opening an
empty dialog.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ZcrKK54K7sRnSkT6a6Jut
@coderabbitai

coderabbitai Bot commented Aug 8, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The worktree list now passes an optional execution host ID when opening the visibility dialog. The dialog normalizes host identity, resolves repositories using settings, and preserves an ID-only fallback for host-less callers. Visibility updates now include the resolved host context. Tests cover repositories with shared IDs across hosts and host-less modal payloads.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes scoping the worktree-visibility toggle to the correct host.
Description check ✅ Passed The description covers the required sections, explains the change, documents testing and review, and states why the build was not run.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/renderer/src/components/sidebar/worktree-visibility-host-identity.test.tsx (1)

84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the host-less fallback visibility value.

The first matching fixture has externalWorktreeVisibility: 'hide'. The fallback must therefore write 'show'. expect.objectContaining({}) accepts either value and cannot detect a changed repository selection.

Proposed test update
     expect(storeState.updateRepo).toHaveBeenCalledWith(
       'repo-1',
-      expect.objectContaining({}),
+      expect.objectContaining({ externalWorktreeVisibility: 'show' }),
       undefined
     )

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 09eae381-7d26-4892-97fd-d91b25da591b

📥 Commits

Reviewing files that changed from the base of the PR and between 6da7b8e and f558667.

📒 Files selected for processing (3)
  • src/renderer/src/components/sidebar/WorktreeList.tsx
  • src/renderer/src/components/sidebar/WorktreeVisibilityDialog.tsx
  • src/renderer/src/components/sidebar/worktree-visibility-host-identity.test.tsx

@manuaudio

Copy link
Copy Markdown
Contributor Author

Closing this as superseded by the host-scoped visibility work merged in #13652. Current main resolves the dialog target with resolveWorktreeVisibilityHostTarget and routes writes through useWorktreeVisibilityHostActions, so rebasing this older implementation would duplicate that path. CodeRabbit’s weaker host-less assertion is valid for this stale test, but it no longer justifies changing a superseded branch.

@manuaudio manuaudio closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants