crueber/walhub

Watch

Fork 0

PR composer: From/To labels instead of base/head, cross-repo selection, dropdown fields (no free-form refs) #328

New issue

Closed

opened 2026-09-11 15:02:28 +00:00 by crueber · 3 comments

crueber commented

2026-09-11 15:02:28 +00:00

Owner

What's requested

On the pull-request composer (/:owner/:name/pulls/new):

"Base ref" / "Head ref" are confusing — rename to From / To. The merge direction should read naturally: From = where the changes come from (today's head), To = where they're going (today's base).
Allow a different org/repo for From and To. The composer currently assumes both endpoints live in the current repo; cross-repo PRs (pulling from a fork) should be specifiable.
From/To should be dropdowns, not free-form text fields.

Current state (code evidence)

web/src/pages/PullNew.jsx — two free-form RefSelect inputs (SSE-streamed ref autocomplete, :33) bound to getBase/getHead (:189-190, ?base=/?head= params prefill). Labels say "base ref"/"head ref"; the preview and compare copy says "Base and head" throughout (:113-148).
The backend already supports cross-repo heads — this is a UI gap, not a model gap: OpenInput carries Fork *ForkInfo ("cross-fork head: fork repo holding head_ref", internal/pulls/service.go:304-310), and the PR doc model has Endpoint{Repo, Ref} on both sides plus Fork{Repo} (internal/pulls/model.go:78-101). OpenPR requires write on the base repo only (:326-328) and read access on the head source — so the server is ready; the composer just never sends a fork.
The ref pickers stream refs for the current repo only (ctx.repoClient.refStream("branches", …), PullNew.jsx:40-41) — a repo selector per side needs to feed each picker from its chosen repo's ref stream (SDK repoClient is constructed per repo; the page may need a second client instance for the From side).

Proposed design

Terminology swap, UI-only: labels become From (head, changes come from here) and To (base, changes go to here). Update the composer labels, the preview copy ("Comparing …", "Base and head are the same ref"), and the ahead/behind line wording. Keep the wire/SDK param names (head_ref/base_ref) unchanged — this is labels and mental-model only (law 12: wire keys never change meaning). Also update ?base=/?head= param labels in the UI if surfaced; the params themselves stay for link compatibility (branch pages deep-link with them).
Repo selectors on both sides. Each side gets a repo dropdown + a ref dropdown:
- To: defaults to the current repo; dropdown lists repos the user can write to (the owners/{owner}/repos listing filtered by the user's permission, or the owner's repos plus the viewer's forks — source selection is the implementer's call; simplest correct v1: default current repo, dropdown of the viewer's accessible repos).
- From: defaults to the current repo (same-repo PR); dropdown includes the current repo and the viewer's forks of it (and optionally any repo the viewer can read — scope decision for the implementer; GitHub's model is "head repo = current or your fork"). The ForkInfo path exists, so selecting a fork feeds fork.repo on submit.
- Ref dropdown per side streams branches from the selected repo (the existing RefSelect SSE picker, re-keyed per repo choice). Same-repo default keeps today's behavior one click deep.
Dropdowns not text fields: the ref pickers already autocomplete from a streamed list — tighten them into true dropdowns (single-select listbox of that repo's branches; keep type-ahead filtering if it's free). Free-form entry is what makes "refs/heads/main" leak into the UI today.
Preview across repos: the compare preview fetches both histories via commits?ref= — cross-repo previews need the head-side commits fetched from the fork's client. Same compareHistories logic; note the reachability rule: OpenPR publishes refs/pull/<num>/head only when the head commit is reachable — cross-fork heads are the designed case, but the UI should surface a clear error when the fork head is unreachable rather than a generic 422.
Permissions surfaced: To-repo options should exclude repos the viewer can't write to (OpenPR requires base write); From-repo options exclude repos the viewer can't read.

Acceptance criteria

Composer labels read From / To (no "base ref"/"head ref" visible anywhere in the UI), with preview/compare copy updated to match.
Both sides offer a repo dropdown (default: current repo) and a ref dropdown fed by the selected repo's branches — no free-form ref entry.
A PR from a fork to the upstream repo can be created through the UI; the created PR's Fork info is correct and the PR page renders it.
Same-repo PRs behave exactly as today (default dropdown values, ?base=/?head= deep links still prefill).
Cross-repo preview works (ahead/behind from the fork's commits) and an unreachable head yields a clear, specific error message.
Repo dropdowns respect permissions (To = writable, From = readable).
No wire/param renames — head_ref/base_ref and ?base=/?head= unchanged; headless test for the label/direction mapping and the repo+ref state machine (repo choice → ref stream re-key).

## What's requested On the pull-request composer (`/:owner/:name/pulls/new`): 1. **"Base ref" / "Head ref" are confusing — rename to From / To.** The merge direction should read naturally: **From** = where the changes come from (today's head), **To** = where they're going (today's base). 2. **Allow a different org/repo for From and To.** The composer currently assumes both endpoints live in the current repo; cross-repo PRs (pulling from a fork) should be specifiable. 3. **From/To should be dropdowns, not free-form text fields.** ## Current state (code evidence) - `web/src/pages/PullNew.jsx` — two free-form `RefSelect` inputs (SSE-streamed ref autocomplete, :33) bound to `getBase`/`getHead` (:189-190, `?base=`/`?head=` params prefill). Labels say "base ref"/"head ref"; the preview and compare copy says "Base and head" throughout (:113-148). - **The backend already supports cross-repo heads** — this is a UI gap, not a model gap: `OpenInput` carries `Fork *ForkInfo` ("cross-fork head: fork repo holding head_ref", `internal/pulls/service.go:304-310`), and the PR doc model has `Endpoint{Repo, Ref}` on both sides plus `Fork{Repo}` (`internal/pulls/model.go:78-101`). `OpenPR` requires write on the **base** repo only (:326-328) and read access on the head source — so the server is ready; the composer just never sends a fork. - The ref pickers stream refs for the current repo only (`ctx.repoClient.refStream("branches", …)`, `PullNew.jsx:40-41`) — a repo selector per side needs to feed each picker from its chosen repo's ref stream (SDK `repoClient` is constructed per repo; the page may need a second client instance for the From side). ## Proposed design 1. **Terminology swap, UI-only:** labels become **From** (head, changes come *from* here) and **To** (base, changes go *to* here). Update the composer labels, the preview copy ("Comparing …", "Base and head are the same ref"), and the ahead/behind line wording. Keep the wire/SDK param names (`head_ref`/`base_ref`) unchanged — this is labels and mental-model only (law 12: wire keys never change meaning). Also update `?base=`/`?head=` param *labels* in the UI if surfaced; the params themselves stay for link compatibility (branch pages deep-link with them). 2. **Repo selectors on both sides.** Each side gets a repo dropdown + a ref dropdown: - **To:** defaults to the current repo; dropdown lists repos the user can write to (the `owners/{owner}/repos` listing filtered by the user's permission, or the owner's repos plus the viewer's forks — source selection is the implementer's call; simplest correct v1: default current repo, dropdown of the viewer's accessible repos). - **From:** defaults to the current repo (same-repo PR); dropdown includes the current repo **and the viewer's forks of it** (and optionally any repo the viewer can read — scope decision for the implementer; GitHub's model is "head repo = current or your fork"). The `ForkInfo` path exists, so selecting a fork feeds `fork.repo` on submit. - Ref dropdown per side streams branches from the selected repo (the existing `RefSelect` SSE picker, re-keyed per repo choice). Same-repo default keeps today's behavior one click deep. 3. **Dropdowns not text fields:** the ref pickers already autocomplete from a streamed list — tighten them into true dropdowns (single-select listbox of that repo's branches; keep type-ahead filtering if it's free). Free-form entry is what makes "refs/heads/main" leak into the UI today. 4. **Preview across repos:** the compare preview fetches both histories via `commits?ref=` — cross-repo previews need the head-side commits fetched from the fork's client. Same `compareHistories` logic; note the reachability rule: OpenPR publishes `refs/pull/<num>/head` only when the head commit is reachable — cross-fork heads are the designed case, but the UI should surface a clear error when the fork head is unreachable rather than a generic 422. 5. **Permissions surfaced:** To-repo options should exclude repos the viewer can't write to (OpenPR requires base write); From-repo options exclude repos the viewer can't read. ## Acceptance criteria - [ ] Composer labels read From / To (no "base ref"/"head ref" visible anywhere in the UI), with preview/compare copy updated to match. - [ ] Both sides offer a repo dropdown (default: current repo) and a ref dropdown fed by the selected repo's branches — no free-form ref entry. - [ ] A PR from a fork to the upstream repo can be created through the UI; the created PR's `Fork` info is correct and the PR page renders it. - [ ] Same-repo PRs behave exactly as today (default dropdown values, `?base=`/`?head=` deep links still prefill). - [ ] Cross-repo preview works (ahead/behind from the fork's commits) and an unreachable head yields a clear, specific error message. - [ ] Repo dropdowns respect permissions (To = writable, From = readable). - [ ] No wire/param renames — `head_ref`/`base_ref` and `?base=`/`?head=` unchanged; headless test for the label/direction mapping and the repo+ref state machine (repo choice → ref stream re-key).

crueber added the

pull requests

label

2026-09-11 15:02:28 +00:00

crueber added this to the v1 milestone

2026-09-11 15:02:28 +00:00

crueber referenced this issue from a commit

2026-09-11 16:53:37 +00:00

Fix #328: PR composer From/To labels + cross-repo repo/ref dropdowns

crueber referenced this issue from a pull request that will close it,

2026-09-11 16:53:44 +00:00

Fix #328: PR composer From/To + cross-repo #336

crueber commented

2026-09-11 16:53:47 +00:00

Author

Owner

Opened PR #336 (fix/issue-328) for review — From/To composer + cross-repo repo/ref dropdowns, fork on submit, per-side preview errors, inline unreachable-head message, headless tests green (641/641 non-smoke), vite build green. Browser proof still open (shared-daemon loopback guard). Not merging.

crueber commented

2026-09-11 17:15:30 +00:00

Author

Owner

REVIEW PR #336 (fix/issue-328, commit e32445a + review fixup 82cc730) — verified in scratch worktree /tmp/pr336 (removed after), main untouched.

ACCEPTANCE CRITERIA (issue #328, all 7):

From/To labels, no base/head ref copy — PASS with one fix (below). PullNew.jsx renders via FROM_LABEL/TO_LABEL; preview/compare/empty/same/merged copy all direction-mapped; pin test covers it.
Repo+ref dropdowns per side, no free-form entry — PASS. RefSelect is a true listbox (filter narrows only, value set only via onPick with a streamed refname); stream re-keys on repo change (PullNew.jsx createEffect on props.full(), options cleared + filter reset). Repo-change-resets-ref pinned in pr-composer.test.js.
Fork→upstream open + PR page render — PASS (code-verified, no live fork exercised). buildOpenCall sends fork:{repo} exactly when From≠To (trimmed, empty-safe), opens through the To repo client (repos.repo(baseRepo).pulls.open — SDK pulls.js:28-31 passes fork through), navigates to //pull/. Pull.jsx:727 renders 'from {fork.repo}'. Backend ForkInfo path untouched, correctly.
Same-repo byte-identical — PASS. Defaults (To refs/heads/main, From empty), ?base=/?head= prefill (search.base→To, search.head→From), payload {title,base_ref,head_ref,body?} identical modulo JSON key order (body omitted-when-empty before and after).
Cross-repo preview + unreachable-head — PASS. Preview uses Promise.allSettled with From history from the fork client; per-side 404s named via previewErrorMessage. Same-repo 422 text verified against backend ('head commit not reachable — push first', internal/pulls/service.go:381) and mapped inline ('push the From branch first', no bare status). Cross-fork unreachable honestly opens HeadPublished=false with the PR-page pending line — confirmed at Pull.jsx:718 pre-fix.
Permission filtering — PASS. To=write+ / From=any-resolved-role via the P6 ladder in lib (read is ladder bottom, null/unknown fails); current repo always present via withCurrent; listing failure degrades to [current]. Sources verified in SDK (ownerRepos core.js:293, repo.permissions access.js:39).
No wire renames + headless cover — PASS. base_ref/head_ref/?base=/?head= pinned; no package.json diff (no new deps, law 1); doc decision appended to docs/go/12_web_ui.md in the established AMENDED format (law 12 AGENTS.md: docs change with code).

LAWS: law 1 deps ✓, law 8 seams ✓ (pr-composer.js imports only compare.js; no upward imports), law 12 wire frozen ✓. Cross-owner out of scope is documented in code + doc decision — acceptable v1 (no listing endpoint serves cross-owner repos).

FIX APPLIED (pushed 82cc730 to origin/fix/issue-328): the one UI-visible leftover — Pull.jsx:718 'head ref pending — push first' → 'From branch pending — push first' (this is exactly the line cross-fork users land on), plus the adjacent 'base ref' policy comment → 'base branch', lib comment updated to match, and a new pin test covering Pull.jsx rendered copy.

NITS (non-blocking, left alone): fmtEndpoint's 3rd param is named currentFull but the caller passes the To repo — display-correct (cross iff From≠To). Mergeability small text still reads 'base X @ / head Y @ ' — pre-existing lowercase, out of scope.

VERIFY: pr-composer.test.js 15/15 ✓; full node --test web/test/unit/*.test.js 645/645, 0 fail ✓; vite build green ✓ (chunk-size warning pre-existing); grep: zero 'base ref'/'head ref' UI leftovers (only an unrelated 'HEAD ref' code comment in activity.js).

NO BROWSER (explicit): node tests + reasoning only, per review instructions; real-browser pass (composer, PR page, /setup, both themes, zero console errors) remains open as the author noted.

MERGE RECOMMENDATION: ready to merge (pending the standard real-browser pass).

REVIEW PR #336 (fix/issue-328, commit e32445a + review fixup 82cc730) — verified in scratch worktree /tmp/pr336 (removed after), main untouched. ACCEPTANCE CRITERIA (issue #328, all 7): 1. From/To labels, no base/head ref copy — PASS with one fix (below). PullNew.jsx renders via FROM_LABEL/TO_LABEL; preview/compare/empty/same/merged copy all direction-mapped; pin test covers it. 2. Repo+ref dropdowns per side, no free-form entry — PASS. RefSelect is a true listbox (filter narrows only, value set only via onPick with a streamed refname); stream re-keys on repo change (PullNew.jsx createEffect on props.full(), options cleared + filter reset). Repo-change-resets-ref pinned in pr-composer.test.js. 3. Fork→upstream open + PR page render — PASS (code-verified, no live fork exercised). buildOpenCall sends fork:{repo} exactly when From≠To (trimmed, empty-safe), opens through the To repo client (repos.repo(baseRepo).pulls.open — SDK pulls.js:28-31 passes fork through), navigates to /<toRepo>/pull/<num>. Pull.jsx:727 renders 'from {fork.repo}'. Backend ForkInfo path untouched, correctly. 4. Same-repo byte-identical — PASS. Defaults (To refs/heads/main, From empty), ?base=/?head= prefill (search.base→To, search.head→From), payload {title,base_ref,head_ref,body?} identical modulo JSON key order (body omitted-when-empty before and after). 5. Cross-repo preview + unreachable-head — PASS. Preview uses Promise.allSettled with From history from the fork client; per-side 404s named via previewErrorMessage. Same-repo 422 text verified against backend ('head commit not reachable — push first', internal/pulls/service.go:381) and mapped inline ('push the From branch first', no bare status). Cross-fork unreachable honestly opens HeadPublished=false with the PR-page pending line — confirmed at Pull.jsx:718 pre-fix. 6. Permission filtering — PASS. To=write+ / From=any-resolved-role via the P6 ladder in lib (read is ladder bottom, null/unknown fails); current repo always present via withCurrent; listing failure degrades to [current]. Sources verified in SDK (ownerRepos core.js:293, repo.permissions access.js:39). 7. No wire renames + headless cover — PASS. base_ref/head_ref/?base=/?head= pinned; no package.json diff (no new deps, law 1); doc decision appended to docs/go/12_web_ui.md in the established AMENDED format (law 12 AGENTS.md: docs change with code). LAWS: law 1 deps ✓, law 8 seams ✓ (pr-composer.js imports only compare.js; no upward imports), law 12 wire frozen ✓. Cross-owner out of scope is documented in code + doc decision — acceptable v1 (no listing endpoint serves cross-owner repos). FIX APPLIED (pushed 82cc730 to origin/fix/issue-328): the one UI-visible leftover — Pull.jsx:718 'head ref pending — push first' → 'From branch pending — push first' (this is exactly the line cross-fork users land on), plus the adjacent 'base ref' policy comment → 'base branch', lib comment updated to match, and a new pin test covering Pull.jsx rendered copy. NITS (non-blocking, left alone): fmtEndpoint's 3rd param is named currentFull but the caller passes the To repo — display-correct (cross iff From≠To). Mergeability small text still reads 'base X @ / head Y @ ' — pre-existing lowercase, out of scope. VERIFY: pr-composer.test.js 15/15 ✓; full node --test web/test/unit/*.test.js 645/645, 0 fail ✓; vite build green ✓ (chunk-size warning pre-existing); grep: zero 'base ref'/'head ref' UI leftovers (only an unrelated 'HEAD ref' code comment in activity.js). NO BROWSER (explicit): node tests + reasoning only, per review instructions; real-browser pass (composer, PR page, /setup, both themes, zero console errors) remains open as the author noted. MERGE RECOMMENDATION: ready to merge (pending the standard real-browser pass).

crueber closed this issue

2026-09-11 17:15:42 +00:00

crueber commented

2026-09-11 17:15:43 +00:00

Author

Owner

Fixed by PR #336 (review clean + one leftover-copy fix by reviewer; wire frozen, cross-repo open, permission filters verified; 645/645), merged. Closing.

crueber referenced this issue from a commit

2026-09-11 17:15:43 +00:00

Merge pull request 'Fix #328: PR composer From/To + cross-repo' (#336) from fix/issue-328 into main

crueber referenced this issue

2026-09-11 18:47:31 +00:00

Create/import: logged-in users bounded to their own username or their orgs as owner (no arbitrary owner strings) #346

crueber referenced this issue

2026-09-11 23:35:40 +00:00

Comments/bodies: autolink #N to issues and PRN to pull requests (boundary-delimited, narrow) #340

crueber referenced this issue

2026-09-12 23:00:33 +00:00

Fork button next to Clone on the code page + fork-creation page (owner/name/visibility/branch/description) and a completed server-side fork operation #424