[MPDX-9764,MPDX-9765,MPDX-9794] Implement goal settings page - #1867
Conversation
* Fix 0-coercion data loss * Add query error state * Remove defaultGoalSettingsValues
canac
added
Preview Environment
|
Preview branch generated at https://9764-goal-settings-page.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against ad90f78
|
canac
left a comment
canac
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — PR #1867
Verdict: ✅ APPROVED WITH SUGGESTIONS · Mode: standard · 7 specialist agents + dependency + gap review
No blockers (nothing ≥ 8.0, no critical). One Important (7.5) finding is strongly recommended before merge; 6 Medium and ~10 low suggestions follow as inline comments.
⚠️ Risk score 10/10 (CRITICAL) — driven by size (38 files, +3075/-2) + the financial goal-calculation domain, not by defects. The code is clean, convention-compliant, and unusually well-tested. Because risk is CRITICAL, this PR is not auto-approve eligible and a senior reviewer should sign off.
Consensus
| Tier | Count |
|---|---|
| Critical (9.0–10.0) | 0 |
| High (8.0–8.9) | 0 |
| Important (7.0–7.9) | 1 |
| Medium (5.0–6.9) | 6 |
| Suggestions (<5.0) | ~10 |
| Pre-existing (info) | 2 |
Top finding
Save has no success/error feedback (7.5) — GoalSettingsForm.tsx handleSubmit awaits the mutation with no try/catch and no snackbar. Flagged independently by the Testing and UX agents against the repo's enqueueSnackbar convention (252 usages; sibling useSaveField.ts). On failure the user sees nothing. Compounds with the PositiveFloat Medium finding: a user entering 0 in a salary/amount field could get a silent no-op save. See inline comment.
⚠️ Cross-cutting compound risk
Finding #1 (silent save) + finding #3 (0 possibly rejected by the PositiveFloat scalar) together mean a 0 entry can fail server-side with zero user feedback. Fixing the error handling (#1) also surfaces #3.
Test-coverage gaps (not posted inline)
- No integration test asserts spouse attributes are cleared to
nullon a married→single save (unit-tested in isolation only). Medium 6.0. - The save success/error branches are untested (add with the #1 fix).
- Field
error/helperTextdisplay path lacks a focused unit test (covered indirectly). ~4.0. - One inline
GqlMockedProviderinGoalSettingsForm.test.tsxlacks typed generics. 3.5.
Non-code confirmations (informational)
- Server-side coach authorization — verify the API resolver scopes the
newStaffGoalCalculationquery +updateNewStaffGoalCalculationmutation byaccountListId. The client can't enforce this and matches the existing coaching trust model; not a front-end defect. - Validation parity — the Yup numeric rules are client-only; ensure the server enforces the same bounds.
- Codegen — confirm
yarn gqlsucceeds in CI (.generated.tsis not committed).
Pre-existing (not this PR)
next.config.tsenvblock already inlines real secrets into the client bundle. This PR only adds the benignDISABLE_NS_GOAL_CALCULATORflag, following theDISABLE_NEW_REPORTSconvention. Out of scope.CoachingDetail.tsxstyle={{ flexGrow: 1 }}is pre-existing context, not added here.
Strengths
- Standards: 0 violations, full checklist PASS (named exports, file naming, i18n with proper interpolation,
idin every selection set, noany/console/new Date(), Formik+Yup, submit disabled while submitting, Luxon for dates). - Data integrity: form→mutation mapping is an explicit allowlist — no editable field dropped, every
spouse*field gated, real0preserved (value === ''), cache normalizes byid. - Dependency impact: LOW/non-breaking — only 2 existing files touched, both backward-compatible; 0 unresolved imports.
- Gap review of the two untested Section files: clean.
Reply /dismiss: <reason> on any Medium/low comment (severity < 7) to acknowledge it. The Important (7.5) finding and the non-code confirmations can't be dismissed.
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Looks great so far. I noticed the Figma also has 'NSO/IBS Cost' and 'MPDRR / F&F Course' which aren't included here, but you're likely aware of that.
| <GoalSettingsNumberField | ||
| name="annualRequestedSalary" | ||
| label={t('Annual Requested Salary')} | ||
| personName={primaryName} | ||
| adornment="currency" |
There was a problem hiding this comment.
From the Figma, it looks like this should have the seniorStaffOnly helper text as well
There was a problem hiding this comment.
I'm not sure on this one. I thought that mid-career new staff could adjust their requested salary, but maybe I'm mistaken. I started a thread with Ryan about it: https://www.figma.com/design/O3zztrqYUv0lxHhYw3Fhw4?node-id=1580-22499&m=dev#1824311925
Correct. Figma makes them seem like they are editable per-calculation. But they are actually set on the new staff cohort. We can show them here once the cohort model exists, but they won't be editable here. |
2 participants
Description
Implement the goal settings page, where admins can edit the calculation fields. Most fields are functional, except for a couple that come from the attendee, which we don't have an API for yet.
There will probably be a few things to fix or improve, but since this is already a huge PR, it might be best to make those improvements in follow-up tickets and PRs unless they are trivial fixes or significant refactorings.
Jira tickets:
Testing
/accountLists/308c5567-a0b6-49f6-922e-f0c338e170ca/coaching/45ef99a5-c7b9-4873-b194-ca4d5b28d09e/nsGoalCalculator)Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions