test(mobile): behavior coverage for the login-flow orchestrator hook (#243) - #453
Conversation
…243) Phase 2b iteration 4 (TESTS cluster). apps/mobile/app/use-login-flow.ts drove the whole magic-code auth flow with no test (only the presentational login-sections were covered). Adds behavior/edge/failure coverage: offline guard blocks send/verify, invalid email is rejected without an API call, a successful send advances to the code step and starts the resend countdown, a successful verify logs in with the returned session and redirects to the safe return url, a failed verify reports the error and resets the code entry, and submission is gated on connectivity + code completeness. Web parity: the web login orchestrator is already covered via login.test.tsx (page-level), so this mobile-only hook test has no web mirror. Refs #243 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary — PR #453
Recommendation: REQUEST CHANGES
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 0 |
| Low / Info | 0 |
Findings
[High] PR's stated web-parity exemption doesn't hold up
- Dimension: Cross-platform parity (root
CLAUDE.md, "Cross-platform parity (MANDATORY)") - Location:
apps/web/__tests__/pages/login.test.tsx(existing, untouched) vs. newapps/mobile/__tests__/hooks/use-login-flow.test.ts - Issue: The PR body claims web coverage already exists via
apps/web/__tests__/pages/login.test.tsx(page-level), so the new mobile hook test needs no web mirror. Verified directly: that file has 7 tests, and only 2 overlap what the new mobile test covers (invalid-email rejection, offline-disables-sending). It never exercises a successfulsendCode+ resend countdown, a successfulverifyCode→login/setAuth→ redirect, or a failedverifyCode→ reset — three of the six behaviors this PR newly covers on mobile. - Further evidence against "platform-adapter difference":
apps/web/__tests__/hooks/already exists as a directory, holdinguse-login-code-entry.test.ts— a dedicated hook-level test foruseLoginFlow's sibling hook (apps/web/hooks/use-login-code-entry.ts). Hook-level testing is the established convention on web for this exact orchestration layer; page-level-only coverage foruse-login-flow.tsis the anomaly, not the norm. - Risk: The web hook (
apps/web/app/(auth)/login/use-login-flow.ts, 245 lines, same responsibilities as the mobile hook) now has materially weaker behavior coverage than its mobile mirror. A regression in web's success/failure verification paths ships undetected. RootCLAUDE.mdrequires behavior-test coverage to land in both apps in the same task; platform-adapter style differences (e.g.,renderHookvs RTL page render) are fine, coverage-depth differences are not. - Fix: Add equivalent behavior coverage for
apps/web/app/(auth)/login/use-login-flow.tsasapps/web/__tests__/hooks/use-login-flow.test.ts: successfulsendCode+ resend countdown, successfulverifyCode+ login + redirect, failedverifyCode+ reset, and explicit submit-gating on connectivity/code-completeness.
Deferred / N/A dimensions
Dead/stale code, SOLID/clean-arch, comment policy, no-workaround, DESIGN.md/AI-slop, i18n, contract drift, security, backend hard rules — all N/A: the diff is a single new test file (apps/mobile/__tests__/hooks/use-login-flow.test.ts, 238 lines added, 0 removed) with no production code, UI, shared-type, or orbit-api surface touched.
Validation
Skipped per CI-adaptation instructions (Build / Unit Tests / SonarCloud run as separate required checks on this PR). PR body self-reports: lint + type-check clean, full mobile vitest suite 1038 passed / 0 failed (+6 new) — not independently re-run in this pass, but the mobile test file was read in full and its assertions correspond to its stated behaviors.
🤖 Automated review via /pr-review
Addresses the #453 review: the web login orchestrator (apps/web/app/(auth)/login/use-login-flow.ts) had materially weaker behavior coverage than its new mobile mirror — login.test.tsx (page-level) only overlapped invalid-email + offline-disables, leaving successful send/verify and verify-failure paths untested. Root CLAUDE.md requires behavior coverage in BOTH apps; a page-level-vs-hook style difference is fine, a coverage-depth gap is not. Adds apps/web/__tests__/hooks/use-login-flow.test.ts (hook-level, the established web convention per use-login-code-entry.test.ts): invalid email rejected without an API call, offline guard blocks sending, successful sendCode advances to the code step + starts the resend countdown, successful verifyCode delegates to handleVerifySuccess (setAuth + redirect) with the returned session, and a failed verifyCode reports the error and resets the code entry. Asserted at the login-form-helpers adapter seam (the web analogue of the mobile hook's inlined login()+router.replace()). Refs #243 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Review Summary — PR #453
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
Prior review resolved
My previous pass (CHANGES_REQUESTED) flagged a High finding: the web hook (apps/web/app/(auth)/login/use-login-flow.ts) lacked behavior coverage equivalent to the new mobile hook test, and the PR's stated web-parity exemption (relying on the pre-existing page-level login.test.tsx) didn't hold up — it missed successful sendCode + resend countdown, successful verifyCode → login → redirect, and failed verifyCode → reset.
Commit e011dbab (test(web): mirror login-flow orchestrator coverage for parity) adds apps/web/__tests__/hooks/use-login-flow.test.ts (184 lines), which now covers exactly those gaps:
sends the code, advances to the code step, and starts the resend countdown— assertsstartResendCountdowncalled (was missing before)verifies the code and completes the session via handleVerifySuccess— asserts the success → session-completion path (was missing before)reports the error and resets the code entry when verification fails— assertsresetCodeDigits/focuson failure (was missing before)- plus invalid-email and offline-blocks-sending, mirroring the mobile suite
Every assertion in both apps/mobile/__tests__/hooks/use-login-flow.test.ts and apps/web/__tests__/hooks/use-login-flow.test.ts was cross-checked against the real hook implementations and confirms actual behavior. Mocking follows each workspace's sanctioned adapter seam (mobile: apiClient/auth-store; web: login-form-helpers), not fetch/BFF routes directly. No production code changed.
The prior parity gap is closed — both platforms now have equivalent hook-level behavior coverage for the login-flow orchestrator. No new Critical/High findings in this diff.
Findings
Low / Info — Pre-existing web-only bug surfaced during parity analysis (not introduced by this diff, non-blocking)
- Location:
apps/web/app/(auth)/login/code-step.tsx:89 - Issue: The verify-code submit button's
disabledexpression (isSubmitting || codeDigits.join('').length !== 6) doesn't checkisOnline, unlike the send-code button (email-step.tsx:57) and unlike mobile'scanSubmitCode, which requiresisOnline. - Risk: A user offline mid-flow on web can tap "Verify" with a complete code;
isOfflinePreflight()insideverifyCode()still safely rejects it, so no data loss — just a minor UX inconsistency versus the rest of the flow. - Fix: Add
|| !isOnlinetocode-step.tsx:89in a follow-up. Not blocking this PR (pre-existing, not touched by this diff).
Deferred / N/A dimensions
Dead/stale code, SOLID/clean-arch, comment policy, no-workaround, DESIGN.md/AI-slop, i18n, contract drift, security, backend hard rules — all N/A: the diff is test files only (mobile + web hook tests), no production code, UI, shared-type, or orbit-api surface touched.
Validation
Skipped per CI-adaptation instructions (Build / Unit Tests / SonarCloud run as separate required checks on this PR).
🤖 Automated review via /pr-review
1 participant
What
Phase 2b iteration 4 of the #243 prod-readiness campaign — the TESTS cluster. The login-flow orchestrator hook drove the entire magic-code auth flow with no dedicated behavior test on either platform. Adds hook-level behavior/edge/failure coverage in both apps (parity):
apps/mobile/__tests__/hooks/use-login-flow.test.ts(new)apps/web/__tests__/hooks/use-login-flow.test.ts(new)Both cover:
sendCode/verifyCode(no API call, offline error surfaced)sendCodeadvances to the code step + starts the resend countdownverifyCodecompletes the session (mobile:login()+router.replace(safe returnUrl); web: delegates tohandleVerifySuccess=setAuth+ redirect) with the returned sessionverifyCodereports the error and resets the code entryEach test asserts at its platform's real adapter seam — mobile mocks
apiClient+ the auth store; web mocks thelogin-form-helpersadapter (fetchAuthEndpoint/handleVerifySuccess). Style differs (react-test-renderervs@testing-library/reactrenderHook), coverage depth matches.Validation
Cross-repo
Paired API PR (push / api-key auth / user-date / play-cleanup coverage): thomasluizon/orbit-api#320 — merged.
Refs #243
🤖 Generated with Claude Code