fix(payouts): stop reporting operator typos as server faults - #74
Merged
Conversation
Fourteen validation sites in the payouts actions threw on user-correctable input. A throw lands on error.tsx, which renders `error.digest` and never `error.message` -- so an operator who mistyped a share percentage got "Something broke... that's a fault on this end, not something you did", no indication of which field, and a form that was gone. `requireAdminAction` went through exactly this conversion already; these are the sites that never did. All fourteen now redirect with a stable `?error=code` that each page looks up in its own message map. The create form additionally echoes the submitted values back, so a rejected submit costs one correction rather than five retypes. Where a redirect cannot carry the input back, the input is prevented instead. The appraisal form's station/region pair became a kind select plus one pattern-guarded id, which makes loot_pool_appraised_fields_ck's exactly-one rule unrepresentable rather than merely checked -- that form is the one whose failure would cost a several-hundred-line loot paste. The shares input gained a real type, min and step. Two defects found while converting: - setParticipantSharesAction guarded positivity with `iskToCents(shares)`, which *throws* on anything its regex rejects. Text in a numeric field -- the likeliest bad input that control gets -- escaped to error.tsx from inside the guard meant to redirect. A format check now runs first. - corpSharePct was write-once. An operator who accepted the create form's default committed the whole roster to 0% with no way back short of deleting the operation. setCorpSharePct adds the correction path, gated by assertEditable and recalculating, since the percentage is an input to every participant's amount. The three irreversible controls (delete pool, remove participant, Finalize) now arm before they fire, using the ConfirmSubmit the admin tier controls already use. Only the *first* mark-paid is gated: recording a payment is what makes the operation permanently un-editable, so every later click is behind a door already shut. Unlock, exclude and save stay one-click -- all reversible.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 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 (5)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Aug 4, 2026
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
) Two ways `/payouts/[id]` told the member something untrue. The tab read "Payout operation" for an operation that isn't there. The segment-scoped `not-found.tsx` shipped in #78 recorded why it couldn't fix that from its own end: a not-found boundary doesn't get to set the title, and `page.tsx`'s static `metadata` is applied even when the page throws. So the title moves to `generateMetadata`, which resolves the same lookup and names the operation — or says it isn't there. That was left as follow-up on the grounds it would cost a second lookup of the same row. It doesn't: both callers go through one `cache()`d loader, and Next resolves metadata and render in the same request. Measured with a counter on the miss path — one lookup per page load, not two. And `/payouts/<not-a-uuid>` returned a 500. The id reached the `uuid` column as a parameter, postgres rejected the cast with 22P02, and the member got "Something broke" — an apology for a server fault, for a mistyped or truncated URL. This is the defect class #74 spent itself fixing across `/payouts/*`. A shape check next to the existing `notFound()` sends it down the 404 path the well-formed-but-missing id already took. The regex is deliberately narrower than postgres's own parser, which was measured accepting braces, hyphenless and oddly-hyphenated forms. None of those can come from this app or a link it renders, and being narrower than the database fails toward the 404 rather than the 500.
7 tasks
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
…out view (#83) * docs(payouts): design phase 2 — overrides, revert, manual entry, pagination Phase 1 shipped the usable core; this specs what makes it pleasant, plus the nine defects it knowingly deferred. One PR. Two decisions worth a reviewer's attention: - Derived payment state comes from paidAmount, not a fold of payout_payment events. payout_payment.at is defaultNow() = transaction START time, so a transaction that starts earlier can take the operation lock later and insert an out-of-order timestamp; id is defaultRandom() and no tiebreak. The fold the phase-1 design called for would read a reverted payment as paid. Fixing it properly needs a monotonic sequence column, i.e. a migration. - esi-ui.open_window.v1 goes into EVE_SSO_SCOPES globally, which flips every existing character to needs_reauth on the next token-health run. Verified that sync keeps working (contacts.ts gates per job), so it is a fleet-wide false alarm that self-heals as members log in, not an outage. Recorded in the spec rather than discovered on deploy day. Also corrects the handover: multiple pools per operation already works end to end. It is a test gap, not a feature gap. * docs(payouts): resolve five review findings in the phase-2 spec - Payment ordering: write `at` as clock_timestamp() inside the existing operation lock, define display order as (at asc, id asc). Removes the reversed pay/revert display without a sequence-column migration. - Manual prices: exactly two decimals, exact cents x qty, bounded against the numeric(20,2) range. Names the appraised/manual inconsistency. - Absurd quantities: bound at MAX_SAFE_INTEGER (a correctness bound, since loot_item.qty is bigint mode:number) plus a separate total-value bound. - Pagination: three queries, not four scoped. The payment query is deleted, not bounded; payout_payment has no operationId anyway. - "Your payouts": finalized operations only, with the reasoning. * docs(payouts): add the phase-2 implementation plan Thirteen TDD tasks covering the phase-2 design: revert, manual item pricing, additive roster entry, the open-info express path, member-facing payout rows, and all nine deferred defects. Also corrects three stale claims in the spec, each verified against the source: phase 1 already tests both loot_item constraints through addAppraisedPool, so only direct-insert and the totalValue < 0 branch are missing; four sites compare granted scopes against config, not three, and the admin table's comparison lives in account-view.ts rather than the page that renders it. * docs(payouts): resolve eight review findings in the phase-2 plan Define the missing revertPaymentAction, re-read the open-info target server-side instead of trusting a caller-supplied character id, branch ESI failures instead of calling every one "offline", enforce the IEEE-754 exactness bound on appraised line totals, clamp payment timestamps forward under the operation lock, resolve the payment actor, and add the rollout and completion-workflow steps. * docs(payouts): re-verify the phase-2 plan against the merged #74 Rebase every actions.ts and payouts.ts citation onto the post-#74 line numbers, make the ERRORS work additive to #74's map instead of replacing it, and adopt #74's convention that only operator typos redirect. Task 5 had reinstated the throwing-guard defect #74 had just fixed on the same control. * docs(payouts): re-verify the phase-2 UI and e2e against merged main Re-derive every page.tsx and e2e citation post-#74, preserve the imports and the mark-paid arming the plan would have deleted, replace #74's anyPaid first-payment check with one revert cannot flip, and extend the per-code e2e table to cover the ten codes this plan adds. * fix(payouts): reject a nonsense split before it looks like a real one computeSplit accepted a negative total and a corp share outside 0-100, producing a plausible split that only died later as a raw check-constraint error. Also drops perShareCents, which nothing read, and adds MAX_MONEY_CENTS for the callers that have to keep money inside numeric(20,2). * fix(payouts): say which paste lines were ignored, and bound the absurd ones A zero-quantity line vanished with no signal, a bare quantity became an item literally named "12", and a 30-digit quantity died downstream as a raw Postgres error. parseLootPaste now returns those lines with a reason, and appraiseLoot carries them to the caller. Nothing is rejected wholesale: a mostly-good paste still appraises. Bounding the quantity did not make the line total exact - the total is a float product, and past 2^53 cents the representable values are 16 cents apart, so 1000000.01 ISK x 1e9 units stored a cent low with nothing to catch it. The product is now bounded too, before the BigInt conversion launders it, and a line past ~90 trillion ISK says so by name. * fix(payouts): name the not-found failures, and timestamp payments causally recordPayment and loadParticipantOperationId threw bare Errors that callers could not tell from a programming mistake; they now throw PayoutNotFoundError alongside the existing forbidden/locked pair. payout_payment.at defaulted to now(), which is transaction START time, so a transaction that took the operation lock later could still record an earlier instant than the event before it. The insert now supplies a clock reading taken from inside the lock and clamped forward past that participant's latest row, so a participant's history is strictly increasing rather than merely usually increasing — clock_timestamp() alone repeats at clock resolution and can step backwards under NTP. The cost is that after a backwards step the timestamp reads later than the wall clock until it catches up; order is what a reader of this history needs, and the instant is what is traded for it. No migration: the column default is untouched, it is simply not what the writer uses. * feat(payouts): let an operator take back a payment they recorded wrong paidAmount, not the payment log, is now what says whether someone has been paid — one column written under the same lock that reads it. Reverting does not reopen the operation's numbers. * fix(payouts): say what the share limit is instead of leaking a numeric overflow shares is numeric(6,2); 10000 was a raw Postgres error. Bounded with a readable message in the service and the action rather than widening the column, which would cost a migration for the sake of a sentence. The action appends its bound to the format/positivity order #74 established rather than replacing it: the guard it would have replaced throws, and throwing from that control is the defect #74 removed. * feat(payouts): add one pilot to a roster without retyping the fleet Additive, unlike setRoster, so share edits survive. Resolves through the same path the paste does, so an alt collapses into their main instead of drawing a second share, and a repeated unresolved name is refused rather than warned about after the fact. Both refusals redirect with a code the page renders, following the conversion #74 made across this file: a blank name box is not a fault on our end, and error.tsx is the only thing a throw can say. * feat(payouts): let an operator price the items the appraisal could not Manual prices are exact to the cent, so the line total is a bigint product with nothing to round. Line and pool totals past numeric(20,2) now say so instead of surfacing a Postgres overflow, and the loot_item constraints get tests that name them. * perf(payouts): bound the payout list to one page of rows listPayoutOperations issued four queries, three unbounded: every loot_pool, payout_participant and payout_payment row in the database, folded in memory. The payment query is deleted rather than scoped -- paidAmount already answers what it was consulted for, and payout_payment has no operationId to scope by. The other two are scoped to the page, which keyset pagination now bounds. The cursor is composite (occurredAt, id): occurredAt is not unique and the id is a random uuid, so a single-column cursor skips every operation sharing a date across a page boundary. * feat(payouts): page the operations list An Older link carrying the composite cursor, matching the audit pager. The heading no longer states a count unless the page provably holds every operation -- a page count presented as a total is wrong the moment a 51st operation exists. A cursor past the end gets an exit link rather than looking like an empty database. * feat(esi): add openInformationWindow and the operator scope gate The scope belongs to the token making the call, so the gate reads the main character's persisted scopes column rather than config: config says what login asks for, and an operator who authorized before the scope existed has a valid session without it. * feat(payouts): open-info server action and the new SSO scope Second documented exception to enqueue-don't-execute, justified in the code: the call persists no state anywhere, so a lost call is a re-click and a duplicate opens the window twice. The action takes a participant id and re-reads the recipient character from the database, so an operator cannot aim their own token at an arbitrary character by editing the posted id. Failures are classified from what ESI actually said -- offline only when its own body says so -- and the 30s client timeout, which is a DOMException rather than an EsiError, no longer escapes as a raw 500. EVE_SSO_SCOPES gains esi-ui.open_window.v1. This flips every existing character to needs_reauth until its holder logs in again and writes one audit row each -- noise, not an outage, since every job gates on the scopes it actually needs. docs/ops.md now says so where the variable is documented. * fix(payouts): use next/link for the past-end exit link eslint's no-html-link-for-pages fires on a literal <a href="/payouts"> because that string matches a static page route exactly -- the "Older" pager link escapes it only because its href is a template literal, not a bare string. Matches the repo convention (Link for static in-page routes; <a> for hrefs built from an expression) already used for the other links on this page. * docs(payouts): document the existing-deployment scope rollout Add the fly secrets set / fly deploy rollout for an already-running deployment to the "Adding an SSO scope" section, transcribed from Task 10's Step 23 -- without it a maintainer reading only docs/ops.md would learn what the scope change does but not that they must run it, and skipping it is a silent no-op. Also note in getOpenInfoTarget's docblock that it takes no actor and calls no operator guard, so a future second caller must add its own gate. * feat(payouts): show every pasted item, name what the parser ignored, add one participant at a time * feat(payouts): show payment history, allow an audited revert, and tell members what they are owed The history list names the operator who recorded each event, resolved the way audit rows resolve an actor -- account id to that account's main character's name. payout_payment.actor has been written since phase 1 and never read; the column already existed, so nothing migrates. It renders 'unknown' when the actor's account was deleted (the FK is on delete set null) or has no main character, which are indistinguishable here. revertPaymentAction catches nothing. Every failure revertPayment can raise is authorization or lifecycle state, never something the operator typed, and #74 drew exactly that line across this file: input rejections redirect with an ?error= code, everything else belongs on error.tsx. This action has no input to reject, so it adds no code to the page's ERRORS map. The revert control arms before it fires, joining the delete-pool, remove- participant, Finalize and first-mark-paid controls #74 already armed inside this table's ConfirmArmScope. Also fixes a defect flagged by earlier tasks: the page's arm-gating const for "mark paid" (renamed anyPaid -> firstPayment) now derives from `locked` (hasPayments) instead of `paymentState === "paid"`. Reverting the only payment on an operation left `locked` true but flipped the old anyPaid to false, re-arming a confirm step for a door that was already shut for good. * test(payouts): cover the reprice, pay, revert, pay loop and the member payout view end to end * fix(payouts): keep a mistyped unit price on the page, not on error.tsx The unit-price control threw on a malformed value while every other money input on the page redirects with a typed ?error= code, so a pasted comma-grouped number sent the operator to the generic error boundary with no idea which field was wrong. Add type=number/step to match the shares input, redirect on rejection with a new price_invalid message, and cover it end to end. Also pin the payout.item_repriced audit target and the MAX_MONEY_CENTS boundary, two cheap gaps the same review flagged. * test(error): break the payouts list on a table the empty page still reads listPayoutOperations skips its loot_pool child query when the first page comes back empty, so renaming loot_pool no longer made /payouts throw and six boundary tests stopped exercising the boundary. Rename payout_operation instead: the operations query has no such short-circuit, and it still meets the constraints the technique needs - read by the page body, not by the guard, not by metadata. * fix(payouts): compute a line total in bigint, not in a float A bound on the product's magnitude was doing duty as a bound on the error in computing it. They are not the same thing: `MAX_EXACT_LINE_CENTS` makes the answer representable, but the double `price` still carries ~1.1e-16 relative error into the multiply, which near 9e15 cents is already about a whole cent. 48804.84 ISK x 1,845,177,173 units sits under the bound and stored 9005357669991731 where the true total is 9005357669991732. So the multiply moves into bigint over the price's own decimal expansion, and the bound goes back to being what it says it is: a ceiling on a single line, refused by name rather than stored wrong. Rounding still happens once, at the line total, half away from zero — the tie-break `Math.round` used, so the only totals that move are the ones the float got wrong. Also from the same review pass: - `decodeDropped` filtered its reason with `in`, which walks the prototype chain, so a crafted `?dropped=` carrying "constructor" or "toString" passed the allowlist and rendered whatever Object.prototype resolved to. - Two tests asserted on "paid", which "unpaid" contains, and so passed on a render where the paid badge was missing entirely; one e2e mark-paid clicked `.first()` rather than the row under test. - A docblock counted fifteen guarded mutations and listed fourteen. - `.env.example` left a space-separated scope list unquoted.
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.
Design sweep synthesis item 1. Five of the seven blockers lived on
/payouts/*, and four were one defect wearing four hats.The defect
A payout operator types something the server doesn't accept. The action
throws. The throw escapes tosrc/app/error.tsx, which renderserror.digestand nevererror.message— correct, since production React replaces the message anyway. So the operator sees "Something broke… that's a fault on this end, not something you did", with no indication of which field, and their work is gone because the form never re-rendered with its values.Fourteen such sites in
src/app/payouts/actions.ts. Exactly one path in that file did it right (the appraisalcatchredirecting?error=appraisal_failed), and it was the template.This conversion is the house pattern, not a new opinion.
requireAdminActionwent through it already; the reasoning is recorded verbatim ine2e/admin.spec.ts— "It used tothrow, which landed on error.tsx… For the case that actually occurs, that copy was a lie."/payoutsis the surface that never got the same treatment.What's here
Error channel. All fourteen sites redirect with a stable
?error=code, resolved through a per-pageERRORSmap — the same convention/loginand/accountalready use. Two helpers typed: never, so TypeScript narrows correctly past them and no call can sit inside atrywhere an enclosingcatchwould swallow the redirect.TriffError,EsiError,PayoutForbiddenErrorandPayoutLockedErrorare untouched: those aren't typos, anderror.tsxis the right destination for them.Value round-trip.
/payouts/newechoes the submitted fields back in the query string and reapplies them asdefaultValue. A rejected submit costs one correction instead of five retypes. Values over 500 chars are dropped rather than round-tripped.Prevention where a redirect can't help. The appraisal form's two free id inputs became a location-kind select plus one pattern-guarded id, so
loot_pool_appraised_fields_ck's exactly-one rule is structurally unrepresentable rather than checked after the fact. That form is the one whose failure would cost a several-hundred-line loot paste, and a redirect cannot carry a paste back — so it's prevented, not explained. Thelocation_exclusivecode disappeared as a result. The shares input gained a realtype/min/step/required.Confirmation on the irreversible controls.
delete pool,remove participantandFinalizenow arm before they fire, via theConfirmSubmit/ConfirmArmScopethe admin tier controls already use. Only the firstmark paidis gated — recording a payment is what makes the operation permanently un-editable and un-unlockable, so every later click is behind a door already shut.Unlock,exclude/includeandsavestay one-click; all are reversible.Two defects found while converting
setParticipantSharesActionguarded positivity withiskToCents(shares), which throws on anything its regex rejects (src/core/payout-split.ts). Typingabcinto a shares box escaped toerror.tsxfrom inside the guard that was supposed to redirect — and text in a numeric field is the likeliest bad input that control gets. A format check now runs first, mirroring the regex-then-parse orderaddFlatPoolActionalready used.corpSharePctwas write-once.createOperationwas its only writer and no edit path existed anywhere insrc/. An operator who accepted the create form'sdefaultValue="0"committed the whole roster to a 0% corp share with no way back short of deleting the operation and rebuilding it.setCorpSharePctadds the correction path — sameassertEditablegate as every other edit, holding the row lock, audited aspayout.corp_share_changedtargeted at the operation uuid, and recalculating, because the percentage is an input tocomputeSplitand a version that only wrote the column would read right on the page and pay out wrong.Unplanned edit worth flagging
Adding
aria-label="save corp share"created a second bare "save" on the detail page, so each participant row's save button gotaria-label={\save ${p.displayName} shares`}` too. Pre-existing ambiguity — the row buttons never named their object — but this change is what made it bite.Verification
Every command was run; nothing is claimed unmeasured.
npm run typechecknpm run lintno-img-elementwarningsnpm run format:checknpm testnpm run test:e2enpm run buildThree existing assertions in
e2e/payouts.spec.tsnow click twice (Finalize, first mark-paid, remove Carol). New coverage is 18 table-driven cases, one per error code on each page — a code with no map entry renders nothing at all, which is the one failure these pages cannot show the operator, and 18 codes landing at once makes that the likely regression. Plus an unknown-code degradation test and three end-to-end ones: the create form coming back filled in, corp-share correction with the split following and the audit row landing, and bad shares (text and zero) landing on the page rather than the boundary. Every case assertsgetByText("Something broke")has count 0 — that's the line that actually tests this change.A
bypassClientGuardhelper strips markup constraints where a server check can only be reached by going around the client guard. Without it the browser silently blocks the submit and the server-side check looks covered while never running.Assertions use
page.locator("p.notice--bad"), nevergetByRole("alert"): arriving from a server action is a soft navigation, so Next's route announcer is populated and also carriesrole="alert".Not in scope
The other four hand-rolled notices on the detail page (same drift, not error-path),
globals.css(no shared stylesheet is touched), the/payoutslist filter/sort and unboundedlistPayoutOperationsquery, and the 404 fallback. All are synthesis items 2+, major rather than blocker.🤖 Generated with Claude Code