refactor(payouts): type the ?error= codes so a missing entry fails typecheck - #85
Merged
Conversation
…pecheck Closes #79. Both payout pages render an operator-facing notice keyed off `?error=`, and a code with no entry in the destination page's map rendered nothing at all — an unchanged form with no explanation, the one failure these pages cannot show an operator. `operationFailed` and `createFailed` took `code: string`, so a typo compiled, passed lint, and deployed; the only defence was the per-code e2e cases. Lift both maps into src/app/payouts/errors.ts as `as const` objects and derive a union per map. `Record<string, string>` was what defeated the type system: it accepts any key, so `keyof typeof` yielded `string`. The helpers now take their own destination's union, so a bad code is a tsc error. The codes stay per-page and deliberately not globally unique. `share_format` and `share_range` exist in both maps with different copy, because the detail page's "The old value is unchanged." is true there and false on the create form. Each page's map is its namespace; the types make it enforceable. Two smaller things fall out. The appraisal catch block built its `?error=appraisal_failed` URL by hand, which would have been the one code still outside the guard, so it goes through `operationFailed` — safe in a catch block, where a redirect's throw propagates past its own try. `lookupErrorMessage` replaces the bare index: codes off a query string are `string`, not a code, and an unrecognized one still degrades to no notice. Message text moved byte-for-byte; no copy was reworded. The per-code e2e cases stay as they are — thinning them is the issue's follow-up, not this.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 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 (4)
Comment |
guarzo
enabled auto-merge (squash)
August 4, 2026 21:46
guarzo
pushed a commit
that referenced
this pull request
Aug 4, 2026
…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.
This was referenced Aug 4, 2026
Closed
guarzo
pushed a commit
that referenced
this pull request
Aug 4, 2026
…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.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
…in/accounts (#89) 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. Co-authored-by: gambtho <thomasgamble2@gmail.com>
This was referenced Aug 4, 2026
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.
Closes #79.
What changed
Both payout pages render an operator-facing notice keyed off
?error=. A code with no entry in the destination page's map rendered nothing at all — an unchanged form with no explanation, which #74 named as the one failure these pages cannot show an operator.operationFailedandcreateFailedtookcode: string, so a typo compiled, passed lint, and deployed; the per-code e2e cases were the only defence.Both maps move to
src/app/payouts/errors.tsasas constobjects with a union derived per map.Record<string, string>was the thing defeating the type system — it accepts any key, sokeyof typeofyieldedstringand derived nothing. Each helper now takes its own destination's union.Two smaller things fall out:
?error=appraisal_failedby hand, which would have been the one code still outside the guard. It goes throughoperationFailednow — safe in acatchblock, where a redirect's throw propagates past its owntryrather than into it.lookupErrorMessagereplaces the bare index. Codes off a query string arestring, not a code; an unrecognized one still degrades to no notice, ase2e/payouts.spec.tsalready asserts.Non-goal honoured: codes are not globally unique
share_formatandshare_rangestay in both maps with different copy. The detail page's "The old value is unchanged." is true there and false on the create form. Per-page maps are the namespace; typing them is what makes the namespace enforceable.The guard, demonstrated
Three deliberate breaks, all caught by
npm run typecheck, all reverted before commit:operationFailed(..., "note_requird")TS2345: Argument of type '"note_requird"' is not assignable…createFailed(..., "share_formatt")TS2345: Argument of type '"share_formatt"' is not assignable…operationFailed(..., "name_required")— valid code, wrong pageTS2345: Argument of type '"name_required"' is not assignable…The third is the one a single shared map would have missed. The
open_info_*indirection maps are covered too — mistyping a value there fails at theoperationFailedcall site withDid you mean "open_info_offline"?.No live bug surfaced
Every code both helpers can emit already had an entry in its destination map, including all seven reachable through
OPEN_INFO_ERROR_BY_REASON/OPEN_INFO_ERROR_BY_FAILURE. Nothing needed inventing.Message text moved byte-for-byte — verified by diffing the extracted map bodies against
main, bothIDENTICAL. No copy was reworded.Scope
The 28 per-code e2e cases stay exactly as they are; thinning them is the issue's follow-up, not this change.
/login,/accountand/admin/accountsstill carry untypedRecord<string, string>maps — the issue names the first two as worth a look, and that stays a separate question.Verification
npm run typecheck→ clean (tsc --noEmit, no output)npm test→Test Files 71 passed (71),Tests 869 passed (869)npm run test:e2e -- payouts.spec.ts -g "error"→32 passed (2.7m)npm run format:check→All matched files use Prettier code style!npm run lint→0 errors, 3 warnings(pre-existing<img>warnings, untouched files)code-reviewer→ no findingsNo migration.