MPDX-9374 - Display effective approved salary instead of current salary by zweatshirt · Pull Request #1815 · CruGlobal/mpdx-react

zweatshirt · 2026-06-02T20:46:50Z

Description

This ticket slipped through the cracks: https://jira.cru.org/browse/MPDX-9374

Sorry about that!

This displays the user and user spouse's current requested salary from their latest approved request.
I took the ticket description to mean we want to remove the current gross salary from the display and replace it with this value. Do we instead want to display both?

Testing

Go to SalaryCalculator
Continue to Step 3
Check that the effective request's salary and spouse salary amount display as expected.

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

… request handling

github-actions · 2026-06-02T20:50:23Z

Bundle sizes [mpdx-react]

Compared against 351639a

Route	Size (gzipped)	Diff
`/accountLists/[accountListId]/hrTools/pdsGoalCalculator`	160.7 KB	-2.19 KB
`/accountLists/[accountListId]/hrTools/pdsGoalCalculator/[pdsGoalId]`	331.72 KB	-4.49 KB

zweatshirt

Multi-Agent Code Review — MPDX-9374

Verdict: ✅ APPROVED_WITH_SUGGESTIONS · Risk: LOW (2/10) · Agents: Architecture, Testing, Standards, UX, Financial Reporting (standard mode)

Small, well-tested display change that swaps the step-3 "Requested Salary" card's first row from current gross salary to the latest effective (approved) salary request, with a dash fallback. Closely mirrors the established SalarySummaryCard pattern. No blockers — lint:ts clean, 13/13 tests pass.

Findings (all non-blocking)

#	Severity	Where	Item
1	Medium 5.5	`RequestedSalaryCard.tsx:144`	Empty-state shows `–` while every other cell here (and `SalarySummaryCard`) shows `$0.00`. Defensible/arguably an improvement — confirm it's intentional.
2	Suggestion 4.0	`RequestedSalaryCard.tsx:144,147`	Truthiness `salary ? … : '–'` renders `–` for a legitimate `$0`. Low risk (minimum-salary floor). Use `salary != null` if `$0` should show as `$0.00`.
3	Suggestion 3.0	`RequestedSalaryCard.tsx:42`	`loading`/`error` not handled — transient `–` during load. Matches sibling; query is cache-warm.
4	Suggestion 3.5	`RequestedSalaryCard.tsx:44`	The `useEffectiveSalaryCalculationQuery` + `orientSalaryRequest` idiom is now in two cards — optional shared hook. (Do NOT lift into context — query is variable-free, cards render in different steps.)
5	Suggestion 3.0	test file	New cell assertions use `waitFor(getAllByRole)` rather than `findBy*`. Matches existing convention in this file; defer.

Financial checklist: arithmetic safe ✅ · rounding at display boundary ✅ · null/undefined handled ✅ (with the 0-truthiness caveat) · currency mixing / Luxon / server aggregations N/A. Orientation argument (hcmUser.staffInfo.personNumber) is consistent with SalarySummaryCard — no perspective-swap bug.

Four of five agents independently converged on item 2; none is a blocker. No debate round was needed (no finding ≥7, strong consensus).

zweatshirt · 2026-06-02T21:00:44Z

-                {formatCurrency(hcmUser?.currentSalary.grossSalaryAmount)}
+                {t('Current Requested Salary')}
              </TableCell>
+              <TableCell>{salary ? formatCurrency(salary) : '–'}</TableCell>


[Medium] Empty-state divergence from the sibling card. This renders `–` for "no approved request," but every other money cell in this table — and all of `SalarySummaryCard` — renders `$0.00` (via `formatCurrency`'s `?? 0`). `SalarySummaryCard` handles the same "no effective request" state by omitting the whole "Old" column instead. The dash is defensible (it distinguishes "no prior request" from "$0.00 approved") and arguably an improvement — flagging only so it's a conscious choice rather than an accident. Confirm it's intentional.

The '-' aligns with the ticket description, but I can change that.

zweatshirt · 2026-06-02T21:00:44Z

              {hcmSpouse && (
                <TableCell>
-                  {formatCurrency(hcmSpouse.currentSalary.grossSalaryAmount)}
+                  {spouseSalary ? formatCurrency(spouseSalary) : '–'}


[Suggestion] Truthiness guard hides a legitimate `$0`. `spouseSalary ? formatCurrency(spouseSalary) : '–'` (and line 144) treats `0` as empty, so a real approved `$0` would render `–` instead of `$0.00`. Real-world risk is low — the requested-salary domain has a minimum-salary floor (Yup `min`) — so this may be exactly the intended behavior. If you want strict correctness, use `salary != null ? formatCurrency(salary) : '–'` and add a `salary: 0` test to pin the behavior. Flagged by 4 of 5 agents; author's call.

zweatshirt · 2026-06-02T21:00:44Z

  const { isUserSosa, blockOnCap } = useSosaBlockOverCap();
  const { markValid, markInvalid } = useAutosaveForm();

+  const { data: effectiveData } = useEffectiveSalaryCalculationQuery();


[Suggestion] Only `data` is destructured — `loading`/`error` aren't handled. During load, `salary` is `undefined`, so the cell transiently shows `–`, which is indistinguishable from the real "no approved request" empty state. Low impact: the sibling `SalarySummaryCard` does the same, and this variable-free query is almost always cache-warm by the time the card mounts. If you address it (e.g. a `Skeleton`), do it across both cards.

zweatshirt · 2026-06-02T21:00:44Z


+  const { data: effectiveData } = useEffectiveSalaryCalculationQuery();
+  const { salary, spouseSalary } =
+    orientSalaryRequest(


[Suggestion] The `useEffectiveSalaryCalculationQuery()` + `orientSalaryRequest(..., hcmUser?.staffInfo.personNumber)` idiom now appears in both this card and `SalarySummaryCard`. A small shared `useEffectiveSalaryRequest()` hook would DRY it. Optional. Note: do NOT lift the value into `SalaryCalculatorContext` — the query takes no variables (Apollo dedupes) and the two cards render in different steps, so leaf-level fetching is correct.

github-actions · 2026-06-02T21:04:37Z

Preview branch generated at https://MPDX-9374.d3dytjb8adxkk5.amplifyapp.com

zweatshirt · 2026-06-02T21:07:17Z

-                {t('Current Salary')}
-              </TableCell>
-              <TableCell>
-                {formatCurrency(hcmUser?.currentSalary.grossSalaryAmount)}


@canac By the ticket description, I wasn't sure if we wanted to include both the Current Salary and the effective Current Requested Salary, or replace the Current Salary. I chose the latter but let me know if I should include both rows.

Probably not both. I think we wanted to show the current requested (not gross) so they can easily compare it with their new requested salary.

github-actions

AI Review Auto-Approval

Risk Level: LOW (2/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.

canac

Looks right to me!

Enhance salary calculation tests and components with effective salary…

337699b

… request handling

zweatshirt commented Jun 2, 2026

View reviewed changes

zweatshirt requested a review from canac June 2, 2026 21:03

zweatshirt marked this pull request as ready for review June 2, 2026 21:03

zweatshirt self-assigned this Jun 2, 2026

zweatshirt added the Preview Environment Add this label to create an Amplify Preview label Jun 2, 2026

zweatshirt commented Jun 2, 2026

View reviewed changes

github-actions Bot approved these changes Jun 2, 2026

View reviewed changes

canac approved these changes Jun 3, 2026

View reviewed changes

zweatshirt enabled auto-merge June 3, 2026 18:25

zweatshirt merged commit 43737ca into main Jun 3, 2026
115 of 120 checks passed

zweatshirt deleted the MPDX-9374 branch June 3, 2026 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPDX-9374 - Display effective approved salary instead of current salary#1815

MPDX-9374 - Display effective approved salary instead of current salary#1815
zweatshirt merged 1 commit into
mainfrom
MPDX-9374

zweatshirt commented Jun 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 2, 2026 •

edited

Loading

Uh oh!

zweatshirt left a comment

Uh oh!

zweatshirt Jun 2, 2026

Uh oh!

zweatshirt Jun 2, 2026

Uh oh!

zweatshirt Jun 2, 2026

Uh oh!

zweatshirt Jun 2, 2026

Uh oh!

zweatshirt Jun 2, 2026

Uh oh!

github-actions Bot commented Jun 2, 2026

Uh oh!

zweatshirt Jun 2, 2026

Uh oh!

canac Jun 3, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

canac left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

zweatshirt commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

github-actions Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle sizes [mpdx-react]

Uh oh!

zweatshirt left a comment

Choose a reason for hiding this comment

Multi-Agent Code Review — MPDX-9374

Findings (all non-blocking)

Uh oh!

zweatshirt Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

zweatshirt Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

zweatshirt Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

zweatshirt Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

zweatshirt Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 2, 2026

Uh oh!

zweatshirt Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

canac Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

AI Review Auto-Approval

Uh oh!

canac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zweatshirt commented Jun 2, 2026 •

edited

Loading

github-actions Bot commented Jun 2, 2026 •

edited

Loading