fix: dedupe Windows notification quick replies (#3467) - #3469
Conversation
Electron 42 can dispatch a single Windows toast inline reply more than once (WinRT and COM activation paths). Guard NOTIFICATIONS_NOTIFICATION_REPLIED with a per-notification replied set, re-armed on each show. Deliberately not cleared on close: the duplicate can arrive after dismiss. Forward-ports #3420 (b62b165) from hotfix/4.15.6 to fix #3467 on the 4.16.0 / dev line. Documents the constraint in AGENTS.md so a future handleActivation path keeps the same Set. Co-authored-by: Jean Brito <jeanfbrito@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
AGENTS.md📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{AGENTS.md,CLAUDE.md}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.spec.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.main.spec.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
WalkthroughThe notification process now deduplicates reply events per notification. Showing a notification re-arms reply handling, while closing it does not. Tests cover duplicate replies and both lifecycle behaviors. Hotfix guidance now requires immediate or same-change forward-porting to ChangesNotification reply deduplication
Hotfix forward-port guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change prevents duplicate Windows notification replies while preserving re-arming behavior for later notifications. No actionable merge-blocking risk remains beyond normal checks and review. Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/notifications/main.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/notifications/main/setup.main.spec.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). Warning Errors were encountered while retrieving linked issues. Errors (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for crediting me. Just out of curiosity, could you clarify why a separate |
It was my mistake, I saw your PR in the night and asked the bot to review it and do the fixes needed. He then understood that were to create a new PR. Already fixed, your colaboration will stay forever with us. Thank you very much @Moku151 . |
Closes #3467
What
Windows notification quick-reply was sending the message twice on 4.16.0 /
dev. This forward-ports the fix from #3420 (b62b165onhotfix/4.15.6) ontodevwith the same semantics.Thanks to @Moku151 for reporting the 4.16.0 regression in #3467 and for opening #3468. That PR correctly identified the missing forward-port; this PR is Jean’s own port of #3420 rather than a merge of #3468 (third-party forward-port of the same code).
Why
The 4.15.6 hotfix landed the
repliedNotificationsSet guard, but it was not forward-ported todev, so 4.16.0 shipped without it. Electron 42 on Windows can emitreplytwice for one toast (WinRT + COM activation paths).How
Same semantics as #3420:
NOTIFICATIONS_NOTIFICATION_REPLIEDwith a per-notificationrepliedNotificationsSetshowclose— the duplicate can arrive after dismiss; clearing there reopens the race (ignore CodeRabbit advice to clear on close)src/notifications/main/setup.main.spec.ts: duplicate reply dispatches once; latershowthen reply is accepted;closedoes not re-armAGENTS.md: documents the reply-dedup constraint (and that a futurehandleActivationpath must use the same Set) plus the hotfix →devforward-port ruleOut of scope: CORE-2571, SUP-1097, and the broader Action Center /
handleActivationwork in #3464.Testing
8 passed (including the three new reply-dedup cases).
Summary by CodeRabbit
Bug Fixes
Tests