Conversation
Bundle sizes [mpdx-react]Compared against 419829b No significant changes found |
|
Preview branch generated at https://MPDX-9912.d3dytjb8adxkk5.amplifyapp.com |
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — 6 agents + adversarial cross-examination
Verdict: APPROVED WITH SUGGESTIONS. No finding survives calibration at severity >= 8.0. One genuine defect worth fixing; the rest are small, verified fixes.
Agents run: Architecture, Data Integrity, Testing, Standards, UX, Financial Reporting. Security was skipped (no auth, API-route, Apollo-config, process.env, workflow, or dependency changes). Every changed file drew findings from 2+ agents, so no coverage-gap pass was needed.
Findings were then put through an adversarial severity round. Two items rated 9.0 by a single agent were revised down after verification, one of them refuted on facts.
The one real defect
useRunAndSendFlow.tsx toasts a green success for sentCount === 0. Flagged independently by 5 of 6 agents, confirmed reachable against mpdx_api origin/master, and reproduced with a probe test. Commented inline.
Refuted during cross-examination
- "Already-sent goals being re-sent is untested" (rated 9.0) — refuted on facts.
mpdGoalAdminHelpers.test.ts:202already exercisespartitionSendablewith aSentrow, asserting it lands inalreadySentand notsendable. The consumer is covered atRunAndSendModal.test.tsx:98, and the server re-filters independently. Tested pure function plus two guards. - "Empty
attendeeIdsarray might send everything" — refuted.find([])returns[], so[]sends nothing; it is also unreachable while Continue is disabled onsendableCount === 0. - "Mocks encode an impossible post-send state" — refuted.
can_run_and_send?is purelyrun_and_send_blockers.empty?and does not depend on whether goals have been sent, socanRunAndSend: trueafter a send is correct.
Findings on lines outside the diff
public/locales/en/translation.json:245 — [Medium] severity 5.0. The _one plural form of "{{count}} MPD Goals were run and sent." is byte-identical to _other. Pre-existing, but this PR adds the per-row entry point where the count is always 1, making "1 MPD Goals were run and sent." the common case rather than an edge case. Worth correcting the _one form here.
src/components/HrTools/CLAUDE.md — [Suggestion] severity 4.0. The "Mock / prototype tools — not wired to a backend" section still says MpdGoalAdmin has "no .graphql", hits "neither API", and has "modals exist, no mutations", and tells readers not to wire tests against a real operation. All four claims are now wrong, and that file auto-loads for anything under HrTools/.
Correction to this PR's own description
The description says Run & Send "emails staff and their coaches". It does not. run_and_send.rb stamps goal_sent_at and goals_sent_at and nothing else — there is no mailer in the service or the mutation. That was MPDX-9842 AC #4, delegated to MPDX-9523, which is currently BLOCKED. The action is still effectively irreversible and does gate staff visibility of the goal, but the email framing overstates present behavior and will be corrected.
Also verified clean
yarn lint:ts and yarn eslint pass; 129/129 tests pass serially. All 5 new i18n keys have matching literal t() strings with no dynamic keys. No any, @ts-ignore, non-null assertions, new Date(), export default, debug output, or unreferenced TODOs added. Cache normalization is sound (NewStaffCohort has merge: true, and id is present on the one normalizable type). Money invariants intact — the null MPD goal still renders an em-dash rather than $0.00, and no new ?? 0 was introduced on an amount field. Test timezone is pinned to UTC in globalSetup.ts, so the hardcoded-ISO date assertions are not a flake risk.
The rotating 1-4 test failures under parallel load are pre-existing and outside this PR — four verification runs had all 12 MpdGoalAdmin suites passing while 9 sibling HrTools suites timed out on waitFor starvation.
…eRunAndSendFlow.tsx:54 Name the withdrawn-id cost of sending explicit attendee ids
# Conflicts: # src/components/HrTools/MpdGoalAdmin/MpdGoalAdminContext.test.tsx # src/components/HrTools/MpdGoalAdmin/mpdGoalAdminMocks.ts
canac
left a comment
There was a problem hiding this comment.
I tested it out, and this works really well!
# Conflicts: # src/components/HrTools/MpdGoalAdmin/GoalsTable/GoalsTable.test.tsx # src/components/HrTools/MpdGoalAdmin/GoalsTable/GoalsTable.tsx
# Conflicts: # src/components/HrTools/MpdGoalAdmin/CohortBar/CohortBar.tsx # src/components/HrTools/MpdGoalAdmin/mpdGoalAdminHelpers.test.ts # src/components/HrTools/MpdGoalAdmin/mpdGoalAdminMocks.ts
Description
Wires the Run & Send buttons (mocked in MPDX-9700) to the real
runAndSendNewStaffCohortmutation from MPDX-9842.useRunAndSendFlowhook, so confirm/send/toast behavior can't drift.sentCount, not the client's guess; the two can differ when a goal stops being Complete before the confirm. A zero count reports "nothing eligible" rather than success.NewStaffCohortAttendeesrefetch; the cohort normalizes itself from the mutation payload.goalSentAtdate, so a row sent in an earlier batch keeps its date while the cohort banner moves to the latest.goalsSentAt. Copy reads "The last batch of MPD goals was run and sent on …" —goalsSentAttracks the most recent batch, so the AC's "All complete goals were …" would be untrue after a partial run.canRunAndSend(its first consumer). The confirm dialog locks down with a spinner while sending.Notes:
attendeeIds. Omitting them means "every Complete goal in the cohort", which diverges from the modal's preview under an active search.mpdx_apichange needed — MPDX-9842 shipped the mutation and both timestamp columns. It stampsgoal_sent_at(the gate on staff seeing their goal) and does not send email; that is MPDX-9523's mailer, still blocked.Related: MPDX-9842, MPDX-9700, MPDX-9693. Reviewed with
/quality:agent-review(6 agents, no blockers); findings fixed in follow-up commits.Testing
Needs MPD Goals team access and a cohort with all 13 training costs entered.
⋮on an Incomplete row — disabled, same tooltip.⋮on a Complete row → "Run & Send this goal" → confirm. Check the toast count, that the row's chip becomes "Sent ", and that the banner appears.Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions