MPDX-9352, MPDX-9353, MPDX-9354 Customized login designs by kegrimes · Pull Request #1687 · CruGlobal/mpdx-react

kegrimes · 2026-04-08T14:19:22Z

Description

Customized login for different user groups requires a few designs:

Add new "HR Tools" tab for MHA, ASR, Salary calculator, Goal calculator, Savings fund transfer, and ministry partner reminders
Add MPGA and staff expenses to "Reports" tab
Change "Tools" to "MPDX Tools"
Add new sidebar for HR Tools
Refactor NoStaffAccount component to include wrong user group message
Create barebones confirmation modal for new user_type field from backend

Not sure what the path name should be for HR Tools

Tickets:

MPDX-9240 --> new tab order
MPDX-9351 --> connect graphql
MPDX-9352 --> access denied page
MPDX-9353 --> confirmation modal
MPDX-9354 --> tab designs & sidebar

Testing

Go to dashboard
Verify modal pops up with user group from backend
Click somewhere else and verify the modal disappears
Refresh the page and verify the modal appears again
Click "No, request changes", and ensure that the modal closed and opened email window
Login to another account
Click "Yes, Confirm" and ensure modal closes with successful snackbar
Reload page and ensure modal does not appear
Click on "Reports" tab and verify staff web reports are present
Click on "HR Tools" tab and verify new forms are present
Click search icon & search "Request"
Check that new request forms are showing up

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

github-actions · 2026-04-08T14:20:05Z

Preview branch generated at https://designs-for-customized-login.d3dytjb8adxkk5.amplifyapp.com

github-actions · 2026-04-08T14:22:51Z

Bundle sizes [mpdx-react]

Compared against 27635fd

Route	Size (gzipped)	Diff
`/accountLists/[accountListId]/hrTools/additionalSalaryRequest`	170.89 KB	added
`/accountLists/[accountListId]/hrTools/goalCalculator`	125.26 KB	added
`/accountLists/[accountListId]/hrTools/goalCalculator/[goalCalculationId]`	434.57 KB	added
`/accountLists/[accountListId]/hrTools/housingAllowance`	160.01 KB	added
`/accountLists/[accountListId]/hrTools/housingAllowance/[requestId]`	174.41 KB	added
`/accountLists/[accountListId]/hrTools/mpReminders`	321.27 KB	added
`/accountLists/[accountListId]/hrTools/salaryCalculator`	157.6 KB	added
`/accountLists/[accountListId]/hrTools/salaryCalculator/[calculationId]`	168.98 KB	added
`/accountLists/[accountListId]/hrTools/staffSavingFund`	108.21 KB	added
`/accountLists/[accountListId]/hrTools/staffSavingFund/transfers`	324.25 KB	added
`/accountLists/[accountListId]/reports/additionalSalaryRequest`	no change	removed
`/accountLists/[accountListId]/reports/goalCalculator`	no change	removed
`/accountLists/[accountListId]/reports/goalCalculator/[goalCalculationId]`	no change	removed
`/accountLists/[accountListId]/reports/housingAllowance`	no change	removed
`/accountLists/[accountListId]/reports/housingAllowance/[requestId]`	no change	removed
`/accountLists/[accountListId]/reports/mpReminders`	no change	removed
`/accountLists/[accountListId]/reports/salaryCalculator`	no change	removed
`/accountLists/[accountListId]/reports/salaryCalculator/[calculationId]`	no change	removed
`/accountLists/[accountListId]/reports/staffSavingFund`	no change	removed
`/accountLists/[accountListId]/reports/staffSavingFund/transfers`	no change	removed

kegrimes · 2026-04-09T14:06:01Z

🤖 Multi-Agent Code Review Report

PR: #1687 — MPDX-9352, MPDX-9353, MPDX-9354 Customized login designs
Branch: designs_for_customized_login → main
Agents: 5 (Architecture, Data Integrity, Testing, UX, Standards)
Review Mode: Standard | Risk Level: HIGH (7/15) | Consensus: 95%

📊 Risk Assessment

Metric	Value
Risk Score	7/15 — HIGH
Files Changed	51 (+749 -113)
Required Reviewer	⚠️ Experienced developer or above

Risk Factors: Shared components modified, new GraphQL operations, cross-cutting navigation changes, new access control logic

🚫 CRITICAL BLOCKERS (Severity 9-10) — Must fix before merge

1. Missing `id` field in `GetUserGroup.graphql` — Apollo cache broken

Severity: 9.7/10 (Consensus from ALL 5 agents)
File: src/components/Shared/ConfirmUserGroupModal/GetUserGroup.graphql:1-5

The query fetches user { userType } without an id field. CLAUDE.md requires: "Always include id fields in queries/mutations for Apollo cache normalization." Every other user query in the codebase (20+) includes id. Without it, Apollo cannot normalize the User object — cache updates from mutations won't propagate, causing stale/inconsistent data.

Fix:

query GetUserGroup {
  user {
    id
    userType
  }
}

Then run yarn gql.

2. Conditional Hook Calls in `MultiPageMenu.tsx` — React Rules violation

Severity: 10/10 (Architecture agent)
File: src/components/Shared/MultiPageLayout/MultiPageMenu/MultiPageMenu.tsx:102-107

Hooks are called inside a ternary expression:

const navItems =
    navType === NavTypeEnum.Reports
      ? useReportNavItems()
      : navType === NavTypeEnum.HrTools
        ? useHrToolsNavItems()
        : useSettingsNavItems();

This violates React's Rules of Hooks. If navType changes between renders, different hooks are called in the same slot, corrupting React's internal state.

Fix: Call all hooks unconditionally:

const reportNavItems = useReportNavItems();
const hrToolsNavItems = useHrToolsNavItems();
const settingsNavItems = useSettingsNavItems();
const navItems =
    navType === NavTypeEnum.Reports
      ? reportNavItems
      : navType === NavTypeEnum.HrTools
        ? hrToolsNavItems
        : settingsNavItems;

Note: This extends a pre-existing pattern, but adding useHrToolsNavItems makes it worse.

🔴 HIGH PRIORITY (Severity 7-8) — Should fix before merge

3. GraphQL operation name starts with "Get"

Severity: 7.0/10 | File: GetUserGroup.graphql:1

CLAUDE.md: "Use descriptive names, not starting with 'Get' or 'Load'." Rename to UserGroup.

4. Fire-and-forget mutations — no error handling

Severity: 7.6/10 (4 agents) | File: ConfirmUserGroupModal.tsx:33-53

Both handleRequestChange and handleCustomConfirm call updateUserOption() without await, .catch(), or result checking. If the mutation fails, the user sees the modal close and believes their preference was saved. handleRequestChange also fires window.location.href = 'mailto:...' regardless of success.

Fix: await the mutation, handle errors with a snackbar, only close on success.

5. Non-localized strings in `getUserType.ts`

Severity: 7.0/10 (4 agents) | File: getUserType.ts:5-12

Hardcoded English strings ('Cru US Staff', 'Cru Global Staff', 'Non Cru User', 'Unknown') displayed in the modal. All user-visible text must use t().

⚠️ IMPORTANT ISSUES (Severity 6-7)

6. Dashboard `useEffect` logic — PENDING check is dead code

Severity: 6.8/10 (ALL 5 agents) | File: Dashboard.tsx:42

