MPDX-9826 Hide Re-Entry and Return Travel

[kegrimes]

Description

On Staff expenses report, hide Re-Entry and Return Travel fund types unless a user is an international staff or staff stint. These new US staff groups have already been added on the backend.

Extra:

• Reorder fund types on staff expenses AND savings fund transfer: Primary, Conference Savings, Savings, ...
• Add Re-entry and Return Travel funds to savings fund transfer

Jira ticket: MPDX-9826

Testing

International Staff: ✅

• Impersonate: barrett.harkins@cru.org
• Go to /reports/staffExpense
• Check that all 5 fund types appear
• Verify the order is correct: Primary, Conference, Savings, Return Travel, Re-Entry
• Go to /hrTools/staffSavingFund/transfers
• Check that all 5 fund types appear
• Verify the order is correct: Primary, Conference, Savings, Return Travel, Re-Entry

Senior Staff: ✅

• Impersonate: courtney.campbell@cru.org
• Go to /reports/staffExpense
• Check that only 3 fund types appear
• Verify the order is correct: Primary, Conference, Savings
• Go to /hrTools/staffSavingFund/transfers
• Check that only 3 fund types appear
• Verify the order is correct: Primary, Conference, Savings

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 /quality:agent-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]

Preview branch generated at https://mpdx-9826-hide-funds-unless-new-group.d3dytjb8adxkk5.amplifyapp.com

[github-actions]

Bundle sizes [mpdx-react]

Compared against 80f6e52

No significant changes found

[kegrimes]

🤖 Multi-Agent Code Review — BLOCKERS FOUND

5 specialized agents + dependency analysis + gap review + 4 cross-examination rounds.

Verdict

2 blockers before merge:

1. overallBalance silently under-reports — a SeniorStaff user holding Return Travel / Re-Entry funds sees $3,500.00 instead of $5,500.00 in the Staff Expense Report headline. Measured empirically.
2. The four-group usStaffGroup predicate has no effective test coverage — the two "does not request" tests pass for every user group, and two of the four gated groups can be deleted from the source with the suite still green.

Risk

10/10 (CRITICAL) — but driven by file count (11 Medium-risk feature files) rather than any high-risk surface. No auth, API, Apollo config, schema, workflow, or dependency files touched. Reviewer level: MID-LEVEL/SENIOR.

Dependency impact

• Breaking changes: none. PrimaryAccount, SavingsAccount, ConferenceSavingsAccount, iconMap — 0 remaining references anywhere.
• CRITICAL fan-out: src/hooks/useIneligibleByGroup.ts — 2 direct consumers, ~18 transitive guarded pages.

---

❓ One open question that decides blocker #1

reportsStaffExpenses is a primary-API root field with no local resolver, so nothing in this repo establishes whether a plain SeniorStaff / NewStaff user can hold a nonzero Return Travel or Re-Entry balance.

• If yes → real money is silently subtracted from the headline total. Blocker stands at 9.0.
• If always $0 for those groups → the under-report is cosmetic and this drops to ~3.0, no longer blocking.

The realistic scenario is the group transition: a Re-Entry fund is at its maximum exactly when HCM flips SeniorInternationalStaff → SeniorStaff. Worth a one-line answer from the API team before merging.

---

✅ Findings the debate rounds cleared

Three findings looked serious in the first pass and did not survive cross-examination. Recording the corrections so they aren't re-litigated:

Finding	Pre-debate	Post-debate	What refuted it
useGetUserQuery missing loading/error	8.5	4.5	Both pages mount inside <UserTypeAccess>, which blocks children on userLoading and returns <LimitedAccess userGroupError /> on error (staffExpense/index.page.tsx:36, StaffSavingFundLayout.tsx:42, UserTypeAccess.tsx:71-77). Two agents ran guarded probes: one fund query, correct variables, first try. The double-fire everyone observed is an artifact of tests rendering the bare component — which is also the root cause of blocker #2.
useIneligibleByGroup access-widening	7.8	5.5	Git evidence: the predecessor hook useUsStaffGroups gated on HCM booleans; main-branch refactor 5ec62c491 replaced it with enum allowlists covering only 4 of 10 groups, dropping international/stint by omission. This PR restores them with four explicit per-group tests. HrTools/CLAUDE.md self-declares its eligibility table non-authoritative and states the guards are "UX gating, not the security boundary."
compareFundTypes intransitivity	7.0	5.5	Real and reproduced by three agents, but fundTypes is a server-side allowlist and both call sites pass literals that are a subset of fundTypeOrder's keys — reachability is zero today. Latent maintenance hazard, one-line fix.

---

Credit where due

• Three hardcoded hex colors (#F08020, #007890, #00C0D8) replaced with theme.palette.* tokens — a genuine design-system win.
• Nested-ternary icon selection and the iconMap string-munging layer both deleted; fund icon/color logic now has one implementation instead of two.
• fundTypeHelpers.test.ts is a new test file for a previously untested module.
• Clean: no any, @ts-ignore, ! assertions, console.*, debugger, bare TODOs, new Date(), fireEvent, commented-out code, or empty catch blocks. yarn eslint and yarn tsc --noEmit pass. All 6 affected suites pass (68 tests).

---

Findings on related files (not in this PR)

[Pre-existing, informational — do not block on this] src/components/HrTools/SavingsFundTransfer/Table/Row/createTableRow.tsx:199-213, 234-242, 263-271

<IconButton><Edit onClick={…} /></IconButton> puts the handler on the child SVG. Mouse clicks work; keyboard Enter/Space fires on the <button> and the handler never runs — keyboard-only users cannot Edit, Cancel Transfer, or Stop Transfer. Sibling buttons at :217-223 and :246-252 do it correctly. Confirmed pre-existing via git blame (69ac02febb, not on this branch). Severity 8.0. Worth a follow-up ticket.

---

Summary

Agent	Critical	High	Medium	Suggestions	Confidence
Architecture	0	0	5	4	High
Testing	1	1	4	6	High
Standards	0	0	5	2	High
UX	0	0	4	5	High
Financial Reporting	1	0	2	3	High
Gap Review	0	0	0	2	High
Total (deduped)	1	1	12	8

To dismiss a finding below severity 7, reply /dismiss: <reason> on its comment.

[kegrimes]

@zweatshirt I've been fighting with Claude for a couple hours now, so could you look this over for me? I think everything should be goo, and your reviews are pretty thorough so I appreciate anything you can give!

[zweatshirt]

@kegrimes What issue are you fighting with? Is it related to the agent-review? If so, please let me know, as I am the one to fix it

[zweatshirt]

If you think there are any suggestions the review agent made where it shouldn't block you, feel free to tag me in those too and I'll try to tweak the agent

[zweatshirt]

This looks and works great! Tried to find something to nitpick. Unsure why Claude is giving you a hard time.

[kegrimes]

@zweatshirt Thank you!

I think it was mostly concerned about the total at the top adding up to all fund accounts even if Re-entry and Return Travel were hidden. That was wrong behavior and it kept mentioning that. Also, the other blocker it found was on a test which I don't think should be blocking at all even though the test wasn't testing correctly. I could just read that as a suggestion and fix it!

I think I was just also annoyed at how long it was taking. I ran two reviews and I think one took like 30-45 min. It kept writing tests and then removing them which I think is beneficial but I felt like there was a lot it was doing for such a simple fix.

[zweatshirt]

@zweatshirt Thank you!

I think it was mostly concerned about the total at the top adding up to all fund accounts even if Re-entry and Return Travel were hidden. That was wrong behavior and it kept mentioning that. Also, the other blocker it found was on a test which I don't think should be blocking at all even though the test wasn't testing correctly. I could just read that as a suggestion and fix it!

I think I was just also annoyed at how long it was taking. I ran two reviews and I think one took like 30-45 min. It kept writing tests and then removing them which I think is beneficial but I felt like there was a lot it was doing for such a simple fix.

Yeah I don't know why it writes scratch tests. That's not something that exists in the prompt for the agent-review. Claude just decided to start doing that one day (even before I made changes to the skill). I'll try to tweak what I can. I think an issue we will run into in the future is that as the model changes, the skill itself will need to be changed to match the model