fix(payouts): stop two strings describing numbers they sit next to incorrectly - #143
Conversation
…correctly The Corp share row read `12.5% (4,318,206.71 ISK + remainder)`, which reads as "this much, and then some rounding on top". It is the opposite: corpAmount is totalValue minus every participant's amount (services/payout-view.ts:215-218), so the remainder is already inside the figure — e2e/payouts.spec.ts:609-621 asserts exactly that, to the cent. The reader is an FC checking a split before finalizing, and this row is the only place the corp's cut appears as ISK; the old string invited them to add a fudge factor to an exact number, so a split that reconciles looks like it does not. Now ", remainder included". The word `remainder` is kept because payouts.spec.ts:619 locates the cell by it. The duplicate-name and roster-clash notices ended with "remove one before finalizing" and "Check before finalizing" outside any editability gate, so a finalized or payment-frozen operation instructed an action that assertEditable rejects, with the control already removed from the page. Someone auditing a completed payout after a dispute either hunts for a button that was deliberately taken away or concludes the operation is still a draft. The notices themselves are worth keeping after finalizing — they explain why one pilot appears twice in a split that already paid out — so only the imperative changes, on a new `amendable` rather than on `canEdit`: `canEdit` folds in the viewer's role, which is right for showing controls and wrong for copy, since a member reading a draft cannot remove a duplicate but the roster is not settled. Skipped: the exclude/include toggle's missing announcement, filed alongside these as a copy fix. It is not one — those controls are server actions with revalidation, so a fix needs #128's `?done=&at=` contract, and it is one of five controls on this page in the same state. Fixing one leaves four inconsistent with it. See #141.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
1 participant
Round 9 of the autonomous
design-sweep-looprun. Branched offmain, independent of #141, #142, and the #133–#138 stack.One sentence: two strings on
/payouts/[id]that tell an operator something untrue about the numbers beside them.What this round resolved
"+ remainder" says the corp gets more than the figure printed next to it
The Corp share row rendered
12.5% (4,318,206.71 ISK + remainder). Read plainly, that is this much, and then some rounding on top — the corp's real take is the printed number plus an unstated amount.It is the opposite.
corpAmountis documented insrc/services/payout-view.ts:215-218asThe remainder is already inside the figure.
e2e/payouts.spec.ts:609-621asserts exactly that: the cell must containtotal − Σparticipants, to the cent.Cost to a user: the one person who reads this row is an FC deciding whether a split is right before finalizing it, and the row is the only place the corp's cut appears as ISK. The string invites them to add a fudge factor to a number that is already exact — so a split that reconciles looks like it does not, and a hand-check against the pool total comes out wrong by the amount they added. On a page whose entire job is making a division of ISK auditable, the summary line should not need mental arithmetic to correct.
Now
12.5% (4,318,206.71 ISK, remainder included). Both branches — the operator'sInlineEditdisplay value and the read-only rendering — carried the same string and both are changed. The wordremainderis retained deliberately:payouts.spec.ts:619locates this cell byhasText: "remainder", and copy is a selector.Two roster warnings tell you to act before finalizing, on operations already finalized
The duplicate-name and linked/unlinked-clash notices end with
remove one before finalizing.andCheck before finalizing.Neither is inside thecanEditgate — they render on finalized operations, and on ones frozen by a payment, where the remove control they refer to is gone and the action they instruct is rejected byassertEditable.Cost to a user: someone auditing a completed payout after a dispute reads an instruction they cannot follow, on a page with no control to follow it with. The likely reactions are both bad — hunt for a button that has been deliberately removed, or conclude the operation is still a draft. The information in the notice is worth keeping after finalizing; it explains why one pilot's name appears twice in a split that already paid out. Only the imperative was wrong.
Gated on a new
amendable, not oncanEdit:canEditfolds in the viewer's role, which is correct for showing controls and wrong for writing copy — a plain member reading a draft operation cannot remove a duplicate themselves, but telling them the roster is settled would be a lie. The distinction is commented at the definition so the next person does not "simplify" the two into one.What was skipped
The exclude/include toggle's missing announcement, which the re-review filed alongside these two as a
/payouts/[id]copy fix. It is not one.The participant controls run server actions with revalidation, so there is no client state to announce from — a fix requires the
?done=&at=redirect contract #128 introduced, on the server action as well as the page. And the toggle is one of five controls on this page in the same condition: delete a pool, remove a participant, mark paid, revert, and this. Fixing one of five would leave four controls behaving inconsistently with their sibling, which is a worse state than all five being uniformly silent.See What needs a human in #141 for the full argument. Unchanged: this is one coherent round of work, and #128 is what makes it tractable for the first time.
Gate results
scripts/check-node-version.shnpm run typechecknpm run lintnpm run format:checkAll matched files use Prettier code style!npm testnpm run test:e2e -- payouts.spec.tsdocker build .npm run test:e2epayouts.spec.tscovers the changed surfacepayouts.spec.tswas run rather than skipped because this round changes user-visible copy, and round 4 of this run shipped a label change that broke 22 specs in this exact file with every static gate green.getByTextandhasTextmatch on substrings, so a copy edit is a selector edit whether or not it was meant as one.Tree held only
src/app/payouts/[id]/page.tsxafter every gate.