GitDesktop / Tickets / #80 feat(git,branches,ui): push or publish any branch to its own remote

Anonymous - 2026-07-18

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

Deploying gitdesktop with Cloudflare Pages

Latest commit: 31348a0

Status: ✅ Deploy successful!

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

Branch Preview URL: https://feat-branch-switcher-push-an.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-18

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-18

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No security issues found.

The only new sink introduced is the remote parameter flowing into git push (both the Tauri command and the MCP push tool). I traced it and confirmed it is safe:

In git_push_core (src-tauri/src/git/remote.rs), a caller-supplied remote is passed through validate_ref_name (rejects empty, leading -, and * ? [ : \ space/control chars) and checked for exact membership in the repo's actual configured remotes (git_remotes → git remote). A URL, flag, or arbitrary transport string cannot satisfy the exact-match existence check, so argument/URL/refspec injection is not reachable.

The remote is only ever emitted as the bare push <remote> argument (build_push_args), never interpolated into a refspec, and the credential config is resolved for that same validated remote.

The MCP path adds ensure_not_flag(r, "remote") on top, and remote requires an explicit branch (the None branch rejects it early).

All frontend changes are ARIA label/htmlFor associations plus remote selection sourced from backend-provided remotes.data — no untrusted input reaching a sink, and client-side anyway.

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-18

Originally posted by: theBGuy

🧭 Context for reviewers — deliberate decisions on this diff (two disjoint packages in one PR: multi-remote push + form-label a11y; dispositions pre-recorded so rounds can ground against them)

Multi-remote push:

Credential scope follows the actual push target (resolve_push_target feeds both the argv AND credential_config_for_remote). Authenticating against origin while pushing to a fork's upstream would be wrong and would undermine the [#74] chain-severing; credential_config_for_remote is genuinely remote-parameterized (resolves THAT remote's URL/host) and origin-cases are byte-identical. The HEAD path (branch: None) keeps the pre-existing origin scoping — a known, pre-existing latent mismatch for a non-origin-tracked HEAD, mitigated by the ambient-fallback retry, deliberately not widened here.

Deliberate behavior change (documented in the table doc-comment + changelog): with no explicit remote, a tracked-NON-origin branch now pushes to its OWN remote instead of the old v1 fallback (origin under its own name). The old arm was UI-unreachable (per-row push required tracked-on-origin) and reachable only via MCP push {branch}; the new default is the honest semantics.

