fix(payouts): resume from the row just paid, not the top of the roster - #151
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 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 (2)
Comment |
52d3258 to
16a5912
Compare
`PayFlow` picked the next focus target with `rows.find(r => r.state ===
"unpaid")` — the *first* unpaid row in the operation, regardless of where
the operator actually was.
That is only correct while the operator works strictly top to bottom. The
moment one pilot is skipped — offline, disputed, paid out of band — every
subsequent payment drags focus and the scroll region back up to that same
skipped row. On a 60-person roster that is fifty-odd jumps to a row the
operator has already decided not to pay, and the component's own docblock
says it "keeps the operator's place".
Now resolves forward from the row just settled, and wraps to the top only
once nothing is left below. The skipped rows are still owed, so falling
off the end has to come back for them rather than jumping to the heading.
`rows.slice(0, from)` stops short of the settled row, so a wrap can never
land on the row just paid.
The wrap announces itself ("Back to the first unpaid.") because a silent
jump *upward* is the same disorientation this change exists to remove.
That is the only user-visible string that changed, and it is reachable
only in a case no existing spec exercised — all 66 payouts specs pass
unchanged.
The revert branch is deliberately untouched: reverting is a correction,
not progress, and keeping focus on the reverted row is asserted at
payouts.spec.ts:2296-2302.
Two specs pin it, both verified to fail with the fix reverted.
16a5912 to
5d626a2
Compare
Round three of an unattended design sweep. One behaviour, one file: the payout flow resumes from the row the operator just paid instead of from the top of the roster.
Based on #149, which is based on #148 — merge both first. See Merge order at the bottom.
What was wrong
PayFlowchose the next focus target like this (pay-flow.tsx:138):That is the first unpaid row in the operation, not the next one after the row just settled. It is correct only while the operator works strictly top to bottom.
The moment one pilot is skipped — offline, disputed, already paid out of band — every subsequent payment drags focus and the scroll region back up to that same skipped row. On a 60-person roster an operator who skips row 3 gets fifty-odd jumps back to row 3, each one costing a scroll back down to where they actually were.
PayFlow's own docblock says it "keeps the operator's place"; the code did the opposite the moment the operator deviated from the order the code assumed.The screen-reader operator gets it worse: the announcement names that skipped row as "Next" after every single payment.
What it does now
Resolve forward from the settled row, and wrap to the top only once nothing is left below it:
The wrap is deliberate, not a fallthrough. Skipped rows are still owed and the operator still has to reach them, so running off the end has to come back rather than jump to the heading as if the work were done.
rows.slice(0, from)stops short of the settled row itself, so a wrap can never land on the row just paid.fromcannot be-1— the!rowguard above already returns when the settled row has left the roster.The wrap announces itself. A silent jump upward is the same disorientation this change exists to remove, so that one case gains a prefix:
Back to the first unpaid.This is the only user-visible string that changed in this round.The revert branch is untouched. Reverting keeps focus on the reverted row, because reverting is a correction rather than progress — asserted at
payouts.spec.ts:2296-2302. It would have been easy to unify the two branches now that both compute a "next"; that would have been a regression.The copy-as-selector check
This repo's browser specs locate cells by substring (
getByText,hasText), so a cosmetic string edit can break specs while typecheck, lint, unit tests and format all stay green — a previous run broke 22 payouts specs exactly that way.So this round ran
e2e/payouts.spec.tsbefore committing rather than trusting a read: 66 passed. The forward-move string is byte-identical under the new code (behindisundefined, so the prefix is empty), and the wrap prefix is reachable only in a case no existing spec exercised.Two specs pin it
Both verified to fail with the fix reverted (
Expected: focused, Received: inactive):A process failure in this round, stated because it affects what you can trust
Partway through this round the shell's working directory was silently reset out of the sweep's worktree and into the primary checkout, by an unrelated
cdin an earlier command. Before it was caught:design-sweep/2026-08-06b-3was created in the primary checkout, offmainrather than off fix(a11y): stop the 28px hit-target grade escaping the admin table rows #149's branch.npm testwere run against that split tree — specs without the fix.Nothing was committed to the primary checkout, and it has been restored to a clean
main. The stray branch is deleted. Every gate quoted below was then re-run from scratch in the worktree, on the correct base, which is also when the two specs first passed — their earlier failure was the split tree, not the fix.Round two's numbers were measured before the reset and are unaffected.
Worth a reviewer knowing because it is the failure mode an unattended run is least able to notice: every command still succeeded, and the only visible symptom was a test failure that looked like a bad fix.
The alternative design, which a review of this commit argued for and this PR did not consider
Wrap-and-announce is not the only defensible answer, and the commit body did not say so. The alternative: falling off the end focuses the heading and announces the count, exactly as the all-paid case already does (
pay-flow.tsx:166-169), and the operator navigates back to whichever skipped row they actually want.The case for it is real. A skipped row is one the operator already made a decision about — offline, disputed, paid out of band — so jumping straight back to it presumes they want to deal with it now, immediately after finishing the roster, which may not be true. And the heading is the one guaranteed-attended landing spot on the page; a live region is easy to miss mid-Tab.
The case for what shipped: it keeps
PayFlow's "keeps the operator's place" promise literally, never landing focus somewhere without saying so, and the audible "Back to the first unpaid" is itself the disclosure that lets an operator disengage. That is why it shipped — but it is a design fork, not a strictly dominant fix, and it deserves a line indocs/settled-design-decisions.mdrecording which was chosen and why, so the next pass does not re-litigate it as a "why not just…". This run did not write that line:settled-design-decisions.mdis the file every reviewer in this sweep is told is closed, and adding a precedent to it is a human's call, not an unattended run's.Gates
Re-run on this branch's tip in the worktree, quoted:
tsc --noEmit— cleaneslint .— cleanprettier --check .— "All matched files use Prettier code style!"npm test— 1134 tests passed, matching the known-good baseline totalnpx playwright test— 231 passed (4.6m), exit 0, with 231✓lines counted in the captured log. That is fix(a11y): stop the 28px hit-target grade escaping the admin table rows #149's 229 plus exactly the two specs this round adds.Not run on this branch:
docker build .andscripts/check-node-version.sh— environment-bound and slow. CI runs both on this PR. Treat them as unchecked until it does.Merge order
Merge #148, then #149, then this. Prefer a merge commit or rebase over squash — squashing a parent rewrites the commits this branch is based on, and this PR will then show conflicts against changes that are, in substance, already there.