refactor(errors): type the ?error= codes on /login, /account and /admin/accounts - #89
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR adds shared typed error maps, redirect builders, and safe message lookup. It applies them to payout, account, login, OAuth, and admin-account flows, replacing local maps and hardcoded error URLs. ChangesApplication error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
…in/accounts Follow-up to #79. #85 did this for the two payout pages and named what it skipped: these three still carried untyped `Record<string, string>` maps, which is the thing that defeats the type system — it accepts any key, so `keyof typeof` yields `string` and derives nothing. A code with no entry in the destination page's map renders nothing at all. Unlike payouts there was no chokepoint to type: six producers wrote the URL by hand, and two of them pick between two DIFFERENT destination pages in a single ternary. So the helpers had to be introduced before anything could be typed. Shape: typed URL BUILDERS returning a string, not payouts' `: never` redirectors. Two incompatible mechanisms consume these — server actions and page guards call `redirect()` (returns `never`, which `denyAdmin` and `redirectOnMutationError` both depend on for their exhaustiveness over a service error union), while the OAuth callbacks are route handlers that must return a Response. A `: never` helper cannot be returned from a route handler, and a Response-returning one would destroy the `never` the two switches rely on. A string sits underneath both. It also lets one call site pick between two destinations with each branch checked against its own page's union. Home: `src/lib/error-redirects.ts`, not `src/app/`. `src/lib/admin-guard.ts` produces one of these codes, and nothing outside `src/app/` imports `src/app/` anywhere in this repo; colocating would have made admin-guard the first inversion, for a UI string. admin-guard already hardcoded that URL and already imports next/navigation. `src/app/payouts/errors.ts` stays where it is — it has no `src/lib` producer, so colocation is still right there. `lookupErrorMessage` moves to the shared module and payouts re-exports it. `redirectOnMutationError` is now exhaustive on a second, independent axis and its docblock says so: the service union says which failures exist, the code union says which ones the page can explain. `adminAccountsErrorUrl` preserves `?tier=pending` via URLSearchParams, the way `createFailed` does. Non-goal honoured: codes are NOT globally unique. `not_admin` stays in both /account and /admin/accounts with different copy — one is the de-roled admin's destination, the other a stale-tab race — and `session_expired` reaches /login from four unrelated producers. No live bug surfaced: every emitted code already had an entry in its destination map. Copy moved byte-for-byte (extracted map bodies diffed against HEAD: all three IDENTICAL) and every generated URL matches the literal it replaced, `?tier=pending&error=` param order included. Also fixes two stale comment references from #85 naming an `ERRORS` symbol that no longer exists in payouts (dropped.ts, e2e/payouts.spec.ts) — names only. No migration. No change to OAuth state consumption, cookie handling or consumeOauthTransaction; only which URL a failure redirects to.
8860388 to
961feba
Compare
2 participants
Closes #88. Follow-up to #79; completes what #85 deliberately left.
What changed
Three pages render an operator-facing notice keyed off
?error=, and all three keptRecord<string, string>maps — the thing that defeats the type system, since it accepts any key and sokeyof typeofyieldsstringand derives nothing. A code with no entry renders nothing at all: the redirect happens, the page loads unchanged, and the member gets no explanation.Unlike payouts there was no chokepoint to type. Six producers wrote the URL by hand, and two of them pick between two different destination pages in a single ternary, so helpers had to be introduced before anything could be typed.
New
src/lib/error-redirects.tsholds threeas constmaps, a union per map, three builders, andlookupErrorMessage.The two architectural calls
Builders returning a
string, not #85's: neverredirectors. Two incompatible mechanisms consume these. Server actions and page guards callredirect()fromnext/navigation, which returnsnever— anddenyAdminandredirectOnMutationErrorboth depend on thatneverfor their exhaustiveness over a service error union. The OAuth callbacks are route handlers using a localto()wrappingNextResponse.redirect, which must return a Response. A: neverhelper cannot be returned from a route handler; a Response-returning one would destroy theneverthe two switches rely on. A string sits underneath both and neither loses anything:Each branch is checked against its own page's union. A per-file helper could not do that — the code and its destination have to be typed together.
Codes live in
src/lib/, notsrc/app/.src/lib/admin-guard.tsredirects to/account?error=not_admin, andgrep -rn 'from "@/app' src/lib src/core src/services src/jobs src/workerreturns nothing — nothing outsidesrc/app/importssrc/app/anywhere in this repo. Colocating would have made admin-guard the first inversion, for a UI string. admin-guard already hardcoded that exact URL and already importsnext/navigation; this only names what was there, and the new module imports nothing itself.src/app/payouts/errors.tsstays put — it has nosrc/libproducer, so colocation is still correct there. Same rule, two answers, because the producer sets differ.lookupErrorMessagemoves to the shared module and payouts re-exports it, so both payout pages' imports are unchanged.Two details worth a look
redirectOnMutationErroris now exhaustive on a second, independent axis, and its docblock says so: the service union says which failures exist, the code union says which ones the page can explain. The?error=axis extends the existing switch rather than sitting beside it.adminAccountsErrorUrlpreserves?tier=pendingby construction viaURLSearchParams— the waycreateFaileddoes it — instead of by remembering to include it.Non-goal honoured: codes are not globally unique
not_adminstays in both/accountand/admin/accountswith different copy./accountis where a genuinely de-roled admin is sent ("your admin access was removed");/admin/accountsis reached by a stale tab whose actor lost the bit between render and click ("refresh to see the current state"). One message would be wrong on one of them.session_expiredlikewise reaches/loginfrom four unrelated producers. Three maps in one file: one shape, three namespaces.The guard, demonstrated
Six deliberate breaks, all caught by
npm run typecheck, all reverted before commit:loginErrorUrl("session_expred")TS2345: Argument of type '"session_expred"' is not assignable…accountErrorUrl("stale_charater")TS2345: Argument of type '"stale_charater"' is not assignable…accountErrorUrl("not_pending")— valid on/admin/accounts, wrong pageTS2345: … not assignable to parameter of type '"already_linked" | … | "not_admin"'loginErrorUrl("not_admin")— valid on both other pagesTS2345: … not assignable to parameter of type '"oauth_denied" | … | "session_expired"'adminAccountsErrorUrl("session_expired")inside the exhaustive switchTS2345: … not assignable to parameter of type '"not_admin" | "last_admin" | "not_pending" | "not_found"'stale_charactermap entryTS2345at both producers — the original failure mode, now a compile errorRows 3 and 4 are the cases a single unified map would have missed; row 4 is a code valid on two other surfaces. Row 6 is the failure this whole change exists to prevent.
No live bug surfaced, no copy changed
Every code the six producers emit already had an entry in its destination map — exact set equality on all three, no gaps and no orphans. Nothing needed inventing.
Message text moved byte-for-byte: extracted map bodies diffed against
HEAD, all threeIDENTICAL. Every generated URL matches the literal it replaced,?tier=pending&error=param order included.Also: two stale comment references from #85
Both said "the
ERRORSmap", a symbol that no longer exists in payouts. Names only — surrounding rationale untouched.src/app/payouts/dropped.ts→OPERATION_ERRORSe2e/payouts.spec.ts→NEW_OPERATION_ERRORS/OPERATION_ERRORSScope
The e2e specs stay exactly as they are; #79's suggestion to thin them now that types hold the invariant is still a separate change.
/admin/accounts's notice guard changed only becauseparams.error && ERRORS[params.error]stops compiling against anas constmap — it is now oneerrorMessage &&, same behaviour.One thing the task description missed and this covers:
src/app/account/page.tsxis a sixth producer of/login?error=session_expired, alongside the five listed. Left hand-written it would have been the most-emitted code on that page sitting outside the guard.No migration. No change to OAuth state consumption, cookie handling, or
consumeOauthTransaction— only which URL a failure redirects to.Verification
npm run typecheck→ clean (tsc --noEmit, no output)npm test→Test Files 71 passed (71),Tests 869 passed (869)npm run test:e2e -- account.spec.ts→18 passed (15.1s)npm run test:e2e -- admin.spec.ts→36 passed (42.4s)npm run format:check→All matched files use Prettier code style!npm run lint→0 errors, 3 warnings(pre-existing<img>warnings, untouched files)Summary by CodeRabbit
Bug Fixes
Documentation