The [#76] hardening is carried, not relaxed: refspecs stay fully qualified in every arm; the remote name is only ever the bare push <remote> argv position, never interpolated into a refspec; it passes validate_ref_name (blocklist rejects :, so URL-shaped strings can't even parse) AND a pre-mutation existence check against git remote output — the primary guard.

Scope decisions (user-confirmed): header SyncControls stays origin-only in v1 (the per-row items cover multi-remote); MCP push gains optional remote (single word ⇒ no serde rename needed; defaulted None ⇒ zero behavior change); force_push remains HEAD-only.

Gating edge (accepted): a branch whose upstream references a remote that no longer exists shows NO push/publish action (hidden, not disabled) — rare, and guessing a destination would be worse.

Live-validated with a real second remote (temp bare repo): flat "Publish to origin"/"Publish to fork" items; publish→fork landed the ref WITH tracking while origin stayed untouched; a tracked-non-origin branch showed "Push to fork/…" and the fork ref advanced to the exact tip. Rust 770/770 (6 new decision-table tests), clippy clean.

Form-label a11y sweep:

Most of the app was already correct — TextField/TextareaField/MarkdownField use useId+htmlFor, CheckboxField wraps natively, dozens of call sites pass htmlFor. The fixed gaps: SelectField (once, in the primitive — covers every call site), both branch comboboxes (triggerId prop / aria-label), and ~12 ad-hoc bare <Label> sites (htmlFor+id, or role="group"+aria-labelledby for segmented/popover-trigger fields whose primitives don't take a single htmlFor). Vendored src/components/ui/** untouched — the primitives were verified to FORWARD id (no silent no-op associations). Zero visual change (attributes only).

aria-label="Compare with branch" (not "Base branch") on the compare picker is deliberate — the Compare tab's frame is "Compare {current} with…"; "Base branch" would misdescribe the control to AT users.

Known leftovers, deliberately out of scope (recorded follow-ups): the AI-settings "API key" heading over a label-prop-less bound TextField; the app-wide question of whether a wrapping <label> around Base UI's Checkbox (role=checkbox button, not a native input) associates for AT; a set of group-headings over already-individually-labeled controls.

Live-validated: clicking the "Base it on" label OPENS the base combobox (label→button activation — the association is functional in the DOM, not decorative). pnpm build green on the merged tree.

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

Related

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

Anonymous - 2026-07-18

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

This is a well-scoped change: the push backend now resolves the correct target remote (branch's own upstream, or an explicit choice) instead of hardcoding origin, the branch switcher exposes per-remote push/publish, and a broad a11y sweep wires labels to controls. The logic is sound, the security-sensitive path (remote name validation + existence check, refspec never interpolating the remote) is carefully guarded, and the Rust decision table is thoroughly unit-tested. No blockers.

Correctness / design

should-fix — the two remote resolvers can silently disagree, hiding a valid push (BranchSwitcher.tsx upstreamRemoteOf vs remote.rs resolve_push_target). pushable is gated on upstreamRemoteOf(branch.upstream, remoteNames) (longest-prefix match against git remote), while the backend, when the UI passes no remote, resolves the target from git's %(upstream:remotename). These normally agree, but they're derived independently from two different sources. If they ever diverge (e.g. a branch.<x>.remote config pointing at a remote whose name the frontend parses differently, or a transient remotes.data shape), the row is hidden or targets a remote the backend won't pick — with no signal to the user. This isn't a crash, just a maintenance/consistency hazard from two sources of truth for "which remote." Worth a short comment cross-referencing the two, or ideally surfacing the resolved remote name from the same backend data the row already has (branch.upstream), so the UI and backend can't drift.

Docs / project conventions

should-fix — marketing site not updated (site/src/pages/index.astro). The repo's standing brief (CLAUDE.md) requires the marketing-site capabilities/FeatureRow to be updated in the same change as a user-facing feature; this PR updated the README (step 1), in-app guide (step 3), and changelog fragments (step 4) but not the site (step 2). "Push/publish without switching" is likely already a site capability, so this may be a deliberate call since it's an enhancement of an existing line — but given the explicit rule, either extend the existing capability copy to mention per-remote/publish-to-any-remote or confirm the omission is intentional.

Readability / consistency

nit — hardcoded element ids instead of useId() (RunWorkflowDialog.tsx wf-inputs-label, McpServerDialog.tsx mcp-scope). Every other file in this a11y sweep derives ids from useId(); these two hardcode string literals. It works today because each dialog is single-instance, but it's inconsistent with the rest of the change and would produce duplicate ids (breaking the htmlFor/aria-labelledby association) if either dialog were ever mounted twice. Switch them to useId() for consistency.

Tests

Coverage of the pure decision table (build_push_args, resolve_push_target) and the MCP wire format is genuinely thorough — request-wins/tracked/gone/untracked/force arms and the +main security guard are all exercised. The two new git_push_core guard paths ("remote requires an explicit branch" and "unknown remote: {r}") aren't unit-tested, but they require a real repo and fit the project's pure-logic-unit-test posture, so that's acceptable rather than a gap.

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-18

Originally posted by: theBGuy

Round-1 dispositions (fixes locally applied + verified; in the next push):

Resolver-drift should-fix — accepted, with your "ideally" taken all the way. Rather than a cross-referencing comment, the authoritative name now ships on the Branch model: git_branches' for-each-ref format gained %(upstream:remotename) as a 6th field → Branch.upstreamRemote (string | null), and the frontend's upstreamRemoteOf longest-prefix helper is deleted — the switcher gates on branch.upstreamRemote + a membership check against remotes.data (kept solely to hide the row when the tracked remote was removed from config). UI and backend now read the same branch.<x>.remote source, so the drift class is gone, not papered over. New real-repo test git_branches_reports_upstream_remote (tracked → Some("origin"), untracked → None, with the ambient branch.autoSetupMerge pinned per this file's hermeticity idiom).

Site should-fix — accepted. The existing capability line was the deliberate reason for the omission (it's remote-neutral), but extending it is strictly better: now "Push or publish a branch to any remote — no checkout needed" (site/src/data/capabilities.ts; site && pnpm build green).

Static-id nit — declined, file-convention grounds. wf-inputs-label and mcp-scope match those files' PRE-EXISTING id conventions at baseline (wf-ref/wf-workflow/wf-pipeline and mcp-desc/mcp-command are all static string ids from before this PR); the sweep's rule was "match the file's existing id convention", and switching only the two new ids to useId() would create in-file inconsistency with their sibling ids. Both dialogs are singletons (single conditional mount), so no duplicate-id path exists. If either file ever grows a second mount, the right move is converting ALL its ids in one pass — noted, not this PR.

Also fixed this round: Copilot's error-copy find (invalid remote name: {r} remap — thread reply has details). Verification after all fixes: cargo 771/771 green (incl. the new test), clippy clean, pnpm build green, site build green.

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-18

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

The gating at line 991–994 covers the trailing ContextMenuSeparator, and the publishRemotes ternary renders nothing when not publishable — so no stray separator. The change is consistent.

This PR does two disjoint things well: the push backend now resolves the actual target remote (branch's own upstream via git's %(upstream:remotename), or an explicit choice) instead of hardcoding origin, with the credential config scoped to that same resolved remote; and a broad a11y sweep wires htmlFor/aria-labelledby associations across dialogs. The core build_push_args/resolve_push_target decision table is pure, single-sourced, and thoroughly unit-tested, and the [#76] refspec hardening is preserved (remote is only ever the bare push <remote> argv, refspecs stay fully qualified). Sound, and no blockers.

Resolved since last review

Resolver drift (BranchSwitcher.tsx) — the frontend upstreamRemoteOf longest-prefix helper is deleted; the switcher now gates on branch.upstreamRemote (git's %(upstream:remotename), surfaced end-to-end through git_branches → Branch.upstream_remote → Branch.upstreamRemote) with a membership check against remotes.data to hide rows whose tracked remote is gone. UI and backend now read the same branch.<x>.remote source, so the two-sources-of-truth hazard is gone. Covered by the new git_branches_reports_upstream_remote test.

Marketing site not updated — site/src/data/capabilities.ts now reads "Push or publish a branch to any remote — no checkout needed."

Misleading invalid branch name on a bad remote (remote.rs, also flagged by Copilot) — the explicit-remote path now remaps the checker's error to invalid remote name: {r} at the call site while keeping validate_ref_name as the guard.

Verified correct

The git_branches 6-field destructuring maps %(upstream:remotename) to upstream_remote positionally, empty-filtered to None; the tracked/untracked test confirms it.

resolve_push_target is called identically for both cred_remote and inside build_push_args, so the credential scope and the argv always agree on the target — no divergence between what's authenticated and what's pushed.

The publishRemotes ternary (0 → nothing, 1 → single item, >1 → per-remote) and its trailing separator are safe: the fragment is gated by canUpdate || behind || pushable || publishable (line 991), so the separator never renders alone.

Readability

nit — hardcoded ids wf-inputs-label / mcp-scope (RunWorkflowDialog.tsx, McpServerDialog.tsx). Every other file in this sweep derives ids from useId(); these two use string literals, which would collide if the dialog were ever mounted twice (breaking the aria-labelledby/htmlFor association). Both dialogs are single-instance today and the author notes these match each file's pre-existing static-id convention, so this is a genuine but minor consistency point — fine to leave, but useId() would remove the latent footgun.

No test gap: the pure decision table (request-wins / tracked-own-remote / copy-elsewhere / untracked-publish / force ordering / +main guard) and the MCP remote wire default are all exercised, and the two git_push_core guard paths ("remote requires an explicit branch", "unknown remote") need a real repo, consistent with the project's pure-logic unit-test posture.

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

Related

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

Anonymous - 2026-07-18

Originally posted by: theBGuy

Round-2 update (fixes locally applied + verified; in the next push):

Static-id nit — accepting after all, at the author's direction, and taken whole. The convention-grounds decline is withdrawn: rather than converting only the two flagged ids (which would have mixed conventions inside those files — the objection on record), BOTH dialogs' full id sets are now useId()-derived via a per-mount idBase + suffix (the ${prefix}-name idiom AiProviderSection already uses): RunWorkflowDialog (workflow/pipeline/ref/inputs-label) and McpServerDialog (name/desc/scope/command/args/url). The latent duplicate-mount footgun is gone file-wide, not just for the sweep's additions.

CI failure — root-caused and fixed. The build job's biome ci ./src/ failed on ONE real format diff: LifecycleEditor.tsx's new role="group" divs exceeded line width and needed wrapping — a genuine miss that had been hiding inside the local Windows CRLF noise (biome lint passes don't catch format). Fixed with a scoped biome check --write; the same wrap applied to the id-conversion lines it lengthened in McpServerDialog. Scoped biome ci on all three files is now clean locally, pnpm build green.

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-18

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

This is a sound PR. The multi-remote push backend (resolve_push_target/build_push_args) is pure, single-sourced across the credential-scope and argv paths, keeps the [#76] refspec hardening intact, and is well-covered by the expanded decision table; the a11y sweep is mechanical label↔control association with no behavior change. No blockers, and I found no new correctness issues in the changes since the last review.

Resolved since last review

Static-id nit (RunWorkflowDialog.tsx, McpServerDialog.tsx) — both dialogs now derive their full id sets from a per-mount useId() idBase + suffix (${idBase}-workflow, ${idBase}-scope, etc.), and the aria-labelledby/id pairs match. The latent duplicate-mount collision is gone; the earlier hardcoded wf-inputs-label/mcp-scope literals are eliminated.

Misleading invalid branch name on a bad explicit remote (remote.rs, Copilot's flag) — confirmed resolved in the current diff: the explicit-remote path remaps the checker error to invalid remote name: {r} while keeping validate_ref_name as the guard, then verifies existence against git_remotes before any mutation.

Verified correct in this round

git_branches 6-field destructuring: one parts.next() was added at the head of the value tuple and one binding (upstream_remote) at the tail — the counts stay balanced (6 calls, 6 bindings), so %(refname:short)…%(upstream:remotename) still map positionally in sequence. The new git_branches_reports_upstream_remote test pins this (tracked → Some("origin"), untracked → None).

LifecycleEditor.tsx role="group" divs are pure line-wrap reflows of already-associated labels — no semantic change.

fields.tsx SelectField useId() is already imported (line 1); the single id feeds both Label htmlFor and SelectTrigger id, unique per instance.

BranchSwitcher gating still holds: publishRemotes is non-empty exactly when publishable, the length === 1 ? … : map renders nothing when empty, and the enclosing fragment is gated by canUpdate || behind || pushable || publishable, so the trailing ContextMenuSeparator never renders alone.

All git_push_core call sites (Tauri command, MCP push, force_push, gate test) pass the new remote argument; usePush/gitPush add it as an optional trailing param, so SyncControls's existing call is unaffected.

No new findings.

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

Related

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

Anonymous - 2026-07-18

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(git,branches,ui): push or publish any branch to its own remote

Milestone

Searches

Help

#80 feat(git,branches,ui): push or publish any branch to its own remote

Branch switching and push flows

Branch switcher UI

Accessibility improvements for form fields

Tests and reliability

Discussion

Deploying gitdesktop with Cloudflare Pages

Related

Correctness / design

Docs / project conventions

Readability / consistency

Tests

Resolved since last review

Verified correct

Readability

Related

Resolved since last review

Verified correct in this round

Related

Latest commit:	`31348a0`
Status:	✅ Deploy successful!
Preview URL:	https://f2380edd.gitdesktop.pages.dev
Branch Preview URL:	https://feat-branch-switcher-push-an.gitdesktop.pages.dev