MPDX-9626, MPDX-9627 MHA UAT fixes

[kegrimes]

Description

1. Need to fix the wrong page title from "Minister's Housing Allowance Request" to "Minister's Housing Allowance Calculation Tool"
2. On the view page, the wrong menu options were showing (Reports vs. HR Tools)
3. Make required fields nullable and ensure empty fields are $0

Jira tickets:

• MPDX-9626
• MPDX-9627

Testing

Name Changes test:

• Go to /hrTools/mhaCalculator
• Verify the title now reads "Minister's Housing Allowance Calculation Tool"
• Click the hamburger icon on the view page
• Ensure "HR Tools" is present, NOT "Reports"

Validation test:

• Go to /hrTools/mhaCalculator
• Create a new MHA and enter in one field (leave the rest blank)
• Click Submit and verify no validation errors appear
• Submit MHA and validate the mutation was submitted with 0's for null fields

Checklist:

• I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
• I have applied the appropriate labels (Add the label "Preview" to automatically create a preview environment)
• I have run the Claude Code /pr-review command locally and fixed any relevant suggestions
• I have requested a review from another person on the project
• I have tested my changes in preview or in staging
• I have cleaned up my commit history

[github-actions]

Preview branch generated at https://MPDX-9626-mha-uat.d3dytjb8adxkk5.amplifyapp.com

[github-actions]

Bundle sizes [mpdx-react]

Compared against f9395e4

No significant changes found

[kegrimes]

🤖 Multi-Agent Code Review Report — PR #1800

PR: MPDX-9626, MPDX-9627 MHA UAT fixes
Branch: MPDX-9626-mha-uat → main
Generated: 2026-05-27
Mode: Standard (6 of 7 agents launched — Security skipped, no auth/API surface)

---

📊 Risk Assessment

Risk Score	7 / 15 — HIGH
Files Changed	6 (+75 / −141 lines)
Day	Wednesday
Required Reviewer	⚠️ Experienced developer (financial domain + removed validation)

Risk factors:

• HrTools/MinisterHousingAllowance — financial calculation domain (auto-triggers Financial Reporting Agent per project rules)
• Yup validation rules removed on 8 monetary fields (.required(), .moreThan(0))
• 2 validation tests deleted with no replacement coverage
• New cross-cutting submit logic (transformNullValues) with branching behavior

---

🚫 Critical Blockers (Severity 9–10)

All five blockers below were independently flagged by 4 or more agents.

1. submitMutation is not awaited → success snackbar fires before submit can fail (Severity 10/10)

File: src/components/HrTools/MinisterHousingAllowance/Steps/StepThree/Calculation.tsx:226-238
Flagged by: Financial (10/10), Architecture (9/10), Data Integrity (8/10), Standards (8/10), Testing (8/10), UX (—)

onSubmit={async (values) => {
  try {
    await transformNullValues(values);
    submitMutation({                                       // ← not awaited
      variables: { input: { requestId: requestData?.id ?? '' } },
    });
    enqueueSnackbar(t('MHA request submitted successfully.'), ...);
    handleNextStep();                                       // ← runs unconditionally
  } catch (error) {}                                        // ← swallows everything
}}

Impact: If submitMutation rejects (network error, server validation, PositiveFloat rejection of zeros), the user sees a green "submitted successfully" snackbar and is advanced to the next step while the request is not actually submitted. For a financial workflow that triggers board approval, this is a critical false-positive.

Required Fix:

• await submitMutation(...)
• Move snackbar + handleNextStep() into the onCompleted path or after the await
• Replace empty catch {} with enqueueSnackbar(t('Unable to submit MHA request.'), { variant: 'error' })

---

2. Sending literal 0 to PositiveFloat schema fields likely rejected by upstream API (Severity 10/10)

File: src/components/HrTools/MinisterHousingAllowance/Steps/StepThree/Calculation.tsx:133-171
Flagged by: Financial (10/10), Architecture (10/10), Data Integrity (5/10 — softer, since PositiveFloat implementations vary)

The generated schema (src/graphql/schema.graphql:6127-6178, 9173) types every monetary field as PositiveFloat. The standard PositiveFloat scalar (graphql-scalars / Apollo Server) rejects values ≤ 0. The pre-PR .moreThan(0) validation existed specifically for this reason.

This PR now actively sends 0 via ?? 0 when fields are blank, while removing the client guard.

Likely failure mode: User leaves a field blank → submits → transformNullValues throws server-validation error → empty catch {} swallows it → user sees the success snackbar from issue #1.

