Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wjames111 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/HrTools/PdsGoalCalculator/Setup/SetupStep.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import { HoursPerWeekGrid } from './HoursPerWeekGrid/HoursPerWeekGrid';
export const SetupStep: React.FC = () => {
const { t } = useTranslation();
const theme = useTheme();
const { calculation, hcmUser, isMutating, setRightPanelContent } =
const { calculation, hcmUser, isFieldSaving, setRightPanelContent } =
usePdsGoalCalculator();
const { data: userData } = useGetUserQuery();
const schema = useMemo(
Expand Down Expand Up @@ -214,7 +214,7 @@ export const SetupStep: React.FC = () => {
label={t('Pay Type')}
helperText={t('Changing this clears Pay Rate.')}
value={calculation?.salaryOrHourly ?? ''}
disabled={!calculation || isMutating}
disabled={!calculation || isFieldSaving('salaryOrHourly')}
Comment thread
wjames111 marked this conversation as resolved.
onChange={(event) => {
const newValue = event.target
.value as DesignationSupportSalaryType;
Expand Down
34 changes: 34 additions & 0 deletions src/components/HrTools/PdsGoalCalculator/Shared/Autosave/usePdsGoalAutoSave.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,38 @@ describe('usePdsGoalAutoSave', () => {
expect(mutationSpy).toHaveGraphqlOperation('UpdatePdsGoalCalculation'),
);
});

it('does not disable a saveOnChange field while an unrelated field is saving', async () => {
const { result } = renderHook(
() => ({
formType: usePdsGoalAutoSave({
fieldName: 'formType',
schema,
saveOnChange: true,
}),
name: usePdsGoalAutoSave({ fieldName: 'name', schema }),
}),
{ wrapper: Wrapper },
);

await waitFor(() => expect(result.current.name.value).toBe('Test Goal'));
expect(result.current.formType.disabled).toBe(false);

// Kick off a save for the `name` field (blur-driven).
act(() => {
result.current.name.onChange({
target: { value: 'Updated Goal' },
} as React.ChangeEvent<HTMLInputElement>);
});
act(() => {
result.current.name.onBlur();
});

// The unrelated formType select must NOT flicker disabled while `name`
// is saving — only same-field saves should disable a select.
await waitFor(() =>
expect(mutationSpy).toHaveGraphqlOperation('UpdatePdsGoalCalculation'),
);
expect(result.current.formType.disabled).toBe(false);
Copy link
Copy Markdown
Contributor Author

@wjames111 wjames111 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);

});
});
15 changes: 9 additions & 6 deletions src/components/HrTools/PdsGoalCalculator/Shared/Autosave/usePdsGoalAutoSave.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,20 @@ export const usePdsGoalAutoSave = ({
...options
}: UsePdsAutoSaveOptions) => {
const saveField = useSaveField();
const { calculation, isMutating } = usePdsGoalCalculator();
const { calculation, isFieldSaving } = usePdsGoalCalculator();

return useAutoSave({
value: calculation?.[fieldName] as string | number | null | undefined,
saveValue: (value) => saveField({ [fieldName]: value }),
fieldName,
...options,
// Block change-driven (select) autosaves while a save 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.
disabled: !calculation || (options.saveOnChange === true && isMutating),
// Block change-driven (select) autosaves while a save for this same field
// 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.
Copy link
Copy Markdown
Contributor Author

@wjames111 wjames111 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

disabled:
!calculation ||
(options.saveOnChange === true && isFieldSaving(fieldName)),
Comment thread
wjames111 marked this conversation as resolved.
});
};
13 changes: 10 additions & 3 deletions src/components/HrTools/PdsGoalCalculator/Shared/Autosave/useSaveField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { useUpdatePdsGoalCalculationMutation } from '../../GoalsList/PdsGoalCalc
import { usePdsGoalCalculator } from '../PdsGoalCalculatorContext';

export const useSaveField = () => {
const { calculation, trackMutation } = usePdsGoalCalculator();
const { calculation, trackFieldMutation } = usePdsGoalCalculator();
const [updatePdsGoalCalculation] = useUpdatePdsGoalCalculationMutation();
const { enqueueSnackbar } = useSnackbar();
const { t } = useTranslation();
Expand All @@ -25,7 +25,7 @@ export const useSaveField = () => {
}

try {
return await trackMutation(
return await trackFieldMutation(
updatePdsGoalCalculation({
variables: {
attributes: {
Expand All @@ -45,14 +45,21 @@ export const useSaveField = () => {
},
},
}),
Object.keys(attributes),
);
} catch {
enqueueSnackbar(t('Failed to save changes. Please try again.'), {
variant: 'error',
});
}
},
[calculation, trackMutation, updatePdsGoalCalculation, enqueueSnackbar, t],
[
calculation,
trackFieldMutation,
updatePdsGoalCalculation,
enqueueSnackbar,
t,
],
);

return saveField;
Expand Down
53 changes: 53 additions & 0 deletions src/components/HrTools/PdsGoalCalculator/Shared/PdsGoalCalculatorContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,19 @@

/** 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;
Copy link
Copy Markdown
Contributor Author

@wjames111 wjames111 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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>,

/** Call with the mutation promise to track the start and end of mutations */
trackMutation: <T>(mutation: Promise<T>) => Promise<T>;
/**
* Like trackMutation, but also marks the listed fields as saving while the
* mutation is in flight so per-field disable checks can scope to the field
* being saved instead of any save in the calculator.
*/
trackFieldMutation: <T>(
mutation: Promise<T>,
fields: string[],
) => Promise<T>;

rightPanelContent: React.ReactNode;
setRightPanelContent: (content: React.ReactNode) => void;
Expand Down Expand Up @@ -108,93 +119,135 @@
const [isDrawerOpen, setIsDrawerOpen] = useState<boolean>(true);
const { trackMutation, isMutating } = useTrackMutation();

const [savingFieldCounts, setSavingFieldCounts] = useState<
Record<string, number>
>({});

const isFieldSaving = useCallback(
(fieldName: string) => (savingFieldCounts[fieldName] ?? 0) > 0,
[savingFieldCounts],
);

const trackFieldMutation = useCallback(
<T,>(mutation: Promise<T>, fields: string[]): Promise<T> => {
setSavingFieldCounts((prev) => {
const next = { ...prev };
for (const field of fields) {
next[field] = (next[field] ?? 0) + 1;
}
return next;
});
return trackMutation(
mutation.finally(() => {
Copy link
Copy Markdown
Contributor Author

@wjames111 wjames111 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

setSavingFieldCounts((prev) => {
const next = { ...prev };
for (const field of fields) {
const remaining = (next[field] ?? 0) - 1;
if (remaining <= 0) {
delete next[field];
} else {
next[field] = remaining;
}
}
return next;
});
}),
);
},
[trackMutation],
);
Copy link
Copy Markdown
Contributor Author

@wjames111 wjames111 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


useEffect(() => {
if (steps.some((s) => s.step === activeStep)) {
return;
}
setActiveStep(steps[0]?.step ?? PdsGoalCalculatorStepEnum.Setup);
enqueueSnackbar(
t('Returned to Setup because the current step is no longer available.'),
{ variant: 'info' },
);
}, [steps, activeStep, enqueueSnackbar, t]);

const stepIndex = useMemo(() => {
const idx = steps.findIndex((s) => s.step === activeStep);
return idx === -1 ? 0 : idx;
}, [steps, activeStep]);

const currentStep = steps[stepIndex];

const percentComplete = Math.round(
safeProgressRatio(stepIndex + 1, steps.length) * 100,
);

const handleStepChange = useCallback(
(newStep: PdsGoalCalculatorStepEnum) => {
if (steps.some((step) => step.step === newStep)) {
setActiveStep(newStep);
} else {
enqueueSnackbar(t('The selected step does not exist.'), {
variant: 'error',
});
}
},
[steps, enqueueSnackbar, t],
);

const handleContinue = useCallback(() => {
if (stepIndex < steps.length - 1) {
setActiveStep(steps[stepIndex + 1].step);
}
}, [stepIndex, steps]);

const handlePreviousStep = useCallback(() => {
if (stepIndex > 0) {
setActiveStep(steps[stepIndex - 1].step);
}
}, [stepIndex, steps]);

const closeRightPanel = useCallback(() => {
setRightPanelContent(null);
}, []);

const toggleDrawer = useCallback(() => {
setIsDrawerOpen((prev) => !prev);
}, []);

const contextValue = useMemo(
(): PdsGoalCalculatorType => ({
steps,
currentStep,
stepIndex,
calculation,
calculationLoading,
summaryData,
percentComplete,
isMutating,
isFieldSaving,
trackMutation,
trackFieldMutation,
hcmUser,
rightPanelContent,
isDrawerOpen,
handleStepChange,
handleContinue,
handlePreviousStep,
setRightPanelContent,
closeRightPanel,
toggleDrawer,
setDrawerOpen: setIsDrawerOpen,
}),
[
steps,
currentStep,
stepIndex,
calculation,
calculationLoading,
summaryData,
percentComplete,
isMutating,
isFieldSaving,
trackMutation,
trackFieldMutation,

Check warning on line 250 in src/components/HrTools/PdsGoalCalculator/Shared/PdsGoalCalculatorContext.tsx

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Large Method 

PdsGoalCalculatorProvider:React.FC<Props> increases from 120 to 159 lines of code, threshold = 100. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
hcmUser,
rightPanelContent,
isDrawerOpen,
Expand Down
Loading