MPDX-9327, MPDX-9670 - Fix MHA "Update Current MHA" button visibility and taken/approved amounts displays#1824
Conversation
|
Preview branch generated at https://MPDX-9327-disable-update-mha.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 2cedf39 No significant changes found |
zweatshirt
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — ✅ APPROVED WITH SUGGESTIONS
Mode: standard · Agents: 5 (Financial Reporting, Architecture, Testing, Standards, UX) · Risk: 4/10 (MEDIUM)
Solid, well-tested refactor: the MHA board-approved dashboard now reads authoritative per-person amounts from HCM (mhaRequest.currentApprovedOverallAmount / currentTakenAmount) via the context, instead of the never-populated request fields that rendered $0. It also fixes the same-value-for-both-rows bug and gates the "Request New MHA" / duplicate buttons. Net tech-debt reduction. No blockers. One Medium item flagged independently by 3 agents.
Medium (5.0) — null HCM amount renders $0.00 instead of an "unknown" state
The new amount cells are number | null; currencyFormat(Number(null)) → $0.00. On the same card, missing dates render a MUI <Skeleton>, so missing money showing a concrete $0.00 is inconsistent and — per this repo's Financial rule ("zero-state that looks like real data") — potentially misleading. It's a net improvement over prior behavior (which showed $0 for everyone) and is crash-safe (Number(null) = 0, no $NaN), so it's non-blocking. Consider rendering a Skeleton/em-dash when the value is null. Worth a one-line product confirmation on whether a board-approved MHA can legitimately be null. See inline comment.
Suggestions (informational, non-blocking)
- Add a null-amount test to pin the empty-state behavior (inline).
- The previous-approved card shows the current HCM snapshot, not the historical request's amount — correct today but a latent coupling; add a clarifying comment (inline).
- Test mocks use
as unknown as HcmData— consistent with existing convention, informational.
Explicitly cleared (not issues)
onError: () => {}on the duplicate mutation — acceptable; documented, and the global Apollo error link surfaces the failure.userHcmData?.mhaRequest.current*optional chaining — type-safe (mhaRequestis non-nullable in the schema) and matches 6+ existing usages. No$NaNpath.- Per-person wiring verified — no staff/spouse cross-wiring; locked by the new distinct-amounts test.
Findings on Related Files / Pre-existing (not line-anchored)
[Pre-existing · informational] CurrentBoardApproved.tsx:204 — spousePreferredName ? spousePreferredName : 'N/A' renders a hard-coded, non-localized 'N/A'. Predates this PR; not in a changed hunk. Consider a follow-up to wrap in t().
| Agent | Critical | High | Important | Suggestions | Confidence |
|---|---|---|---|---|---|
| Financial Reporting | 0 | 0 | 0 | 3 | High |
| Architecture | 0 | 0 | 0 | 1 | High |
| Testing | 0 | 0 | 0 | 3 | High |
| Standards | 0 | 0 | 0 | 2 | High |
| UX | 0 | 0 | 1 | 2 | High |
| Total (deduped) | 0 | 0 | 1 | ~4 | High |
Generated by /quality:agent-review (standard mode). Agents converged — no severity ≥7 findings and no conflicts, so the debate round was unnecessary.
| > | ||
| {currencyFormat( | ||
| Number(approvedOverallAmount), | ||
| Number(userApprovedOverallAmount), |
There was a problem hiding this comment.
{userApprovedOverallAmount == null
? <Skeleton width={80} variant="text" />
: currencyFormat(userApprovedOverallAmount, currency, locale, { showTrailingZeros: true })}If a board-approved MHA can never legitimately be null, this is lower priority — worth a quick product confirmation. Flagged by UX, Financial, and Testing agents.
There was a problem hiding this comment.
This is usually preferred than an empty string
| expect(queryByText('Jane')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('shows distinct per-person approved and claimed amounts from HCM', () => { |
There was a problem hiding this comment.
it('renders $0.00 when HCM approved/claimed amounts are null', () => {
// contextValue with userApprovedOverallAmount: null, userTakenAmount: null
// assert the intended rendering (currently $0.00; or a dash if the Medium finding is adopted)
});This locks whatever the chosen empty-state behavior is.
| <Stack direction="column" width={mainContentWidth} mt={4}> | ||
| <CurrentBoardApproved request={previousApprovedRequest} /> | ||
| <CurrentBoardApproved | ||
| request={previousApprovedRequest} |
There was a problem hiding this comment.
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: MEDIUM (4/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)
This PR was auto-approved because:
- The multi-agent AI review determined it is medium risk
- No blocking issues were found
- All suggestions have been posted as review comments for the developer to consider
If you believe this PR needs human review, dismiss this approval and request a review manually.
…nce context and components
…usingAllowance components
552cb17 to
3f6c3d1
Compare
Description
Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions