feat(auth): release Sign in with Apple to all users#933
Conversation
Remove the `?feature=apple` querystring gate so the Sign in / Sign up with Apple button is rendered for every visitor on /login and /signup. The `appleEnabled` template variable and `featureToggle` dependency on the auth page are gone with it.
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 6m 52s —— View job High Priority Issues (Must Fix)None found Medium Priority Issues1. Apple button is now decoupled from whether the
|
High Priority Issues (Must Fix)None found Medium Priority Issues1. Apple button is now decoupled from whether the
|
|
@claude Fix the review issues. Use your Read tool to read the local file Context
Review to AddressSee the review comment above this one for the issues to fix. |
|
Claude finished @FagnerMartinsBrack's task in 33m 58s —— View job ✅ Fixed the medium-priority review issuePushed Problem: The PR made the Apple button render unconditionally, but Approach — tie visibility to backend availability, not a querystring:
Why this shape:
Verification (full
Low-priority suggestion (route-reachable assertion) was intentionally left per the auto-fix scope (high/medium only), though the new absence tests already restore the negative coverage keyed on real backend availability. |
Tie Apple sign-in button visibility to backend availability
(Boolean(deps.appleAuth)) instead of rendering it unconditionally, so the
button is never shown without a mounted /auth/apple route. Threaded
appleEnabled through every LoginPage/SignupPage render path and restored
the {{#if appleEnabled}} template guard.
Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 7m 26s —— View job Review — PR #933 (release Sign in with Apple to all users) @
|
High Priority Issues (Must Fix)None found Medium Priority Issues1. Two OAuth-error
|
|
@claude Fix the review issues. Use your Read tool to read the local file Context
Review to AddressSee the review comment above this one for the issues to fix. |
|
Claude finished @FagnerMartinsBrack's task in 21m 7s —— View job ✅ Fixed the medium-priority review issuePushed Problem: The prior fix tied the Apple button to Approach — thread
Why this shape:
Verification (full check with
|
Thread appleEnabled into the two OAuth-error LoginPage renders that were missed, so the Apple button no longer vanishes on error-fallback pages in production. - apple-auth.page.ts: pass appleEnabled: true on the callback error render (the Apple router only mounts when appleAuth is set, so it is always true). - google-auth.page.ts: add appleEnabled to the route deps and pass it on the callback error render; server.ts sources it from Boolean(deps.appleAuth), the same value that mounts the Apple route. - auth.component.ts: make appleEnabled required on AuthFormData so the compiler forces every LoginPage/SignupPage caller to supply it (compile-time constraint over runtime validation). - Add route tests asserting the Apple section renders on both OAuth-error pages when Apple is configured. Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FagnerMartinsBrack
had a problem deploying
to
staging
GitHub Actions
Failure
|
@claude CI has FAILED. Use your Read tool to read the local file Context
Failed Jobs
|
|
Claude finished @FagnerMartinsBrack's task in 9m 12s —— View job CI Failure Diagnosis — PR #933 (attempt 1 of 5)
TL;DR — this failure is not caused by this PR, and it is not fixable from this branchThis PR's own changes are fully green: 2850 unit + 11 integration tests pass, coverage passes, That test does not exist on this branch — it was added on Evidence that
|
| CI run | Ref | web-tests result |
|---|---|---|
| 28744076544 | main push @ 5b993302 |
❌ same 145px→153px snapshot failure |
| 28743889714 | #936 merge CI | ❌ same 145px→153px snapshot failure |
| 28744163422 | this PR (#933 merged w/ main) | ❌ same failure, inherited |
#936 was merged with a red web-tests job. It committed baseline PNGs (crawl-bookmark-{light,dark}-chromium-linux.png, 145×54) that never matched the CI chromium‑linux render (153×54).
Root cause
The capsule width is driven entirely by the rendered width of the bold prefix + time label. The test pins the instant (2026-03-26T14:32:00Z), timezone (UTC), viewport, and waits for the exact text 26 Mar '26, 14:32 + document.fonts.ready — so the text is identical, yet the element is a deterministic 8px wider in CI. That is a font‑metric mismatch: the baseline was generated in a non‑CI environment whose fonts render ~8px narrower than the CI runner's. maxDiffPixelRatio/threshold can't absorb it because a size mismatch fails before pixel comparison, and retries: 0 means one flake fails the run.
Why I did not push a change to this branch
- The failing test/snapshot is not part of this PR (auth‑only:
server.ts,apple-auth.page.ts,auth.component.ts,auth.page.ts,google-auth.page.ts+ tests). Editing fix(hutch): render the crawl bookmark as one seamless capsule #936's snapshot here would violate the fixer's "do not make unrelated changes / never reverse intentional changes" rules and pollute a deliberately minimal auth PR with an unrelated binary. - Regenerating the baseline correctly requires the
web-testsCI environment; a snapshot regenerated elsewhere risks re‑failing. The fix belongs where the test lives — onmain. - I can only push to this PR branch (triggered on the PR), not to
main.
Recommended fix (on main, owned by #936)
Regenerate both baselines in the CI environment and commit the 153×54 PNGs (playwright test crawl-bookmark-visual.e2e-local.ts --update-snapshots), or make the pixel assertion robust to font metrics. Once main is green, this PR goes green automatically on its next merge‑CI (no auth changes needed).
No fix pushed to this branch — doing so would be an unrelated change, and no auth change can turn this failure green. The block is a pre‑existing main breakage from #936.
· Branch claude/apple-login-release-r2hczs
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 5m 32s —— View job Review — PR #933 (release Sign in with Apple to all users) @
|
High Priority Issues (Must Fix)None found Medium Priority IssuesNone found Low Priority Suggestions1. The
|
|
@claude remove the appleEnabled toggle and props completely, don't leave trash code behind. Make deps.appleAuth mandatory to all callers |
|
Claude finished @FagnerMartinsBrack's task in 34m 0s —— View job ✅ Removed the
|
| File | Change |
|---|---|
server.ts |
appleAuth is required (was appleAuth?); Apple router mounted unconditionally (dropped if (deps.appleAuth)); both appleEnabled: Boolean(deps.appleAuth) threads removed. |
auth.page.ts |
Dropped appleEnabled from AuthDependencies and all 7 LoginPage/SignupPage render paths. |
auth.component.ts |
Dropped appleEnabled from AuthFormData and both template renders. |
login/signup.template.html |
Dropped the {{#if appleEnabled}} guard — the button always renders (nested {{#if returnUrl}} left intact). |
apple-auth.page.ts / google-auth.page.ts |
Dropped appleEnabled from the OAuth-error renders (and Google's route deps). |
app.ts (dev root) |
Provides appleAuth unconditionally — real config when APPLE_LOGIN_* is set, else a dev stub. |
web-test-harness/bundle.types.ts |
apple: AppleAuthBundle (was | undefined) — mandatory on TestAppFixture. |
test-fixtures/fixture.ts |
Default fixture wires a stub Apple bundle so every test app mounts the route. |
auth/*.route.test.ts |
Removed the now-impossible "Apple unconfigured" cases and the appleEnabled-only error-page scaffolding; positive tests use the default fixture. |
One judgment call — the dev composition root (app.ts, only hit by pnpm dev, not CI): making appleAuth mandatory means it can never be undefined. I kept the real Apple wiring when APPLE_LOGIN_* is set, and stub it otherwise — because requiring the keys in local dev would break pnpm dev for anyone without them (.envrc doesn't provide them), and Apple's handshake can't complete against localhost regardless. If you'd rather dev demand the keys like prod (requireEnv, dropping the stub), say the word and I'll switch it.
Verification — pre-commit hook ran the full nx run-many --target=check --all across all 37 projects, green:
-
hutch: 2861 unit + 11 integration + 5 E2E — all pass
-
Coverage: functions 100%, statements 99.52%, branches 97.14%, lines 99.52% — all thresholds met
-
tsc+biome+knip+ unused-css clean;appleEnabledhas zero remaining references repo-wide -
server.ts—appleAuthrequired; dropappleEnabled; mount unconditionally -
auth.page.ts/auth.component.ts— dropappleEnabledeverywhere -
login/signup.template.html— drop{{#if appleEnabled}} -
apple-auth.page.ts/google-auth.page.ts— dropappleEnabled -
Composition roots provide
appleAuthunconditionally -
Tests — drop the "Apple unconfigured" & appleEnabled scaffolding
-
pnpm nx run … check(full sweep, all 37 projects green) -
Commit & push
· Branchclaude/apple-login-release-r2hczs
Sign in with Apple ships to everyone, so the appleEnabled toggle is dead
weight. Make deps.appleAuth a required dependency instead: the
/auth/apple route is now always mounted, so the button always resolves
to a real destination — no conditional render needed.
- server.ts: appleAuth is required; mount the Apple router
unconditionally; drop both appleEnabled threads.
- auth.page.ts / auth.component.ts: drop appleEnabled from the deps,
AuthFormData, and every LoginPage/SignupPage render path.
- login/signup.template.html: drop the {{#if appleEnabled}} guard.
- apple-auth.page.ts / google-auth.page.ts: drop appleEnabled from the
OAuth-error renders (and Google's route deps).
- Composition roots always provide appleAuth: prod already did; the dev
root stubs it when APPLE_LOGIN_* is unset (localhost can't complete
Apple's handshake anyway); the default test fixture wires a stub so
`apple` is mandatory on TestAppFixture.
- Tests: drop the now-impossible "Apple unconfigured" cases and the
appleEnabled-only error-page scaffolding.
Co-authored-by: Fayner Brack <FagnerMartinsBrack@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FagnerMartinsBrack
temporarily deployed
to
staging
GitHub Actions
Inactive
|
@claude Review this PR. Use your Read tool to read the local file Context
|
|
Claude finished @FagnerMartinsBrack's task in 7m 6s —— View job Review — PR #933 (release Sign in with Apple to all users) @
|
Sign in with Apple shipped to all users (#933) while this branch's Apple-revocation seam was still a no-op, tripping the fail-closed deletion-compliance lock in auth.route.test.ts ({reachable:true, persisted:false}). This wires the revocation the lock demands: 1. The code exchange now requires Apple's refresh_token — an exchange without one fails the login instead of minting an unrevokable account — and returns it as AppleTokenResult.appleRefreshToken. 2. Every Apple login persists the token on the users row (the passwordHash precedent): createAppleUser stores it at signup, and the new saveAppleRefreshToken conditional update stores the freshest grant on each returning login, which also backfills accounts minted during the gap between #933 and this change. Read back via findAppleRefreshTokenByUserId over the userId-index; deleted with the row by closeUserAccount. Mirrored in the in-memory fixture. 3. The delete worker's no-op seam is replaced by a real initRevokeExternalIdpTokens: look up the stored token (absent for password/Google accounts → log-and-return), POST it with a freshly minted ES256 client secret to https://appleid.apple.com/auth/revoke, and throw on non-2xx so the SQS record redrives while the token is still readable (revocation precedes closeUserAccount). Apple returns 200 for already-revoked tokens (RFC 7009), so redrives converge. 4. The worker Lambda gets the four APPLE_LOGIN_* env vars, and the delete-account queue's maxReceiveCount rises 3 → 12 (~3h of retries at the 900s visibility timeout) so an Apple outage rides out in-queue instead of falling to the 14-day DLQ after 45 minutes. The handler-level test now runs the real revoker against the in-memory auth store and a fake Apple endpoint, proving deletion revokes the stored grant end-to-end; route tests pin token persistence on the signup, returning-login, and race-fallback paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 participants
Summary
Remove the
?feature=applequerystring gate so the Sign in / Sign up with Apple button is now rendered for every visitor on/loginand/signup.{{#if appleEnabled}}block inlogin.template.htmlandsignup.template.html.appleEnabledprop fromLoginPage/SignupPageand thefeatureToggledependency frominitAuthRoutes.?feature=apple-gated cases).Test plan
pnpm nx run hutch:check(unit + integration tests + coverage)Generated by Claude Code