feat(payouts): keep the operator's place while paying a roster - #145
Conversation
…nounced ISK amount pay-flow.tsx re-exported copyAmountId back through the client boundary that pay-flow-ids.ts exists to avoid, with no importer to justify it. And every "Next:" assertion in payouts.spec.ts truncated before the ISK amount, so a wrong or unformatted figure in the announcement would never fail a test.
… mean
Four cross-file references drifted as the surrounding files grew during the
branch. Each one makes a checkable claim, so a wrong number is worse than no
number — it sends the next reader to unrelated code and quietly discredits the
comment around it.
pay-flow.tsx page.tsx:866/891 -> 888/901 (MarkPaidForm/RevertForm)
pay-flow.tsx copy-amount-button.tsx:81 -> 88 (its role="status")
pay-flow.tsx page.tsx:211 -> 224 (firstPayment)
copy-amount-button page.tsx:841 -> 861-868 (the render, not the comment
above it; 861-862 is where the condition shows the
button does not depend on paid state)
Comments only. No behaviour change.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 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 (3)
📝 WalkthroughWalkthroughThe payout roster now uses shared payment-flow components. It derives active payment rows, assigns stable focus targets, announces server-confirmed state changes, and manages focus after payment or reversion. End-to-end tests cover finalized rosters and accessibility behavior. ChangesPayout accessibility flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant PayoutPage
participant PayFlow
participant Server
Operator->>PayoutPage: submit payment or reversion
PayoutPage->>PayFlow: dispatch action tracking
PayFlow->>Server: wait for confirmed state
Server-->>PayoutPage: render updated roster
PayFlow->>Operator: announce result and move focus
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…flow-focus # Conflicts: # src/app/payouts/[id]/page.tsx
…merge Merging main moved page.tsx around, so the same four cross-file references drifted a second time. Same reasoning as d8ff8a6: a wrong number sends the next reader to unrelated code. pay-flow.tsx page.tsx:888/901 -> 871/884 (MarkPaidForm/RevertForm) pay-flow.tsx page.tsx:224 -> 201 (firstPayment) copy-amount-button page.tsx:861-868 -> 844-851 Twice in one branch is the convention telling on itself; noted on the PR.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/payouts/`[id]/page.tsx:
- Line 689: Move the shared "roster-heading" value into pay-flow-ids.ts
alongside copyAmountId, export it, and import that symbol in the page component.
Use the shared identifier for both the roster heading id and the headingId prop
so these values cannot drift.
In `@src/app/payouts/`[id]/pay-flow.tsx:
- Line 151: Memoize the object passed as the value of PayFlowContext.Provider so
its identity remains stable while the memoized dispatch is unchanged. Update the
provider in PayFlow and retain dispatch as the sole dependency, preventing
MarkPaidForm and RevertForm subtrees from re-rendering on unrelated message or
pending changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1394b98-3f69-4d2e-87f7-e65bf83eefa2
📒 Files selected for processing (6)
e2e/payouts.spec.tssrc/app/_components/ui.tsxsrc/app/payouts/[id]/copy-amount-button.tsxsrc/app/payouts/[id]/page.tsxsrc/app/payouts/[id]/pay-flow-ids.tssrc/app/payouts/[id]/pay-flow.tsx
…alue churning Two review findings from #145, both small and both real. `"roster-heading"` was spelled out twice — once as the `id` on the heading, once as `PayFlow`'s `headingId` — with nothing checking they match. It moves to `pay-flow-ids.ts`, which exists for exactly this and is already reachable from both sides of the client boundary. The e2e selectors keep the literal on purpose: a test that imports the id it asserts on cannot catch the id changing. `<PayFlowContext.Provider value={{ dispatch }}>` built a fresh object every render. `children` is a prop, so React already bails out of re-rendering the table when only `message` or `pending` moved — but that bailout does not cover context consumers, and every row's control is one. So each announcement re-rendered a dozen forms for nothing. Memoized on `dispatch`, which changes only when `rows` does, which is when those rows should re-render anyway.
|
Both findings applied in 21ff008 — thanks, both were real.
Context value memo — correct, and worth spelling out why the usual counter-argument does not apply here. Also on this branch since the first review: merged Verification on |
1 participant
Pull request
What changed and why
Paying out a fleet on
/payouts/[id]costs more than it should, and the cost isnot the clicking.
mark paidrenders only while a row is unpaid, so it unmountsitself on success; focus falls to
<body>; and the operator re-scans a 5-15 rowtable for the next unpaid name after every ISK transfer. Nothing tracks
position.
So: keep the operator's place.
A new
PayFlowprovider wraps the roster and hosts one effect. When a paymentsettles it announces progress and the next recipient into a live region, and
moves focus to that row's
copy amountbutton — the one control in the actioncell that renders regardless of paid state, and therefore the only stable target
across both transitions. On the last row it announces "All N paid" and focuses
the roster heading instead. A revert announces too, but holds focus on its own
row rather than advancing.
The effect lives above the table because both
mark paidandrevertunmountthemselves on success; an effect hosted in either button would race its own
unmount. (
InlineEditcan useuseActionStateprecisely because it neverunmounts across its own call. That pattern does not transfer here.)
The load-bearing property: no optimistic UI. Every announcement derives from
server-rendered
rows, never from the action promise. The live region can onlysay "Paid Ada Relay" on a render where the server says Ada is paid. A failed
action throws to
error.tsxand the component unmounts with its watch stillarmed, announcing nothing. On a money ledger that is worth the extra round trip.
Also here: the first payment on an operation freezes it permanently — pools,
roster and shares can no longer be reopened with Unlock — and had no confirm
step. It now gets
ConfirmSubmit, with the consequence sentence inaria-describedbyand permanently.visually-hidden(#112: revealing inside atdwidens the cell, moves the armed button out from under a stationary mouse,and disarms the control).
Deliberately not here, after checking how the corp actually pays a fleet:
no "mark all paid" (EVE has no multi-recipient transfer, so bulk would decouple
the ledger from reality), no reordering or filtering (rosters are 5-15 people),
and no change to
recordPayment,markPaidAction,revertPaymentAction, oranything under
src/services/,src/core/,src/db/,drizzle/.page.tsxshows ~379 changed lines;git diff -wputs the real edit at 45/28.The rest is re-indentation from wrapping the table in the provider.
Why
pay-flow-ids.tsis its own modulecopyAmountIdis called bypage.tsx(a Server Component, at render time) andby
pay-flow.tsx(a Client Component, inside an effect). Every export of a"use client"file becomes a client reference, and a Server Component cannotinvoke a client reference as a plain function. The 18-line directive-free module
is what lets both sides share one definition of the id format.
What CI cannot check
No manual screen-reader pass. The announcement text and focus destinations
are asserted in Playwright (six new tests), but nobody has heard NVDA or
VoiceOver read them. There is no jsdom in this repo, so client behaviour is
Playwright-only by necessity.
Everything else is CI's. Locally, on
d8ff8a6:Deploy notes
None. No migration, no new secret, no
fly.tomlchange.Flags
Known residual: stale attribution, not stale state. If this tab's props say
"unpaid" because another operator paid the row after the render,
recordPaymentreturns early (
payouts.ts:818),markPaidActionrevalidates regardless(
actions.ts:609), and the effect announces "Paid X" for a transfer thisoperator never made. Generally: any path where the submit lands but the row's
state doesn't move leaves the watch armed, and the next render that flips that
row announces as if this click caused it. The bound holds either way — the
announced state is true, and the audit log names the real payer. Only
attribution is wrong. Closing it needs a return value from the action saying
whether this call was the one that wrote, which is a service-layer change this
pass deliberately does not make.
Follow-up, out of scope:
ConfirmSubmit->Submitoccupy the same JSX slotwith different component types, so a fast click during the remount can be lost.
Pre-existing — the identical ternary sat inline before this branch — but this
change increases exposure, because focus now lands on the next row immediately
and invites the fast follow-up click the old re-scan pause used to prevent. It
belongs in its own change against those two components.
Accepted edge: paying out of roster order announces a "Next:" pointing at a
row above the one just paid. It matches the specified algorithm (next unpaid in
roster order, not next after this row) and the roster is short enough that this
is a curiosity rather than a problem.
Fixed along the way: four cross-file
file.tsx:NNNcomment references haddrifted stale as the files grew during the branch (
d8ff8a6, comments only), anda dead
export { copyAmountId }re-export was re-publishing the id helper throughthe very client boundary the module split exists to avoid (
e8f6c08).Summary by CodeRabbit
New Features
Tests