Required Action: Verify with backend whether PositiveFloat accepts 0. If yes, no change needed here (still blocked by #1). If no, switch to conditional spread to omit empty fields rather than coercing to 0:

requestAttributes: {
  ...(values.mortgageOrRentPayment != null && { mortgageOrRentPayment: values.mortgageOrRentPayment }),
  // …
}

---

3. Empty catch {} block silently swallows all submit errors (Severity 9/10)

File: src/components/HrTools/MinisterHousingAllowance/Steps/StepThree/Calculation.tsx:237
Flagged by: All 6 agents (Standards 8/10, Architecture 9/10, Testing 8/10, UX 8/10, Data 8/10, Financial 10/10)

Direct violation of the project rule in .claude/rules/code-review.md: "No empty catch {} blocks that swallow errors silently."

Compounds issues #1 and #2 — without this guard removed, the user has no diagnostic path when submit fails.

Required Fix: At minimum surface an error snackbar; ideally also log via the existing error reporter.

---

4. transformNullValues does NOT recompute overallAmount — submitted total can be stale (Severity 9/10)

File: src/components/HrTools/MinisterHousingAllowance/Steps/StepThree/Calculation.tsx:133-171
Flagged by: Financial (10/10), Data Integrity (7/10), Architecture (—)

The per-field autosave hook (useSaveField.ts:36-49) recalculates overallAmount via calculateAnnualTotals and persists it on every blur. transformNullValues writes new field values (turning prior null → 0) but does not include overallAmount in the payload.

Concrete failure scenario: User switches rentOrOwn in Step 2 (which clears overallAmount: null via RentOwn.tsx:64) → fills Step 3 → submits → server-stored overallAmount stays null → the receipt view (CurrentRequest.tsx:125) renders $0.00 for a request that actually has non-zero fields.

Required Fix: Include overallAmount in the transformNullValues payload, computed from the with-zero-defaults values (round at the storage boundary, matching useSaveField convention).

---

5. Own branch of transformNullValues has zero test coverage (Severity 9/10)

File: src/components/HrTools/MinisterHousingAllowance/Steps/StepThree/Calculation.test.tsx:195-302
Flagged by: Testing (9/10), Financial (—), Standards (6/10), Architecture (—)

The only updateMutation-payload assertion (lines 272-285) covers the Rent branch — and only one field (unexpectedExpenses) actually exercises the ?? 0 path; the other four are typed with real values. The Own branch (which sends 8 fields and is the default rentOrOwn in TestComponent) is never asserted.

Combined with the deletion of the two prior validation tests, the net effect is less coverage of more behavior.

Required Fix: Add at minimum:

• One test: Rent mode, all 5 cost-of-home fields blank → submit → updateMutation called with all zeros
• One test: Own mode, all 8 fields blank → submit → updateMutation called with all zeros
• One test: submitMutation rejects → user sees error feedback (drives the issue add notifications #3 fix)

---

🔴 High-Priority Issues (Severity 7–8)

6. Autosave path sends null, submit path sends 0 — data semantic drift (Severity 8/10)

Flagged by: Data Integrity (7/10), Architecture (7/10), Financial (6/10)

useCustomAutosave.ts:124-130 persists null when a field is cleared on blur. transformNullValues then overwrites those nulls with 0 on submit. The two write paths now disagree about what "empty" means, and the on-submit coercion erases the audit-trail distinction between "user left blank" and "user explicitly entered 0."

Per the project rule (.claude/rules/code-review.md): "null is not silently coerced to 0 where it should surface as 'unknown.'"

Recommendation: Either (a) make autosave also send 0, or (b) change transformNullValues to send null (and verify backend acceptance), or (c) use conditional spread to omit empty fields entirely.

---

7. Submit button not disabled while submitting (Severity 7/10)

Flagged by: UX (7/10), Standards (—)

DirectionButtons.tsx:140-148 uses disabled={submitCount ? !isValid : false} — it never reads Formik's isSubmitting. Double-clicks during the network round-trip can fire duplicate submits.

Direct violation of the standards checklist item: "Submit buttons are disabled while isSubmitting is true."

---

8. transformNullValues keys off cached requestData.rentOrOwn instead of the form/prop value (Severity 7/10)

Flagged by: Financial (7/10), Testing (6/10), UX (7/10), Architecture (—), Data Integrity (5/10)

Calculation.tsx:134-135 reads rentOrOwn from requestData?.requestAttributes, while the rest of the component (line 100, 214, 329) uses the rentOrOwn prop. If a Step 2 selection hasn't round-tripped yet and the user navigates to Step 3 quickly, transformNullValues can choose the wrong branch — persisting Own-only fields for a Rent request, or vice versa.

Data Integrity verified RentOwn.tsx:50-67 does persist the change before navigation in the normal flow, but in flaky-network conditions this is still a race. Recommend using the rentOrOwn prop (single source of truth) throughout this file.

---

9. submitMinistryHousingAllowanceRequest mutation selection set is missing id (Severity 7/10)

Flagged by: Data Integrity (6/10)

MinisterHousingAllowance.graphql:93-103 returns requestAttributes but does NOT include id. After submit, Apollo cache cannot normalize the result to the existing entity → cached status may show stale "Pending" until next refetch.

Direct violation of the standards checklist: "Every query/mutation selection set includes id on normalizable types."

Required Fix: Add id to the submit mutation selection set; consider refetchQueries: ['MinistryHousingAllowanceRequests'] on submit.

---

10. Yup validation removed without confirmed server-side parity (Severity 8/10)

Flagged by: Standards (8/10), Architecture (10/10), Financial (—)

Project rule (.claude/rules/code-review.md): "every Yup rule must have a server-side equivalent." This PR removes them all from monetary fields. The fix path here is intertwined with #2 — the resolution depends on what the backend actually enforces.

---

⚠️ Important Issues (Severity 5–7)

11. Empty-state UX: $0.00 submissions look like real data (Severity 6/10)

Flagged by: Financial (5/10), UX (9/10)

Project rule: "A report with zero amounts should render 'no data' — not '$0.00' that looks like real data." With validation removed, the receipt and request-summary cards (CurrentRequest.tsx:125, RequestSummaryCard.tsx:98) now reach a state where they render $0.00 for a submitted request. Recommend at least a "submitting all-zero values — are you sure?" guard in SubmitModal.

12. Duplicate logic across transformNullValues branches (Severity 5/10)

Flagged by: Architecture (8/10), Testing (5/10), Standards (6/10), Financial (3/10)

The Own branch is a strict superset of the Rent branch (Rent fields + 3 Own-only fields). Refactor:

const baseAttributes = {
  mortgageOrRentPayment: values.mortgageOrRentPayment ?? 0,
  furnitureCostsTwo:     values.furnitureCostsTwo     ?? 0,
  repairCosts:           values.repairCosts           ?? 0,
  avgUtilityTwo:         values.avgUtilityTwo         ?? 0,
  unexpectedExpenses:    values.unexpectedExpenses    ?? 0,
};
const ownExtras = rent ? {} : {
  rentalValue:       values.rentalValue       ?? 0,
  furnitureCostsOne: values.furnitureCostsOne ?? 0,
  avgUtilityOne:     values.avgUtilityOne     ?? 0,
};
return updateMutation({
  variables: { input: { requestId: requestData?.id ?? '', requestAttributes: { ...baseAttributes, ...ownExtras } }}
});

13. Dead alert branch + misleading copy after validation removal (Severity 6/10)

Flagged by: Architecture (6/10)

Calculation.tsx:374-394 still renders t('Please enter a value for all required fields.') based on errors — but the only remaining "required" fields (phone, email, checkbox) have their own specific error messages. Either delete this branch or rewrite the copy.

14. Function name transformNullValues does not match what it does (Severity 5/10)

Flagged by: Architecture (5/10)

The function performs a network mutation, not a transformation. Caller await transformNullValues(values) does not suggest a server round-trip. Rename to saveDefaultedAttributes or extract to a useMhaSubmit() hook.

15. requestId: requestData?.id ?? '' — empty-string fallback hits the backend (Severity 6/10)

Flagged by: Data Integrity (6/10), Financial (—)

If requestData is null, the mutation runs with an empty string and the backend rejects it — silently, due to issue #3. Guard with an early return, or disable the submit button until requestData?.id exists.

16. mutationSpy count assertion is fragile (Severity 5/10)

Flagged by: Testing (6/10)

The new test asserts expect(mutationSpy).toHaveBeenCalledTimes(5) with comment "4 field updates + 1 submit." This is correct only because the context's updateMutation is a jest.fn() (bypassing Apollo) — if a future refactor routes transformNullValues through a real Apollo hook, the count silently changes. Prefer asserting specific operation names via mutationSpy.toHaveGraphqlOperation('...').

---

💡 Suggestions (Severity 3–5)

• Use generated TypeScript input type for transformNullValues literal so future schema drift breaks at compile time (Data Integrity, 4/10): annotate with MinistryHousingAllowanceRequestAttributesInput
• Stale translation keys in public/locales/en/translation.json — 'Required field.', 'Must be greater than $0.', and the old "Minister's Housing Allowance Request" variants are now dead in code (UX agent verified via grep). Safe to clean up on next translation cycle
• Duplicate page title string appears 3× in pages/.../mhaCalculator/ with no shared constant (Architecture, 6/10) — future renames will require 3+ file edits in lockstep
• Remove validateOnChange/validateOnBlur from <Formik> (Calculation.tsx:224-225) — they are now mostly inert for the monetary block (UX, 4/10)
• useEffect inside Formik render-prop (Calculation.tsx:254-257) — pre-existing anti-pattern, not introduced by this PR but visible
• transformNullValues redundancy with autosave — each field already autosaves on blur, so the submit-time write is mostly a no-op rewrite. Worth a follow-up: only fire the catch-up update when values differ from requestData.requestAttributes

---

✅ Wins in this PR

• ✅ Removed border: '2px solid red' inline sx — a real theme-token violation, now gone
• ✅ Title fix is correct and consistent across all 3 sites (verified by UX agent)
• ✅ NavTypeEnum.HrTools / HeaderTypeEnum.HrTools fix matches the unchanged sibling index.page.tsx
• ✅ All new user-visible strings properly wrapped in t() with {{mode}} interpolation variables (not template literals)
• ✅ No new any, no console.log, no new Date(), no commented-out code, no global fetch mocking
• ✅ Removed touched, errors destructures are correctly cleaned up
• ✅ Named exports preserved; file naming compliant

---

📋 Standards Checklist

Category	Status	Notes
Exports & Naming	✅	Named exports throughout; export default in page files is the required Next.js Pages Router exception
Localization	✅	Proper t() usage and {{mode}} interpolation
GraphQL & Apollo	⚠️	Submit mutation missing id in selection set (#9)
TypeScript	✅	No new any
Forms	❌	Yup parity with server not confirmed (#10); submit button not disabled while submitting (#7)
Testing	⚠️	Coverage reduced: Own branch + all-empty submit untested (#5)
Code Quality	❌	Empty catch {} (#3); fragile mutation count assertion (#16)

---

📦 Dependency Impact

• CostOfHome.tsx → 1 importer (parent Step component)
• FairRentalValue.tsx → 1 importer (parent Step component)
• Calculation.tsx → ~14 importer matches, but most are likely false-positive substring matches against Calculation in unrelated GoalCalculator components. Low real blast radius.
• No breaking changes (no removed exports)

---

🎯 Recommended Next Steps

Must fix before merge (in order of severity):

• Add await to submitMutation; move snackbar + handleNextStep after success; surface errors via snackbar (Tasks #1, add notifications #3)
• Verify backend behavior for PositiveFloat with value 0; choose between ?? 0 and conditional spread (contacts #2)
• Include overallAmount in transformNullValues payload (Bump axios from 0.20.0 to 0.21.1 #4)
• Add id to submitMinistryHousingAllowanceRequest selection set (Bump @storybook/addon-viewport from 6.1.17 to 6.1.18 #9)
• Add tests for the Own branch of transformNullValues and for an all-empty Rent submission (Move to Yarn V2 and upgrade packages #5)
• Confirm Yup/server validation parity, or document why removed (Bump @next/bundle-analyzer from 10.0.6 to 10.0.7 #10)

Should fix before merge:

• Disable submit button while isSubmitting (Make prettier config closer to defaults #7)
• Use rentOrOwn prop (not requestData) in transformNullValues (MPDX-6917-Remove-Lodash #8)
• Reconcile autosave (null) vs submit (0) semantics (Remove ts-node #6)
• Add empty-state guard in SubmitModal for all-zero submissions (Bump next from 10.0.6 to 10.0.7 #11)

Nice to have:

• Deduplicate transformNullValues branches (Bump @storybook/react from 6.1.17 to 6.1.18 #12)
• Delete dead "required fields" alert branch (Bump @storybook/addon-knobs from 6.1.17 to 6.1.18 #13)
• Rename transformNullValues or extract to useMhaSubmit() hook (MPDX-6916: Replace Moment #14)
• Guard against empty requestId (Fix-Dropdown-Menu-Item #15)
• Stronger mutation assertion in test (toHaveGraphqlOperation) (MPDX-6926-Fix-Filter-For-Tasks #16)

---

Agent Confidence Summary

Agent	Critical	Important	Suggestions	Confidence
💰 Financial	3	4	4	High
🧪 Testing	3	4	4	High
👤 UX	0	5	4	High
🏗️ Architecture	2	5	3	High
💾 Data Integrity	0	6	4	High (Medium on submit-cache semantics)
📋 Standards	2	4	—	High

Convergence: Every critical finding above was independently flagged by 4 or more agents — strong consensus, no debate rounds needed.

---

🤖 Generated by MPDX Multi-Agent Code Review System (6 of 7 agents, smart selection)

[wjames111]

Looks great!