Repository navigation
fix(a11y): announce new-tab links and the sign-in error; add /core-push; pin visual dates - #17
Merged
JakeGinnivan merged 4 commits intoSep 3, 2026
Conversation
Upstreamed from dddperth#150. AppLink becomes the single link component: it picks Link vs anchor from the shape of `to` and applies the new-tab treatment itself, so call sites can no longer forget rel="noopener" or the announcement. MdxLink and the two other copies of the is-this-external check are gone. The aria-label case needs the hint folded into the label rather than an srOnly span, because aria-label replaces element content when the accessible name is computed. Getting that backwards produces markup that looks right and says nothing, so the tests assert the computed accessible name in both directions. auth.login's error box gets role="alert" plus focus-on-render. Both halves are needed: the container isn't in the DOM before the error exists, so there was no live region to observe, and the error arrives in a fresh server-rendered document with focus at the top of the page. The stub's e2e content fixture gains one external link so the visual baselines actually cover the new ↗ glyph.
Changes move in both directions between a fork and core, but only one direction had a skill. The pull was documented and the push was rediscovered from scratch each time — including the part that actually matters, which is what must NOT travel up. `git subtree split --prefix=core` works and fast-forwards cleanly onto ddd-core/main, which makes it a tempting way to publish. It is the wrong tool: the split carries the fork's whole core/ tree, so it would overwrite upstream's 63 stub-branded visual baselines with conference-branded screenshots, restore the fork's nx.json/vitest.workspace.ts deletions, and push the fork-shape tsconfig/vite/project.json overrides that /new-conference applies. /core-push is therefore a curated PR, and keeps the split only as a cross-check that nothing was left behind by accident. It also records the layer mapping the fork's own layout implies but never stated: conference/content/pages has a counterpart in conference-stub, while the fork's root CLAUDE.md and ARCHITECTURE.md deliberately do not — core has its own, describing the shared upstream rather than one conference. Copying those up is a mistake that looks like diligence. Renamed for symmetry: /core-pull and /core-push read as a pair in a way that /pull-upstream and /core-push do not. The skill description keeps "pull-upstream" as a trigger phrase so the old name still invokes it, and scripts/sync-core-skills.mjs already removes skills core has renamed away, so forks drop the stale copy on their next pull.
The stub's e2e content fixture gained an external link so the baselines actually cover the new-tab ↗, which makes that page ~120px taller. Regenerated in the pinned Playwright container (v1.62.1-noble), not on the host. Exactly 9 of 63 baselines change — the 3 engines x 3 viewports for content-page-mdx-template. The other 54 are untouched, which is the meaningful signal: the srOnly hints and role="alert" add DOM nodes but nothing visible.
Content pages render an "Important Dates" card whose DaysLeftPill counts down to the conference date, so /e2e-content-fixture captured a live "N days left". That is a fresh pixel diff every day, on a committed baseline — the suite decays on a timer rather than on a code change. Found the honest way: regenerating for an unrelated change silently folded in "46 days left" -> "44 days left", two days having passed since the previous run. The count of changed files still matched what that change predicted, so it looked correct. The visual projects now seed __devDateOverride, the same cookie date-states.spec.ts already uses. Two traps, both of which fail *silently* — the cookie is ignored and the page renders at the live clock: - The domain must come from baseURL. Under `pnpm vr` Playwright runs in the container and reaches the host as host.docker.internal, so a hardcoded localhost is dropped. - The value must not need URL-encoding. The provider decodeURIComponent's it, so an ISO datetime's colons have to be percent-encoded to survive the cookie header. Using a date-only value sidesteps this entirely. Because both failures are silent, visual.spec.ts now asserts the countdown reads the pinned number before trusting any baseline. Verified it works by breaking the domain: the guard fails while all 63 baselines still pass, since the pill is far below the 2% tolerance. Without the guard that break is invisible. Regenerated with --update-snapshots=all: 33 baselines carry the countdown (agenda, home, sponsors, and content-page at tablet/desktop — it is hidden in the small sidebar at mobile). Every diff is under 0.32%, all of it the countdown text. Re-running clean afterwards passes 72/72 with no churn. VISUAL_DATE and its expected countdown live in e2e/routes.ts beside the other fork-owned fixtures, so a fork edits one file.
JakeGinnivan
deleted the
jakeginnivan/upstream-a11y-new-tab-link-and-login-error-announc
branch
September 3, 2026 06:07
JakeGinnivan
added a commit
that referenced
this pull request
Sep 8, 2026
`setup` fans out to four generators with no ordering, so nx runs them
concurrently — and all four write into `website/`. `react-router typegen`
clears and rewrites `.react-router/types/` as it goes, so its rmdir can hit a
directory another generator has just repopulated:
ENOTEMPTY: directory not empty, rmdir
'.../website/.react-router/types/app/routes/+types'
That kills the visual-regression job before it takes a single screenshot. It
hit three PRs in a row (#16, #17, #18), each going green on a plain re-run,
which is what a race looks like rather than a real failure.
`parallelism: false` on the four setup targets serialises them. Verified by
running `nx setup` from a clean tree and again over existing generated output
— the repeat run is the case CI was failing on.
This is a stopgap: the targets are still unordered relative to each other, and
the workflow additionally runs `nx setup website` explicitly even though
`e2e-visual` already dependsOn it, so setup runs twice per job. Worth tidying
properly, but serialising fixes the flake now.
Claude-Session: https://claude.ai/code/session_019eNk5wET3eWEqRFFtLihTV
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
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.
Upstreams dddperth#150, and adds the missing half of the fork/core skill pair.
1.
fix(a11y): announce new-tab links and the sign-in errorBuilt and reviewed in the DDD Perth fork, now moved into core so every fork gets it.
AppLinkbecomes the single link component. It picks<Link>vs<a>from the shape oftoand applies the new-tab treatment itself, so a call site can no longer forgetrel="noopener"or the announcement.MdxLinkand the two other copies of the is-this-external check are gone. The branch has to live in the component because almost every href here is a runtime value (sponsor.website,action.href), so "is this external?" isn't answerable by reading the JSX.Two details that are easy to get backwards, both now covered by tests asserting the computed accessible name:
aria-labelreplaces element content, so ansrOnlyspan inside a labelled icon link is silently dropped. Those call sites need the hint folded into the label instead. Getting it backwards produces markup that looks right and says nothing."Register as a Volunteer(opens in a new tab)". An NBSP doesn't survive either.Axe cannot catch any of this — an unannounced new-tab link is a valid link to every automated rule — which is why the coverage is unit tests rather than an added e2e scan.
auth.login's error box getsrole="alert"plus focus-on-render. Both halves are needed: the container isn't in the DOM before the error exists, so there was no live region to observe, and the error arrives in a fresh server-rendered document with focus at the top of the page.The stub's
e2e-content-fixture.mdxgains one external link so the visual baselines actually cover the new↗. Without it the suite passes while rendering no external text link at all — invisible to the very suite meant to catch it.2.
feat(skills): add/core-push, rename/pull-upstream→/core-pullChanges move in both directions, but only one direction had a skill. The push was rediscovered from scratch each time — including the part that actually matters, which is what must not travel up.
git subtree split --prefix=coreworks and fast-forwards cleanly ontoddd-core/main, which makes it a tempting way to publish. It is the wrong tool. Verified on this very change: the split would havenx.json/vitest.workspace.tsdeletions, andtsconfig.json/vite.config.ts/project.jsonoverrides that/new-conferenceapplies.So
/core-pushis a curated PR, and keeps the split only as a cross-check that nothing was left behind by accident.It also records the layer mapping the layout implies but never stated:
conference/content/pageshas a counterpart inconference-stub, while the fork's rootCLAUDE.md/ARCHITECTURE.mddeliberately do not — core has its own, describing the shared upstream rather than one conference. Copying those up is a mistake that looks like diligence. (This PR'sCLAUDE.mdsection was hand-written for core, not copied.)Renamed for symmetry. The skill description keeps
pull-upstreamas a trigger phrase so the old name still invokes it, andscripts/sync-core-skills.mjsalready removes skills core has renamed away, so forks drop the stale copy on their next pull.Verification
vitest runnx e2e(a11y)tsc --noEmiteslint .worker-configuration.d.ts)react-router buildpnpm vrBaselines, and a bug found while reviewing them
The stub's fixture gained an external link, so that page is ~120px taller.
Regenerated in the pinned Playwright container (
v1.62.1-noble), not on the host.Reviewing the regenerated images turned up a pre-existing bug unrelated to
this change, now fixed here (
64ff40e) since it would otherwise keepcorrupting every future regeneration:
The visual baselines were rotting on a timer. Content pages render an
"Important Dates" card whose
DaysLeftPillcounts down to the conference date,so
/e2e-content-fixturecaptured a live "N days left" — a fresh pixel diffevery day, on a committed baseline. It surfaced because the first regeneration
here silently folded in
46 days left->44 days left, two days havingpassed since the previous run. The changed-file count still matched what this
PR predicted, so it looked correct.
The
visual-*projects now seed__devDateOverride(the cookiedate-states.spec.tsalready uses). Two traps, both of which fail silently —the cookie is ignored and the page renders at the live clock:
baseURL. Underpnpm vrPlaywright runsinside the container and reaches the host as
host.docker.internal, so ahardcoded
localhostis dropped.decodeURIComponentsit, so an ISO datetime's colons must be percent-encoded to survive the cookie
header. A date-only value sidesteps this.
Because both failures are silent,
visual.spec.tsnow asserts the countdownreads the pinned number before trusting any baseline. Verified by breaking the
domain on purpose: the guard fails while all 63 baselines still pass, because
the pill is far below the 2%
maxDiffPixelRatio. Without the guard, that breakis invisible.
VISUAL_DATEand its expected countdown live ine2e/routes.tsbeside theother fork-owned fixtures, so a fork edits one file.
33 baselines now differ from
main: the 9content-page-mdx-templatefrom this PR's fixture line, plus the countdown on
agenda,home,sponsorsand
content-pageat tablet/desktop (the pill is hidden in the small sidebar atmobile). Every countdown diff is under 0.32%. A clean re-run afterwards passes
72/72 with no churn, which is the point — they are now stable.
Note for reviewers
A fresh worktree has an empty
.wrangler/state/, sonx e2efails the voting tests withno such table: voting_globalsuntil you runnx d1-migrate-local website. Also notenx testdepends onbuild—app/lib/dates/dev-date-override.test.tsgreps the built worker, so a barevitest runfails those 2 tests until a build exists.https://claude.ai/code/session_012ZUK9WJ1TGNQzBSNwwrBx1