Conversation
Bundle sizes [mpdx-react]Compared against 5e57129
|
wjames111
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — Verdict: ✅ APPROVED WITH SUGGESTIONS
6 specialized agents (Security, Architecture, Data Integrity, Testing, UX, Standards) + Financial Reporting domain agent + dependency analysis. No blockers. Clean, well-structured, fully-tested UI skeleton on mock data — highest finding severity is 6.5 (Medium).
Risk
Rubric score 10/10 (CRITICAL) is driven entirely by file count (10 new component/hook files) + volume. Genuine risk is LOW–MEDIUM: mock data only, no GraphQL/mutations/auth-logic/money-math wired, every component has a colocated test, and the one modified file (useHrToolsNavItems.ts) is purely additive (LOW dependency impact, no breaking changes, nav id → route confirmed consistent).
Medium-priority (worth fixing now — cheap, prevents latent bugs once real data lands)
- Selection set never pruned on search/cohort change — toolbar count can read "N selected" with nothing visible;
clearSelectionis wired to nothing. (Data Integrity + Testing + UX) — see inline comment. - "Select all" header checkbox missing
indeterminatestate — partial selection looks like "none". (UX + Data + Testing) — see inline comment. - Page-reset effect keys on
rows.length, not filter identity — latent: an equal-length filter/cohort switch on a later page won't reset. (Architecture + 3 others) — see inline comment.
Suggestions (informational)
- Brittle non-RTL test query (inline).
- Inert interactive controls (Print All, Run and Send All, Assign Coach, View/Edit, MoreVert) have no handlers — expected for a skeleton; consider
disabled+tooltip or a tracked follow-up so focusable controls don't read as broken. - Missing test branches:
Incompletestatus Chip, rows-per-page change, select-then-filter selection. - Consider a thin
useMpdGoalCohorts()hook now so the eventual GraphQL swap is contained to one file. - Forward-looking for real-data wiring: hardcoded
'USD'+ nocurrencyCodeonStaffGoalRow; nullable goal should render "not set" not$0.00; drawer should become a focus-trappedDialog/Drawer; stricter cross-staff admin authz on future queries.
Clean bills of health
- Security: no secrets/env-exposure/XSS/redirects/GraphQL;
blockImpersonatingNonDevelopersmatches all 10 sibling HrTools pages. - Standards: full PASS — named exports (page
export defaultis the required Next.js convention), i18n complete & extracted, noany/@ts-ignore/new Date()/console/TODO, tests colocated. - Financial: no calculation code — single display-boundary
currencyFormatcall, no arithmetic/aggregation/date math.
|
@EJTison Here's the MPD Goal Calculator Admin Table page's basic skeleton, nothings wired in yet but this will probably be what we want to work off of. I'll try to merge it today but if not you can just branch off this. |
wjames111
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — Verdict: ✅ APPROVED WITH SUGGESTIONS
6 agents (Security · Architecture · Testing · UX · Standards · Financial Reporting) · Standard mode · consensus reached with no cross-agent challenges, so no debate round was needed.
This is a clean, unusually well-tested UI skeleton. All data is mock (mockData.ts), the whole feature is gated behind DISABLE_MPD_GOAL_ADMIN (404 via getServerSideProps + hidden nav item), and no data crosses a trust boundary. Access control, i18n, TypeScript, and MUI conventions match sibling HR Tools exactly.
Verdict rationale
0 blockers. No Critical/High/Important (≥7.0) findings. Everything below is Medium/Suggestion — informational, none block merge.
Risk
Mechanical score: 10/10 (CRITICAL) — inflated by file count (next.config.ts +3, ~10 medium component/page/hook files +10, 300+ non-test lines +3). Actual blast radius is LOW: isolated net-new feature, flagged to 404, mock data only, zero GraphQL/mutation/auth changes. Because the mechanical level is CRITICAL, the auto-approve workflow was intentionally not triggered — a 20-file / +1500-line PR merits a human glance regardless.
Highest-value items
- Page can go out of range from non-filter data changes — the reset effect keys on
[search, selectedCohortId]butpageslicesrows; clamp on render instead (GoalsTable.tsx). - Two untested feature-flag branches —
getServerSidePropsnotFound gate (index.page.tsx) and the nav-itemhideItem(useHrToolsNavItems.ts). clearSelectionnever called after Run & Send — sent rows stay checked, inviting a duplicate send once wired (GoalsTableToolbar.tsx).- Six inert skeleton controls (View/Edit, Assign Coach, MoreVert, More Actions, Print All) — focusable enabled buttons with no handler; wire or
disabledbefore un-flagging.
All are latent (masked by mock data) rather than currently-breaking. No money arithmetic exists in this PR — sendableCount counts rows, not amounts.
Prior review
The 4 findings from the previous agent-review run (selection pruning, indeterminate checkbox, rows.length page-reset, brittle test query) have all been resolved in the current code. 👍
Summary
| Agent | Crit | High | Important | Med/Sugg | Confidence |
|---|---|---|---|---|---|
| Security | 0 | 0 | 0 | 1 | High |
| Architecture | 0 | 0 | 0 | 5 | High |
| Testing | 0 | 0 | 0 | 6 | High |
| UX | 0 | 0 | 0 | 5 | High |
| Standards | 0 | 0 | 0 | 1 | High |
| Financial | 0 | 0 | 0 | 2 | High |
|
Preview branch generated at https://MPDX-9696.d3dytjb8adxkk5.amplifyapp.com |
|
@canac do you mind giving this a once over? It should all be self contained, and it's really just a jumping off point/rough draft. |
a020511 to
df4c0ea
Compare
Description
Adds the MPD Goal Calculator – Admin Table page skeleton (scaffold + static UI) under HR Tools, built from the New Staff Goal Calculation Figma. It mirrors the existing
MpdSupervisorReportpage pattern (route →ReportPageWrapper→ context provider →SidePanelsLayout→ content + right drawer).pages/accountLists/[accountListId]/hrTools/mpdGoalAdmin/index.page.tsxsrc/components/HrTools/MpdGoalAdmin/:MpdGoalAdminContext— page state (active tab, selected cohort, search, row selection, drawer)CohortBar— training-cohort selector + summary stats (Training Size, NSO Date, Training Cost)GoalsTableToolbar— search + bulk-action buttons (Print All / Run & Send All ↔ More Actions / Run & Send Selected)GoalsTable— paginated MUI table with the design's columns, statusChips, currency-formatted MPD goal, row selection, and an "Assign Coach" prompt for unassigned rowsStaffDetailDrawer— right-panel placeholderMpdGoalAdmin— main content: sticky header, Active/Scenario tabs, client-side search filteringmockData.ts/mpdGoalAdminHelpers.ts— typed mock cohort data + enumsmpdGoalAdmininsrc/hooks/useHrToolsNavItems.ts, gated byinMpdGoalCalcIneligibleGrouppublic/locales/en/translation.jsonIntentionally out of scope (deferred): all GraphQL/data fetching (mock data only), real button actions (Print All, Run & Send, View/Edit training cost, row Actions menu, Assign Coach), Scenario Goals tab content, the full staff-detail drawer, and the Edit-Training-Cost / Assign-Coach / Batch-Send side flows. The bulk-action buttons and links are present but inert by design for this first pass.
Jira: MPDX-9696
Testing
/accountLists/<id>/hrTools/mpdGoalAdmin)Automated:
yarn test src/components/HrTools/MpdGoalAdmin(21 passing),yarn lint:ts(clean).Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions🤖 Generated with Claude Code