design sweep: make each surface answer the question it exists for - #128
Merged
Conversation
…it left open Eighteen reviewer reports, the ranked backlog they reconcile into, a comparison against the Aug-4 run, and SECOND-PASS.md for everything deliberately not fixed. The comparison is the part worth reading: the sort key changed from recurrence to cost-to-a-user, and the evidence that it mattered is one finding moving from rank 3 to rank 12 with its description unchanged. It also records four claims in the backlog that did not survive contact with the code, rather than quietly dropping them.
…nounces to nobody
Two confirmation shapes, because one does not fit both places a control
can live. ConfirmNotice handles controls outside a client-state boundary:
the action redirects with ?done= and focus moves to the notice on every
'at' change. ConfirmGroup/ConfirmingForm handles controls inside a
Disclosure drawer, where a redirect would replace the route tree and
collapse the very drawer the admin opened to press the button.
That drawer failure is not hypothetical -- an e2e run of the first,
all-redirect version caught it, and the docblocks record it so the next
person does not rediscover it the same way.
Notice now mounts unconditionally rather than behind {err && ...}. A
role=alert node inserted already holding its text is announced far less
reliably than a region born empty and mutated, so the guard was quietly
defeating the live region it existed to provide.
ConfirmCost emits its class unconditionally and hides with
.visually-hidden, closing a 641-851px reflow window where the control
disarmed itself.
Type scale gains --t-caption and --t-detail, both already load-bearing
under raw declarations across three-plus unrelated components.
…-auth that failed The page's four mutating actions all landed silently: the pressed control unmounts on success (a 'make main' row becomes the main row), so the press left no confirmation and focus stranded on <body>. A keyboard or screen-reader member had no way to know it worked. Each now redirects with ?done= and lands on ConfirmNotice, with 'at' in the URL so the second press of a different character announces too. The re-authorization round trip is the more serious half. It could report success on a return that had not actually granted the scope -- on the page PRODUCT.md says exists to answer 'why didn't my Discord role show up', which makes a false NOMINAL the exact failure the page is there to prevent. accountConfirmation is split into view.ts as a pure function so each outcome gets a unit test rather than a browser.
…f green The page read its freshness from the newest sync_run row, so a worker that had stopped entirely still rendered as healthy for as long as the last run's timestamp stayed recent. An admin's whole reason to open this page is to find out whether the thing is running, and it answered yes while it was not. Freshness now comes from pgboss.version.maintained_on, which every running supervisor loop advances on a ~120s cadence, and the worker row escalates with the same grades the job rows use rather than sitting outside the scheme. The three enqueue actions confirm through useActionState rather than redirecting -- their forms sit inside a Disclosure, and a redirect closes the drawer. Queued-at and last-run stay separately labelled facts so neither is read as dating the other.
…ever A permanent failure -- a role hierarchy change, a bot missing a permission, a member the API rejects -- reproduces identically every hourly tick for as long as its cause persists, and plain logAudit turned that into one audit row per tick indefinitely, burying the audit log under a single unfixed problem. logAuditIfChanged writes the first occurrence and writes again the moment details change, and stays quiet in between. It collapses reruns of an unchanged failure; it does not suppress the failure. Key order is normalised before comparing, because jsonb does not preserve it -- a raw JSON.stringify would read Postgres's own canonicalisation as a change and defeat the dedupe on every call. Known gap, recorded in docs/design-sweep/SECOND-PASS.md rather than fixed here: a permanent failure partway through the add/remove loops skips the discord.role_changed write for calls that already succeeded, so roles can change with no audit row. That is a behaviour change outside this pass's scope.
… find one member Eight of the nine mutating actions changed the pressed control out from under itself -- a locked tier's button goes disabled and cannot hold focus, approve's pending-only buttons unmount, freeze/wake and grant/revoke swap branches, unlink turns into bare text. Focus fell to <body> every time, so an admin working a long roster had to re-traverse the document to learn whether the press landed. Two plumbing shapes, not one, and the e2e suite is what forced that: the four cell-level actions redirect with ?done=, the four inside the row's Disclosure return through useActionState, because a redirect resets the drawer's useState and closes it. The first version of this fix redirected everywhere and the drawer collapsed on the first tier change. ?q= searches main name, alt names, Discord handle and account uuid -- the roster is one row per account and the only handle an admin usually has is a name. accountsConfirmation is pure and unit-tested per outcome.
An audit row's details carried account uuids, which is exactly the identifier nobody can read and nobody has written down. Reading the log meant copying a uuid out to the roster and back for every row that mattered. detailAccountNames resolves them alongside the role names that were already being resolved, so a row reads as a sentence about people. Sticky table head with scroll-margin so a row jumped to from a filter does not land underneath it.
…t is rejected operationFailed's ?error= redirect could only carry a fixed code, never the value that produced it, so a rejected edit re-rendered the page from server state and the typed value was simply gone. On a money screen, at the exact moment the operator most needed to see what they had entered in order to fix it. useActionState returns the rejected value alongside the code instead of navigating. InlineEditField is the single-field version reused by every one-input editor on the page; FlatPoolForm is the three-field one, because a mistyped total should not also cost the note explaining where the number came from. Rejected inputs take focus and select, so a correction is one keystroke rather than a hunt. Known and documented in-file, not fixed here: a successful add leaves its fields populated. defaultValue sets the value attribute, which a browser ignores once the input's dirty flag is set. Pre-existing -- the server-rendered form behaved the same -- but it means Add flat pool can be pressed twice. Tracked in SECOND-PASS.md.
The page asked a member to grant ESI scopes and named them in EVE's own vocabulary, which tells someone deciding whether to trust this app nothing. describeScope gives each one a plain sentence about what authGD does with it -- including that write_contacts deletes, which is the one a cautious person most deserves to be told before clicking. Adds e2e/login.spec.ts. The route had no spec at all, which is a strange gap for the only page an unauthenticated visitor can reach.
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThis pull request adds design-sweep records and implements related updates for confirmations, account search, worker heartbeats, audit details, payout forms, typography, responsive layouts, and accessibility. It also adds unit and end-to-end coverage for the changed flows. ChangesDesign-sweep records
Shared interaction and visual system
Account and administration
Worker health and synchronization
Audit and Discord role operations
Payout forms
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🤖 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 `@docs/design-sweep/audit-admin-audit.md`:
- Around line 80-83: Update the documentation’s timestamp width rationale to
state that “365d ago” is 8 characters, while four-digit day values such as
“1095d ago” are 9 characters and require overflow handling. Keep the explanation
aligned with the current three-digit day sizing and avoid changing the
implementation.
In `@docs/design-sweep/audit-login.md`:
- Around line 27-28: Update the seal image’s sizes attribute in the component to
use the mobile slot width at max-width: 40rem and 180px otherwise, while
preserving the proposed srcset entries. Verify the generated srcset remains
available for browser selection.
- Around line 19-20: The scope mapping in the `<dl>` must provide a
corresponding `<dd>` for every rendered `<dt>`. Update the scope-description
lookup and fallback in the documented scope-rendering flow to supply a non-empty
fallback description for unknown identifiers, while preserving the existing
mapped descriptions and identifier formatting.
In `@docs/design-sweep/audit-payout-detail.md`:
- Around line 31-38: Include setNameAction in the useActionState-based
state-preservation migration alongside the other field-level editors. Ensure
rejected name edits return validation state to the client leaf, render the
message beside the name field, and avoid redirecting or remounting so the
entered value is preserved.
- Around line 212-216: Update the proposed confirmation flow around
appraiseAction to either return the pool index and total needed for the “pool 3
added, 4.82b ISK” status message, or revise the confirmation to use only the
existing ok result. Keep clearing the textarea on successful submission and
ensure the status remains accessible.
- Around line 135-141: Update the “six notices” guidance to state that five
notices must mount unconditionally using the empty-slot pattern, with
errorMessage as the sole navigation-time exception. Explicitly list the five
derived warnings: unresolved items, both roster clashes, and the dropped report.
In `@docs/design-sweep/audit-payouts-list.md`:
- Around line 27-31: Separate the static date_future error code from its dynamic
message in NEW_OPERATION_ERRORS and OPERATION_ERRORS. Keep only the code in the
definitions, then have createOperationAction and setOccurredAtAction interpolate
the current UTC date when formatting the redirect response, ensuring no literal
placeholder such as <date> is emitted.
- Around line 27-31: Update the client-side date validation associated with the
payout operation forms so the date maximum cannot become stale across UTC
midnight. Recompute the `max` value when validation occurs or remove the native
`max` guard and preserve the server-side `date_future` validation with a
persistent field error, while keeping valid current-date submissions working.
In `@docs/design-sweep/audit-shell.md`:
- Around line 162-175: Update the documented anchor sizing recommendation in the
CSS example so its resulting target reaches the stated 28px design-system
minimum, adjusting the block padding and corresponding negative margins while
preserving zero net row-height impact. Keep the dependent header-height and
scroll-margin measurements consistent with the revised target.
In `@docs/design-sweep/COMPARISON.md`:
- Around line 97-116: Update the Phase 0 correction count in the “Where Aug-5
was not better” section to match the four listed correction bullets and the
subsequent “all four” statement. Change the opening “Three of the 24 items”
wording to “Four of the 24 items,” leaving the bullets unchanged.
- Around line 71-73: Update the e2e verification count in the Aug-5 summary to
match the authoritative PR total of 205, and explicitly identify whether the
number represents tests, cases, or specs.
In `@docs/design-sweep/PREAMBLE.md`:
- Around line 103-106: Remove the stale “Known open defect” block about the
`.st` Status token from PREAMBLE.md. Keep the surrounding design-sweep guidance
unchanged, since `.st` is documented as already declaring font-weight: 600 in
DESIGN.md and SYNTHESIS.md.
In `@docs/design-sweep/SYNTHESIS.md`:
- Around line 371-394: Reconcile the command-chain count between the “Proposed
command chain” table and the Aug-5 reference in COMPARISON.md. Either add the
missing command and corresponding validation as step 14 in the table, or update
the comparison’s reported chain length to 13, keeping all related counts
consistent.
In `@e2e/admin.spec.ts`:
- Around line 1580-1583: Update the height measurement callback in the
surrounding test helper to query the specific .scroller--tall element targeted
by the rule, rather than the generic .scroller selector. Preserve the existing
clientHeight measurement while ensuring it remains bound to the region under
test.
In `@e2e/sync.spec.ts`:
- Around line 100-115: Update teardown cleanup via resetDb or the relevant test
teardown to remove the pgboss.version row inserted by setHeartbeat,
conditionally handling cases where the pgboss.version table does not exist.
Ensure the 999999 heartbeat fixture is cleared before subsequent sync tests run.
In `@src/app/admin/accounts/page.tsx`:
- Around line 246-264: Update the Submit control in the accounts search form to
display “Search” instead of “Find”, while leaving the input’s existing “Find”
label unchanged. Use the Submit component near the accounts-search input as the
target so the two controls have distinct accessible names.
In `@src/app/admin/accounts/view.ts`:
- Around line 101-131: Make DONE_CODES the single source of truth by declaring
it as a readonly tuple and deriving AdminAccountsDoneCode from its element
values, rather than maintaining a separate union. Update the declaration order
and typing so isDoneCode continues narrowing against the derived type, while
preserving the exhaustive accountsConfirmation switch behavior. Apply the same
array-to-derived-union pattern to the corresponding duplicated definitions in
account view.
In `@src/app/payouts/`[id]/add-participant-form.tsx:
- Around line 45-53: Update the useEffect in the participant form to detect a
successful submission via state?.ok and reset the form, clearing the participant
field after the add completes. Preserve the existing rejected behavior that
focuses and selects the input, and ensure the reset prevents repeated
submissions from creating another payout.participant_added audit record.
In `@src/app/payouts/`[id]/appraise-form.tsx:
- Around line 66-73: Update the successful appraisal handling in the useEffect
to reset or remount the rawPaste textarea whenever state?.ok is true, while
preserving the existing dropped redirect behavior. Ensure subsequent submissions
cannot reuse the previous pasted loot and create a duplicate pool.
In `@src/app/payouts/actions.ts`:
- Around line 301-307: Update the numeric validation in the action state path
around the totalValue check to reject negative totals by removing support for
the optional minus sign in its regex. Ensure values such as "-1" return the
existing total_invalid response with preserved fields instead of reaching
addFlatPool.
In `@src/services/audit.ts`:
- Around line 68-73: Add a generated database migration creating an index on
audit_log covering action, target, and id in descending order to support the
failure deduplication query in the audit lookup. Use the project’s existing
migration and schema naming conventions, and include the corresponding migration
metadata if required.
- Around line 68-78: Make the lookup-and-insert sequence in the audit flow
around runJob and logAudit concurrency-safe: serialize it with a
transaction-scoped advisory lock or enforce uniqueness through an audit-entry
fingerprint constraint. Ensure concurrent executions cannot both pass the
unchanged check and insert duplicates, and add a test covering concurrent audit
processing.
In `@tests/health-service.test.ts`:
- Around line 74-106: Isolate the pgboss.version mutations in the health tests
so they cannot affect subsequent worker-queues tests. Update the setup and test
data around beforeAll, the empty-table test, and the “reads the newest
maintained_on” test to run in a transaction that is rolled back or against a
dedicated database, ensuring the version = 999999 row is never left in the
shared database.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85a830e2-bfff-466b-8a44-1a24c1c90693
📒 Files selected for processing (70)
DESIGN.mddocs/design-sweep/COMPARISON.mddocs/design-sweep/PREAMBLE.mddocs/design-sweep/SECOND-PASS.mddocs/design-sweep/SYNTHESIS.mddocs/design-sweep/audit-account.mddocs/design-sweep/audit-admin-accounts.mddocs/design-sweep/audit-admin-audit.mddocs/design-sweep/audit-admin-sync.mddocs/design-sweep/audit-boundaries.mddocs/design-sweep/audit-login.mddocs/design-sweep/audit-payout-detail.mddocs/design-sweep/audit-payouts-list.mddocs/design-sweep/audit-shell.mddocs/design-sweep/critique-account.mddocs/design-sweep/critique-admin-accounts.mddocs/design-sweep/critique-admin-audit.mddocs/design-sweep/critique-admin-sync.mddocs/design-sweep/critique-boundaries.mddocs/design-sweep/critique-login.mddocs/design-sweep/critique-payout-detail.mddocs/design-sweep/critique-payouts-list.mddocs/design-sweep/critique-shell.mde2e/admin.spec.tse2e/audit.spec.tse2e/error-boundary.spec.tse2e/login.spec.tse2e/payouts.spec.tse2e/shell.spec.tse2e/sync.spec.tssrc/app/_components/confirm-group.tsxsrc/app/_components/confirm-notice.tsxsrc/app/_components/confirm-submit.tsxsrc/app/_components/ui.tsxsrc/app/account/actions.tssrc/app/account/page.tsxsrc/app/account/view.tssrc/app/admin/accounts/actions.tssrc/app/admin/accounts/page.tsxsrc/app/admin/accounts/view.tssrc/app/admin/audit/page.tsxsrc/app/admin/audit/summarize.tssrc/app/admin/sync/actions.tssrc/app/admin/sync/page.tsxsrc/app/admin/sync/view.tssrc/app/api/health/sync/route.tssrc/app/globals.csssrc/app/login/page.tsxsrc/app/payouts/[id]/add-participant-form.tsxsrc/app/payouts/[id]/appraise-form.tsxsrc/app/payouts/[id]/clear-stale-query.tsxsrc/app/payouts/[id]/flat-pool-form.tsxsrc/app/payouts/[id]/inline-edit-field.tsxsrc/app/payouts/[id]/page.tsxsrc/app/payouts/actions.tssrc/core/contact-result.tssrc/core/health.tssrc/jobs/discord-roles.tssrc/services/accounts.tssrc/services/audit.tssrc/services/health.tstests/account-page.test.tstests/accounts.test.tstests/admin-accounts-view.test.tstests/audit-resolve.test.tstests/audit-summarize.test.tstests/discord-roles-job.test.tstests/health-core.test.tstests/health-service.test.tstests/sync-view.test.ts
Main's #124 rewrote the payout detail page and added its own in-place editor, `InlineEdit`, whose docblock stated the invariant "every action passed to InlineEdit rejects by redirecting, never by returning state" and added: "If that day comes, control the value here rather than adding a special case." This branch's `StringFieldEditState` conversion is that day, so rather than ship two overlapping editors the resolution folds this branch's value preservation into main's component and deletes the one added here (`inline-edit-field.tsx`). All eight in-place editors on the page now behave identically: `setNotesAction` and `setCorpShareAction` were converted to the same state-returning shape. `page.tsx` was resolved to main's structure (it changed there by 1758 lines across three PRs; one commit here touched it by 252), then this branch's one behavioural change — the extracted `AddParticipantForm` and `FlatPoolForm` — re-applied on top. One defect fell out of the combination rather than out of either side. Main made the appraise form conditional on `pools.length === 0`, with a second call site under `pools.length > 0` inside "Add another paste". This branch had just moved the dropped-lines payload out of a `redirect()` and into `useActionState`, pushed to `?dropped=` from an effect — and two call sites under opposite conditions unmount the component on the very commit the first paste succeeds, so that effect never ran and the "N items ignored" notice was lost for the first paste of every operation. The form is now one slot with a `collapsed` prop, which keeps it mounted across the switch and renders identically. Verified: typecheck, lint, format:check clean; npm test 77 files / 1132 passed; test:e2e 210 passed.
Three conflicts, all in text this branch and #129 both rewrote: - `confirm-submit.tsx`: main documents why Finalize/Unlock pass `alwaysHidden` (copy, not reflow); this branch documents why the account page's Discord row is safe to reveal (`.facts__lead > .confirm-cost { flex-basis: 100% }`, added here after the reveal was found to disarm the control between 641-851px). Both kept — they are about different controls. - `setBattleReportUrlAction`: main extracted the scheme check into `battleReportUrlProblem`, shared with the composer, but still rejected by redirecting. Takes main's helper and this branch's return-state rejection, so the two call sites now refuse the same way and the typed URL survives. Its e2e test moves from `p.notice--bad` to `span.inline-form__err` and now also asserts the value is still there. - `setNotesAction`: reverted to main's `Promise<void>`. #129 moved notes out of `InlineEdit` into a standing textarea that owns its own state, so there is no rejected value for `StringFieldEditState` to echo back. The four-editors docblock says so rather than counting four. Verified: typecheck, lint, format:check clean; npm test 77 files / 1134 passed; test:e2e 216 passed.
…e insert CodeRabbit's review of #128. `-1` passed the action's shape regex, so it reached `addFlatPool`, which throws for a negative total -- and a throw goes to the error boundary, taking the operator's note and raw paste with it. That loss is the exact thing `FlatPoolEditState` exists to prevent, so the guard belongs where the state is still in hand. `setItemPriceAction` already spells its regex without the minus; this now matches. The copy names negatives too. The form's `min="0"` blocks this in a browser, so there is no e2e route to the branch and no test accompanies the change -- it is defence for a hand-built request, which is when it matters that the failure is readable. Also from the same review: - The pgboss version-row fixtures leaked past their own files. `truncateAll` and `resetDb` both skip the pgboss schema, so a version pg-boss will never ship (999999) sat in the table it reads on `boss.start()` to decide whether its schema is current. `tests/health-service.test.ts` now snapshots and restores the table around its describe (a copy table, so it survives pg-boss adding a column); `e2e/sync.spec.ts` drops its sentinel row. - /admin/accounts named its search field and its submit button both "Find". The field is now "Name or handle", which also tells an operator what to type. - One zoom test measured `.scroller` on a page with two of them. Plus SECOND-PASS.md's record of what the review left open (the audit_log index and the DONE_CODES duplication -- both already on the list), and the prompt for handing that list to a fresh session.
One conflict, in DESIGN.md's label-register paragraph: both sides rewrote the `.st` weight note and told opposite stories about it. Main's #130 is the correct one and is what survives. This branch had "corrected" the paragraph to say `globals.css` has declared 600 on `.st` for as long as the rule existed, so the document had always been wrong. Checking the history rather than the current file: `.st` was added in #11 with no `font-weight` and inherited 400; #118 added the explicit 600 and rewrote `.st--lead`'s comment to say the weight there was "redundant with `.st`'s own 600 NOW". So the original paragraph described a real inconsistency that was real when written and has since been fixed -- which is exactly what main says. The correction was itself the error, made by reading the stylesheet as it stands today and mistaking that for how it has always stood.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/app/admin/accounts/page.tsx (1)
92-119: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize repeated query parameters before string operations.
Next.js App Router
searchParamsvalues can bestring | string[] | undefined. With/admin/accounts?q=Azzy&q=Zed,params.qis an array, soparams.q?.trim()throws before rendering. Type all keys accordingly, normalize values before use, use normalizedqfordefaultValue, and add an end-to-end regression test.🤖 Prompt for 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. In `@src/app/admin/accounts/page.tsx` around lines 92 - 119, Update the admin accounts page searchParams type to allow string arrays, normalize repeated query-parameter values to a single string before validation or string operations, and use the normalized q value for the search input defaultValue. Add an end-to-end regression test covering repeated q parameters such as q=Azzy&q=Zed to ensure the page renders without throwing.Source: MCP tools
e2e/sync.spec.ts (1)
100-128: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTrack ownership of the
pgbossobjects and sentinel row.
setHeartbeatcreates objects thatafterAllnever drops. Record pre-existing objects and row state, restore them after the tests, and drop only objects created by this helper.🤖 Prompt for 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. In `@e2e/sync.spec.ts` around lines 100 - 128, Update setHeartbeat to capture whether the pgboss schema, pgboss.version table, and sentinel version row already existed before creating or updating them, then add cleanup in the test suite’s afterAll flow to restore any pre-existing row state and drop only objects created by setHeartbeat. Leave pre-existing schemas, tables, and row values untouched.src/app/payouts/errors.ts (1)
136-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the action-state rule in this comment.
The comment says that input rejections redirect. The current payout field actions return
useActionStatevalidation state instead. Update this rule so future field actions do not reintroduce redirects that lose rejected input.🤖 Prompt for 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. In `@src/app/payouts/errors.ts` around lines 136 - 142, Update the lifecycle/input-handling rule in the comment near the payout error map: state that input rejections return useActionState validation state rather than redirecting, while lifecycle errors continue to go to error.tsx. Keep the existing generic guidance for always-open editable fields and avoid changing runtime behavior.src/app/payouts/actions.ts (1)
203-206: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject nonexistent calendar dates.
new Date()normalizes values such as"2026-02-30"instead of returningdate_invalid. Use a shared parser that requiresYYYY-MM-DDand compares the parsed UTC components with the submitted values. Apply it in both the create action andsetOccurredAtAction.🤖 Prompt for 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. In `@src/app/payouts/actions.ts` around lines 203 - 206, Replace direct new Date parsing with a shared YYYY-MM-DD parser that validates the format and compares parsed UTC year, month, and day against the submitted values, rejecting normalized nonexistent dates with date_invalid. Reuse this parser in both the create action and setOccurredAtAction while preserving existing name and date validation behavior.
🤖 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 `@e2e/sync.spec.ts`:
- Around line 24-28: Update the test.afterAll cleanup to ignore errors only when
the database reports SQLSTATE 42P01 (missing pgboss schema), and rethrow all
other cleanup failures. Ensure pool is closed in a finally block regardless of
cleanup outcome.
In `@src/app/payouts/`[id]/inline-edit.tsx:
- Around line 124-142: Update the success path in the useEffect handling state
so it records pending focus instead of immediately focusing triggerRef while
editing is still active. Add an effect tied to editing that focuses triggerRef
and clears the pending flag once editing becomes false, preserving the existing
success announcement and cleanup behavior.
---
Outside diff comments:
In `@e2e/sync.spec.ts`:
- Around line 100-128: Update setHeartbeat to capture whether the pgboss schema,
pgboss.version table, and sentinel version row already existed before creating
or updating them, then add cleanup in the test suite’s afterAll flow to restore
any pre-existing row state and drop only objects created by setHeartbeat. Leave
pre-existing schemas, tables, and row values untouched.
In `@src/app/admin/accounts/page.tsx`:
- Around line 92-119: Update the admin accounts page searchParams type to allow
string arrays, normalize repeated query-parameter values to a single string
before validation or string operations, and use the normalized q value for the
search input defaultValue. Add an end-to-end regression test covering repeated q
parameters such as q=Azzy&q=Zed to ensure the page renders without throwing.
In `@src/app/payouts/actions.ts`:
- Around line 203-206: Replace direct new Date parsing with a shared YYYY-MM-DD
parser that validates the format and compares parsed UTC year, month, and day
against the submitted values, rejecting normalized nonexistent dates with
date_invalid. Reuse this parser in both the create action and
setOccurredAtAction while preserving existing name and date validation behavior.
In `@src/app/payouts/errors.ts`:
- Around line 136-142: Update the lifecycle/input-handling rule in the comment
near the payout error map: state that input rejections return useActionState
validation state rather than redirecting, while lifecycle errors continue to go
to error.tsx. Keep the existing generic guidance for always-open editable fields
and avoid changing runtime behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f953ff3-9fe8-4251-8111-3c63e930d60b
📒 Files selected for processing (17)
docs/design-sweep/SECOND-PASS-PROMPT.mddocs/design-sweep/SECOND-PASS.mde2e/admin.spec.tse2e/payouts.spec.tse2e/sync.spec.tssrc/app/_components/confirm-submit.tsxsrc/app/_components/ui.tsxsrc/app/admin/accounts/page.tsxsrc/app/globals.csssrc/app/payouts/[id]/add-participant-form.tsxsrc/app/payouts/[id]/appraise-form.tsxsrc/app/payouts/[id]/flat-pool-form.tsxsrc/app/payouts/[id]/inline-edit.tsxsrc/app/payouts/[id]/page.tsxsrc/app/payouts/actions.tssrc/app/payouts/errors.tstests/health-service.test.ts
…tes, land focus after an inline save
CI's unit job failed on a change made to satisfy an earlier review: the
snapshot/restore added to tests/health-service.test.ts left a hand-rolled,
EMPTY pgboss.version behind on a fresh database. boss.start() reads that table
to decide whether its schema is installed, so worker-queues.test.ts then took
the migrate path and died on the pgboss.job it never created. It passed
locally only because this dev database already had a real pgboss schema. Both
fixtures now record what they created and remove exactly that; e2e/sync.spec.ts
probes once (a second probe would see its own inserts and call them
pre-existing) and closes its pool in a finally regardless of what cleanup did.
Verified by dropping the schema and running the full suite: 77 files, 1134
passed.
new Date("2026-02-30") is not invalid, it is March 2nd. The create action and
setOccurredAtAction both took that silently, storing a day three off what was
submitted on a record operators reconcile against their own logs. Both now go
through one strict YYYY-MM-DD parser that compares the parsed UTC parts back
against the digits.
InlineEdit's trigger button does not exist while the editor is open, so
focusing it in the same tick that closes the editor is a no-op against a null
ref and focus falls to <body> — a keyboard operator working down a roster
returns to the top of the document after every save. Save and cancel now
record the intent and an effect acts on it once the trigger is mounted. The
new assertion fails against the old code.
/admin/accounts declared q?: string where Next passes string | string[]. A
repeated param reached .trim() on an array and took the whole roster down with
a 500 — the same defect /admin/audit already fixed, resolved the same way.
Also corrects a stale comment in payouts/errors.ts: input rejections come back
as useActionState state now, they do not redirect (actions.ts already says so).
format:check, typecheck, lint clean; npm test 1134 passed; payouts, admin and
sync e2e specs pass.
#131 converts six `&&`-gated `Notice` call sites to unconditionally mounted regions, and two of its files are ones this branch also rewrote. `/account`: adjacency only. Main's comment and Notice are kept verbatim, with this branch's ConfirmNotice restored below them. ConfirmNotice was already mounted unconditionally and carries `live={false}` deliberately — focus, not the live region, does the announcing there — so #131's argument does not apply to it and it needs no conversion. `/admin/accounts`: main's third slot, `{params.queued === "account" && …}`, is dropped rather than merged. `?queued=account` was `syncAccountAction`'s own scheme and this branch supersedes it with the `?done=&name=&at=` triple every cell-level action shares; `doneUrl` (actions.ts) drops `queued` on purpose and `params` no longer declares it, so main's converted block would not compile here. Its text survives in `accountsConfirmation`'s `sync` case, rendered by the ConfirmNotice that replaced it. Main's errorMessage conversion and its comment are taken as-is. format:check, typecheck, lint clean; npm test 77 files / 1134 passed; test:e2e 218 passed.
guarzo
enabled auto-merge (squash)
August 6, 2026 04:32
…ons, correct the sweep docs Second CodeRabbit pass. Most of the batch was already fixed in fd506c1/09a5e90 and is skipped; what remained: /login rendered `<dt>` with no `<dd>` for a scope `describeScope` does not know. A `dt` alone is invalid in a `<dl>`, and AT groups a term with the next definition it finds — so an undescribed scope was read as meaning whatever the scope below it means, on the one page whose job is to say what is being granted. The default case now returns an honest "no description here, ask the deployment" line and the `<dd>` is unconditional. The e2e comment claiming the dd/dt count catches a fall-through is corrected: the per-row text assertions are what catch it now. `DONE_CODES` and its union are derived from one `as const` tuple in both `admin/accounts/view.ts` and `account/view.ts`, so the runtime guard and the exhaustive switch cannot drift. Docs: "365d ago" is 8 chars (four-digit days are the 9); the login srcset proposal needs the 132px mobile slot in `sizes`; setNameAction belongs in the useActionState list; the appraise confirmation cannot name a pool index or total that `AppraiseActionState` does not carry; five of the six notices need unconditional mounting, not four; `date_future` cannot hold a `<date>` placeholder in a static map, and `max` goes stale across UTC midnight; the th-anchor padding has to be 0.35rem to reach the 28px floor; COMPARISON's "three" was four, its chain is 13 steps, and its test counts now match the verified 1134/218. PREAMBLE's `.st` claim is marked corrected in place rather than deleted — SYNTHESIS and COMPARISON both cite what it said. typecheck, lint, format:check clean; npm test 77 files/1134 passed; npx playwright test 218 passed.
guarzo
disabled auto-merge
August 6, 2026 05:00
This was referenced Aug 6, 2026
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
…correctly (#143) 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.
guarzo
added a commit
that referenced
this pull request
Aug 6, 2026
…e bind Two findings in one file, both about the same six buttons. The drawer's approve pair ran alumni-before-associate while the filter chips and the set-tier row directly below both ran member-associate-alumni. The two tier control groups sit in the same `.btn-group` position and an admin scans straight from one to the other, so the pair is now derived from TIERS rather than hand-listed, and cannot drift again. The predicate excludes `member` by `Exclude<(typeof TIERS)[number], "member">` rather than by naming the two survivors. CodeRabbit caught that the original `t is "associate" | "alumni"` is a legal predicate for a wider union: a fourth tier would land in the array at runtime while the type narrowed it away, and `approveAction.bind` would typecheck against the stale pair and fail only when someone pressed the button. Verified rather than asserted — temporarily adding a "future" tier to TIERS gives, with the derived form: src/app/admin/accounts/page.tsx(694,67): error TS2345: Argument of type '"associate" | "alumni" | "future"' is not assignable to parameter of type '"associate" | "alumni"'. which is the compile failure the comment claims as its guard. Rebased off the design-sweep stack onto main, so this carries #128's `ConfirmGroup`/`ConfirmingForm` structure and `approveAction`'s fifth `identity` argument. The derivation's argument is unchanged under that signature. Gates: typecheck, lint, format:check, npm test (77 files, 1134 tests), and e2e/admin.spec.ts (49 passed) all green on the merged content.
This was referenced Aug 7, 2026
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.
A full design sweep of every rendered surface — eighteen reviewer reports reconciled into one ranked backlog, then fourteen of its top items worked in sequence. The sweep's own documents are in
docs/design-sweep/, including what it got wrong and what it deliberately left.The four blocking items
/accountreported a re-authorization that had not happened. The page exists to answer "why didn't my Discord role show up," and it could renderNOMINALon a return that never granted the scope — the specific failure the page is there to prevent./admin/synccould not see a dead worker. Freshness came from the newestsync_runrow, so a worker that had stopped entirely still read as healthy while the last run's timestamp stayed recent. It now readspgboss.version.maintained_on, which every running supervisor loop advances, and the worker row escalates on the same grades the job rows use./payouts/[id]discarded what the operator typed on a rejected edit.operationFailed's?error=redirect can carry a code but never the value that produced it, so a rejected edit re-rendered from server state and the typed number was gone — on a money screen, at the moment the operator most needed to see it./admin/accountsgave a press nowhere to land. Eight of nine mutating actions change the pressed control out from under itself; focus fell to<body>every time, so an admin working a long roster had to re-traverse the document to learn whether anything happened.Two things only editing could find
Worth calling out because eighteen reviewers reading source missed both.
redirect()resets clientuseState. It replaces the whole route tree even when the destination is the route you are already on, so any action whose control lives inside aDisclosurecollapses the drawer on press. The root cause was a docblock asserting "a soft navigation reconciles this component in place" — true at its first two call sites, false at the third./admin/accountstherefore ships two plumbing shapes rather than one: cell-level actions redirect with?done=, drawer actions return throughuseActionState.A CSS class rename is invisible to the unit suite by construction. A
.dim→.dim-inkswap broke twoe2e/audit.spec.tsselectors whilenpm teststayed green. Caught two steps later. Procedure changed mid-run to the relevant e2e spec after every step, not just at the end.Verification
Up from 201 e2e tests. Four of the new ones cover
/login, which had no spec atall; the rest arrived with the two merges of
maindescribed below.Merged
mainthree times since opening (through #130). Two of those merges neededhand resolution and both are worth a reviewer's eye:
was moving its dropped-lines notice from a
redirect()intouseActionState. Eachchange is correct alone; together, the first successful paste of every operation
unmounted the component in the same commit its state arrived, so the effect that
shows "N items ignored" never ran. Neither branch's tests could see it. The form is
now one JSX slot with a
collapsedprop, which keeps main's layout exactly and thecomponent instance alive.
directions. Main's is the true one; this branch's "correction" was made by reading
the current stylesheet and assuming it had always read that way. Resolved to main's.
Known open, deliberately not fixed here
All tracked in
docs/design-sweep/SECOND-PASS.md. The one a reviewer should weigh before approving:src/jobs/discord-roles.ts:193-212drops the audit record of role changes that succeeded.logAudit(discord.role_changed)sits inside thetry, after both loops. A member needing+alumni −membergets the add applied; if the remove then throws a non-transientDiscordApiError, control jumps to the catch and the audit write is skipped. The role was added andaudit_logsays nothing. It compounds with the dedupe added in this PR: the catch'slogAuditIfChangedwrites byte-identicaldetailsnext tick and gets suppressed too, so a tick that really changed roles can leave no audit record at all. Same shape on the deprovision path at:84-114.This is pre-existing in shape but newly interacts with the dedupe. It is a behaviour change in
src/jobs/, outside what this pass was scoped to, and is first on the second pass.Also open: both payouts add-forms keep their values after a successful add (
defaultValuecannot clear a dirty input), so "Add flat pool" can be pressed twice;workerHeartbeatreturnsnullfor every error, the same value that means "worker never ran";accountsConfirmationtakesstringwhereAdminAccountsDoneCodeis exported ten lines above; and the nav membership rule, which needs a product decision.docs/design-sweep/SECOND-PASS-PROMPT.mdis that list turned into a prompt a fresh session can be handed directly.CodeRabbit's review is worked through in
09a5e90: the flat-pool action's regex admitted a negative total (it reachedaddFlatPool, threw, and cost the operator the note and paste the state shape exists to preserve), two pgboss version-row test fixtures leaked past their own files,/admin/accountsnamed its search field and its submit button both "Find", and a zoom test measured.scrolleron a page with two of them. The two it raised that are not fixed here are theaudit_logindex (needs a generated migration, so it needs sign-off) and theDONE_CODESduplication — both already on the second-pass list.Where to look
src/app/_components/confirm-group.tsxandconfirm-notice.tsxare the two new primitives everything else consumes — their docblocks carry the reasoning, including the failing e2e run that produced the two-shape design.src/services/audit.ts'slogAuditIfChangedis the only change with an unindexed lookup on a table that grows without bound; its docblock says what the fix is and why the migration is not in this PR.The commits are grouped by theme rather than by the order the work happened. Only the final tree is verified; intermediate commits are readable but were not individually tested.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation