[no-Jira] Fix send newsletter confirm button - #1981
Conversation
|
Preview branch generated at https://fix-send-newsletter-confirm.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 32ab085 No significant changes found |
canac
left a comment
canac
left a comment
There was a problem hiding this comment.
Multi-Agent Review — APPROVED WITH SUGGESTIONS
5 specialized agents (Architecture, Testing, Standards, Data Integrity, UX) + dependency analysis, run through cross-examination and rebuttal rounds.
Risk: 3/10 (LOW) — 4 files, +106/−62, single feature folder, no breaking changes.
All three stated bugs are genuinely fixed and correctly rooted. Verified by isolated revert (each fix has a test that fails without it) and by end-to-end probes: Confirm All now submits all visible contacts with user edits preserved across the refetch. The onError → try/catch change is the correct fix — in Apollo Client 3, supplying onError makes mutate() resolve, which is exactly why the success toast fired on top of the error toast. Removing the in-place existingItem.sendNewsletter = … mutation and the stale-closure setState fixes bug #3 at the root rather than patching it.
⚠️ 1 Important finding (7.0) — recommended before merge
awaitRefetchQueries: true makes a successful write report failure (FixSendNewsletter.tsx:85 and :120). All 5 agents flagged it. The awaited refetch shares a try with the mutation, so the parameterless catch can't distinguish them: when the mutation succeeds and the refetch fails, the user gets Error updating contact {{name}}, no success toast, and the row still in place — inviting a retry of work that already landed.
Proven by two independent runtime probes and confirmed at Apollo 3.7.1 source (QueryManager.js:243 — Promise.all(refetches) rejects the mutation promise, and cache writes execute before those promises are awaited).
git log -S"awaitRefetchQueries" returns one commit — 5f315c15a "Fix bulk confirming hidden contacts" — which added the flag on both paths, including the single-contact handler unrelated to that commit's subject. It isn't mentioned in the PR description. This is the only newly-introduced defect in the review, and it reintroduces this PR's own bug class through a different door.
See the inline comment on :85 for the fix, including a companion change the debate surfaced late (the obvious remedy introduces an empty-state flash that the current code does not have — measured, with a two-line fix).
The one test worth adding
The visibleContactIds filter (:100-104) has zero coverage. Two agents independently deleted the .filter(...) line and the entire suite still passed. It is the only thing preventing an account-list switch from submitting list A's contact IDs against list B's accountListId — the page is never remounted on switch (_app.page.tsx:128 keys on router.route; ProfileMenu.tsx:200-208 pushes that same pattern). One ~40-line churn test covers this and the useful half of the assertion issue on FixSendNewsletter.test.tsx:347.
Standards
Zero violations. yarn eslint exit 0, yarn lint:ts clean, 18/18 pass, i18n keys verified present in public/locales/en/translation.json. The added comment on :100 complies with the one-line-WHY rule and its claim was verified accurate.
Worth adding to the PR description
awaitRefetchQueries: trueis a behavior change on every confirm and is currently undeclared.- Confirm All now writes computed defaults (including
NONE) for contacts the user never touched. That matches the modal copy, but before this PR only one contact was ever submitted. - Incidental win: with Confirm All previously submitting ~1 contact, an account list with >100 invalid statuses was effectively unfixable through this tool. It now works as a confirm-100-and-refetch loop.
Notes on cross-feature consistency (no action here)
FixEmailAddresses.tsx:212/:265 still uses the onError: option style this PR abandons, so it still shows the success toast alongside the error toast — the same bug fixed here. FixMailingAddresses and FixCommitmentInfo use different patterns and are unaffected. None of the sibling tools accumulate the stale entries that motivated this PR's filter, so no parallel fix is needed.
Findings below are severity-tagged. Reply /dismiss: <reason> on any finding under 7.0 you disagree with.
![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 (3/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
Datadog safety net monitors detected three issues on the Fix Send Newsletter page.This PR fixes all three.
Stuck spinner (
Contact.tsx) —.finally(() => setUpdatingSingle(false)). A failed confirm leaves the row mounted, so previously the button stayed disabled with a spinner forever; only a reload recovered. This is the one that produced the observed dead-click + rage-click pattern (MUI setspointer-events: nonewhen disabled, so the clicks land on the wrapper — still namedCONFIRM, still doing nothing).Contradictory toasts (
FixSendNewsletter.tsx) — replaced Apollo'sonErroroption withtry/catch.onErrorresolves the promise, so'Newsletter updated!'was firing on top of'Error updating contact X'.Confirm All submitting one contact (
Contact.tsx) — functionalsetContactUpdatesupdater. Every row seeds its default from a mount effect in the same commit; the non-functional update meant they all overwrote each other and only the last survived. Also removes the in-placeexistingItem.sendNewsletter = …mutation, and the now-unusedcontactUpdatesprop.Testing
Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions