[no-jira] - Backend admin UI adjustments - #2019
Conversation
wjames111
left a comment
wjames111
left a comment
There was a problem hiding this comment.
Multi-agent review — 7 specialized reviewers
Verdict: BLOCKERS FOUND. Risk CRITICAL (10/10) — driven by breadth (36 files spanning routes, feature components, GraphQL operations and the global theme), not by any single dangerous file. Required reviewer level: SENIOR.
Agents: Security, Architecture, Data Integrity, Testing, UX, Standards, plus this repo's Financial Reporting domain agent. Selection followed .claude/rules/code-review.md; Security was added as a judgement call because the PR introduces a route rendering another household's goal data from a URL parameter.
The two blockers, both proven by mutation rather than argued
- The unsaved-changes guard has no coverage on any path the app uses (9.5). Changing it to
dirty && !returnUrl— silently discarding edits on every production path — leaves 390 tests passing. - Nothing clicks the red Save & Share (9.0), the PR's headline behaviour change. It currently works (probed), but either half could regress green.
Findings ledger
Findings 1-24 are posted as inline comments on the relevant lines. The rest are below.
| # | Sev | Finding |
|---|---|---|
| 25 | 6.5 | The coaching page's new returnUrl/returnLabel is untested — its test file was untouched, while the new Staff Details page got exactly that test. grep returnLabel across all tests returns nothing, so the ?? t('Back to Table') fallback is only ever exercised on its default side. |
| 26 | 6.5 | The save-failure try/catch in GoalSettingsForm is untested. Its whole purpose is not navigating away on failure; drop the return and the user is bounced to the table with edits gone, suite still green. |
| 27 | 6.0 | The tab round trip was lost. push-shape and render-from-URL are each asserted, but nothing connects them, so renaming an enum value on one side only would pass both. aria-selected also unasserted. |
| 28 | 6.0 | Two new cross-feature import edges: MpdGoalAdmin reaches into NsGoalCalculator/GoalSettings for isCalculationComplete, and mpdGoalAdminHelpers.ts (a .ts) imports a type from a .tsx, pointing the data layer at the presentation layer. Moving the predicate to HrTools/Shared/ resolves both. |
| 29 | 6.0 | The completeness rule now has a second, unlinked encoding in the Yup schema. They agree today; nothing keeps them agreeing, and drift means a green "Complete" chip above a red Save button. A test asserting the two field lists match would fail CI instead. |
| 30 | 5.5 | returnToTable() does two unrelated things depending on a prop, and the discard() branch is unreachable in production — which is why three of this PR's Cancel tests assert behaviour no page has. |
| 31 | 4.0 | Scenario name cell should be component="th" scope="row" so the status cell announces which scenario it belongs to (20+ in-repo precedents). Same shape in GoalsTable. |
| 32 | 4.0 | StatusChip's border: 'none' passes for an accidental reason (jest-dom parses it to '', matching the computed empty value; without the line it's 0px). borderWidth: '0px' is more honest. |
| 33 | 4.0 | goalSettingsSchema.test.ts name says "required fields only" but arrayContaining cannot verify "only" — add a length assertion. |
| 34 | 3.5 | Dead beforeEach(() => push.mockClear()) — jest.config.js already sets clearMocks: true. The two new test files disagree with each other on this. |
| 35 | 3.5 | import { scenarioGoalUrl as buildScenarioGoalUrl } exists only to make room for a shadowing local wrapper; both call sites could use the shared helper directly. |
| 36 | 3.0 | StatusChip keeps color={color} only for the semantic class. Adding onClick/onDelete later would pull in MUI's clickableColorX/deletableColorX hover rules, which the sx does not override — the tint would invert. |
Verified good, with evidence
- Chip contrast passes AAA in all four states (measured under the real theme): warning 8.74:1, success 9.80:1, info 9.58:1, error 10.73:1 — and matches the Figma tints exactly (
#FFF4E5/#663C00). - Cache normalization intact —
idselected in both changed operations, no typePolicy needed, no cross-table collision. - No in-place mutation of cache-owned data; the load-bearing
[...attendee.coordinators]defensive copy is preserved. tenure: 0handled correctly in both directions — verified by running yup directly, and pinned by an existing test.- The 7
TestRouterwrappers are purely structural — no assertion loosened, removed or re-scoped. StatusChip's assertions are not vacuous — proven by reverting the theme change and thesxline.- lint, tsc, prettier and codegen all clean; 392 tests pass in the PR's scope.
- No merge-order dependency.
accountListIdis already onapi.stage.mpdx.organdyarn gqlruns clean against it.
Pre-existing, informational only
- The scenario route shares Finding 6's guard gap (that line is not in this diff).
- Filled colour chips already render white-on-white text (1:1) because the theme forces
backgroundColor: '#fff'— which is precisely whyStatusChipmust override the background at all. - Tab content is not a
tabpanel(noaria-controls/role="tabpanel"). MpdGoalAdmin.test.tsx:143awaitsuserEvent.click, against repo convention (2028 bare vs 33 awaited).- act() warnings are endemic here (this PR's new page test emits 8; the untouched sibling emits 5,
GoalSettingsView.test.tsx18) — not a PR defect.
Findings deliberately not addressed: none yet — the author is reviewing the ledger before deciding.
| {...props} | ||
| > | ||
| <GoalSettingsForm accountListId={accountListId} /> | ||
| <GoalSettingsNavigationProvider> |
There was a problem hiding this comment.
This harness omits returnUrl, so returnToTable() takes the discard() branch and router.push is never involved. All three production pages do pass a returnUrl, so the dialog -> Discard -> navigate sequence (the actual data-loss prevention) is untested.
Proven by mutation: changing the guard to if (formRef.current.dirty && !returnUrl) — which silently discards edits on every production path — leaves 390 tests passing.
Best fixed in GoalSettingsView.test.tsx, the only place the provider wraps both the sidebar and the form: type into a field, click Back to Table, assert the dialog appears and push was not called, then Discard and assert push was called with the return URL.
|
|
||
| await waitFor(() => expect(saveButton).toBeDisabled()); | ||
| await waitFor(() => | ||
| expect(saveButton).toHaveClass('MuiButton-containedError'), |
There was a problem hiding this comment.
This test asserts only the MUI colour class. The nearby "blocks submit and flags the field" test types an invalid value and tabs away; it never clicks the button (it couldn't, when the button was disabled). So the PR's headline behaviour change is unexercised.
A probe confirms it currently works — REQUIRED MESSAGES VISIBLE: ["Benefits Plan is required"], MUTATION FIRED: false — but a regression in either half (silent no-op, or an invalid payload escaping to UpdateNewStaffGoalCalculation) would ship green.
Suggested: render an incomplete mock (benefitsPlan: null), click the red button, assert the required message is visible and expect(mutationSpy).not.toHaveGraphqlOperation('UpdateNewStaffGoalCalculation'). That assertion also replaces the class-name coupling with intent.
| } | ||
| }, [returnUrl, router]); | ||
|
|
||
| const leave = useCallback(() => { |
There was a problem hiding this comment.
This provider's docblock says it "owns everything that leaves Goal Settings". It owns Back-to-Table and Cancel. The same sidebar has three NavItems whose onSelect calls setView, each of which unmounts the Formik tree and destroys unsaved edits with no confirmation. There is also no routeChangeStart / beforePopState / beforeunload hook, so browser Back and tab-close lose edits too.
Two agents independently rated this 7.0-7.5, and the framing is worth keeping: this is arguably worse than no guard, because a confirmation on Cancel teaches the user that this sidebar protects their edits — and the three items below the divider silently do not.
Smallest fix: generalise leave to leave(proceed?: () => void) defaulting to returnToTable, and route each setView through it.
Require marital status, reject the '' sentinel, and correct the chip doc comment
…istration Add an ErrorOutline icon and a describeChild tooltip to Save & Share while invalid, and unregister the form on unmount.
Wait on the Unsaved Changes dialog closing and the valid-form button color instead of always-true assertions, and drop the beforeEach that clearMocks already covers
Label spouse required-field errors per person, require maritalStatus, and cover married and zero-tenure cases in the schema test
Wrap setActiveTab in useCallback([router]) and list it in the context useMemo deps
Adds describes for parseMpdGoalAdminTab, mpdGoalAdminUrl, staffDetailsUrl, scenarioGoalUrl and goalStatusColor
useAccountListId returns string and throws when absent, so drop the ?? '' fallbacks and the aliased scenario URL wrapper
Replace raw .click() with userEvent.click and await the push assertion via waitFor
Bundle sizes [mpdx-react]Compared against 42130df
|
wjames111
added
Preview Environment
|
Preview branch generated at https://backend-admin-ui-adjustments.d3dytjb8adxkk5.amplifyapp.com |
…stments # Conflicts: # src/components/HrTools/MpdGoalAdmin/MpdGoalAdminContext.tsx
1 participant
Description
A batch of adjustments to the MPD Goal Calculator admin table and Goal Settings.
no chip anywhere has a box shadow now. Goal status chips (admin table, scenario table, Goal
Settings header) share a new
StatusChipwith the solid tinted fill from the designs."Active MPD Goals", plus a Goal Status column showing Complete/Incomplete.
back to the tab a goal was opened from. Opened from the coaching page instead, the link reads
"Back to Coaching" and returns there; with no table behind it, the link is not rendered at all.
confirmation when there are unsaved edits: Keep Editing stays with the edits intact, Discard
Changes returns to the table. An untouched form leaves straight away.
stays clickable so submitting names the missing fields under each one. A successful save returns
to the table the goal was opened from.
mpdGoalAdmin/staff/[staffAccountListId]route that renders the same Goal Settings view.Depends on CruGlobal/mpdx_api#3595, which exposes the
accountListIdthe View/Edit link needs.Codegen will fail here until that ships to staging.
Two items from the request turned out to need no change and are unchanged: coordinators already
render under the Coach field in Goal Settings (still MPDX-9796 placeholder data), and training
costs already show "Provide Training Cost" with an orange warning icon while Run and Send All and
Print All are already disabled.
Testing
edit, Discard Changes returns to the Active Goals tab
the calculation year), check Save & Share turns red and clicking it names the missing fields
column, open a scenario goal and check Back to Table returns to that tab
Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions