[MPDX-9536, MPDX-9570, MPDX-9572, MPDX-9573] - PDS fix calculations

[wjames111]

Description

This work aims to fix some of the calculations we came across in our review Monday. I've listed out the following tickets and their descriptions:
MPDX-9536 - Fix Goal Amount in the Goal Card. This number should match what's inside the form
MPDX-9570 - Bug showing 600% on Credit Card Fees. This appears to fixed although I don't believe I made changes to address it. Either someone already fix, or there was a weird merge conflict that caused the 600 bug.
MPDX-9572 - switch order of attrition, credit card fees, and assessment so it matches the order they show in the table. This has been corrected, but the formula itself has been updated as well so the label now reflects the updated formula.
MPDX-9573 - Assessment is wrong calculation. I triple checked the math, it should be correct based on the formula provided in the spreadsheet. However the number does not match the spreadsheet even though it should, I believe something is not working correctly in the spreadsheet.

Testing

• Go to PDS GC
• Check each ticket is fixed

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]

Bundle sizes [mpdx-react]

Compared against f8f8712

No significant changes found

[wjames111]

Multi-Agent Code Review — PR #1774

Mode: standard · Agents: 5 specialized + 1 gap-review (Opus)

Verdict: BLOCKERS_FOUND — 1 critical issue must be resolved before merge.

Top blockers

• F1 (sev 9.5): salaryCalculation.ts:34 and pdsGoalConstants.ts:50 reinterpret goalGeographicConstantMap as multiplicative (1.06), but the sibling calculateNewStaffTotals.ts:144 still reads the same field as additive ((geographicMultiplier + 1) * base, default 0). Same backend field — two interpretations. Only one matches production data; the other is silently wrong by ~94% of salary.

Risk Assessment

• Risk Score: 7/10 · Risk Level: HIGH · Required Reviewer: SENIOR
• Financial calculation semantics change for HR goal totals. A misunderstanding silently mis-states staff support targets.

Findings summary

Tier	Count
Critical Blocker (9.0-10.0)	1
High Priority Blocker (8.0-8.9)	0
Important (7.0-7.9)	3
Medium (5.0-6.9)	5
Suggestions (<5.0)	5

See inline comments for line-anchored details and recommended fixes.

---

Findings on Related Files (Not in This PR)

These findings were surfaced during F1 investigation. They are on files outside the PR diff and cannot be posted as line comments. They are informational — they do not count toward the blocker count.

[Critical 9.5/10] src/components/HrTools/GoalCalculator/Shared/calculateNewStaffTotals.ts:127-144 — Still uses additive convention (geographicMultiplier + 1) * base with ?? 0 default. Must be reconciled with whichever convention wins F1. If the multiplicative semantic ships (this PR's choice), this file silently doubles compensation for any New Staff goal with a known geographic location.

[Medium 6.0/10] src/components/HrTools/GoalCalculator/GoalCalculatorTestWrapper.tsx:191 — Test fixture still uses additive value percentageMultiplier: 0.06. Must be updated alongside any convention decision.

[Medium 6.0/10] src/hooks/useGoalCalculatorConstants.test.tsx:42,48 — Test fixture uses additive values (0 and 0.12). Same.

[Suggestion 3.0/10] src/components/HrTools/PdsGoalCalculator/SupportItem/salaryBreakdown.test.tsx:144 — Stale formula string 'Pay Rate × (1 + Geographic Multiplier)' no longer matches any production formula. (Line is unchanged context, not in the diff, so not flagged inline.) Update to 'Monthly Base × Geographic Multiplier'.

[Suggestion 3.5/10] Test file rename inconsistency — buildPdsGoalConstants.test.ts was not renamed to pdsGoalConstants.test.ts to match the source rename. Pre-existing.

[Suggestion 3.0/10] src/components/HrTools/PdsGoalCalculator/calculations/pdsGoalConstants.ts:10 — PdsGoalTotalConstants interface name is stale after calculatePdsGoalTotal was removed; consider renaming to PdsGoalConstants. (Line 10 is unchanged context, not in the diff.)

---

Cross-cutting consistency check

Operation: "compute geographic salary adjustment"

Code path	Default	Formula	Convention
PdsGoalCalculator/.../salaryCalculation.ts:34	?? 1	monthlyBase * geographicMultiplier	Multiplicative
GoalCalculator/Shared/calculateNewStaffTotals.ts:144	?? 0	(geographicMultiplier + 1) * base	Additive

Both read from the same goalGeographicConstantMap populated by useGoalCalculatorConstants.ts:50-54. Resolution requires backend confirmation of what mpdGoalGeographicConstants.percentageMultiplier actually returns, then aligning whichever calculator is wrong (and the three test fixtures noted above).

Architecturally preferred resolution: Normalize at the hook boundary in useGoalCalculatorConstants.ts:50-54 and use a branded type (type GeographicFactor = number & { __brand: 'GeographicFactor' }) so downstream calculators cannot reinterpret it silently.

---

Generated by /quality:agent-review (standard mode, Opus). Reply /dismiss: <reason> on any non-blocking comment (severity < 7) to acknowledge.

[wjames111]

Multi-Agent Code Review — PR #1774 (re-review)

Mode: Standard · Agents: 6 specialized reviewers + 1 consolidated debate round
Risk: 6/10 (MEDIUM) · Verdict: BLOCKERS FOUND

Verdict summary

2 critical blockers must be resolved before merge. Both were independently flagged by 4–5 of the 6 agents with high confidence and concrete code/math citations.

1. Geographic-multiplier convention split (sev 9.5) — PdsGoalCalculatorTestWrapper.tsx:197,201,205 mocks percentageMultiplier: 1, 1.06, 1.12 (multiplicative) but salaryCalculation.ts:34 (unchanged) still computes monthlyBase * (1 + geographicMultiplier) (additive). Every other test fixture and consumer in the repo (6 separate sites) uses the additive convention. Latent bug: any test exercising geographicLocation: 'None' against this wrapper silently produces doubled salary.
2. Credit Card Fees display formula mismatch (sev 9.0) — otherBreakdown.tsx:119-121 still shows (Subtotal + Attrition) × {{rate}} but OtherExpenses.ts:57-59 was changed to a gross-up. The displayed formula no longer matches the rendered number (~$31 discrepancy on the canonical $7800 subtotal example). The Assessment row's display formula was updated in the same hunk; Credit Card Fees was missed.

Risk factors

• 14 files under src/components/HrTools/PdsGoalCalculator/ (Medium-Risk per repo rules)
• Financial calculation domain — formula churned 4× across branch history
• Previous /agent-review on this PR flagged a severity 9.5 convention-split bug; this re-review finds it has been re-introduced in a different location (test wrapper) rather than resolved

Math verification

• Gross-up identities verified algebraically — creditCardFees / (subtotal + attrition + creditCardFees) === r and assessment / (adminBase + assessment) === adminRate. Implementation correctness ✓
• PdsGoalCard.test.tsx:20 expected $8,073.02 reproduces exactly: subtotal $6300 × 1.06 / (0.94 × 0.88) = $8,073.02. Not a magic number ✓
• usePdsSummaryData.test.ts:319 expected 9995.16 reproduces exactly with the new gross-up formulas ✓

Findings overview

Severity tier	Count	Issues
Critical (9.0-10)	2	F1 convention split, F2 CC fees display drift
High (8.0-8.9)	0	—
Important (7.0-7.9)	0	—
Medium (5.0-6.9)	2	F3 lost test coverage, F6 opaque gross-up display
Suggestion (<5.0)	5	F4 redundant subscription, F5 missing rate guards, F7/F8/F9 display polish

Agent summary

Agent	Critical	Important	Medium	Suggestions
Architecture	1	3	—	2
Data Integrity	2	1	—	2
Testing	0	3	—	4
UX	1	3	—	3
Financial Reporting	1	2	—	1
Standards	0	0	—	0 (all PASS)

Cross-cutting findings

• Fix one, fix all violation: developer updated Assessment label gross-up but missed Credit Card Fees label (identical math structure, adjacent row). F2 above.
• Display layer needs a coordinated pass: F2 (blocker) + F6 + F7 + F8 + F9 all touch otherBreakdown.tsx / salaryBreakdown.tsx formula rendering. Treat F2 as blocking; consolidate the rest into a polish ticket.

Recommendations

1. Revert the test wrapper to additive convention (0, 0.06, 0.12) — minimum fix for F1.
2. Update the Credit Card Fees display formula to mirror the Assessment row's ÷ {{divisor}} − ... pattern — fix for F2.
3. Add a part-time-hourly overallTotal test to usePdsSummaryData.test.ts to restore coverage lost when calculatePdsGoalTotal.test.ts was deleted.
4. Optional follow-up: document MpdGoalGeographicConstant.percentageMultiplier semantics in the GraphQL schema (the field name implies multiplicative but the consumer treats it as additive — naming is misleading).

Open questions for human review

• The PR description for MPDX-9573 notes the assessment number "does not match the spreadsheet even though it should" — a finance reviewer should validate the gross-up math against the source-of-truth spreadsheet before merge.
• The geographic-multiplier convention has been flipped 4× in the branch history. Consider a one-line schema comment to permanently document the contract.

[wjames111]

Multi-Agent Review — PR #1774

Verdict: BLOCKERS_FOUND — 1 high-priority blocker plus 1 important issue must be resolved (or human-acknowledged) before merge.

Risk: HIGH (7/10) — financial-math change with display-side impact. The PR has two PR-level wins (correct gross-up math; consolidating goal-card display onto the same usePdsSummaryData source as the rest of the calculator). Net architectural debt is significantly reduced.

Agents: 5 (Architecture, Testing, Standards, UX, Financial Reporting). Debate round resolved 4 challenges and merged 2 duplicate findings.

---

Top-of-Pile (one-liners)

1. [High 8.0] Long gross-up formula labels will clip in the salary/other breakdown grids. Comment posted at otherBreakdown.tsx:120.
2. [Important 7.5] Goal card total jumped ~9.5× ($849.44 → $8,073.02). This is a genuine bug fix (old code showed only the assessment line item; new code shows full overallTotal), but staff and downstream consumers need to know. Comment posted at PdsGoalCard.tsx:29.

---

High Priority Blocker (sev 8.0)

Severity: 8.0/10 — Flagged by: UX (8.0), Architecture (confirmed via codebase read)

[High 8.0] otherBreakdown.tsx:120 / 133 + salaryBreakdown.tsx:79 — Long formula labels will clip in the data grid.

The new gross-up formula strings are ~58-100 characters (vs. ~30 for the old strings). The category cell stacks category over formula in a flex column inside MUI X DataGrid's default fixed rowHeight={52}. Neither SalarySection.tsx:47 nor OtherSection.tsx:48 (both non-PR files) sets getRowHeight={() => 'auto'}. The longest formula — (Subtotal + Attrition + Credit Card Fees) ÷ (1 − 12%) − (Subtotal + Attrition + Credit Card Fees) — will wrap to 2-3 lines on desktops < ~1100px wide and on mobile/tablet, and the third line will clip.

Recommended fix (one line in each non-PR container file):

// SalarySection.tsx and OtherSection.tsx (BOTH are outside this PR's diff)
<StyledGrid
  // ...existing props
  getRowHeight={() => 'auto'}
  // ...
/>

Alternatively, shorten the in-PR formula labels (use a friendlier label in the cell + the verbose math in a tooltip — see Dismissed Findings F6 below; the author has already pushed back on that direction for this PR).

---

Important Issue (sev 7.5)

Severity: 7.5/10 — Flagged by: Financial (7.5), UX-3 (7.0, merged)

[Important 7.5] PdsGoalCard.tsx:29 — Goal Amount display jumps ~9.5× on existing data.

The old calculatePdsGoalTotal function returned only otherExpenses.assessment (one line item). The new code reads summaryData.overallTotal = subtotal + attrition + creditCardFees + assessment (the full funding goal). This is the correct fix — the field was always labelled "Goal Amount" but was previously displaying only the admin-assessment line. Math verified algebraically: with payRate=60000 and the test fixture, the new value is $9,995.16 vs. the previously-displayed assessment-only $1,199.42.

Action required (non-code):

• Confirm with the business owner that the goal card was always intended to show the full funding goal.
• Search for downstream consumers of goalTotal / overallTotal (other dashboards, exports, MCC/SOSA aggregations) that may be calibrated against the previous (buggy) value.
• Add release notes / in-product callout: staff will see their goal jump ~9.5× and reasonably suspect the calculator is broken.

Comment posted at the line.

---

Medium Priority (sev 5.0-6.9)

Line-level comments posted on each. Summary:

Sev	Finding	File:Line
6.0	Asymmetric test coverage: no creditCardFees === 0 test when rate = 0 (the sibling assessment test has one)	OtherExpenses.test.ts:142
6.0	SalaryBreakdownRow lacks the tooltip field that OtherBreakdownRow has. UX-2 (dismissed) and UX-4 (below) both want this — the affordance is now load-bearing.	salaryBreakdown.tsx (interface)
5.5	Stale formula text in test fixture (salaryBreakdown.test.tsx:140, 147) — still uses 'Pay Rate × (1 + Geographic Multiplier)'. Plus: new production formula strings have no test coverage. Lines 140/147 are outside this PR's diff — see body, not a line comment.	salaryBreakdown.test.tsx:140,147 (not in diff)
5.5	Monthly Base × {{rate}} renders as Monthly Base × 100% when geographicMultiplier=0 (default/unknown location) — noise, not signal. Conditionally render the formula.	salaryBreakdown.tsx:79
5.0	Missing tests for null payRate, null hoursWorkedPerWeek, and geo-key-with-null-multiplier paths in usePdsSummaryData. Gross-up (x)/(1−r) will NaN-cascade.	usePdsSummaryData.test.ts

---

Suggestions (sev < 5.0) — informational, do not block verdict

• [Suggestion 4.5] PdsGoalCard.test.tsx:20 — $8,073.02 is not algebraically derivable; depends on graphql-ergonomock seeded RNG for the 9 reimbursable fields the wrapper doesn't pin. Cheap fix: pin the reimbursable fields in PdsGoalCalculatorTestWrapper.tsx. Comment posted.
• [Suggestion 4.5] usePdsSummaryData.test.ts:334 — Part-time test only asserts > 0. The deleted test had a toBeCloseTo(434.83, 0) numeric pin. Add an exact value. Comment posted.
• [Suggestion 4.5] otherBreakdown.tsx:133 — Display formula reads (Subtotal + Attrition + Credit Card Fees); code computes subtotal + creditCardFees + attrition. Mathematically equivalent but a parity-hygiene issue: code-vs-display drift is how silent bugs land. Align order. Comment posted.
• [Suggestion 4.5] otherBreakdown.tsx:120 — Add an inline comment near the gross-up display strings explaining that the algebraic identity x ÷ (1−r) − x is intentional and mirrors OtherExpenses.ts. Prevents a future maintainer from "simplifying" back to x × r (reintroducing the bug this PR fixes).
• [Suggestion 4.5] usePdsSummaryData.test.ts — No test asserts that a non-zero geographicMultiplier actually flows into overallTotal. Add a withGeo > withoutGeo test. The deleted file had this; the new file lost it.
• [Suggestion 4.0] PdsGoalCard.tsx:24 — Redundant useGoalCalculatorConstants() call: usePdsSummaryData invokes it internally. Cheap (cache-first) but architecturally noisy and creates a small race where constantsLoading=false but summaryData=null. Keystone fix: expose loading from usePdsSummaryData and drop this hook from the component. Comment posted.
• [Suggestion 4.0] Add an end-to-end hand-verifiable usePdsSummaryData test with a fully zeroed reimbursable payload so the math is reproducible from comments (no ergonomock dependency).
• [Suggestion 4.0] A11Y-1: Removing the geographic-multiplier row changes the salary table structure; confirm no other view/aria-relationship depends on this row.
• [Suggestion 4.0] Three new translation keys are not yet in public/locales/en/translation.json and one old key ('Monthly Base × (1 + Geographic Multiplier)') is now orphaned. Run yarn extract before merge so translators (Crowdin) see the new strings. Translation file is not in this PR's diff.
• [Suggestion 3.5] A11Y-2: Unicode − (U+2212, true minus) is new to this codebase; screen-reader pronunciation varies. The tooltip-affordance approach (UX-2, dismissed) would side-step this. Low risk; keep the character for visual parity with ÷ and ×.
• [Suggestion 3.0] PdsGoalCard does not communicate "summary unavailable" — when buildPdsGoalConstants returns null post-load, the card renders $0.00 indistinguishable from a real zero goal. (Pre-existing — flagged for visibility; the consolidation in this PR makes it slightly more invisible.)
• [Suggestion 3.0] Removal of consistency with calculatePdsGoalTotal test block — appropriate (the second source-of-truth function is gone), but the structural cross-check safety net is permanently retired. Informational.

---

Pre-existing Issues (Informational)

• [Pre-existing, sev 4.0] PdsGoalCard.tsx — summaryData == null && !loading collapses silently to $0.00. Not introduced by this PR; documented above as a Suggestion.
• [Pre-existing, sev 2.5] A11Y-3 — InfoIcon tooltip in otherBreakdown.tsx:163-167 isn't wrapped in an IconButton, so keyboard users can't focus it. Pre-dates the PR.

---

Dismissed Findings (Acknowledged by Developer)

These were flagged again by the relevant agents in this review pass but match prior /dismiss replies. They do not count toward the verdict.

• [Dismissed, sev 5.5] otherBreakdown.tsx:137 — Gross-up formulas opaque to non-finance users (UX-2, this run). Dismissed by @wjames111: "might revisit this later. Right now getting the formula correct and keeping parity between calculations and the formula displays is the priority." Financial agent supports this prioritization (literal-math display is more auditable than a friendlier rewording).
• [Dismissed, sev 2.0] OtherExpenses.ts:63 — No guards on rate ≥ 1 for gross-up divisions (Financial #4, this run, conceded in debate down from 4.5). Dismissed by @wjames111: "this is the general direction the codebase has taken." Confirmed by codebase scan — the sister GoalCalculator/Shared/calculateTotals.ts:92 follows the same pattern.

---

Cross-Cutting Checks

• "Fix one, fix all": Architecture and Financial both verified src/components/HrTools/GoalCalculator/Shared/calculateTotals.ts:92 already uses the gross-up pattern. The PDS calculator is being brought into consistency with the (correct) sister — no parallel-fix gap.
• Safeguard parity: N/A — no auth/validation/data-mutation operations changed.
• Dependency impact: Architecture verified no callers of the deleted calculatePdsGoalTotal remain. Safe deletion.

---

Review Summary Table

Agent	Blocker	Important	Medium	Suggestions	Confidence
Architecture	0	0	0	6	High
Testing	0	0	2	7	Medium-High
Standards	0	0	1	1	High
UX	1	0	2 (after dismissals)	7	High
Financial Reporting	0	1	1 (after dismissals)	3	High
Active total	1	1	5	11

---

Generated by /quality:agent-review (multi-agent cross-examination review).

[wjames111]

**[High] Long gross-up formula labels will clip in the data grid.** — sev 8.0/10

Flagged by: UX (8.0), Architecture (rendering-contract confirmation).

The new formula here grows from ~30 chars to ~58 chars, and the Assessment formula at line 133 reaches ~100 chars. The category cell at styledGrid.tsx:27-36 stacks the category label over the formula in a flex column. MUI X DataGrid defaults to rowHeight={52} and neither SalarySection.tsx:47 nor OtherSection.tsx:48 (both non-PR files) override it with getRowHeight={() => 'auto'}. The longest formula will wrap to 2-3 lines on viewports < ~1100px and on tablet/mobile, then clip at the third line.

Cheapest fix (one line per non-PR container file):

// in SalarySection.tsx and OtherSection.tsx
<StyledGrid
  // ...existing props
  getRowHeight={() => 'auto'}
  // ...
/>

Alternative: shorten the in-PR formula labels — see Dismissed Findings F6 in the PR body (author has already pushed back on rewording for this PR scope; the container-level fix is the lower-friction path).

[wjames111]

**[Medium] Part-time test assertions are weak compared to the deleted equivalent.** — sev 5.0/10

Flagged by: Testing #2 (5.5), expanded to cover other null paths.

The deleted calculatePdsGoalTotal.test.ts asserted toBeCloseTo(434.83, 0) on a specific computed part-time total. The replacement here only checks overallTotal > 0, workComp > 0, and benefits === 0. A formula regression on the part-time branch (e.g., flipping the workComp sign, missing the gross-up, wrong constants) would still satisfy these assertions.

Add the numeric pin:

// Hand-derivation (verify against your wrapper constants):
// grossMonthlyPay = (25 * 20 * 52) / 12 = 2166.67
// employerFica ≈ 173.33; salarySubtotal ≈ 2340
// reimbursable monthly + annual / 12 ≥ floor 300 (depends on wrapper defaults)
// 403b = grossMonthlyPay * 0.08 ≈ 173.33
// workComp = grossMonthlyPay * 0.17 ≈ 368.33
// subtotal ≈ ... attrition ≈ ... ccFees = (.)/(.94) - .; assessment = .
expect(result.current?.overallTotal).toBeCloseTo(<computed>, 0);
expect(result.current?.otherTotals.workComp).toBeCloseTo(<computed>, 1);
expect(result.current?.otherTotals.benefits).toBe(0);

Also add (this describe block is the right home):

1. null payRate test — the gross-up (x)/(1-rate) would NaN-cascade if payRate=null isn't guarded all the way through.
2. geographic multiplier flowthrough — withGeo.overallTotal > baseline.overallTotal (the deleted file had this; the new file lost it).

[github-actions]

Preview branch generated at https://MPDX-9536.d3dytjb8adxkk5.amplifyapp.com