Repository navigation
fix(a11y): palette ARIA was invalid, component source was keyboard-unreachable - #76
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…adge Audit of the whole site with axe injected into every frame (a top-frame run audits the chrome and none of the product) plus manual checks for what axe structurally cannot see. 75 -> 67 violation nodes across 40 page/theme/viewport combinations, and the interactive states axe never reaches by default went 9 nodes -> 0. The two that actually block someone: The command palette's listbox was invalid ARIA three ways: `ul[role=listbox] > li > button[role=option]` gave the listbox a `listitem` child (aria-required-children, CRITICAL), gave each option a plain `<li>` parent (aria-required-parent, CRITICAL, x4), and left four bare `listitem`s (serious). So `aria-activedescendant` pointed at an option the listbox does not own: no "option 2 of 8", no reliable active-option announcement, on the only search reachable from all 542 component pages. The `role="option"` button is now a direct child. Pixel-identical in both themes. Every component's source was unreachable by keyboard. A `<pre>` with 976x4465 of content in a 686x538 box, zero focusable descendants, no `tabindex` -- a keyboard-only visitor could not scroll the source of any component. axe MISSED this one entirely; it only flagged the smaller props-table wrapper, and only at 390px. Found by measuring overflow directly. Same class fixed in the props table, /theming's token table and two code blocks, /docs' error document, and /writing's code blocks and tables (160/285/293px of real overflow measured on one post). That fix then exposed a pre-existing hazard: site-shell's ArrowLeft/ArrowRight prev/next-component shortcut preventDefaults unless focus is in an input, slider or contenteditable. Measured -- focus in the source box, one ArrowRight navigated to a different component instead of scrolling. So the newly-focusable boxes still could not scroll horizontally and a reader lost their place. The guard now also bails for any ancestor that can still scroll horizontally in the pressed direction, and hands the shortcut back at the edge. Also: the handle-rename input at account-handle.tsx had no label, no htmlFor partner, no aria-label, not even a placeholder -- announced as bare "edit text". Named now. Two `color-contrast` deltas that appeared in the before/after are NOT claimed as fixes; they are run-to-run sampling variance on accent links. Verified independently rather than on the auditing agent's word: with the palette open and filtered, axe reports NO violations for aria-required-children, aria-required-parent, listitem or aria-valid-attr-value; and of three scrollable boxes on a component page, zero are unreachable by keyboard. Separately, the directree directory badge. Unlike the five opaque pills already in the footer it has NO background of its own -- bare text and a logo -- so one variant always loses: their lightmode art is #0d1b1f (invisible on #0a0a0a) and darkmode is #faf5f1 (invisible on white). Both are published, so both ship and CSS picks, the same `dark:` swap the theme and cursor toggles use, correct on the first painted frame with no JS. Their verifier finds its asset either way since both are in the markup. The first version of that had the accessible name on one image, which is right in one theme and BACKWARDS in the other: CSS chooses which <img> paints but cannot move `alt`, so in light mode the visible badge carried alt="" aria-hidden while the hidden one was the thing announced. Caught in verification, not review. The name is on the anchor now, which is theme-independent and announces exactly once. Verified in both themes: one variant visible and loaded at 200x33, both images aria-hidden, link named, zero horizontal overflow. Node was upgraded to v26.11.1 by another process mid-change; engines allow >=22.18 and tsc, source invariants and build were all re-run clean on it rather than assumed. Signed-off-by: Nik <84yk8btb9f@privaterelay.appleid.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nikolas-sapa
force-pushed
the
fix/a11y-audit
branch
from
October 14, 2026 03:29
32d23c6 to
f37f628
Compare
nikolas-sapa
added a commit
that referenced
this pull request
Oct 14, 2026
/submit opens a PR as the submitting user, in 9 sequential GitHub calls
with up to 9s of deliberate sleep in the fork poll, with no timeout on
any call and no maxDuration on the route. The branch name is
deterministic (`submit/${slug}`), and createBranch threw on any non-ok
response including 422.
So a function killed mid-flow, after the fork and branch existed, left
the slug unsubmittable FOREVER: every later retry hit 422 "Reference
already exists" and threw. The contributor could not recover without
manually deleting the branch on their own fork. This is the contribution
path for a project with almost no outside contributors, so one person
hitting it probably never came back.
Reproduced on unmodified shipped code, not argued from reading it.
`origin/main:lib/github-submit.ts` was extracted verbatim (sole edit: a
`public status` constructor parameter property rewritten longhand,
because Node's strip-only .ts loader rejects that syntax — diffed to
prove nothing else changed) and the route's exact call sequence replayed
against a fake GitHub at the fetch boundary. Attempt 1 killed at call 6:
branch survived, 0 files written, 0 PRs. Attempts 2, 3 and 4 — each with
a full budget and no kill — then died identically on `Could not create
branch submit/retry-trap-demo (422)` after 5 calls. Zero PRs ever.
THREE traps existed, not the one the audit found:
ensureBranch 422 "Reference already exists"
putFile 422 `"sha" wasn't supplied` (one step later)
openPullRequest 422 "A pull request already exists"
Each now resumes, but only on POSITIVE confirmation — it re-reads the
ref, the file, or the PR list. A 422 that cannot be confirmed is still a
real failure and still throws, rather than being pattern-matched out of
GitHub's prose.
Kept the deterministic branch name instead of uniquifying per attempt. A
unique branch retries just as cleanly but leaves a dead branch on the
contributor's fork and a duplicate upstream PR for every failed attempt:
permanent maintainer cleanup to spare the contributor one click.
Pre-flight delete is worse — it can destroy work.
Clobber protection: when the branch exists AND an open PR is found, the
PR is adopted and nothing is written, so a resubmit cannot rewrite files
a maintainer is mid-review on.
Closed the window as well as the trap: GITHUB_CALL_TIMEOUT_MS of 8s per
call (the precedent set in #73), a 45s whole-submission AbortSignal
budget intersected into every call, a fork poll that will not sleep past
that budget, and maxDuration = 60 on the route.
Security properties verified rather than assumed. The PR-adoption path
cannot cross users: `ensureFork` returns `owner: data.owner.login` read
from the fork response made with the CALLER's own token, never from env
and never from client input; `findOpenPullRequest` filters
`head=<that login>:<branch>` and `openPullRequest` posts
`head: ${forkOwner}:${branch}` from the same value. UPSTREAM_OWNER /
UPSTREAM_REPO only ever name the target repo, never the head. So user B
submitting a slug user A already submitted resolves to
`B:submit/<slug>` on B's own fork and can never adopt A's PR.
SUBMIT_TOKEN_BINDING_SECRET untouched and still fails closed. This
change does touch the route that owns that secret, so "no hits in the
diff" was not good enough: all 14 lines mentioning the secret,
`github_not_configured`, `resolveGithubToken` or 428 are text-identical
to origin/main with only line numbers shifted by the maxDuration comment
block above them, and `resolveGithubToken` itself is byte-identical
(diff -q clean). The missing-secret branch still returns
`github_not_configured` and no unbound token is ever issued. The new
test references neither the secret nor that error string and adds no
credentials.
scripts/test-submit-retry.ts — 17 checks, no framework, driven against a
fake GitHub at the fetch boundary. Mutation-proven: reverting the
ensureBranch 422 resume (the original bug) takes it to exit 1 with 10
failing checks and the message `Could not create branch
submit/retry-trap-demo (422)`; restoring it returns exit 0. Five further
mutants — no AbortSignal, putFile not resuming, openPullRequest not
adopting, the open-PR pre-check dropped, the fork poll ignoring its
budget — each fail with their own named check.
Verified on this rebase onto #76: tsc exit 0, source invariants 0
problems, submit-retry PASS, build exit 0.
Separately established, not a code change: the 3 tsc errors in
convex/auth.ts seen in this worktree are an install artifact.
`@auth/core` is a `peer: true` lockfile entry at 0.41.3, a peer of
@convex-dev/auth and absent from package.json, and the global ~/.npmrc
sets legacy-peer-deps=true, which makes npm skip auto-installing peers.
It was missing from node_modules in this worktree only, present in all
five siblings. Installing it with --no-save --package-lock=false took
tsc to 0 errors with package.json and package-lock.json untouched. CI
has no legacy-peer-deps=true and never saw these.
Signed-off-by: Nik <84yk8btb9f@privaterelay.appleid.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nikolas-sapa
added a commit
that referenced
this pull request
Oct 14, 2026
…ug (#78) /submit opens a PR as the submitting user, in 9 sequential GitHub calls with up to 9s of deliberate sleep in the fork poll, with no timeout on any call and no maxDuration on the route. The branch name is deterministic (`submit/${slug}`), and createBranch threw on any non-ok response including 422. So a function killed mid-flow, after the fork and branch existed, left the slug unsubmittable FOREVER: every later retry hit 422 "Reference already exists" and threw. The contributor could not recover without manually deleting the branch on their own fork. This is the contribution path for a project with almost no outside contributors, so one person hitting it probably never came back. Reproduced on unmodified shipped code, not argued from reading it. `origin/main:lib/github-submit.ts` was extracted verbatim (sole edit: a `public status` constructor parameter property rewritten longhand, because Node's strip-only .ts loader rejects that syntax — diffed to prove nothing else changed) and the route's exact call sequence replayed against a fake GitHub at the fetch boundary. Attempt 1 killed at call 6: branch survived, 0 files written, 0 PRs. Attempts 2, 3 and 4 — each with a full budget and no kill — then died identically on `Could not create branch submit/retry-trap-demo (422)` after 5 calls. Zero PRs ever. THREE traps existed, not the one the audit found: ensureBranch 422 "Reference already exists" putFile 422 `"sha" wasn't supplied` (one step later) openPullRequest 422 "A pull request already exists" Each now resumes, but only on POSITIVE confirmation — it re-reads the ref, the file, or the PR list. A 422 that cannot be confirmed is still a real failure and still throws, rather than being pattern-matched out of GitHub's prose. Kept the deterministic branch name instead of uniquifying per attempt. A unique branch retries just as cleanly but leaves a dead branch on the contributor's fork and a duplicate upstream PR for every failed attempt: permanent maintainer cleanup to spare the contributor one click. Pre-flight delete is worse — it can destroy work. Clobber protection: when the branch exists AND an open PR is found, the PR is adopted and nothing is written, so a resubmit cannot rewrite files a maintainer is mid-review on. Closed the window as well as the trap: GITHUB_CALL_TIMEOUT_MS of 8s per call (the precedent set in #73), a 45s whole-submission AbortSignal budget intersected into every call, a fork poll that will not sleep past that budget, and maxDuration = 60 on the route. Security properties verified rather than assumed. The PR-adoption path cannot cross users: `ensureFork` returns `owner: data.owner.login` read from the fork response made with the CALLER's own token, never from env and never from client input; `findOpenPullRequest` filters `head=<that login>:<branch>` and `openPullRequest` posts `head: ${forkOwner}:${branch}` from the same value. UPSTREAM_OWNER / UPSTREAM_REPO only ever name the target repo, never the head. So user B submitting a slug user A already submitted resolves to `B:submit/<slug>` on B's own fork and can never adopt A's PR. SUBMIT_TOKEN_BINDING_SECRET untouched and still fails closed. This change does touch the route that owns that secret, so "no hits in the diff" was not good enough: all 14 lines mentioning the secret, `github_not_configured`, `resolveGithubToken` or 428 are text-identical to origin/main with only line numbers shifted by the maxDuration comment block above them, and `resolveGithubToken` itself is byte-identical (diff -q clean). The missing-secret branch still returns `github_not_configured` and no unbound token is ever issued. The new test references neither the secret nor that error string and adds no credentials. scripts/test-submit-retry.ts — 17 checks, no framework, driven against a fake GitHub at the fetch boundary. Mutation-proven: reverting the ensureBranch 422 resume (the original bug) takes it to exit 1 with 10 failing checks and the message `Could not create branch submit/retry-trap-demo (422)`; restoring it returns exit 0. Five further mutants — no AbortSignal, putFile not resuming, openPullRequest not adopting, the open-PR pre-check dropped, the fork poll ignoring its budget — each fail with their own named check. Verified on this rebase onto #76: tsc exit 0, source invariants 0 problems, submit-retry PASS, build exit 0. Separately established, not a code change: the 3 tsc errors in convex/auth.ts seen in this worktree are an install artifact. `@auth/core` is a `peer: true` lockfile entry at 0.41.3, a peer of @convex-dev/auth and absent from package.json, and the global ~/.npmrc sets legacy-peer-deps=true, which makes npm skip auto-installing peers. It was missing from node_modules in this worktree only, present in all five siblings. Installing it with --no-save --package-lock=false took tsc to 0 errors with package.json and package-lock.json untouched. CI has no legacy-peer-deps=true and never saw these. Signed-off-by: Nik <84yk8btb9f@privaterelay.appleid.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Site-wide audit with axe injected into every frame (a top-frame-only run audits the chrome and none of the product), plus manual checks for what axe structurally cannot see.
75 → 67 violation nodes across 40 page/theme/viewport combinations. The interactive states axe never reaches by default went 9 nodes → 0.
The two that actually block someone
1. The command palette's listbox was invalid ARIA three ways.
ul[role=listbox] > li > button[role=option]gave the listbox alistitemchild (aria-required-children, critical), gave each option a plain<li>parent (aria-required-parent, critical, ×4), and left four barelistitems (serious).So
aria-activedescendantpointed at an option the listbox doesn't own: no "option 2 of 8", no reliable active-option announcement — on the only search reachable from all 542 component pages. Therole="option"button is now a direct child. Pixel-identical in both themes.2. Every component's source was unreachable by keyboard.
A
<pre>with 976×4465 of content in a 686×538 box, zero focusable descendants, notabindex. A keyboard-only visitor could not scroll the source of any component.axe missed this entirely — it only flagged the smaller props-table wrapper, and only at 390px. Found by measuring overflow directly. Same class fixed in the props table,
/theming's token table + 2 code blocks,/docs' error document, and/writing's code blocks and tables (160/285/293px of real overflow measured).3. That fix then exposed a pre-existing hazard.
site-shell's ArrowLeft/ArrowRight prev/next shortcutpreventDefaults unless focus is in an input, slider or contenteditable. Measured: focus in the source box, one ArrowRight navigated to a different component instead of scrolling. So the newly-focusable boxes still couldn't scroll horizontally and a reader lost their place. The guard now also bails for any ancestor that can still scroll horizontally in the pressed direction, and hands the shortcut back at the edge.4. The handle-rename input had no label, no
htmlForpartner, noaria-label, not even a placeholder — announced as bare "edit text".Not claimed
Two
color-contrastdeltas appeared in the before/after and are not claimed as fixes — they're run-to-run sampling variance on accent links.Verified independently
Not taken on the auditing agent's word:
aria-required-children,aria-required-parent,listitemoraria-valid-attr-valueAlso: directree badge
Unlike the five opaque pills already in the footer, directree's has no background of its own — bare text and a logo — so one variant always loses: their lightmode art is
#0d1b1f(invisible on#0a0a0a), darkmode is#faf5f1(invisible on white). Both are published, so both ship and CSS picks — the samedark:swap the theme and cursor toggles use, correct on the first painted frame with no JS. Their verifier finds its asset either way since both are in the markup.The first version of that had an a11y bug of its own. The name was on one image, which is right in one theme and backwards in the other: CSS chooses which
<img>paints but cannot movealt, so in light mode the visible badge carriedalt="" aria-hiddenwhile the hidden one was the thing announced. Caught in verification, not review. The name is on the anchor now — theme-independent, announces exactly once.Verified both themes: one variant visible and loaded at 200×33, both images
aria-hidden, link named, 0px horizontal overflow.Verification
npx tsc --noEmit— exit 0npm run test:source-invariants— 0 problemsnpm run build— exit 0, compiled 22.1sNode was upgraded to v26.11.1 by another process mid-change.
enginesallows>=22.18, and all three gates were re-run on the new major rather than assumed to still hold.🤖 Generated with Claude Code