MPDX-9904 - Include 403b values and special needs left in NS Goal admin preview - #1989
Conversation
Bundle sizes [mpdx-react]Compared against 5f0edc3 No significant changes found |
zweatshirt
added
the
Preview Environment
|
Preview branch generated at https://MPDX-9904.d3dytjb8adxkk5.amplifyapp.com |
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review
Verdict: APPROVED WITH SUGGESTIONS — 0 critical, 0 high, 1 Important (7.5), ~14 Medium/Suggestion after dedup.
6 specialized agents (Architecture, Testing, Standards, Data Integrity, UX, Financial Reporting). Security agent not launched — no trigger matched (no pages/api/**, auth, Apollo link/client, env, or workflow changes).
Risk: 6/10 MEDIUM (reviewer override, recorded). The pattern-derived pre-review score was 8/10 HIGH; that score is computed from file globs and line count before any code is read, and it misdescribes this change. The +2 High tier on src/components/**/*.graphql exists for cache-normalization and pagination risk, which three additive scalar fields under fetchPolicy: 'no-cache' do not carry (verified: the preview payload's wire __typename is NewStaffGoalCalculationPreview, no typePolicy exists for either type, and NewStaffGoalCalculationCalculations has no id so it cannot be normalized independently). NsGoalCalculatorTestWrapper.tsx also scored as a feature file because the Low-Risk glob is **/*.test.{ts,tsx} and a test wrapper does not match it. Follow-up owed: amend .claude/rules/code-review.md so this is fixed for future PRs rather than overridden per-PR.
Verified clean
yarn lint:ts exit 0 · yarn eslint (no --fix) exit 0 · yarn prettier --check clean (3.6.2) · codegen freshness proven from generated content (*.generated.ts is gitignored) · all three new fields confirmed Float, null: false in calculations_type.rb:17,22,87 and inherited by NewStaffGoalCalculationPreviewType · previewNewStaffGoalCalculation present in rootFields.generated.ts:132 (primary API, no REST-proxy mixing) · Standards Checklist: no violations.
The feature itself is implemented the right way for this tree: no client-side goal math was added (src/components/HrTools/CLAUDE.md forbids it for NsGoalCalculator), the three figures are server-computed and pass through formatCurrency untouched, and the wider selection set reuses the existing single preview round-trip rather than adding a request.
Important (1)
useMpdGoalPreview.ts:170 — severity 7.5 — the dirty gate diverges from settledPreview. Flagged independently by 5 of 6 agents. See the inline comment.
Also worth addressing
- Test 1 cannot fail for anything this PR changes —
sectionProps.calculationsand the provider'scalculation.calculationsare the same object reference. Inline comment on the test. - No in-flight/stale affordance on the three figures; the only preview indicator lives in a non-sticky header that scrolls out of view. Inline comments on both sections.
previewMockcannot express a throwing mock, so the.catchpath added by this PR is untestable through the wrapper. Inline comment.- Assertions are not row-anchored (
GoalSettingsPreviewContext.test.tsx, tests 2 and 4):findByText('$175.00')/('$210.00')only prove both strings exist somewhere, so swapping the primary and spouse 403(b) rows still passes. Use the working idiom fromGoalSettingsForm.test.tsx:158-165—(await findByText('403(b) Amount — John')).parentElement).toHaveTextContent('$175.00'). - A previewed
0is untested, so swapping any of the three??inGoalSettingsPreviewContext.tsxto||would silently fall back to the saved figure with a green suite.specialNeedsLeft: 0("fully raised") is a real domain state. - Client/server validation parity: Yup
percentage()is.max(100)(inclusive) while Rails validatesless_than: 100(new_staff_goal_calculation.rb:79,new_staff_validations.rb:25; the guard exists because a 100% election grosses up by1/(1-1) = ∞). Entering exactly100therefore passes the client, fires a preview, and hard-fails server validation — which silently reverts all three money figures to their saved values. Consider.lessThan(100). - Ergonomock RNG coupling: the tests type
'10','12','50'into fields whose initial values are auto-generated from[1, 100). If a future selection-set change shifts the draw order and the initial percentage lands on exactly10,dirtynever flips and the tests fail on an opaque timeout. Pincontribution403bPercentageandnsoSpecialNeedsSupportReceivedinsavedCalculation. useMpdGoalPreview.ts:139deletes the only "why" comment on the.catchwhile growing that block — it explained why a failure writes state rather than clearing it (recording the failed attributes is what stops the spinner and drives the saved-value fallback). Worth restoring one line.
Ruled out (checked, not defects)
Apollo cache poisoning via the partial calculations selection; null as a legitimate server value colliding with the failure sentinel (all three fields are non-null); preview computed from persisted rather than edited attributes (assign_attributes runs in memory before valid?); stale figures after save (the update mutation re-selects the full fragment, so the cache write and enableReinitialize land together); and figures surviving Cancel (resetForm clears dirty, which nulls the line items in the same render).
Findings on Related Files (Not in This PR)
src/components/HrTools/NsGoalCalculator/GoalSettings/goalSettingsSectionProps.ts:10-14 — severity 5.0 — flagged by two agents. The JSDoc on the calculations prop still reads "Computed worksheet amounts for the saved calculation, used for read-only derived displays (e.g. the 403(b) contribution amount)" — but after this PR both sections read those displays from previewCalculations and have zero remaining calculations. dereferences. The file isn't in this diff, but this diff is what made the sentence wrong, so the one-line reword belongs here. A reader trusting it will wire the next derived display straight to the prop and silently bypass the preview.
| const failed = settledPreview !== null && settledPreview.monthlyGoal === null; | ||
|
|
||
| const previewGoal = settledPreview?.monthlyGoal ?? null; | ||
| const previewLineItems = dirty ? (preview?.previewLineItems ?? null) : null; |
There was a problem hiding this comment.
Repro in three keystrokes: set 403(b) Contribution to 10, let the preview settle (403(b) Amount shows the previewed value), then type another digit → 101. GoalSettingsNumberField sets only min: 0 with no max, so 101 is freely typeable; Yup's percentage().max(100) then fails, so previewable → null. Now settledPreview → null, calculating → false, failed → false, previewGoal → null, and the header renders the saved goal with no spinner, no diff, and no "Preview unavailable" — visually identical to a clean form. Meanwhile dirty is still true, so the 403(b) Amount and Left to Raise rows keep serving figures computed from the superseded 10%.
The repo already asserts the opposite contract for the goal: MpdGoalPreview.test.tsx:363-381, "drops the preview when a previewed edit is then made invalid."
One-line fix that preserves the documented anti-flicker hold (previewable stays non-null throughout a valid in-flight edit):
const previewLineItems =
previewable !== null ? (preview?.previewLineItems ?? null) : null;Lines 171-172 carry the same dirty gate but predate this PR and are defensible for warning booleans, where holding avoids re-announcing. A displayed dollar figure isn't a warning.
| goalCalculationMock?: | ||
| | DeepPartial<NewStaffGoalCalculationQuery> | ||
| | ApolloErgonoMockMap; | ||
| previewMock?: DeepPartial<PreviewNewStaffGoalCalculationMutation>; |
There was a problem hiding this comment.
Consequence: the .catch branch this PR adds to useMpdGoalPreview.ts:139-153 — all three line items → null → provider falls back to saved figures — cannot be tested through this wrapper. That's the path where three money figures change meaning, and per the .max(100) vs less_than: 100 parity gap noted in the review body it is reachable by ordinary input, not just by a server outage.
Widen to match the sibling:
previewMock?: DeepPartial<PreviewNewStaffGoalCalculationMutation> | ApolloErgonoMockMap;A cheaper partial is available today without changing this type: previewNewStaffGoalCalculation is Maybe<>, so previewMock={{ previewNewStaffGoalCalculation: null }} reaches the same all-null state via .then. That covers the behaviour but leaves the literal .catch block uncovered.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: MEDIUM (6/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.
previewGoalMock must spell the new fields out: its return type is the concrete mutation type, not a partial.
Narrowed to the three preview-aware fields; every other calculations field would still be the saved value.
Dimmed and aria-busy while in flight: the figures still hold the previous edit's values, and the header's spinner is off-screen by these rows.
The server grosses up by 1/(1-rate) and validates less_than: 100. Scoped to these two fields; 100 is valid for percentages used as plain multipliers.
Section props deliberately differ from the provider, so each assertion proves the figure came from the context rather than the prop.
1 participant
Description
specialNeedsLeftTesting
Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions