Conversation
Bundle sizes [mpdx-react]Compared against 4a196e3 No significant changes found |
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — PR #2016
Verdict: APPROVED WITH SUGGESTIONS. 0 critical, 0 high-priority, 0 important-tier findings. 6 specialized agents (Architecture, Testing, Standards, Data Integrity, UX, Financial Reporting) plus dependency-impact analysis. Risk 8/10 (HIGH) — though that score is driven by file count under broad medium-risk globs, not by intrinsically dangerous changes; no auth, API, migration, or Apollo-config files are touched.
All 13 changed files drew findings from 2+ agents, so no coverage-gap pass was needed.
Already fixed in 1b628ba (found by this review)
WCAG AA contrast failure, severity 6.5. theme.palette.warning.dark resolved to the MUI default #e65100 — 3.79:1 on white, against the 4.5:1 required for body2's 14px text. src/theme.ts never defines a warning palette key (line 35 is inside a local statusColors object; line 263 defines the separate statusWarning token), so it fell through to the MUI default. This was also a regression: the View/Edit link it replaced used MUI Link's default primary #05699B at 6.00:1, so the one control that unblocks Run & Send was made the least readable text on the bar. The inline comment asserting .dark cleared AA was wrong. Now uses theme.palette.statusWarning.main (Cru vermilion #D34400, 4.57:1) — the theme's own warning token, semantically a warning rather than an error, with existing precedent in OutstandingNeeds.tsx and OutstandingCommitments.tsx. Ratios independently recomputed, not taken on trust.
Two hypotheses tested and rejected
- Apollo cache partial write. The
UpdateNewStaffCohortpayload omitsgoalsSentAtwhile the query selects it. Not a bug: Apollo writes only fields present in the document being written, both selection sets carryid, and the mutation has always omittedname/trainingSize/date. The persisted production cache also self-heals viacache-and-networkwith noreturnPartialData. - Invalid-date render throw.
DateTime.fromISOon a malformed string yields a truthy-but-invalid DateTime that would throw inIntl.DateTimeFormat. Two agents independently found 15-20+ existingdateFormatShort(DateTime.fromISO(...))call sites, including the adjacentnsoDateline. Established repo convention, not a PR-introduced defect.
Closed as belonging to another PR
Three agents flagged that nothing refreshes goalsSentAt after a Run & Send. Real, but already solved in #2014, whose RunAndSendNewStaffCohort mutation selects goalsSentAt and normalizes over the cached cohort. No action here.
Conflict with #2014 — read before merging
#2014 (MPDX-9912, open) adds the same goalsSentAt field with an incompatible representation: string | null with node.goalsSentAt ?? null, against this PR's DateTime | null with DateTime.fromISO(...). Both touch Cohort, cohortNodeToCohort, mpdGoalAdminMocks.ts and NewStaffCohorts.graphql. Whoever merges second must reconcile the two representations, not just resolve the textual conflict.
Timezone: worth knowing, deliberately not changed
goalsSentAt is an instant rendered in the viewer's local zone with no zone label, while the repo's existing dateTimeFormat carries timeZoneName: 'short'. Date and time cannot disagree with each other (both formatters use the same DateTime and default zone), but newStaffCohorts is team-wide, so two admins in different zones see two different times for the same batch. Left as-is pending a product call — see the inline comment.
kegrimes
left a comment
There was a problem hiding this comment.
This looks great and tested well ✅ ! Nice work Will!
One thing I did notice is that when I entered in the training costs for the cohort that did not have values and submitted them, it took me to the first cohort page. We probably would want that to stay on the same cohort the training costs were updated for but that can be a separate ticket!
| <Link | ||
| component="button" | ||
| type="button" | ||
| underline="hover" |
There was a problem hiding this comment.
optional: you could combine these two links as they are pretty much the same thing!
Description
Closes the last two open scope bullets of MPDX-9692; the rest of that ticket (the cohort and attendees queries, dropping
mockData) already shipped in #1996 and #2013. First, the cohort bar now shows a warning-styled Provide Training Cost prompt instead of the plainView/Editlink when the selected cohort has no costs, tooltipped with the reason. Second, a newGoalsSentBannerreports the last Run & Send from a newly-queriedgoalsSentAt, reading "Goals were last run and sent on 8/10/2026 at 3:40 PM UTC." — deliberately not the ticket's original "All complete goals were run and sent…", becausegoalsSentAtrecords only the most recent batch and Run & Send can send a selected or search-filtered subset. That wording is a spec defect I've raised separately. Supporting it needed atimeFormathelper insrc/lib/intlFormat.ts, which carriestimeZoneNamelike itsdateTimeFormatsibling since cohorts are team-wide and admins span time zones.Conflicts with #2014 — please read before merging. That PR (MPDX-9912, open) adds the same
goalsSentAtfield with an incompatible representation:string | nullvianode.goalsSentAt ?? null, against this PR'sDateTime | nullviaDateTime.fromISO(...). Both touchCohort,cohortNodeToCohort,mpdGoalAdminMocks.tsandNewStaffCohorts.graphql, so whoever merges second must reconcile the two representations, not just resolve the textual conflict. Related: #2014 also selectsgoalsSentAton its Run & Send mutation payload so it normalizes over the cached cohort — that resolves the "banner shows a stale timestamp right after a send" gap, which this PR cannot fix alone because Run & Send is still a stub here.Testing
Automated:
yarn test src/components/HrTools/MpdGoalAdmin src/lib/intlFormat.test.ts— 14 suites, 200 tests passing.yarn lint:tsclean.Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions