Conversation
The Form Type, Employment Status, and Pay Type selects briefly flickered disabled whenever any other field (e.g. Benefits) was being saved, because they were gated on the global isMutating flag. Track saving state per field via trackFieldMutation so a select only disables while its own save is in flight — preserving the same-field race protection without affecting unrelated fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bundle sizes [mpdx-react]Compared against bdb41fd No significant changes found |
wjames111
left a comment
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — VERDICT: BLOCKERS_FOUND
6 agents · Architecture · Data Integrity · Testing · UX · Standards · Financial Reporting
Top blocker
Cross-field stale-write race (consensus from Data Integrity, UX, Financial Reporting). The PR narrows the in-flight save guard from "any mutation in flight" to "same field in flight," but UpdatePdsGoalCalculation responses carry the full PdsGoalCalculationFields fragment. A second save's response can land after a first save's, stamping stale field values (notably formType) into the Apollo cache — the exact silent goal-total miscalculation the in-line comment warns about. See inline comment on usePdsGoalAutoSave.ts.
Risk assessment
- Risk: 5/10 — MEDIUM
- Reviewer level recommended: mid-level / senior
Findings summary
| Tier | Count |
|---|---|
| Critical (9-10) | 0 |
| High Priority Blocker (8-8.9) | 1 |
| Important (7-7.9) | 2 |
| Medium (5-6.9) | 2 |
| Suggestions (<5) | ~10 |
Verdict reasoning
The per-field disable guard is correctly motivated for rapid same-field toggling, but is insufficient because (a) the server response carries the full fragment, so cross-field responses can interleave and stomp the cache, and (b) the Pay Type → payRate: null multi-field atomic save isn't protected on the Pay Rate text field (which has saveOnChange=false). Either widen the guard back for formType/salaryOrHourly, or drop the saveOnChange === true qualifier so blur-driven fields also disable during their own save.
Findings on Related / Pre-existing Code (Not in This PR's Diff)
[Suggestion] SetupStep.tsx:316-332 (Geographic Multiplier Autocomplete)
- Severity: 4.0/10
- Flagged by: UX, Data Integrity
- Note:
disabled={!calculation}only — no in-flight protection of any kind. Same race class as the blocker above. Pre-existing (not introduced by this PR). Worth a follow-up ticket, not a blocker here.
Standards & Quality summary
- Named exports: PASS
- TypeScript hygiene: PASS (no
any, no@ts-ignore, no!) - i18n: PASS (no new strings)
- Lint/TS clean for touched files: PASS
- Test colocation +
GqlMockedProvidertyped generics: PASS isMutatingis NOT dead code — page-levelSavingStatusstill consumes it viapages/.../pdsGoalCalculator/[pdsGoalId].page.tsx.
To resolve: address the blocker comment on usePdsGoalAutoSave.ts, then either fix or reply /dismiss: <reason> to severity < 7 findings you disagree with. Severity 7.0-7.9 findings cannot be dismissed but don't trigger BLOCKERS_FOUND on their own.
wjames111
left a comment
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — Standard Mode
Verdict: APPROVED WITH SUGGESTIONS
Risk: 5/10 (MEDIUM)
Agents: 5 specialized reviewers (Architecture, Data Integrity, Testing, UX, Standards)
Diff: 7 files, +214/-9
Headline
The per-field saving state machine is correct under concurrency, handles resolve and reject paths, and is well-tested. No blocking issues in PR files. The most consequential finding is a related-file finding (not in this PR): the PR description names Pay Type as one of three selects being fixed, but the Pay Type select still binds to the global isMutating flag — see the "Findings on Related Files" section below.
Summary of in-PR Findings
| Tier | Count | Notes |
|---|---|---|
| Critical (9-10) | 0 | — |
| High (8-9) | 0 | — |
| Important (7-8) | 0 in-PR (1 related-file, informational) | Cannot be dismissed if any |
| Medium (5-7) | 7 | Posted as line comments below |
| Suggestions (<5) | ~10 | Informational |
All Standards-checklist items PASS. yarn lint + yarn lint:ts confirmed clean. Apollo cache normalization preserved (id + __typename in optimistic response).
Cross-cutting consistency check
The new trackFieldMutation is correctly invoked by the single consumer (useSaveField) with Object.keys(attributes), so multi-field atomic writes (e.g. salaryOrHourly + payRate: null) correctly tag both fields as saving for the lifetime of the single mutation. HoursPerWeekGrid still uses trackMutation (correctly — it's a multi-step hours workflow, not a per-field save).
Findings on Related Files (Not in This PR)
These were identified during review on files related to the changes but not directly modified in this PR. They cannot be posted as line comments.
[Important] src/components/HrTools/PdsGoalCalculator/Setup/SetupStep.tsx:217 — Pay Type select still uses global isMutating
- Severity: 7.5/10
- Flagged by: Architecture + UX (consensus)
- Problem: The PR description states the fix removes the disabled flicker for "Form Type, Employment Status, and Pay Type selects" in the Setup step. Form Type and Employment Status route through
AutosaveTextField → usePdsGoalAutoSave, which the diff updates correctly. But Pay Type is a manual<TextField>(it must atomically clearpayRate), and atSetupStep.tsx:217it still readsdisabled={!calculation || isMutating}. After this PR merges, Pay Type will still flicker disabled while Benefits / Goal Name / Pay Rate is being saved — exactly the behavior the PR claims to remove. - Recommended fix (single-line change you can land on this branch as a follow-up commit):
// SetupStep.tsx:41 — pull isFieldSaving instead of isMutating
const { calculation, hcmUser, isFieldSaving, setRightPanelContent } =
usePdsGoalCalculator();
// SetupStep.tsx:217
disabled={
!calculation ||
isFieldSaving('salaryOrHourly') ||
isFieldSaving('payRate')
}useSaveField already passes Object.keys(attributes), so the multi-field atomic write tags both fields and only Pay Type + Pay Rate will disable during their own save.
Pre-existing notes (informational)
useSaveField.ts:52catch { ... enqueueSnackbar ... }drops the error object entirely (noconsole.error, no Sentry). Server validation messages are invisible. Pre-existing pattern; the PR rewrites lines around it.- Form-wide:
disabledis used as a "saving in progress" signal wherearia-busywould be more semantically correct. Pattern is repo-wide; the codebase already usesaria-busyelsewhere (CsvHeaders.tsx:337,CsvPreview.tsx:346).
![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 (5/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.
wjames111
added
On Staging
|
Preview branch generated at https://MPDX-9611.d3dytjb8adxkk5.amplifyapp.com |
1 participant
Description
isMutatingflag from the calculator context.PdsGoalCalculatorContext: a newisFieldSaving(fieldName)selector and atrackFieldMutation(promise, fields)wrapper that tags the fields being written for the duration of the mutation.useSaveFieldnow usestrackFieldMutationand passesObject.keys(attributes), so each in-flight save tags only the fields it actually writes.usePdsGoalAutoSaveand the manual Pay Type select inSetupStepnow checkisFieldSaving(fieldName)instead ofisMutating. Same-field race protection onformType(and friends) is preserved — only the cross-field flicker is gone.saveOnChangeselect.Testing
salaryOrHourly + payRate: nullwrite still happens.Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions🤖 Generated with Claude Code