if (!loading && !userGroup && userGroup !== 'PENDING') {

When userGroup is 'PENDING', !userGroup is already false — the !== 'PENDING' check is never reached. Works by accident but misleading. Simplify to !loading && !userGroup.

7. Hardcoded `NavMenu` array indices

Severity: 7.0/10 (2 agents) | File: NavMenu.tsx:190-212

navPages[3], navPages[4], navPages[5] are brittle. If page order changes, menus silently break. Use navPages.find(p => p.id === '...') instead.

8. Fragmented localization strings

Severity: 6.0/10 (2 agents) | Files: ConfirmUserGroupModal.tsx:70-79, LimitedAccess.tsx:34-46

Sentences split around <Link> into multiple t() calls. Translators cannot reorder. Use <Trans> component or interpolation.

💡 MEDIUM PRIORITY & SUGGESTIONS

No test for getUserType utility (4 branches, only 1 tested indirectly)
Missing await on userEvent.click() in ConfirmUserGroupModal.test.tsx:99,133,169
No test for useHrToolsNavItems hook
No error handling for useGetUserGroupQuery failure (renders "Unknown" silently)
window.location mock cleanup not in afterEach
Consider skip: !open on useGetUserGroupQuery to avoid unnecessary requests
Consider merging userType fetch into existing GetDashboard query

📦 Dependency Impact

Component	Dependents
MultiPageHeader	18 files
MultiPageMenu	6 files
LimitedAccess	6 files
useNavPages	6 files

Breaking Change: NoStaffAccount export removed → replaced by LimitedAccess (all usages migrated ✅)

✅ What's Good

Clean refactor of NoStaffAccount → LimitedAccess with broader use case support
Proper separation of HR Tools nav items into dedicated hook
Good test coverage for new components (ConfirmUserGroupModal, LimitedAccess)
Navigation restructuring is well-organized (Reports / HR Tools / MPDX Tools)
Consistent use of GqlMockedProvider patterns in tests

🎯 Checklist

Must fix:

Add id field to GetUserGroup.graphql + re-run yarn gql
Fix conditional hook calls in MultiPageMenu.tsx
Rename GetUserGroup → UserGroup
await mutations + add error handling in ConfirmUserGroupModal.tsx
Localize strings in getUserType.ts

Should fix:

Simplify dead PENDING check in Dashboard.tsx:42
Replace hardcoded navPages[N] indices in NavMenu.tsx
Fix fragmented localization strings

🤖 Generated by AI Multi-Agent Review (5 agents: Architecture, Data Integrity, Testing, UX, Standards)

kegrimes · 2026-04-09T17:34:21Z

@canac I won't merge this PR until customized login is fully done since I don't want users to be able to click on the new request forms if they don't have access. Also, what do you think the path name should be for the HR Tools? Right now it is /requests, but I don't think that is fitting.

canac

This is solid! I think /hrTools is a find name for the URL. I think it's a little better than requests, since it also includes the goal calculator.

If this PR will be sitting open for a while, one thing you could do is have a feature-flag environment variable, like DISABLE_HR_TOOLS. We set up Amplify to set that flag in production and check it in this PR.

I assume in a future PR we'll be checking the user type to determine which nav menus to show and hide?

src/components/Dashboard/Dashboard.tsx

src/components/Shared/ConfirmUserGroupModal/UserGroup.graphql

src/components/Shared/ConfirmUserGroupModal/ConfirmUserGroupModal.tsx

src/components/Dashboard/Dashboard.tsx

src/components/Shared/ConfirmUserGroupModal/ConfirmUserGroupModal.tsx

canac · 2026-04-09T20:01:17Z

You can also look into using the useUserPreference hook. The interface is like useState, but it automatically saves to a user-preference behind the scenes.

…d_login

kegrimes · 2026-04-10T12:47:54Z

@canac I tried to use the user preference hook, however, there was a timing issue where the modal would open/close based on the default option which wasn't correct. I thought it better to use the query and mutation directly.

Also, do you think we should rename /tools to /mpdxTools for consistency?

canac · 2026-04-10T16:18:59Z

That makes sense! Just wanted to make sure you knew about that hook if you didn't before.

Also, do you think we should rename /tools to /mpdxTools for consistency?

Probably not because we'd have to update documentation links and any users' bookmarks would break. I think /tools is a little cleaner too.

canac

Code looks good! I'm going to wait to approve until either we're ready to launch these forms or until we set up the feature flag to hide them in production (I can help you set that up if you want!).

canac · 2026-04-10T16:22:35Z

src/hooks/useUserPreference.ts

      const serializedValue =
        typeof newValue === 'string' ? newValue : JSON.stringify(newValue);
-      updateUserOption({
+      return updateUserOption({


Do we still need this change?

canac · 2026-04-10T16:24:34Z

src/hooks/useNavPages.tsx

+      },
+      {
+        id: 'mpdx-tools-page',
+        title: t('MPDX Tools'),


What do you think about about keeping this as "Tools"?

Also, what about moving it before "HR Tools" here and anywhere else?

Suggested change

title: t('MPDX Tools'),

title: t('Tools'),

Add request forms to navigation bar

8cac75f

kegrimes self-assigned this Apr 8, 2026

kegrimes added the Preview Environment Add this label to create an Amplify Preview label Apr 8, 2026

Katelyn Grimes added 5 commits April 8, 2026 11:08

Updated tab designs from stakeholders

dcd3d3e

Added sidebar for hr tools tab

c226b5f

Fixed some failing tests

4443b37

Refactor no staff account to include wrong user group

a286a7b

Added confirmation modal on dashboard

a0aaa6d

kegrimes added the Staging API Run GraphQL codegen against the staging API label Apr 9, 2026

Save user options on confirm and request changes

ff2bde0

Claude agent review suggestions

afe138a

kegrimes requested a review from canac April 9, 2026 17:32

canac reviewed Apr 9, 2026

View reviewed changes

src/components/Shared/ConfirmUserGroupModal/ConfirmUserGroupModal.tsx Outdated Show resolved Hide resolved

Katelyn Grimes added 3 commits April 9, 2026 16:27

Review suggestions

6c9bce0

Revert user preference hook

f055c9f

Merge remote-tracking branch 'origin/main' into designs_for_customize…

2e9d47e

…d_login

kegrimes requested a review from canac April 10, 2026 12:47

canac reviewed Apr 10, 2026

View reviewed changes

Conversation

kegrimes commented Apr 8, 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 Apr 8, 2026

Uh oh!

github-actions bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle sizes [mpdx-react]

Uh oh!

kegrimes commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Multi-Agent Code Review Report

📊 Risk Assessment

🚫 CRITICAL BLOCKERS (Severity 9-10) — Must fix before merge

1. Missing id field in GetUserGroup.graphql — Apollo cache broken

2. Conditional Hook Calls in MultiPageMenu.tsx — React Rules violation

🔴 HIGH PRIORITY (Severity 7-8) — Should fix before merge

3. GraphQL operation name starts with "Get"

4. Fire-and-forget mutations — no error handling

5. Non-localized strings in getUserType.ts

⚠️ IMPORTANT ISSUES (Severity 6-7)

6. Dashboard useEffect logic — PENDING check is dead code

7. Hardcoded NavMenu array indices

8. Fragmented localization strings

💡 MEDIUM PRIORITY & SUGGESTIONS

📦 Dependency Impact

✅ What's Good

🎯 Checklist

Uh oh!

kegrimes commented Apr 9, 2026

Uh oh!

canac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

canac commented Apr 9, 2026

Uh oh!

kegrimes commented Apr 10, 2026

Uh oh!

canac commented Apr 10, 2026

Uh oh!

canac left a comment

Choose a reason for hiding this comment

Uh oh!

canac Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

canac Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kegrimes commented Apr 8, 2026 •

edited

Loading

github-actions bot commented Apr 8, 2026 •

edited

Loading

kegrimes commented Apr 9, 2026 •

edited

Loading

1. Missing `id` field in `GetUserGroup.graphql` — Apollo cache broken

2. Conditional Hook Calls in `MultiPageMenu.tsx` — React Rules violation

5. Non-localized strings in `getUserType.ts`

6. Dashboard `useEffect` logic — PENDING check is dead code

7. Hardcoded `NavMenu` array indices