Conversation
Replace the external StaffWeb MPGA URL with the in-app /reports/mpgaIncomeExpenses route so users stay in MPDX when looking up their averages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bundle sizes [mpdx-react]Compared against 7469fb8 No significant changes found |
wjames111
added
On Staging
|
Preview branch generated at https://MPDX-9618.d3dytjb8adxkk5.amplifyapp.com |
wjames111
left a comment
wjames111
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — Verdict: APPROVED WITH SUGGESTIONS
Risk: 1/10 (LOW) · Files: 1 · Lines: +9 / −4 · Mode: standard · Agents: 5
Nice, tight refactor — replacing an external StaffWeb link with an internal Next.js route and matching the codebase's established Link component={NextLink} and useAccountListId() ?? '' conventions. Verified that the target route pages/accountLists/[accountListId]/reports/mpgaIncomeExpenses/index.page.tsx exists.
Summary of findings
| Severity | Count | Notes |
|---|---|---|
| Critical / High blockers | 0 | None |
| Important (7.0–7.9) | 1 | Instruction text references a section label that doesn't exist on the destination |
| Medium (5.0–6.9) | 1 | Defensive ?? '' fallback produces a malformed URL when accountListId is undefined |
| Suggestions (< 5.0) | 3 | Translation key churn (run yarn extract), anchor text wording, impersonation edge case |
Recommended before merge
Fix finding #1 (the “Ministry Expenses section” copy mismatch — see the inline comment on line 36). The destination report's labeled sections are Summary, Expenses Categories, Income, Expenses, and Monthly Summary — there is no section literally named Ministry Expenses. Ministry does appear as one of four slices inside the Expenses Categories pie chart, so the data is reachable, but the wording will send users hunting for the wrong label.
Pattern compliance (verified)
- Named exports only ✓
Link component={NextLink}matches 9+ existing usages insrc/components/HrTools/**✓useAccountListId() ?? ''matches 7+ existing usages insrc/components/HrTools/GoalCalculator/**✓- All user-visible strings wrapped in
<Trans t={t}>✓ - No
any, no@ts-ignore, no non-null assertions ✓ - Target route exists and is reachable ✓
Auto-approval eligibility
This PR qualifies for auto-approval (LOW risk, APPROVED_WITH_SUGGESTIONS). To dismiss any non-blocking finding you disagree with, reply /dismiss: <reason> on the inline comment.
| return ( | ||
| <InstructionsWrapper> | ||
| <Typography variant="body2"> | ||
| <Trans t={t}> |
There was a problem hiding this comment.
The <Trans> body text changed (link anchor went from MPGA tool on StaffWeb to MPGA report, and the trailing sentence was shortened). Since i18next-parser.config.js uses keySeparator: false and defaultValue: key, the English source text IS the translation key — so the old entry in public/locales/en/translation.json will become orphaned after this PR lands.
Action: run yarn extract and commit the regenerated public/locales/en/translation.json. Non-English locales will fall back to the source text until CrowdIn re-syncs (normal flow).
![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 (1/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.
1 participant
Description
/accountLists/{id}/reports/mpgaIncomeExpenses).Link component={NextLink}for client-side routing anduseAccountListId()to build the path, consistent withSummaryReport.tsx.Jira: MPDX-9618
Testing
Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions