Repository navigation
fix(web): keep thread notification tags within the Windows toast limit - #12289
satyalyadav wants to merge 1 commit into
Conversation
Windows drops renderer notifications when the tag plus origin exceeds the platform toast budget, and the composite environmentId:threadId tag is 73 characters. Digest the pair into a 16-character tag so background thread alerts reach the Action Center again. Fixes pingdotgg#12287
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a small, self-contained web bug fix that shortens Windows notification tags while preserving per-thread replacement behavior. Production changes are limited to notification identity and are covered by focused unit tests. Notes:
You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds a bounded hash-based notification tag helper. Desktop notification coordination now uses explicit generated tags for pending-notification replacement. Tests cover tag generation and updated coordinator expectations. ChangesThread notification tags
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Windows notifications now use tags within the stated platform limit while retaining per-environment and per-thread replacement behavior. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
I tested this patch on Windows. Completion banners and sound worked after reloading the app, but clicking a notification did not bring T3 to the foreground.
Clicking the banner did not bring T3 to the foreground in either window state. Clicking its Notification Center entry also failed to bring T3 forward in the minimized test. Electron logged These results support the tag-length fix and confirm banner/sound delivery with the patch in this development setup. Click-to-open remains unresolved. I have not tested a patched Windows installer build, whether notifications from different conversations remain separate, macOS/Linux, or T3 in a standalone Windows browser. I have no video recording; the evidence consists of manual observations and local diagnostic logs. AI assistance: OpenAI Codex, using Codex CLI 0.155.1 through its app-server integration in the T3 Code desktop app; model: |
|
Thanks for the detailed Windows verification, and for isolating the tag failure with the 73-character tag. On click-to-open: this PR only changes the tag value and the pending-notification map key, so it does not touch click handling, and your report does not establish it as a regression. It is worth checking on unpatched main, and if it reproduces there it belongs in its own issue with that repro. If you have time, the behavior this PR actually changes is replacement, so a check that two different conversations stay separate and that a second completion replaces the first would be useful evidence. |
|
Thanks for the suggested checks. I ran an automated native Windows follow-up using the same isolated source build: PR Real Codex turns were started through the isolated server's local API while the desktop client stayed in the background. Windows' notification-history API showed:
The application tags were This verifies separation and replacement in native Windows notification history for that development build. It does not add a visual banner/sound check. I also tested clean, unpatched main at AI assistance: OpenAI Codex; harness: Codex CLI 0.155.1 app-server integration; host/interface: T3 Code desktop app; model: |
|
Thanks for the thorough follow-up, Scott. The A to B to A history check plus the unpatched-main control is exactly the evidence this needed, and I appreciate you closing the click-to-open question with it. I have added a summary of your verification to the PR description. |
Dismissing prior approval to re-evaluate 13ebd25
…indows toast limit Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Written by Hi! We are cleaning up open PRs, and this one does not say which model or harness was used to create it. If this change is really important, we recommend rebuilding the PR with a newer model and noting the model and harness in the PR description. |
|
Note Written by Reopening, this was closed by mistake. Sorry for the noise! |
What Changed
threadNotificationTag(environmentId, threadId)inapps/web/src/threadNotifications.ts: a stable 16-character digest of theenvironmentId:threadIdpair.ThreadNotificationCoordinatornow uses that tag fornew Notification({ tag })and as the pending-notification map key, so per-environment/thread replacement behavior is unchanged.Why
Windows silently drops renderer notifications when the tag plus origin exceeds the platform toast budget (electron/electron#40433), and the composite tag is 73 characters because
environmentIdandthreadIdare both UUIDs. The app-side path still ran, which is why affected users heard the sound and saw the taskbar badge while the toast never appeared and the Action Center stayed empty. The digest keeps environment and thread uniqueness while staying well below the documented 34-character bound and the origin-dependent budget.Fixes #12287
Tests
apps/web/src/threadNotifications.test.ts: the tag is 16 characters, stable, and distinguishes environments and threads.vp test run --project unitover the new test plus both coordinator test files: 32 passed.tsc --noEmitinapps/webpasses.Independent Windows verification by @ScottN-PV (Windows 11 25H2, Electron 44.4.2, source build): the shortened tag reaches Windows Notification Center, the 73-character tag fails with the documented Electron error, banners and sound work after reload, and notification history shows distinct tags per conversation with same-conversation replacement while the other conversation's entry stays unchanged. Click-to-open did not bring the app forward, and that is not a regression from this PR: unpatched main rejects the tag, so no notification exists there to click.
UI Changes
Not applicable; no visual changes.
Checklist
Summary by CodeRabbit