From 8a90be613ccb235d2999235fe1e37b39988907e4 Mon Sep 17 00:00:00 2001 From: Jacobus Geluk Date: Fri, 21 Aug 2026 13:30:30 +0100 Subject: [PATCH] fix(site): list only main, develop and branches with an open pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #249 filtered on branch existence, which still left merged-but-undeleted branches in the picker: production advertised 9 branches, of which one had an open pull request. The listing is now main + develop + every branch with a pull request in flight. Anything else is a unit of work rather than a version of the specification. Routing stays permissive, and the advertisability filter moved out of getSpecVersions() into getListedSpecVersions() to keep it that way. Filtering during discovery would unroute those deployments as well as hide them, so a preview link shared on a pull request would break the moment it merged — the opposite of what is wanted. (This also fixes that for dependabot/*, which #249 had inadvertently unrouted.) dependabot/* stays unadvertised even while its pull request is open: a dependency bump is not a version of the specification. ballot/* is no longer special-cased. A ballot is listed while its pull request is open, and afterwards lives on as the frozen archive entry if adopted, or as an unlisted but still-resolvable preview URL. Simulated against live data the listing becomes the archive, develop and issue/184-schedule-model, dropping 7 merged branches. Note the token permission changes with the endpoint: /pulls requires Pull requests: Read-only, where /branches needed Contents: Read-only. Documented in site/.env.example and site/README.md. Resolves: #251 --- site/.env.example | 10 +-- site/README.md | 25 ++++--- site/src/lib/spec-versions.ts | 126 ++++++++++++++++++++-------------- 3 files changed, 94 insertions(+), 67 deletions(-) diff --git a/site/.env.example b/site/.env.example index 2d8f2ba..6838a1e 100644 --- a/site/.env.example +++ b/site/.env.example @@ -22,13 +22,13 @@ VERCEL_ORG_ID= # Fine-grained token setup: # Resource owner EKGF # Repository access Only select repositories -> EKGF/dprod -# Permissions Contents: Read-only +# Permissions Pull requests: Read-only # -# "Contents (read)" is what GET /repos/{owner}/{repo}/branches requires; see -# https://docs.github.com/en/rest/branches/branches. GitHub adds the mandatory +# "Pull requests (read)" is what GET /repos/{owner}/{repo}/pulls requires; see +# https://docs.github.com/en/rest/pulls/pulls. GitHub adds the mandatory # "Metadata: Read-only" automatically once any repository permission is -# selected. No write permissions, and no Pull requests permission — the branch -# lookup does not touch that endpoint. +# selected. No write permissions, and no Contents permission — the listing +# only asks which pull requests are open. # # A classic token needs `public_repo` instead. # diff --git a/site/README.md b/site/README.md index 365887a..191627b 100644 --- a/site/README.md +++ b/site/README.md @@ -11,17 +11,20 @@ The Next.js app served at `https://ekgf.org/dprod` (proxied verbatim from the | Source | Question it answers | Env vars | |---|---|---| | Vercel deployments API | Which branches have a READY deployment? | `VERCEL_TOKEN`, `VERCEL_PROJECT_ID`, `VERCEL_ORG_ID` | -| GitHub branches API | Which of those branches still exist? | `GITHUB_TOKEN` | +| GitHub pulls API | Which of those branches have an open pull request? | `GITHUB_TOKEN` | Both are cached for 60 seconds via `next: { revalidate: 60 }`, so a new branch appears without redeploying `develop`. -A branch deleted from `origin` drops off the listing, **including `ballot/*`**. -Its Vercel deployment survives and `/spec/` still resolves, so any URL -already published keeps working — it is simply no longer advertised. This is -deliberate (issue #249): the listing answers "which versions exist now?", and -a deleted branch does not. If a ballot needs to stay listed after its branch is -gone, keep the branch on `origin` rather than special-casing it here. +The listing is `main` + `develop` + every branch with an **open pull request**. +Everything else is left out: a merged branch nobody deleted, an abandoned +experiment, a `dependabot/*` bump. Those previews stay routable — a link shared +on a pull request keeps working after it merges — but they are not advertised +as versions of the specification. + +`ballot/*` is not special-cased. A ballot branch is listed while its pull +request is open; afterwards it lives on as the frozen archive entry (`main`) +if it was adopted, or as an unlisted but still-resolvable preview URL. There are two entry points, and the difference matters: @@ -37,11 +40,11 @@ There are two entry points, and the difference matters: Required, and it must be **valid** — an expired or revoked token behaves exactly like a missing one. Fine-grained tokens expire, so this will recur. -A fine-grained token needs **`Contents: Read-only`** on `EKGF/dprod` — that is -the permission `GET /repos/{owner}/{repo}/branches` -[requires](https://docs.github.com/en/rest/branches/branches). GitHub adds the +A fine-grained token needs **`Pull requests: Read-only`** on `EKGF/dprod` — +that is the permission `GET /repos/{owner}/{repo}/pulls` +[requires](https://docs.github.com/en/rest/pulls/pulls). GitHub adds the mandatory `Metadata: Read-only` automatically. No write permissions, and no -Pull requests permission: the lookup does not use that endpoint. +`Contents` permission. The repository is public, so the endpoint would also answer an unauthenticated request — but that path carries GitHub's 60-requests-per-hour-per-IP limit, diff --git a/site/src/lib/spec-versions.ts b/site/src/lib/spec-versions.ts index af5e02c..be1a4b1 100644 --- a/site/src/lib/spec-versions.ts +++ b/site/src/lib/spec-versions.ts @@ -33,10 +33,10 @@ export type SpecVersion = { /** True if this version is the production branch (develop) */ isProduction: boolean; /** - * Whether the branch still exists upstream. `null` means the lookup was - * unavailable, so existence is unknown — never treat that as "yes". + * Whether the branch has an open pull request. `null` means the lookup was + * unavailable, so it is unknown — never treat that as "yes". */ - existsUpstream: boolean | null; + hasOpenPullRequest: boolean | null; kind: SpecVersionKind; }; @@ -48,7 +48,7 @@ const ARCHIVE_1_0: SpecVersion = { origin: "", isCurrent: false, isProduction: false, - existsUpstream: true, + hasOpenPullRequest: false, kind: "archive", }; @@ -56,15 +56,23 @@ const ARCHIVE_1_0: SpecVersion = { const EXCLUDED_BRANCHES = new Set(["main"]); /** - * Branch prefixes that are never spec versions. A dependency bump produces a - * perfectly valid preview deployment, but it is not a version of the - * specification and only adds noise to the picker. + * Branch prefixes that are not *advertised* as spec versions. + * + * These branches still deploy, and `/spec/` still routes to them — a + * preview link shared on a pull request has to keep working. They are simply + * not listed, because the picker answers "which versions of the specification + * are there?", and a dependency bump is not one even while its pull request + * is open. + * + * Applied in `getListedSpecVersions()` only, never in `getSpecVersions()`. + * Filtering during discovery would unroute the deployments as well as hide + * them. */ -const EXCLUDED_BRANCH_PREFIXES = ["dependabot/"]; +const UNADVERTISED_BRANCH_PREFIXES = ["dependabot/"]; function isAdvertisableBranch(branch: string): boolean { if (EXCLUDED_BRANCHES.has(branch)) return false; - return !EXCLUDED_BRANCH_PREFIXES.some((prefix) => branch.startsWith(prefix)); + return !UNADVERTISED_BRANCH_PREFIXES.some((prefix) => branch.startsWith(prefix)); } function branchToSlug(branch: string): string { @@ -118,29 +126,29 @@ async function fetchDeployments(): Promise { } } -type GitHubBranch = { - name?: string; +type GitHubPullRequest = { + head?: { ref?: string }; }; /** - * Every branch that currently exists in the repository, or `null` when the - * lookup was unavailable. + * The branches with an open pull request, or `null` when the lookup was + * unavailable. * - * Existence — not open-PR status — is the right question. A branch whose PR - * has merged is normally deleted, and its Vercel preview, while still - * reachable by URL, is no longer a version of anything. Conversely a branch - * can legitimately exist with no open PR (merged but kept, or pushed before - * the PR is raised), and an open-PR filter hid those too. + * This is what "a version worth listing" means here: `main` and `develop` are + * shown unconditionally, and every other branch earns its place by having a + * pull request in flight. A branch whose PR has merged stops being listed even + * if nobody deleted it, which is the case that made the picker unusable — it + * was showing seven merged branches alongside one live one. * - * Returns `null` rather than an empty set on failure, so callers can tell - * "no branches" apart from "could not ask" — very different things. + * Returns `null` rather than an empty set on failure, so callers can tell "no + * open pull requests" apart from "could not ask" — very different things. * - * Every failure path logs the status and GitHub's own message. The bug this - * replaced (issue #249) was undiagnosable from outside precisely because it - * failed silently: a present-but-rejected token looks exactly like a missing - * one when nothing is logged. + * Every failure path logs the status and GitHub's own message. The original + * bug (issue #249) was undiagnosable from outside precisely because it failed + * silently: a present-but-rejected token looks exactly like a missing one when + * nothing is logged. */ -async function fetchExistingBranches(): Promise | null> { +async function fetchOpenPullRequestBranches(): Promise | null> { const token = process.env.GITHUB_TOKEN; if (!token) { @@ -149,7 +157,8 @@ async function fetchExistingBranches(): Promise | null> { // degraded-but-workable path — it fails continuously. console.warn( "[spec-versions] GITHUB_TOKEN is not set; cannot determine which " + - "branches still exist. Showing the archive and develop only.", + "branches have open pull requests. Showing the archive and develop " + + "only.", ); return null; } @@ -165,35 +174,35 @@ async function fetchExistingBranches(): Promise | null> { }; const branches = new Set(); - // The repository has far fewer than 100 branches today, but paginate - // anyway: silently truncating would hide live branches. + // Paginate: silently truncating at 100 would hide live pull requests. for (let page = 1; page <= 10; page++) { let res: Response; try { res = await fetch( - `https://api.github.com/repos/EKGF/dprod/branches?per_page=100&page=${page}`, + "https://api.github.com/repos/EKGF/dprod/pulls" + + `?state=open&per_page=100&page=${page}`, { headers, next: { revalidate: 60 } }, ); } catch (error) { - console.warn("[spec-versions] GitHub branch lookup threw:", error); + console.warn("[spec-versions] GitHub pull request lookup threw:", error); return null; } if (!res.ok) { // GitHub's body distinguishes the cases that matter: "Bad credentials" - // (401, token expired or revoked), "API rate limit exceeded" (403), - // and resource-not-accessible (403, token lacks Metadata: Read-only). + // (401, token expired or revoked), "API rate limit exceeded" (403), and + // "Resource not accessible" (403, token lacks Pull requests: Read-only). const detail = await res.text().catch(() => ""); console.warn( - `[spec-versions] GitHub branch lookup failed: ${res.status} ` + + `[spec-versions] GitHub pull request lookup failed: ${res.status} ` + `${res.statusText}. ${detail.slice(0, 300)}`, ); return null; } - const pageBranches = (await res.json()) as GitHubBranch[]; - for (const branch of pageBranches) { - if (branch.name) branches.add(branch.name); + const pulls = (await res.json()) as GitHubPullRequest[]; + for (const pull of pulls) { + if (pull.head?.ref) branches.add(pull.head.ref); } - if (pageBranches.length < 100) break; + if (pulls.length < 100) break; } return branches; } @@ -214,15 +223,18 @@ export async function getSpecVersions(): Promise { const currentBranch = process.env.VERCEL_GIT_COMMIT_REF; const versions: SpecVersion[] = [ARCHIVE_1_0]; - const [deployments, existingBranches] = await Promise.all([ + const [deployments, openPullRequestBranches] = await Promise.all([ fetchDeployments(), - fetchExistingBranches(), + fetchOpenPullRequestBranches(), ]); const seen = new Set(); for (const dep of deployments) { const branch = dep.meta?.githubCommitRef; - if (!branch || !isAdvertisableBranch(branch) || seen.has(branch)) continue; + // Deliberately not filtered by isAdvertisableBranch(): this list is what + // the middleware routes with, and a preview URL already shared must keep + // resolving even when the branch is not advertised. + if (!branch || EXCLUDED_BRANCHES.has(branch) || seen.has(branch)) continue; seen.add(branch); const slug = branchToSlug(branch); @@ -240,7 +252,9 @@ export async function getSpecVersions(): Promise { origin: `https://${slug}.dprod-preview.ekgf.org/dprod`, isCurrent: branch === currentBranch, isProduction: branch === "develop", - existsUpstream: existingBranches ? existingBranches.has(branch) : null, + hasOpenPullRequest: openPullRequestBranches + ? openPullRequestBranches.has(branch) + : null, kind: "vercel-branch", }); } @@ -263,7 +277,7 @@ export async function getSpecVersions(): Promise { export type ListedSpecVersions = { versions: SpecVersion[]; /** - * False when branch existence could not be determined, so the list is the + * False when the pull request lookup failed, so the list is the * fail-closed minimum rather than the real set. Surfaced in the UI: a * silently short list is as misleading as a silently long one. */ @@ -271,19 +285,28 @@ export type ListedSpecVersions = { }; /** - * The spec versions to show a reader: the archive, develop, and branches that - * still exist upstream. + * The spec versions to show a reader: + * + * - `main`, the frozen OMG standard, as the archive entry; + * - `develop`, the current working draft; + * - every branch with an open pull request, i.e. work actually in flight. + * + * Anything else — a merged branch nobody deleted, an abandoned experiment, a + * dependency bump — is not a version of the specification and is left out. + * Those previews stay routable through `getSpecVersions()`, so a link shared + * on a pull request keeps working after it merges; it is only the listing that + * is opinionated. * - * Fails *closed*. When branch existence is unknown the picker shows only the - * archive and develop, because the alternative — what shipped before issue - * #249 — was every branch ever deployed, including many deleted months - * earlier. A short list is a smaller lie than a wrong one, and - * `getSpecVersions()` still routes any preview URL that has been handed out. + * Fails *closed*. When the lookup fails the picker shows only the archive and + * develop, because the alternative — what shipped before issue #249 — was + * every branch ever deployed, including many deleted months earlier. A short + * list is a smaller lie than a wrong one. */ export async function getListedSpecVersions(): Promise { const versions = await getSpecVersions(); const complete = !versions.some( - (version) => version.kind === "vercel-branch" && version.existsUpstream === null, + (version) => + version.kind === "vercel-branch" && version.hasOpenPullRequest === null, ); return { complete, @@ -291,7 +314,8 @@ export async function getListedSpecVersions(): Promise { (version) => version.kind === "archive" || version.isProduction || - version.existsUpstream === true, + (version.hasOpenPullRequest === true && + isAdvertisableBranch(version.branch)), ), }; }