MPDX-9530 - PDS and MPD Goal Calculators: Shared Goal Card

[zweatshirt]

Description

Reduce duplication by making GoalCard a shared component between MPD Goal Calculator and PDS Goal Calculator
Jira ticket

Testing

• Go to PDS Goal Calculator and MPD Goal Calculator and test that View, Delete behavior still behave, and that the cards stylistically look the same as before creating the shared component.

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]

Preview branch generated at https://shared-goal-card.d3dytjb8adxkk5.amplifyapp.com

[github-actions]

Bundle sizes [mpdx-react]

Compared against 2f90b0b

No significant changes found

[zweatshirt]

Multi-Agent Code Review — PR #1760

Verdict: APPROVED WITH SUGGESTIONS — No blockers, no important issues. One legitimate Medium-priority i18n concern (orphaned translation key) and a few stylistic / follow-up suggestions.

Risk: 6/10 (MEDIUM) — refactor of a top-level Shared component affecting two feature trees.

Agents: Architecture, Testing, Standards, UX, Data Integrity (5 specialized reviewers)

Severity-spread investigation

During debate, Standards (8.5 BLOCKING) and Testing (7.0 Important) flagged a missing beforeEach(mutationSpy.mockClear()) in the new shared test, claiming the cancel test would fail with expected 0 calls, received 1. Architecture rated this 3.0 (cosmetic) noting the project has clearMocks: true. Direct verification of jest.config.js confirms clearMocks: true — Jest auto-clears all mocks between tests project-wide, so the explicit beforeEach is redundant and the shared test is correct as-is. The Standards/Testing BLOCKING flags are invalidated by this fact.

Highlights

What this PR does well

• Net −196 source lines via extraction; ~150 lines of duplicate layout/structure consolidated into one shared component
• MPD GoalCard gains ellipsis + tooltip behavior — visual parity with PDS card
• Removes the unused star/primary toggle feature (only GoalsList.tsx:76 calls <GoalCard>, never with renderStar)
• Test reorganization: presentational behavior covered by 8 shared-component tests; wrappers retain only data-plumbing tests (delete mutation, view URL, calculated total)
• Type system enforces parity — both wrappers must satisfy GoalCardProps
• All new t() strings are existing translation keys (no new entries needed in translation.json)

Acknowledged trade-offs

• Tooltip-on-hover always shows (even for non-truncated names) — explicitly chosen by author over runtime DOM-measurement complexity
• MPD card width changed from minWidth: 350 (flexible) to width: 350 (fixed) — intentional parity with PDS
• clearMocks: true makes the existing beforeEach(mutationSpy.mockClear()) in the MPD wrapper test redundant — leaving the inconsistency as a follow-up suggestion

Medium Priority (only one substantive finding)

M1. <Trans> variable rename will orphan the existing translation key (Severity 6.0)

See inline comment on Shared/GoalCard/GoalCard.tsx:93. The existing key in public/locales/en/translation.json:543 references goal.name ?? t('Unnamed Goal') literally; the new code uses displayName, so yarn extract produces a new key and the old one is pruned. ~10 locales lose this confirmation string until re-translated. Recoverable but worth deciding before merge.

Pre-existing (informational)

• Tooltip on a non-focusable Typography is keyboard-inaccessible. Same pattern in MPGAIncomeExpensesReport, SavingsFundTransfer. Codebase-wide concern, not a regression. Worth a separate ticket.

Review Summary

Agent	Critical	High	Important	Medium	Suggestions	Confidence
Architecture	0	0	0	2	2	High
Testing	0	0	0	0	4	Medium (after clearMocks correction)
Standards	0	0	0	1	1	Medium (after clearMocks correction)
UX	0	0	0	0	4	High
Data Integrity	0	0	0	0	2	High
Total	0	0	0	3	13	High overall

[github-actions]

AI Review Auto-Approval

Risk Level: MEDIUM (6/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)

This PR was auto-approved because:

• The multi-agent AI review determined it is medium 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.

[zweatshirt]

@wjames111 Was just hoping for a quick look at this at this refactor, especially a double check on user behavior (e.g. viewing and deleting). Thanks for considering Will!

There is an existing bug I am going to fix shortly after merging this in a separate PR. The steps to reproduce the bug are:

1. Select 'View' on a goal card.
2. Rename the goal in the calculator settings.
3. Go back to the goal list dashboard
4. Delete the goal you just renamed
5. A query will occur on a deleted goal since it doesn't get removed from the cache properly.

[wjames111]

Thanks for consolidating this, looks great!

[wjames111]

Question: Would it be easier if we just passed in the number to goal amount, then use the currencyFormat and locale from this shared component?

[wjames111]

Thoughts on putting this component into Reports/Shared rather then the global shared folder?

[wjames111]

Question: Thoughts on moving the Delete mutation directly into PdsGoalCard rather then passing it in?