No-Jira - Disable Continue on Setup Step until values filled by user#1722
No-Jira - Disable Continue on Setup Step until values filled by user#1722zweatshirt wants to merge 2 commits intomainfrom
Conversation
…and SetupStep components
Bundle sizes [mpdx-react]Compared against 2cfad2c No significant changes found |
zweatshirt
force-pushed
the
pds-disable-continue
branch
from
aee607f to
6a3da71
Compare
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review
Verdict: APPROVED_WITH_SUGGESTIONS
Risk: MEDIUM (5/10)
Mode: standard (5 agents: Architecture, Testing, Standards, UX, Financial Reporting)
Summary
The PR correctly wires AutosaveForm + disableNext to disable the Continue button when Setup step fields are empty. The shared-component change (DirectionButtons) was carefully scoped — the new disableNext prop defaults to undefined, so existing callers (MHA StepOne/Two/Three, AdditionalSalaryRequest) are unaffected. Verified across all 5 callsites.
What landed well
- Mirrors the SalaryCalculator pattern (
AutosaveForm+StepContentextraction). - New
disableNextprop is purposefully separate fromisValidto avoid disabling Continue in the ASR RequestPage flow. - Conditional fields (
hoursWorkedPerWeek,benefits) re-enable Continue correctly on unmount via the existinguseAutosavecleanup. - i18n: all new required-error messages wrapped in
t(). - Financial Reporting agent: no financial calculation code in this PR — all checks pass or N/A.
Top findings
-
[Important 8.0]
SetupStep.tsx:259— The Geographic Multiplier Autocomplete displays'None'when the underlying value isnull(value={calculation?.geographicLocation ?? 'None'}), which now looks like a filled field even though the required check fails. Users see'None'selected but Continue disabled with no error state on the control. See the inline comment on the new useEffect for the suggested fix. -
[Medium 7.0]
DirectionButtons.tsx:146— Disabled Continue has no tooltip/aria explanation. Once the Autocomplete error state is fixed, field-level messages cover most cases, but a<Tooltip>on the button itself still helps. -
[Medium 6.5]
SetupStep.tsx:69— The newgeographicLocationyup rule is effectively dead (the runtime check is a manualuseEffecttesting truthiness). Either delete the rule or introduceAutosaveAutocompleteso validation flows throughuseAutoSaveconsistently.
Important-tier note
WARNING: 1 Important finding (severity 8.0) — strongly recommended fix that doesn't block merge but can't be dismissed via
/dismiss. Address in this PR or create a follow-up ticket.
Agents
| Agent | Findings |
|---|---|
| Architecture | 3 medium + 2 suggestions (useEffect pattern, prop bloat, duplicate hook call) |
| Testing | 2 medium (Submit-branch leak test, schema-invalid value test) + 2 suggestions |
| Standards | 2 concerns (useEffect pattern, server-side validation parity WARN); checklist otherwise PASS |
| UX | 1 important + 2 medium + 3 suggestions (Autocomplete, tooltip, loading flicker) |
| Financial Reporting | No financial calculation code in this PR |
No agent challenged another's findings. No severity spreads ≥4 points. All 5 changed files received coverage from 2+ agents — no gap review needed.
|
|
||
| const autosaveForm = useOptionalAutosaveForm(); | ||
| const geographicLocationValue = calculation?.geographicLocation; | ||
| useEffect(() => { |
There was a problem hiding this comment.
Suggested fix:
<Autocomplete
options={locations}
value={calculation?.geographicLocation ?? null}
onChange={(_, newValue) => saveField({ geographicLocation: newValue })}
disabled={!calculation}
size="small"
renderInput={(params) => (
<TextField
{...params}
label={t('Geographic Multiplier')}
required
error={!!calculation && !calculation.geographicLocation}
helperText={
!!calculation && !calculation.geographicLocation
? t('Geographic Multiplier is a required field')
: t('If not applicable, select "None"')
}
/>
)}
/>Passing null lets MUI render the field empty with floating label. required + error + helperText match the other fields' UX and add aria-invalid for screen readers.
| .nullable() | ||
| .required(t('Benefits is a required field')) | ||
| .min(0, t('Benefits must be a positive number')), | ||
| geographicLocation: yup |
There was a problem hiding this comment.
Options:
- Cheap: delete the yup rule with a comment (validation is driven by the effect).
- Preferred (likely follow-up PR): introduce
AutosaveAutocompletemirroringAutosaveTextFieldso validity flows throughuseAutoSave. That also addresses the IMP-1 UX symptom because the wrapper would consumeerror/helperTextfrom the schema.
Flagged by Architecture and Standards agents.
| payRate: yup | ||
| .number() | ||
| .nullable() | ||
| .required(t('Pay Rate is a required field')) |
There was a problem hiding this comment.
| name: yup.string().required(t('Goal Name is a required field')), | ||
| status: yup.string().nullable(), | ||
| salaryOrHourly: yup.string().nullable(), | ||
| status: yup |
There was a problem hiding this comment.
| variant="contained" | ||
| color="primary" | ||
| onClick={overrideNext ?? handleNextStep} | ||
| disabled={disableNext} |
There was a problem hiding this comment.
Suggested:
<Tooltip title={disableNext ? t('Complete all required fields to continue') : ''}>
<span>
<Button
variant="contained"
color="primary"
onClick={overrideNext ?? handleNextStep}
disabled={disableNext}
aria-describedby={disableNext ? 'continue-disabled-reason' : undefined}
>
{buttonTitle ?? t('Continue')}
<ChevronRight sx={{ ml: 1 }} />
</Button>
</span>
</Tooltip>(The <span> wrapper is required because disabled buttons don't fire pointer events — MUI tooltip gotcha.)
Note: SalaryCalculator's ContinueButton has the same gap — fixing it here improves both.
| additionalApproval?: boolean; | ||
| splitAsr?: boolean; | ||
| disableSubmit?: boolean; | ||
| disableNext?: boolean; |
There was a problem hiding this comment.
Follow-up refactor idea (not blocking this PR): consolidate to a single disabled prop computed by callers. The Formik callers already have isValid/submitCount locally. Shrinks the contract from 'know the branch + pick right prop' to 'compute disabled once.'
| formTitle={currentStep.title} | ||
| handleNextStep={handleContinue} | ||
| handlePreviousStep={handlePreviousStep} | ||
| disableNext={!allValid} |
There was a problem hiding this comment.
Suggested: include the loading gate in the disabled check.
const { currentStep, stepIndex, steps, handleContinue, handlePreviousStep, calculation } =
usePdsGoalCalculator();
...
disableNext={!calculation || !allValid}The Autocomplete at SetupStep.tsx:263 already gates itself on !calculation — the Continue button should follow suit.
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('disables Continue on Setup step when a required field is missing', async () => { |
There was a problem hiding this comment.
it('disables Continue on Setup step when a required field is invalid', async () => {
const { findByRole } = render(
<PdsGoalCalculatorTestWrapper
calculationMock={{ ...completeSetupMock, payRate: -5 }}
>
<PdsGoalCalculator />
</PdsGoalCalculatorTestWrapper>,
);
const continueButton = await findByRole('button', { name: /continue/i });
await waitFor(() => expect(continueButton).toBeDisabled());
});| expect(await findByRole('button', { name: title })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('disables Continue when disableNext is true', async () => { |
There was a problem hiding this comment.
it('does not disable Submit even when disableNext is true', async () => {
const { findByRole } = render(
<TestComponent isSubmission={true} disableNext={true} />,
);
expect(await findByRole('button', { name: /submit/i })).toBeEnabled();
});![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.
1 participant
Description
Testing
reports/goalCalculatorChecklist:
/pr-reviewcommand locally and fixed any relevant suggestions