Conversation
|
Preview branch generated at https://MPDX-9914.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 914884c No significant changes found |
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Review, 6 specialist agents plus dependency impact
Verdict: BLOCKERS FOUND. 2 blocking, 4 important. Agents were selected by this repo's own .claude/rules/code-review.md triggers: Architecture, Testing, Standards, Data Integrity, UX, and the Financial Reporting domain agent. Security did not trigger (no pages/api/**, Apollo link/client, next.config, env, or .claude/** changes).
The two blockers
1. Deploy ordering, and it is stricter than it looks. CI codegens against production, not staging: ci.yml:21 resolves API_URL to https://api.mpdx.org/graphql unless the Staging API label is present, and ci.yml:196-199 blocks merge while that label is applied. So every red check on this PR has one root cause, namely that yarn gql cannot resolve newStaffCohortAssignableCoaches or coachId, and it clears only when the API reaches production. Current state: merged to mpdx_api master, not yet on staging, not in production. No change to this PR fixes it. Please do not triage the 12 red chunks as broken work.
2. A test that cannot fail for what its name promises. See the inline comment on GoalsTableToolbar.test.tsx:341. Proven by deliberately breaking the source and observing a green suite.
Note on the risk score
The rules file scores this 10/10 CRITICAL, but that is driven almost entirely by file count, seven ordinary component files at +1 each, rather than by anything dangerous. There is no auth, no Apollo cache config, no API route and no dependency change here. Read the reviewer recommendation as experienced, not senior maintainer.
Dismissed as a false positive
as unknown as on the throwing test mocks was flagged at 5.0 by one agent and explicitly cleared by another, which checked roughly 15 sibling sites including two pre-existing in this same file. Established idiom, not a violation, so it is not reported.
Verified clean
Localization is fully compliant: all three new strings go through t(), keys are static, no interpolation inside a key, and the translation.json diff is three pure single-line additions with no yarn extract spray. No any, no @ts-ignore, no non-null assertions, no fireEvent, no debug output. The mockData.ts deletion is safe, since it had exactly one export and zero remaining importers, and it was the one mock module imported by production code, so removing it is a net improvement. The cross-repo contract matches the API branch field for field. newStaffCohortAssignableCoaches is genuinely unpaginated, so there is no silent truncation. The Financial Reporting agent confirmed its path trigger was a false positive and that attendeeToRow's goal fields are byte-identical.
Findings on related files (not in this PR)
[Medium] src/components/HrTools/CLAUDE.md:175 still reads "only mockData.ts's mockCoaches is still stubbed, pending MPDX-9914". This PR deletes that file and closes that ticket. Flagged by four agents. That file auto-loads for anyone touching HrTools/, so leaving it stale actively misinforms the next reader. One-line fix, but it is not in this PR's diff so it cannot be posted as a line comment.
Findings below severity 7.0 can be dismissed with /dismiss: <reason> as a reply.
canac
left a comment
There was a problem hiding this comment.
This looks good! It's awesome to see this coming together!
It looks like the coaching list needs to be deduplicated, but I think that should be a backend change.

Description
Wires the MPD Goal Admin coach picker to the real API, replacing the mock coach list and the local-state-only assignment.
AssignCoach.graphql:NewStaffCohortAssignableCoachesquery andAssignCoachToNewStaffCohortAttendeemutation. The mutation selectsnewStaffCohortAttendees { id coach { ... } }so the payload normalizes over the cached attendee rows.MpdGoalAdminContextruns the coaches query for the selected cohort and exposesassignableCoaches.assignCoach(rowIds, coachId)is now a real mutation, modelled onrunAndSendNewStaffCohortfrom MPDX-9912 includingrefetchQueries+awaitRefetchQueries.coachOverridesis gone.TODO(MPDX-9914)comments inGoalsTableandGoalsTableToolbarare resolved, andmockData.tsis deleted.AssignCoachModalgains aloadingprop, an explanatory message when a cohort genuinely has no eligible coaches, and an inline error that keeps the modal open with the selection intact when assignment fails.Stacked on CruGlobal/mpdx_api#3590, which adds both operations. Coach eligibility is sourced from OneApp's
mpd_coachrole rather than typed in — that PR has the reasoning, but briefly: staging has ~190 login emails mapping to two or more distinct people, so identifying a coach by email either picks an arbitrary person or blocks those coaches outright.Jira: MPDX-9914 (frontend half of MPDX-9685).
CI will be red until the API reaches staging
src/graphqlis gitignored (.gitignore:42), so CI regenerates the schema fromapi.stage.mpdx.org, which does not yet serve these fields.GqlMockedProviderbuilds its mock schema from that file, so the MpdGoalAdmin tests will fail on CI until mpdx_api#3590 is merged and deployed to staging. They pass locally against the API branch's SDL. Please don't read that failure as a defect in this PR — but equally, don't merge this before the API is on staging.Testing
Requires mpdx_api#3590 deployed to staging, and an account with
mpd_supervisor_admin.mpd_coachholders in OneApp and check the modal explains that rather than showing an empty dropdownTo exercise it before the API is on staging, run codegen against the API branch:
Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions