Conversation
Bundle sizes [mpdx-react]Compared against de02b3a No significant changes found |
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — Verdict: CLEAN
5 specialized agents (Architecture, Testing & Quality, Standards, UX, Financial Reporting) plus dependency analysis and a 3-agent cross-examination + rebuttal round.
No blockers. No findings at severity >= 5.0. Highest surviving finding is 4.5. All findings below are Suggestion tier — informational, no /dismiss required.
The debate changed the outcome
Both findings that started above the suggestion threshold were conceded downward by the agents that raised them:
| Finding | Pre-debate | Post-debate | Resolution |
|---|---|---|---|
| Heading/body copy mismatch | 6.2 (Important) | 4.0 | UX revised after learning the string is MPDX-9697's verbatim spec and the author had already flagged it. Rename withdrawn — reviewers should not override product copy. |
| CohortBar "0 New Staff" contradiction | 5.5 | 3.5 | UX conceded to Financial's argument (headcount not money; bounded by adjacency; file not in diff). Also accepted a correction: the View/Edit link is disabled (CohortBar.tsx:99). |
Guard should use !cohorts.length |
4.0 | withdrawn | Architecture conceded on the merits — !selectedCohortId is the real predicate; its own fix would break silently if a deselect affordance were added. |
Verification performed
Testing agent mutation-tested the new guard: dropping !error && fails the existing error test, dropping !loading && fails the loading test, and reverting the component fails all 3 new tests. Every clause is genuinely covered. yarn eslint, yarn lint:ts, and 117/117 folder tests all clean.
Risk & impact
Risk 3/10 (LOW) · Reviewer level: ANY · No breaking changes (no export added or removed; ActiveGoalsContent is module-local) · MpdGoalAdmin.tsx has 1 dependent; mpdGoalAdminMocks.ts change is purely additive (6 to 7 exports) · NullStateBox confirmed untouched · No translation key collisions, and other locales are conventionally not updated in-PR (verified across the 7 prior commits touching en/translation.json).
Design question for the ticket owner (escalated)
The UX agent raised a reframing worth more than any individual finding:
If the designer confirms "No Training Selected" is the right heading, the design presumes a state where trainings exist and none is chosen — and that state is unreachable by construction, because
MpdGoalAdminContext.tsx:109-113auto-selectscohorts[0]the instant any cohort loads. If the Figma shows a first-run "pick a training" screen, the auto-select is the defect and the copy was right all along.
So the Figma check is not only "which words" — whoever resolves it should check the auto-select behaviour against the design too.
Findings on Related Files (Not in This PR)
These were identified during review but are on files outside this PR's diff, so they cannot be posted as line comments. All informational — they do not count toward the verdict.
[Suggestion] src/components/HrTools/MpdGoalAdmin/CohortBar/CohortBar.tsx:90-93 — Severity 3.5/10 — Flagged by UX and Financial (converged)
In the empty state the stat row still renders "Training Size: 0 New Staff" and "NSO Date: —" above the "no trainings available" message. The internal inconsistency is that line 93 signals absence with an em-dash while line 91 asserts a hard 0, from the same selectedCohort?. optional chain. Not a money value, so it does not meet the "$0.00 that looks like real data" bar. Recommended action: follow-up ticket, consider selectedCohort ? t('{{count}} New Staff', ...) : '—'.
[Suggestion] src/components/HrTools/MpdGoalAdmin/PrintCohortGoalsButton/PrintCohortGoalsButton.test.tsx:76 — Pre-existing flake, unrelated to this PR
The Testing agent ran the decisive experiment: reverted all three changed files to HEAD~1 and reproduced the failure in 1 of 4 runs with this PR entirely absent (~20-30% under parallel folder execution, 0% in isolation, 6/6 solo). The test asserts toBeDisabled() synchronously after act(() => ctx.setSelectedCohortId(...)), racing the cache update. Introduced in 770ce692f, last touched in de02b3a42 — both before this PR. Recommended action: separate ticket; it will intermittently redden CI regardless of what merges.
[Suggestion] src/components/HrTools/CLAUDE.md — Stale documentation
Still describes MpdGoalAdmin as a mock-data prototype with "no .graphql", which stopped being true when #1996 wired in the real NewStaffCohorts / NewStaffCohortAttendees queries. Recommended action: separate ticket.
Agent Summary
| Agent | Critical | High | Important | Suggestions | Confidence |
|---|---|---|---|---|---|
| Architecture | 0 | 0 | 0 | 1 | High |
| Testing & Quality | 0 | 0 | 0 | 3 | High |
| Standards | 0 | 0 | 0 | 0 | High |
| UX | 0 | 0 | 0 | 7 | High |
| Financial Reporting | 0 | 0 | 0 | 1 (related file) | High |
| Total | 0 | 0 | 0 | 12 |
Standards returned fully clean against the entire repo checklist, having verified the translation sort order programmatically against three candidate comparators. Financial correctly returned "No financial calculation code in this PR."
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: LOW (3/10)
Verdict: CLEAN (no issues found)
This PR was auto-approved because:
- The multi-agent AI review determined it is low risk
- No blocking issues were found
If you believe this PR needs human review, dismiss this approval and request a review manually.
|
Preview branch generated at https://MPDX-9697.d3dytjb8adxkk5.amplifyapp.com |
…ntCohortGoalsButton tests
Description
When no NSO cohorts exist, the MPD Goal Admin Active Goals tab offered a live "Run and Send All" button. Clicking it opened a confirm dialog reading:
Verified by rendering the pre-change component against an empty
newStaffCohortsresult:RUN_AND_SEND_ALL_DISABLED = false, and the dialog opens on click.GoalsTableToolbar.tsxalready carries a comment about this exact hazard —// Otherwise the modal can claim "0 out of 0" beside the error alert— withdisabled={loading || !!error}. That guards the error case; it misses the zero-cohort case, whereloadingisfalseanderrorisundefined.This PR replaces the toolbar and table with an empty state when no cohort is selected, which removes the actionable CTA. The user-facing message is the smaller half of the change.
MpdGoalAdmin.tsx: when!error && !loading && !selectedCohortId, aNullStateBoxreplaces bothGoalsTableToolbarandGoalsTable.CohortBarstays mounted so the layout does not jump.MpdGoalAdminContext's auto-select has not run —MpdGoalAdminContext.tsx:225holdsloadingtrue viacohorts.length > 0for exactly that tick.ActiveGoalsContent; nesting the new branch in the existing double ternary would have made a quadruple-nested conditional.role="status"on the empty state so it is announced after the loading spinner unmounts.noCohortsMockfixture, reused inMpdGoalAdminContext.test.tsxto drop a duplicated inline literal.Reachability: the page is gated on
RequiredUserGroupEnum.MpdGoalCalcandnewStaffCohortsis team-wide, so "zero cohorts" means none exist in the database — the launch window before the first cohort is created, and any unseeded environment. Not a permanent production state, but real at rollout.Closes the last outstanding scope bullet of MPDX-9697. The rest of that ticket (consuming the
newStaffCohortslist query, pagination, role-scoped/newest-first ordering, dropping the mock cohort list) already landed in #1996, which pulled the cohort query in as a prerequisite for MPDX-9913.Open question for design: the ticket specifies the heading "No Training Selected", but that state is unreachable —
MpdGoalAdminContext.tsx:109-113auto-selectscohorts[0]whenever any cohort loads, so the only reachable case is "no trainings exist". Either the body copy should change to match the heading, or the heading should change to match reality — or the design intends a genuine "pick a training" first-run screen, in which case the auto-select is what needs revisiting. Flagged in the review threads; not resolved here.Testing
The empty state only appears when no cohorts exist at all, so the tests are the reliable way to exercise it:
yarn test MpdGoalAdmin mpdGoalAdmin— 14 suites, 121 tests.newStaffCohorts.nodesto[], then:newStaffCohortsquery still shows the red alert with the toolbar above it, and the initial load still shows the spinner.Also fixes a pre-existing flake in
PrintCohortGoalsButton › is disabled with an explanation until the cohort has training costs(~20-30% under parallel jest, green in isolation; reproduced with this PR entirely absent).Root cause, proven by instrumenting the test across repeated runs: its
waitForCohorts()helper waited onctx.cohortsbeing non-empty, which is one render too early.MpdGoalAdminContext.tsx:109-113auto-selects the first cohort in a passive effect that commits after that render. On a failing run the probe recordedafterWait=""— the test then calledsetSelectedCohortId('spring-nso-2027'), the still-pending auto-select flushed with a staleselectedCohortId === ''closure and queuedselectCohort('fall-nso-2026')after the test's update, so the selection ended on the fall cohort — which has training costs, leaving the button enabled. On passing runs the probe recordedafterWait="fall-nso-2026".The fix waits on
selectedCohortIdinstead, which can only become non-empty after the auto-select has settled. Verified: 8 consecutive full-folder runs clean, with the probe confirmingafterWaitis never empty.MpdGoalAdminContext.test.tsxuses the same harness but waits onfilteredRows, which is strictly downstream of the auto-select (the attendees query is skipped until a cohort is selected), so it was already safe and is unchanged.Docs:
src/components/HrTools/CLAUDE.mdstill described MpdGoalAdmin as a mock prototype with no.graphql, which stopped being true at #1996. Updated, plus a per-form gotcha recording the team-wide cohort scoping and the auto-select behaviour above.Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions