Conversation
Bundle sizes [mpdx-react]Compared against 9e1a327 No significant changes found |
wjames111
added
On Staging
|
Preview branch generated at https://MPDX-9541.d3dytjb8adxkk5.amplifyapp.com |
wjames111
left a comment
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — APPROVED WITH SUGGESTIONS
Risk: LOW (3/10) · Mode: Standard · Agents: Architecture, Testing, Standards, UX
The stated goal — hide Back on the first step of the PDS Goal Calculator — is implemented correctly via showBackButton={!isFirstStep}. No blockers. Three medium-priority concerns and two suggestions are inline below; the layout-shift concern (M1) is the most important to confirm with design before merge.
Medium Priority Findings (inline)
- M1 — DirectionButtons layout shift affects MHA & ASR consumers, not just PDS (UX + Architecture, 6.5)
- M2 — Dead
justifyContentconditional and unconditionally-rendered empty Box (Architecture, 6.5) - M3 — No structural test coverage for the DirectionButtons re-layout (Testing + Architecture, 6.5)
Suggestions (inline)
- S1 — Add an
isLastStepedge-case test (Testing, 4.0) - S2 — Align
name: 'Back'query with the file's/regex/iconvention (Testing, 3.0)
Out-of-Scope Suggestion (no inline anchor)
- S3 — Migrate inline
<ChevronLeft />/<ChevronRight />Button children to MUIstartIcon/endIcon(UX, 3.5). Matches the pattern inSalaryCalculator/StepNavigation/StepNavigation.tsxand auto-marks the icons decorative for screen readers. Worth a follow-up ticket; out of scope here.
Working-Tree Note (NOT a PR finding — informational)
A local unstaged edit on src/components/HrTools/PdsGoalCalculator/PdsGoalCalculator.test.tsx:107 changes the line in this PR from
await findByRole('button', { name: /continue/i });to
await findByRole('button', { name: /continue/i }).toBeInTheDocument();The second form is broken — findByRole returns Promise<HTMLElement>, and Promise has no .toBeInTheDocument method, so this throws at runtime. The line in the actual PR diff is correct; please discard the local edit before your next commit:
git checkout -- src/components/HrTools/PdsGoalCalculator/PdsGoalCalculator.test.tsxReview generated by /quality:agent-review. Reply /dismiss: <reason> to any inline finding (severity < 7) you want to acknowledge and exclude from the verdict.
| )} | ||
| </Box> | ||
|
|
||
| <Box sx={{ display: 'flex', gap: 2 }}> |
There was a problem hiding this comment.
Wrapping Discard + Back into a single left-side flex Box (and Continue/Submit into this new right-side Box) is silently visible to every existing caller that passes both handleDiscard and showBackButton:
MinisterHousingAllowance/Steps/StepTwo/RentOwn.tsx— Discard + Back + ContinueMinisterHousingAllowance/Steps/StepThree/Calculation.tsx— Discard + Back + SubmitAdditionalSalaryRequest/RequestPage/RequestPage.tsx— middle pages with editable request
Old layout: Discard at far-left, Back+Continue grouped on the right.
New layout: Discard+Back grouped on the left, Continue/Submit alone on the right.
The prior commit 7df16b9c8 Fix styling on back/continue buttons suggests the change is intentional, but the PR title only references PDS. Recommend confirming with design that the new MHA Step 2 / Step 3 / ASR middle-page arrangement is intended, or grabbing before/after screenshots for QA.
Flagged by Architecture + UX agents.
| @@ -101,16 +101,15 @@ export const DirectionButtons: React.FC<DirectionButtonsProps> = ({ | |||
| : 'space-between', | |||
There was a problem hiding this comment.
With the new structure, the outer container always has two child Boxes. The ternary !handleDiscard && !showBackButton && !isSubmission ? 'flex-end' : 'space-between' only triggers flex-end when the left Box is empty (0-width), so flex-end and space-between are visually identical in that case. The conditional is dead, and the empty Box is rendered even when neither Discard nor Back is present.
Suggested cleanup:
<Box sx={{ mt: 5, display: 'flex', justifyContent: 'space-between' }}>
{(handleDiscard || showBackButton) && (
<Box sx={{ display: 'flex', gap: 2 }}>
{handleDiscard && (...)}
{showBackButton && (...)}
</Box>
)}
<Box sx={{ display: 'flex', gap: 2 }}>{/* Continue / Submit */}</Box>
</Box>With only the right Box present, space-between and flex-end produce the same single-child result.
Flagged by Architecture agent.
| </Button> | ||
| )} | ||
|
|
||
| <Box sx={{ display: 'flex', gap: 2 }}> |
There was a problem hiding this comment.
DirectionButtons.test.tsx exists and continues to pass (its assertions are role-based, so the structural change doesn't break them). But there's no test asserting that the new (Discard | Back | Continue/Submit) layout renders as expected — especially the !handleDiscard && !showBackButton && !isSubmission case, where the left Box is now empty.
Adding a structural assertion in DirectionButtons.test.tsx would protect every consumer (MHA, ASR, PDS) against future regressions in the flex shape.
Flagged by Testing + Architecture agents.
| formTitle={currentStep.title} | ||
| handleNextStep={handleContinue} | ||
| handlePreviousStep={handlePreviousStep} | ||
| showBackButton={!isFirstStep} |
There was a problem hiding this comment.
MainContent wraps DirectionButtons in {!isLastStep && (...)}, so on the last step the entire row disappears. The new tests cover step 0 (Back hidden) and step 1 (Back shown) but not the SummaryReport (last) step.
Not strictly required for this PR, but a 'hides the entire DirectionButtons row on the last step' test would round out the multi-step nav coverage.
Flagged by Testing agent.
| ); | ||
|
|
||
| await findByRole('button', { name: /continue/i }); | ||
| expect(queryByRole('button', { name: 'Back' })).not.toBeInTheDocument(); |
There was a problem hiding this comment.
The rest of this test file uses regex matchers (name: /continue/i) but the three new tests use exact match (name: 'Back'). I verified there's no Back to ... collision in PdsGoalCalculator/**, so the exact match is safe — but name: /back/i would be consistent with the file-local convention.
Flagged by Testing agent.
![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: LOW (3/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)
This PR was auto-approved because:
- The multi-agent AI review determined it is low 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.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 4-space to 2-space indentation change was unintentional and created noise in the PR. Restoring locales to match MPDX-9475. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Description
mainonce that PR mergesTesting
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions