MPDX-9604 Hide save button when no changes present

[kegrimes]

Description

Currently, Ministry Partner Reminders shows the "Save" button regardless of changes present or not. When there are no changes and the user clicks "Save", there is an information snackbar that says "No changes have been made". Instead, we want to hide the "Save" button until there are changes present.

Jira ticket: MPDX-9604

Testing

Impersonate: caleb.cox@cru.org

• Go to /hrTools/partnerReminders
• Verify "Save" button not present
• Select a different reminder status
• Check that "Save" button appears
• Click & ensure status was changed

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]

Bundle sizes [mpdx-react]

Compared against 61a52a6

No significant changes found

[github-actions]

Preview branch generated at https://MPDX-9604-hide-save-button.d3dytjb8adxkk5.amplifyapp.com

[kegrimes]

🤖 Multi-Agent Code Review — PR #1798

PR: MPDX-9604 Hide save button when no changes present
Branch: MPDX-9604-hide-save-button → main
Mode: 🏃 Quick (3 agents, Haiku)
Files: 2 (+38 -45)

---

📊 Risk Assessment

Metric	Value
Risk Score	4/10 — MEDIUM
Day	Friday (small isolated UI change — low blast radius)
Required Reviewer	Entry-level or above can review
Blast Radius	Single component, single feature (Ministry Partner Reminders)

Risk factors: HrTools feature component (+2), test file (+1), 50–200 LOC (+1), single-feature scope (1.0×).

---

🚦 Findings Summary

Priority	Count	Top Item
🔴 High Priority	1	Hidden button conflicts with on-page copy + codebase uses disabled pattern
⚠️ Important	2	Missing isSubmitting disable; missing explicit positive-case test
💡 Suggestions	3	Memoize getUpdates; remove dead mockEnqueue.mockClear(); edge-case tests

No critical blockers. The change is small, targeted, and the new test correctly uses queryByRole for absence assertions.

---

🔴 HIGH PRIORITY (Severity 7–8)

1. Hidden-vs-disabled Save button — UX conflict + codebase divergence

Severity: 7.5/10
File: PartnerRemindersReport.tsx:215-242
Flagged by: UX (9/10), Standards (5/10)

Two converging issues:

1. Instructional copy contradicts behavior. Lines 215–217 tell the user:

   "When you're done, click the 'Save' button at the bottom of the page."

   On first load (no changes), the button is hidden — the user reads the instructions, scrolls to the bottom, and finds nothing. Reads as a bug, not a feature.

2. Codebase convention is disabled, not hidden. The UX agent grep'd HrTools and found established patterns like disabled={!values.formType || isSubmitting} (CreateGoalDialog, TransferModal). CLAUDE.md / code-review.md also specifies under Forms: "Submit buttons are disabled while isSubmitting is true." — implying a stable, visible affordance.

Recommendation (pick one):

• Preferred: <Button disabled={getUpdates(values).length === 0 || isSubmitting}> — keeps the button visible, matches codebase pattern, satisfies the isSubmitting standard, and the instructional copy still makes sense.
• Alternative: Keep the hide-on-clean behavior but update lines 215–217 to: "Make changes to any reminder status, then click the 'Save' button that appears at the bottom of the page."

---

⚠️ IMPORTANT (Severity 5–7)

2. Submit button doesn't honor isSubmitting

Severity: 6/10
File: PartnerRemindersReport.tsx:163,239
Flagged by: Standards

The Formik render-prop destructures { submitForm, values } but not isSubmitting. The button can be re-clicked while the mutation is in-flight. Per code-review.md (Forms checklist): "Submit buttons are disabled while isSubmitting is true."

Low practical risk (mutation has onCompleted/refetch), but it's a documented project standard. Naturally resolved if you adopt the disabled pattern from finding #1.

3. No explicit positive-case test

Severity: 6/10
File: PartnerRemindersReport.test.tsx:128-133,158-193
Flagged by: Testing

The "Save button hidden when no changes" case is covered. The "Save button appears after a change" case is only implicitly covered by the mutation test at lines 158–193 (it succeeds only because the button rendered). A regression that breaks the conditional render would fail both tests for the wrong reason.

Recommendation: Add a dedicated test:

it('should show save button when changes are made', async () => {
  const { findByRole, getByRole } = render(<TestComponent />);
  await findByRole('combobox', { name: /reminder status/i });
  userEvent.click(getByRole('combobox', { name: /reminder status/i }));
  userEvent.click(getByRole('option', { name: 'Monthly' }));
  expect(await findByRole('button', { name: 'Save' })).toBeInTheDocument();
});

---

💡 SUGGESTIONS (Severity 3–5)

4. getUpdates(values) called twice per render

Severity: 4/10
File: PartnerRemindersReport.tsx:128-137,145,238
Flagged by: Testing, UX, Standards (3-agent consensus)

Called once on every render in the conditional (line 238) and again inside handleSave (line 145). Tiny in practice, but inconsistent with the rest of the component which memoizes derived state (transformedData, initialValues both use useMemo).

Recommendation:

const updates = useMemo(
  () =>
    Object.entries(values.status)
      .filter(([id, statusCd]) => initialValues.status[id] !== statusCd)
      .map(([id, statusCd]) => ({ rowId: id, statusCd })),
  [values.status, initialValues.status],
);

Then use updates.length > 0 for the conditional and pass updates directly into handleSave. Note: this needs to move inside the Formik render-prop since values is only available there.

5. Dead mockEnqueue.mockClear() in renamed test

Severity: 3/10
File: PartnerRemindersReport.test.tsx:129
Flagged by: Testing, Standards

The renamed test no longer asserts on mockEnqueue, so the mockClear() is dead code. Safe to remove.

6. Edge-case test coverage

Severity: 3/10
Flagged by: Testing

Optional: tests for zero reminders (empty list) and multi-row selective changes (only changed rows appear in updates).

---

✅ What Looks Good

• Named export, file naming, colocated test — all compliant.
• All user-visible strings use t(). Removed 'No changes have been made' is correctly dropped.
• Uses Luxon (not new Date()), uses useUpdateMinistryPartnerRemindersMutation with refetchQueries: ['MinistryPartnerReminders'] for cache correctness.
• New test uses typed GqlMockedProvider<{...}> and correctly uses queryByRole for absence assertion.
• No any, no new @ts-ignore, no console.log, no commented-out code.
• <SimpleScreenOnly> wrapping is preserved — print rendering unaffected.

---

📋 Agent Vote Summary

Agent	Confidence	Critical	Important	Suggestions
🧪 Testing	High	0	2	2
👤 UX	High	1	1	2
📋 Standards	High	0	1	2

---

🎯 Recommended Next Steps

Before merging:

• Resolve hidden-vs-disabled decision (finding Tasks #1). The cheapest, codebase-consistent fix is disabled={getUpdates(values).length === 0 || isSubmitting} — covers Tasks #1, contacts #2, and partially Bump axios from 0.20.0 to 0.21.1 #4 in one change.
• Add explicit "Save button appears after change" test (finding add notifications #3).

Nice-to-have:

• Remove dead mockEnqueue.mockClear() at line 129 (finding Move to Yarn V2 and upgrade packages #5).
• Memoize the updates calculation if you keep the conditional-render approach (finding Bump axios from 0.20.0 to 0.21.1 #4).

---

Generated by MPDX Multi-Agent Review (Quick Mode) | 3 agents · Haiku · ~1 min

[wjames111]

Sorry for the late review, everything looks great!