Skip to content

fix(payouts): resume from the row just paid, not the top of the roster - #151

Merged
guarzo merged 1 commit into
mainfrom
design-sweep/2026-08-06b-3
Aug 6, 2026
Merged

fix(payouts): resume from the row just paid, not the top of the roster#151
guarzo merged 1 commit into
mainfrom
design-sweep/2026-08-06b-3

Conversation

@guarzo

@guarzo guarzo commented Aug 6, 2026

Copy link
Copy Markdown
Owner

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 #148merge both first. See Merge order at the bottom.

What was wrong

PayFlow chose the next focus target like this (pay-flow.tsx:138):

const next = rows.find((r) => r.state === "unpaid");

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:

const from = rows.findIndex((r) => r.id === pending.id);
const ahead = rows.slice(from + 1).find((r) => r.state === "unpaid");
const behind = ahead ? undefined : rows.slice(0, from).find((r) => r.state === "unpaid");
const next = ahead ?? behind;

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. from cannot be -1 — the !row guard 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.ts before committing rather than trusting a read: 66 passed. The forward-move string is byte-identical under the new code (behind is undefined, 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):

  • skipping a row advances past it, not back to it — pay row 2 of 3 with row 1 skipped; focus must land on row 3, and nothing may announce a wrap.
  • running off the end wraps back to the skipped row and says so — pay the last row with row 1 still owed; focus must land on row 1 and the announcement must carry the prefix.

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 cd in an earlier command. Before it was caught:

  • A branch design-sweep/2026-08-06b-3 was created in the primary checkout, off main rather than off fix(a11y): stop the 28px hit-target grade escaping the admin table rows #149's branch.
  • This round's two new specs were written there, while the source fix was written (by absolute path) into the worktree.
  • Typecheck, lint, format and npm test were 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 in docs/settled-design-decisions.md recording 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.md is 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 — clean
  • eslint . — clean
  • prettier --check . — "All matched files use Prettier code style!"
  • npm test1134 tests passed, matching the known-good baseline total
  • npx playwright test231 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 . and scripts/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.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 48 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f2d5f8bc-3cda-4279-81ea-37cbe0da4bed

📥 Commits

Reviewing files that changed from the base of the PR and between f28ea4b and 5d626a2.

📒 Files selected for processing (2)
  • e2e/payouts.spec.ts
  • src/app/payouts/[id]/pay-flow.tsx

Comment @coderabbitai help to get the list of available commands.

@guarzo
guarzo force-pushed the design-sweep/2026-08-06b-3 branch from 52d3258 to 16a5912 Compare August 6, 2026 12:36
Base automatically changed from design-sweep/2026-08-06b-2 to main August 6, 2026 12:40
`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.
@guarzo
guarzo force-pushed the design-sweep/2026-08-06b-3 branch from 16a5912 to 5d626a2 Compare August 6, 2026 12:40
@guarzo
guarzo merged commit c5ff5ea into main Aug 6, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant