MPDX-9367 - PDS Calc Salary Subtotal section

[zweatshirt]

Description

• Adds the Salary Subtotal section to PdsGoalCalculator
• I ran a review on this, it came back well with primary one issue: salaryOrHourly not being defined will cause this content to not render correctly. I have a follow up PR to address this issue

MPDX-9367

Testing

• Go to reports/pdsGoalCalculator
• Fill in the Setup Section fully
• Go to the Support Item step (which we need to rename)

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-9367.d3dytjb8adxkk5.amplifyapp.com

[github-actions]

Bundle sizes [mpdx-react]

Compared against d2abb90

Route	Size (gzipped)	Diff
/accountLists/[accountListId]/reports/pdsGoalCalculator/[pdsGoalId]	324.26 KB	+3.49 KB

[zweatshirt]

🤖 Multi-Agent Code Review Report

PR: #1721 — MPDX-9367 - PDS Calc Salary Subtotal section
Branch: MPDX-9367 → main
Reviewer: AI Multi-Agent Review (Standard Mode)
Agents: 5 of 7 (Architecture, Testing, Standards, UX, Financial — Opus)
Cost: ~$3 | Time: ~6 min

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

📊 RISK ASSESSMENT

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Metric	Value
Risk Level	HIGH — financial code, experienced reviewer+ should approve
Risk Score	~13 (components +10, volume +3)
Files Changed	15 (+681 / -19)
Day	Thursday (safe to land)

Risk Factors

• Financial code: salary / FICA / geographic multiplier math (Financial agent triggered)
• 10 non-test files under src/components/Reports/PdsGoalCalculator/** (Medium-Risk +1 each)
• 700-line change in a single feature area (volume +3, scope 1.0×)
• No auth / API / GraphQL schema / Apollo cache changes — Security and Data Integrity agents correctly skipped
• 1 high-impact file: PdsGoalCalculatorTestWrapper.tsx (17 dependents — test-only behavior change)

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

🎯 TL;DR

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

No blockers. The core salary math is correct, tests are thorough, standards compliance is clean (no any, no console.log, no hardcoded strings, named exports throughout, i18n interpolation done right, test coverage colocated and typed).

Top concerns (consensus across 2-3 agents):

1. Silent empty state when FICA constant / calculation is loading (severity ~6.3/10, 3 agents)
2. Null payRate silently renders a $0 breakdown that looks like valid data (6/10, Financial)
3. Financial test precision — toBeCloseTo bounds too loose to catch penny-level regressions (6/10, Testing)
4. mergeWith array-replace customizer in shared test wrapper silently changes array-merge semantics for 17 test consumers (6/10 Testing, 4/10 Architecture)
5. Potential 1¢ reconciliation drift between displayed Subtotal and (displayed Gross + displayed FICA) (6/10, Financial)

None of the above block merge. Address (1)–(3) before merging; (4) and (5) can ship with follow-up tickets.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

🚫 CRITICAL BLOCKERS (Severity 9-10)

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

None. Core math audited clean against prior art in src/components/Reports/GoalCalculator/Shared/calculateNewStaffTotals.ts:144,210.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

⚠️ IMPORTANT ISSUES (Severity 6-8)

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

1. Silent empty state when constants or calculation are loading / missing

Consensus severity: 6.3/10 · Flagged by Financial (7), Architecture (6), UX (6)
Files: SalarySection.tsx:47-52, SalarySection.tsx:75-77

Problem: useGoalCalculatorConstants() exposes loading and error, but neither is consumed. When employerFicaRate === undefined (constants loading, error, or truly missing) or when calculation is null (calc query in flight), rows.length === 0 and the whole component returns null — no heading, no skeleton, no error alert.

SalaryStep is literally <SalarySection />, so the user navigates to the Salary step and sees a blank panel with no explanation. This is the "zero-state rendered as nothing" pattern the CLAUDE.md UX rules call out: "every Apollo query must render a loading state … not render nothing or flash stale content."

Recommendation:

• Destructure loading and error from useGoalCalculatorConstants.
• While loading: render <Typography variant="h6">{t('Salary')}</Typography> + MUI <Skeleton>.
• On error or definitively missing employerFicaRate after load completes: render the heading + <Alert severity="error">{t('Salary calculation is temporarily unavailable')}</Alert>.
• Optionally lift this to PdsGoalCalculatorLayout so every step benefits — MonthlyReimbursableSection has the same pre-existing gap.

---

2. Null payRate silently renders a $0 salary breakdown

Consensus severity: 6/10 · Flagged by Financial (6)
Files: salaryCalculation.ts:23-24, SalarySection.tsx

Problem: payRate ?? 0 means before a user enters pay, the Salary step shows $0 Monthly Base / Gross / FICA / Subtotal — indistinguishable from "pay rate of zero." A missionary could print or export a sheet with a $0 salary goal and believe it reflects reality. The CLAUDE.md Financial Reporting rules explicitly call out this pattern: "A report with zero donations should render 'no data' — not $0.00 that looks like real data."

Recommendation: In SalarySection.tsx, gate the breakdown on calculation.payRate != null. If null, render an empty-state panel prompting "Enter your pay rate in Setup to see your salary breakdown." Keep the ?? 0 in calculateSalaryTotals as defense-in-depth — don't propagate it to the view.

---

3. toBeCloseTo precision too loose for money

Consensus severity: 6/10 · Flagged by Testing (6)
Files: salaryCalculation.test.ts:38-42,58-64

Problem: toBeCloseTo(5300) with the default 2 means ±$0.005 — a penny-level regression in grossMonthlyPay would pass silently. toBeCloseTo(4333.333, 2) has the same fuzz. These math paths are deterministic JS floating-point; assertions should be exact.

Recommendation:

• Where the expression has no floating-point remainder (60000/12 = 5000, 5000*0.08 = 400, (60000/12)*1.06 = 5300): use toBe(5000), toBe(400), toBe(5300).
• Where there is a remainder ((25*40*52)/12): either toBeCloseTo(..., 5) or compute with the same expression to get byte-identical floats.

---

4. mergeWith array-replace customizer — silent cross-test behavior change

Consensus severity: 5/10 · Flagged by Testing (6), Architecture (4)
Files: PdsGoalCalculatorTestWrapper.tsx:152-182
Dependency blast radius: 17 consumers of this wrapper (dependency analysis confirmed).

Problem: Switching from merge (array-concat) to mergeWith with an Array.isArray(srcValue) ? srcValue : undefined customizer (array-replace) changes how constantsMock composes against the defaults. The new default seeds an EMPLOYER_FICA_RATE: 0.08 entry in mpdGoalMiscConstants; any test that overrides mpdGoalMiscConstants now fully replaces that default, whereas merge previously would have concatenated. Silent future-footgun: a later test overriding mpdGoalMiscConstants for a different category will lose the FICA seed without realizing it.

Recommendation:

• Add a comment at the wrapper documenting the array-replace behavior (especially that the default FICA seed is dropped when you pass mpdGoalMiscConstants: [...]).
• Or split the default constants into an exported helper (e.g., DEFAULT_MPD_GOAL_MISC_CONSTANTS) so tests can explicitly [...DEFAULT_MPD_GOAL_MISC_CONSTANTS, yourExtraConstant].
• The test suite itself passes today (spot-checked MonthlyReimbursableSection.test.tsx, PdsGoalsList.test.tsx — neither reads the FICA constant), so this is prevention, not a broken test.

---

5. Potential 1¢ display reconciliation drift (Gross + FICA vs. Subtotal)

Consensus severity: 6/10 · Flagged by Financial (6)
Files: salaryBreakdown.tsx, salaryCalculation.ts

Problem: Subtotal renders currencyFormat(subtotal) where subtotal = grossMonthlyPay + employerFica (both un-rounded). Gross and FICA rows each render their own values independently via currencyFormat. For inputs whose un-rounded decimals straddle a cent boundary, users can see displayed(gross) + displayed(fica) ≠ displayed(subtotal) by $0.01. Finance-sensitive users do verify column math. The current tests happen to reconcile (4333.333… + 346.666… = 4680.000) — different inputs will not.

Recommendation: Decide a convention and document it. Either:

• (A) Round to cents once at the display boundary per row, then sum the rounded values for the Subtotal (guarantees visual reconciliation, slight loss of precision).
• (B) Keep raw-sum Subtotal and add a property-based or fuzz test asserting |displayed(gross) + displayed(fica) - displayed(subtotal)| ≤ $0.01 across varied fractional inputs.

---

6. calculations/ folder vs. sibling Shared/ precedent

Consensus severity: ~5/10 · Architecture (6) vs Standards (✅ coherent) — mild disagreement

Files: src/components/Reports/PdsGoalCalculator/calculations/

Problem (Architecture view): GoalCalculator/Shared/ houses calculateTotals.ts, calculateNewStaffTotals.ts, etc. — the established pattern is "pure math helpers live in Shared/." This PR introduces a third convention: calculations/ as a new sibling. Now there are three places to look for calculator math (feature-local, Shared/, calculations/).

Standards counterpoint: calculations/ is a coherent sub-organization under a feature folder — CLAUDE.md leaves sub-organization to the feature — and the precedent is thin because it's new. Not a violation, just a pattern choice.

Resolution: Not blocking. The current layout is defensible, but if retained, consider migrating PdsGoalCalculator/Shared/ helpers into calculations/ in a follow-up so there's one place to find math per feature. Alternatively, move salaryCalculation.ts and reimbursableExpenses.ts into the existing Shared/ to match GoalCalculator. Document the choice.

---

7. SalaryConstants interface lives in view module, not calc module

Consensus severity: 6/10 · Flagged by Architecture (6)
Files: salaryBreakdown.tsx:30-33, salaryCalculation.ts

Problem: SalaryConstants = { geographicMultiplier, employerFicaRate } describes inputs to calculateSalaryTotals but is declared in the view file. If a future change adds a third constant, calculateSalaryTotals (positional args) and SalaryConstants must be updated in lockstep — with no compile-time link between them.

Recommendation: Move SalaryConstants into calculations/salaryCalculation.ts and refactor calculateSalaryTotals(calc, geo, fica) → calculateSalaryTotals(calc, constants: SalaryConstants). Gives you one source of truth and a type-checked contract.

---

8. Visual inconsistency with TotalReimbursableSection hero pattern

Consensus severity: 6/10 · Flagged by UX (6)
Files: SalarySection.tsx, TotalReimbursableSection.tsx

Problem: Reimbursable totals render as a bold 2.5rem hero number inside a Card. Salary uses a DataGrid with the Subtotal as "just another row" (bold + top border). On a combined goal page, this creates uneven visual hierarchy: the reimbursable total reads as headline, the salary subtotal does not.

Recommendation: Either (a) wrap SalarySection in a Card to match, (b) render Subtotal as a separate hero below the grid, or (c) confirm with design that this asymmetry is intentional. Flag for designer/PM sign-off — this is a design decision, not a code bug.

---

9. Mobile layout collision

Consensus severity: 6/10 · Flagged by UX (6)
Files: salaryBreakdown.tsx:122-138

Problem: Columns set minWidth: 200 + minWidth: 140 = 340px + DataGrid chrome. On common mobile widths (320-375px) the grid horizontal-scrolls, and formula subtext wraps awkwardly. No theme.breakpoints.down('sm') adaptation.

Recommendation: Either lower the minWidths (category 160, amount 100) or add a breakpoint-specific stacked layout. Verify on a 375px viewport before merge.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

💭 SUGGESTIONS (Severity 3-5)

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Financial

• Add missing edge-case tests (3/10): negative payRate, Number.MAX_SAFE_INTEGER, employerFicaRate = 0 (vs. undefined), hourly with 0 hours (vs. null hours), salaried with explicit 0 payRate.
• Hardcoded USD in currencyFormat(amount, 'USD', locale) (3/10): consistent with the rest of PdsGoalCalculator (US-only). Add a short comment noting "PDS salary is always USD" to prevent future drift.
• id: 'total' vs. label 'Subtotal' vs. testId 'salary-subtotal' (3/10): align to 'subtotal' throughout for consistency (also requires updating the two CSS / getRowClassName selectors in SalarySection.tsx).
• Hourly-to-monthly formula comment (3/10): * 52 / 12 is industry-standard but has no intra-repo precedent. A short comment referencing the MPDS business rule would help future maintainers.

UX

• Raw fontSize: '1rem' / '0.8125rem' (3/10): prefer theme.typography.body1.fontSize / body2.fontSize for theme alignment.
• Unicode math glyphs (÷ × ½) in translation source (low): travel through CrowdIn OK but ½ in "Employer ½ FICA" is fragile across locales. Consider ASCII (1/2) or a translator note.
• Heading semantics (3/10): <Typography variant="h6" component="h2"> to give screen-reader users explicit heading-level semantics.

Testing

• Pin testId presence on load-bearing rows (gross-monthly-pay, employer-fica, salary-subtotal) so SalarySection.test.tsx doesn't silently break if a testId is dropped in salaryBreakdown.tsx.
• Add assertion that SalaryStep renders SalarySection's grid (e.g., findByRole('grid', { name: 'Salary breakdown' })) — the current one-line pass-through isn't asserted.
• Test the top-border classname application for gross-monthly-pay and total rows.

Architecture / Standards

• SalaryStep.tsx is a one-line pass-through (3/10): consider folding into SalarySection, or document why the step wrapper exists (likely for useSteps wiring). Not blocking.
• salaryBreakdown.tsx camelCase-with-JSX (3/10): defensible under the "utility module that returns JSX" convention — Standards agent confirmed precedent (createRows.tsx, createTableRow.tsx, styledComponents.tsx, etc.). Minor aesthetic preference; not a violation.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

📦 DEPENDENCY IMPACT

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

🚨 CRITICAL IMPACT — 1 file

• PdsGoalCalculatorTestWrapper.tsx has 17 dependent test files. The merge → mergeWith behavior change affects every consumer's array-merge semantics. See Important Issue Bump axios from 0.20.0 to 0.21.1 #4.

✅ Breaking changes to exports: none.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

📝 REVIEW SUMMARY

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Agent	Critical	Important	Suggestions	Confidence
💰 Financial	0	3	4	High
🏗️ Architecture	0	3	4	High
🧪 Testing & Quality	0	2	5	High
👤 UX	0	3	4	High
📋 Standards	0	0	3	High
Total (de-duplicated)	0	9	~15	High

Checklists

Financial:

• Arithmetic safe (no mid-calc rounding): ✅
• Currency mixing prevented: ✅ (single-currency USD domain)
• Rounding at display boundary only: ⚠️ (Subtotal raw vs. rounded row values — Issue Move to Yarn V2 and upgrade packages #5)
• Null/undefined amounts handled: ⚠️ (see Issue contacts #2)
• Luxon (not new Date()): N/A
• Server-provided aggregations preferred: ✅ (no server-side field exists)
• Geographic multiplier convention matches codebase: ✅ (matches calculateNewStaffTotals.ts:144)
• Yearly→monthly propagation to all salary consumers: ✅

Standards:

• File naming: ✅ · Exports: ✅ · GraphQL: N/A
• Localization: ✅ · TypeScript: ✅ · Testing: ✅
• Code quality (no console.log / any / TODO / commented code / new Date()): ✅
• Package management: N/A · Folder pattern: ✅ (with Architecture caveat)

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

🎯 RECOMMENDED NEXT STEPS

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Before merge (fix or consciously defer):

• Address loading/error/empty states — don't let the Salary step silently disappear (Issue Tasks #1)
• Gate the breakdown on payRate != null — don't render $0 as if valid data (Issue contacts #2)
• Tighten financial test precision — toBe for exact expressions, toBeCloseTo(..., 5) for remainders (Issue add notifications #3)
• Smoke-test on a 375px viewport to confirm mobile layout is acceptable (Issue Bump @storybook/addon-viewport from 6.1.17 to 6.1.18 #9)
• Designer/PM sign-off on visual hierarchy vs. TotalReimbursableSection (Issue MPDX-6917-Remove-Lodash #8)

Follow-up ticket candidates (can ship):

• Document mergeWith behavior in PdsGoalCalculatorTestWrapper and/or export a default-constants helper (Issue Bump axios from 0.20.0 to 0.21.1 #4)
• Decide and document Subtotal rounding convention; add regression test (Issue Move to Yarn V2 and upgrade packages #5)
• Move SalaryConstants into calculations/salaryCalculation.ts (Issue Make prettier config closer to defaults #7)
• Reconcile calculations/ vs. Shared/ as the canonical home for calculator math (Issue Remove ts-node #6)

Merge decision: ✅ Approve with changes. No blockers. The PR is well-structured, well-tested, and standards-compliant.

---

Review by AI Multi-Agent System v3.0. Debate rounds skipped — agent findings converged cleanly with no material contradictions. Standard mode · 5 of 7 agents · ~$3 · ~6 min. Generated by /agent-review skill.

[zweatshirt]

@wjames111 Sorry to add you to review, this is "critical" according to /quality:agent-review

[wjames111]

Looks great! Let me know once this merges.