MPDX-9611 Scope PDS autosave disable to per-field saves by wjames111 · Pull Request #1797 · CruGlobal/mpdx-react

wjames111 · 2026-05-21T18:57:18Z

Description

The Form Type, Employment Status, and Pay Type selects in the PDS Goal Calculator Setup step briefly rendered disabled whenever an unrelated field (e.g. Benefits) was being saved, because they were gated on the global isMutating flag from the calculator context.
Added per-field saving state to PdsGoalCalculatorContext: a new isFieldSaving(fieldName) selector and a trackFieldMutation(promise, fields) wrapper that tags the fields being written for the duration of the mutation.
useSaveField now uses trackFieldMutation and passes Object.keys(attributes), so each in-flight save tags only the fields it actually writes.
usePdsGoalAutoSave and the manual Pay Type select in SetupStep now check isFieldSaving(fieldName) instead of isMutating. Same-field race protection on formType (and friends) is preserved — only the cross-field flicker is gone.
Added a regression test that saving an unrelated field does not disable a saveOnChange select.
Jira: MPDX-9611

Testing

Open the PDS Goal Calculator Setup step for a Full-time Salaried goal so all of Form Type, Employment Status, Pay Type, Pay Rate, and Benefits are visible.
Edit the Benefits field, then tab/click away to trigger the autosave.
Watch Form Type, Employment Status, and Pay Type while the Benefits save is in flight — they should remain interactive (no flicker to a disabled state).
Toggle Form Type (Default ↔ Simple) rapidly and confirm the Form Type select itself is briefly disabled while its own save is in flight — same-field race protection still applies.
Toggle Pay Type (Salaried ↔ Hourly) and confirm the Pay Type select disables only during its own save, and the atomic salaryOrHourly + payRate: null write still happens.

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

🤖 Generated with Claude Code

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>

github-actions · 2026-05-21T19:00:57Z

Bundle sizes [mpdx-react]

Compared against fdfd564

No significant changes found

wjames111

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 + GqlMockedProvider typed generics: PASS
isMutating is NOT dead code — page-level SavingStatus still consumes it via pages/.../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 · 2026-05-21T20:08:02Z

  /** Whether any mutations are currently in progress */
  isMutating: boolean;
+  /** Whether a save mutation tagged with the given field name is in flight */
+  isFieldSaving: (fieldName: string) => boolean;


[Important] **Loose `string` typing erases the schema constraint that exists upstream** (Architecture 7.0 + Standards pattern deviation).
The new API accepts arbitrary strings:

isFieldSaving: (fieldName: string) => boolean (this line)

trackFieldMutation: <T>(mutation: Promise<T>, fields: string[]) => Promise<T> (line 51-54)

But callers actually have keyof DesignationSupportCalculationUpdateInput available (usePdsGoalAutoSave.ts already enforces it). SetupStep.tsx:217 passes the bare literal 'salaryOrHourly' — a typo or rename would compile cleanly and silently leave the Pay Type select un-disabled.

Suggested patch:

import { DesignationSupportCalculationUpdateInput } from 'src/graphql/types.generated'; ... isFieldSaving: ( fieldName: keyof DesignationSupportCalculationUpdateInput, ) => boolean; ... trackFieldMutation: <T>( mutation: Promise<T>, fields: Array<keyof DesignationSupportCalculationUpdateInput>, ) => Promise<T>;

And in useSaveField.ts:48, isolate the Object.keys widening with one cast:

Object.keys(attributes) as Array<keyof DesignationSupportCalculationUpdateInput>,

wjames111 · 2026-05-21T20:08:02Z

+    await waitFor(() =>
+      expect(mutationSpy).toHaveGraphqlOperation('UpdatePdsGoalCalculation'),
+    );
+    expect(result.current.formType.disabled).toBe(false);


[Important] **This test does not actually catch the regression it claims to.**
The final assertion runs after await waitFor(() => mutationSpy...). By that point, the mock mutation has resolved and savingFieldCounts.name is back to 0. The assertion passes both pre-fix (broad isMutating) and post-fix.

Compare with the working pattern at lines 124-145, which polls for true then false to catch the transition.

Suggested patch — sample synchronously mid-save:

act(() => { result.current.name.onBlur(); }); + // Sample synchronously mid-save: savingFieldCounts.name === 1 here, but + // the mock mutation has not yet resolved. Pre-fix (gating on isMutating), + // formType.disabled would have been true at this moment. + expect(result.current.formType.disabled).toBe(false); + await waitFor(() => expect(mutationSpy).toHaveGraphqlOperation('UpdatePdsGoalCalculation'), ); expect(result.current.formType.disabled).toBe(false);

wjames111 · 2026-05-21T20:08:02Z

+      );
+    },
+    [trackMutation],
+  );


[Medium] **No test for multi-field atomic save.** The Pay Type path passes `['salaryOrHourly', 'payRate']` into `trackFieldMutation` (`useSaveField.ts:48`). This is the entire reason `fields` is an array, but no test verifies that BOTH fields are marked saving during a multi-field write. Consider a direct context test that calls `trackFieldMutation` with two fields and asserts `isFieldSaving` returns `true` for both.

wjames111 · 2026-05-21T20:08:02Z

+        return next;
+      });
+      return trackMutation(
+        mutation.finally(() => {


[Medium] **No test for the failure path.** `mutation.finally(...)` correctly decrements on rejection in principle, but no test exercises that branch. A future refactor moving the decrement into `.then(...)` would leak counts (the field stays disabled forever) and no test would catch it. A test using a rejecting Apollo mock and asserting `isFieldSaving(field) === false` after rejection would close the gap.

wjames111 · 2026-05-21T20:08:02Z

+    // is in flight: rapid back-and-forth toggles can otherwise land out of
+    // order in the Apollo cache. formType is the load-bearing case — its
+    // value reshapes the goal calculation, so a stale final value silently
+    // understates the total. Other fields' saves do not affect this one.


[Suggestion] The comment says "Other fields' saves do not affect this one," but as noted on line 32, that's true for the UI disable behavior but **not** for cache state — a concurrent save's full-fragment response can briefly stamp the cache and momentarily un-do this field's optimistic write. If the team accepts that trade-off, please reword the comment to acknowledge it. If the team adopts the fix on line 32, the whole comment block can be tightened.

wjames111 commented May 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPDX-9611 Scope PDS autosave disable to per-field saves#1797

MPDX-9611 Scope PDS autosave disable to per-field saves#1797
wjames111 wants to merge 1 commit into
mainfrom
MPDX-9611

wjames111 commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

wjames111 left a comment

Uh oh!

Uh oh!

Uh oh!

wjames111 May 21, 2026

Uh oh!

wjames111 May 21, 2026

Uh oh!

wjames111 May 21, 2026

Uh oh!

wjames111 May 21, 2026

Uh oh!

wjames111 May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

wjames111 commented May 21, 2026

Description

Testing

Checklist:

Uh oh!

github-actions Bot commented May 21, 2026

Bundle sizes [mpdx-react]

Uh oh!

wjames111 left a comment

Choose a reason for hiding this comment

Multi-Agent Code Review — VERDICT: BLOCKERS_FOUND

Top blocker

Risk assessment

Findings summary

Verdict reasoning

Findings on Related / Pre-existing Code (Not in This PR's Diff)

Standards & Quality summary

Uh oh!

Uh oh!

Uh oh!

wjames111 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

wjames111 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

wjames111 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

wjames111 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

wjames111 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant