[Agent Review Lab - Do not merge] - /agent-review-lab verify testing

[zweatshirt]

Description

Please explain a bullet-point summary of the changes.
List any PRs that this PR is dependent on and any Jira tickets that this PR is related to.

Testing

• Go to ...
• Do ...
• Check that ...

Checklist:

• I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
• I have applied the appropriate labels (Add the label "Preview" to automatically create a preview environment)
• I have run the Claude Code /quality:agent-review command locally and fixed any relevant suggestions
• I have requested a review from another person on the project
• I have tested my changes in preview or in staging
• I have cleaned up my commit history

[github-actions]

Bundle sizes [mpdx-react]

Compared against 0a0f7c3

No significant changes found

[zweatshirt]

🧪 [LAB] Posted by /quality:agent-review-lab (sandbox) — not the production review, and does NOT trigger auto-approve.

🔴 Verdict: BLOCKERS FOUND

1 critical blocker must be resolved before merge — the double-submission guard, the component's entire reason to exist, is non-functional. This was proven by an executable scratch test during evidence verification (/agent-review-lab verify), not just inferred from reading the code.

• 🔴 SubmitButton.tsx — setIsSubmitting(true) is never called, so disabled={isSubmitting} is always false. A scratch test that rendered the button with a pending onSubmit and asserted toBeDisabled() failed — the button never disables, so a double-click fires two concurrent submits.

Risk Assessment

MEDIUM — Blast Radius 1 · Complexity 2 · Sensitivity 1 · Recoverability 1 (effort 3, danger 2). Shared-tier component with async correctness logic; 0 importers today.

Dependency Impact

• SubmitButton.tsx — 0 dependents (new, unwired); no breaking changes.
• Naming collision: SubmitButton is already exported by Shared/Modal/ActionButtons/ActionButtons.tsx and imported by ~55 files app-wide. This third SubmitButton under Shared/ invites confusion.

Evidence Verification (Stage 4C)

• Test command: yarn jest <file> (auto-detected)
• Baseline: OK — the existing suite passes (green) despite the broken guard, which is itself corroboration for the false-confidence finding.
• Candidates verified: 1 of 1 — ✅ REPRODUCED SubmitButton.tsx:24 (guard never disables the button).

Suggested regression test to add (fails today, passes after the fix):

it('disables while in flight and fires onSubmit only once on a double click', async () => {
  let resolve;
  const onSubmit = jest.fn().mockReturnValue(new Promise((r) => (resolve = r)));
  const { getByRole } = render(<SubmitButton onSubmit={onSubmit}>Submit</SubmitButton>);
  const button = getByRole('button', { name: 'Submit' });

  userEvent.click(button);
  await waitFor(() => expect(button).toBeDisabled());
  userEvent.click(button);
  expect(onSubmit).toHaveBeenCalledTimes(1);
  resolve();
});

Review Summary

Agent	Critical	High/Important	Suggestions	Confidence
Architecture	1	1	2	High
Testing	1	2	—	High
Standards	1	1	—	High
UX	1	1	3	High

Dismissed Findings (Acknowledged by Developer)

These were reviewed and dismissed by the author before posting; they do not count toward the verdict.

• SubmitButton.tsx:31 — No loading indicator (6.0). Dismissed by: @zweatshirt — "Will add the loading indicator separately"
• SubmitButton.tsx:31 — No aria-busy during submission (4.0). Dismissed by: @zweatshirt — "Minor UX, not necessary"
• SubmitButton.tsx:31 — Hardcoded variant="contained", no ButtonProps passthrough (3.0). Dismissed by: @zweatshirt — "Will be implemented later"

---

Standard mode + verify · 4 agents (Architecture, Testing, Standards, UX) + dependency + evidence verification. This is a sandbox review and does not affect auto-approval.

[zweatshirt]

[Critical] **Double-submit guard is dead — `setIsSubmitting(true)` is never called.** ✅ REPRODUCED by a scratch test during `verify`.

isSubmitting starts false and the only state write is setIsSubmitting(false) on line 27, so disabled={isSubmitting} (line 31) is always false. The button never disables while onSubmit is in flight — a user can double-click and fire two concurrent submits, the exact "two donations from one intent" bug the doc comment promises to prevent.

Evidence: rendering with a pending onSubmit and asserting expect(button).toBeDisabled() fails — the rendered <button> has no disabled attribute mid-flight. The existing test is green, so nothing catches this.

Fix (also resolves the missing try/finally below):

const handleClick = async () => {
  setIsSubmitting(true);
  try {
    await onSubmit();
  } finally {
    setIsSubmitting(false);
  }
};

Flagged by all 4 agents (Architecture 9, Testing 9, UX 9, Standards 8).

[zweatshirt]

[Important] **No `try/finally` — the error path bricks the button.** `await onSubmit()` isn't wrapped. Today a rejection is an unhandled promise rejection; once the guard above is fixed *without* a `finally`, a failed submit (network/GraphQL error) leaves `isSubmitting === true` forever — button stuck disabled, no retry. The `finally` in the Critical fix above resolves this. Per repo UX rules, do **not** add a manual snackbar — the global Apollo error link already toasts GraphQL/network errors.

[zweatshirt]

[Important] **Test gives false confidence — none of the documented contract is covered.** It asserts only `onSubmit` was called once after one click; it never issues a second click and never inspects `disabled`, so it passes whether or not the guard works. Baseline confirmed this suite is green against the broken implementation. Add a double-click-while-pending regression test (see the suggested test in the review summary) so the guard can't silently break again.

[zweatshirt]

[Medium] **Naming collision / duplication.** `SubmitButton` is already exported by `Shared/Modal/ActionButtons/ActionButtons.tsx` and imported by ~55 files app-wide (plus a local one in `HrTools/SalaryCalculator/StepNavigation`). A third `SubmitButton` sitting directly under `Shared/` is the most discoverable location and will be confused with the adopted one on auto-import. Rename (e.g. `AsyncSubmitButton`) or fold the async-guard behavior into the existing component — cheap now with 0 importers, expensive later.

[zweatshirt]

[Medium] **`await userEvent.click(...)` violates the repo checklist** ("Never `await` `userEvent` calls"). Drop the `await`. Beyond style, awaiting the click flushes the microtask queue before any assertion, which is part of why the in-flight disabled state couldn't be observed — making the guard test easy to omit.

[zweatshirt]

[Suggestion] Named `SubmitButton` and semantically submits, but renders a default `type="button"` wired to `onClick` — it won't respond to Enter inside a form and bypasses Formik's `handleSubmit`. Either commit to `type="submit"` semantics (and align with Formik's `isSubmitting`) or rename to reflect it's a generic async action button.

[zweatshirt]

[Dismissed by author: Will add the loading indicator separately] [Medium] No loading indicator during submission. Even once the disabled state works, a greyed-out button is a subtle cue with no spinner; on a slow request users may perceive it as unresponsive. MUI v7 `` gives spinner + disable + `aria-busy` in one.

[zweatshirt]

[Dismissed by author: Minor UX, not necessary] [Suggestion] No `aria-busy` / live-region signal during submission, so screen-reader users get no announcement that a submit is in progress. Resolved for free by the MUI `loading` prop.

[zweatshirt]

[Dismissed by author: Will be implemented later] [Suggestion] `variant="contained"` is hardcoded and no `ButtonProps` (color, `type`, `sx`, `data-testid`) are forwarded, limiting reuse for a `Shared/` component. Consider spreading `...rest` of `ButtonProps` with `variant` defaulting to `contained`.