Skip to content

fix(web): keep thread notification tags within the Windows toast limit - #12289

Open
satyalyadav wants to merge 1 commit into
pingdotgg:mainfrom
satyalyadav:fix/windows-notification-tag
Open

satyalyadav wants to merge 1 commit into
pingdotgg:mainfrom
satyalyadav:fix/windows-notification-tag

Conversation

@satyalyadav

@satyalyadav satyalyadav commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

What Changed

  • Added threadNotificationTag(environmentId, threadId) in apps/web/src/threadNotifications.ts: a stable 16-character digest of the environmentId:threadId pair.
  • ThreadNotificationCoordinator now uses that tag for new 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 environmentId and threadId are 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.
  • Existing coordinator assertions now expect the digest. vp test run --project unit over the new test plus both coordinator test files: 32 passed.
  • tsc --noEmit in apps/web passes.

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

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Summary by CodeRabbit

  • Bug Fixes
    • Improved desktop thread notification handling so notifications for the same thread are more reliably grouped, updated, or dismissed.
    • Added consistent notification identification across environments and threads, reducing the risk of duplicate or incorrectly replaced notifications.
  • Reliability
    • Notification identifiers now remain within platform limits while preserving distinction between different threads and environments.

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
Copilot AI lite review requested due to automatic review settings September 17, 2026 18:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 17, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 17, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 13ebd25

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:

  • Diff unchanged. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 477a1c06-7a41-46a3-8fb1-9ee52b3084b0

📥 Commits

Reviewing files that changed from the base of the PR and between 4749035 and 13ebd25.

📒 Files selected for processing (4)
  • apps/web/src/components/ThreadNotificationCoordinator.test.tsx
  • apps/web/src/components/ThreadNotificationCoordinator.tsx
  • apps/web/src/threadNotifications.test.ts
  • apps/web/src/threadNotifications.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Thread notification tags

Layer / File(s) Summary
Bounded tag generation
apps/web/src/threadNotifications.ts, apps/web/src/threadNotifications.test.ts
Adds a typed FNV-1a helper that returns stable 16-character hexadecimal tags. Tests verify length, stability, and distinctness.
Coordinator tag integration
apps/web/src/components/ThreadNotificationCoordinator.tsx, apps/web/src/components/ThreadNotificationCoordinator.test.tsx
The coordinator receives explicit tags, keys pending notifications by those tags, derives tags with the helper, and updates desktop notification assertions.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: bil0000

Merge Risk: ⚪ Minimal · up to 13ebd

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed [#12287] The coordinator now derives each browser notification tag from threadNotificationTag(environmentId, thread.id). The helper hashes the environmentId:threadId pair and returns a fixed 16-ch…
Out of Scope Changes check ✅ Passed All reviewed changes support [#12287]. The helper implements the short digest, the coordinator preserves notification replacement behavior with the digest key, and the tests cover the required propert…
Description check ✅ Passed The description includes the required What Changed, Why, UI Changes, and Checklist sections. It explains the Windows issue, the implementation, test coverage, and the absence of UI changes. The additi…
Title check ✅ Passed The title clearly and concisely describes the main change: keeping thread notification tags within the Windows toast limit.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@ScottN-PV

ScottN-PV commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

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.

  • Setup: Windows 11 Home 25H2, build 26200.9457; T3 built from source with Electron 44.4.2. The app loaded at t3code-dev://app, using separate test settings and conversations.
  • Code tested: the changes from PR commit 13ebd2589a2970ecd759fc90061e2e3d2d226f30, applied to upstream commit 5781b5240bd5d2e21c651f6b228975ac40cbd67b.
  • Tag-length comparison: I created two notifications through the development app's JavaScript Console. A notification with tag t3-control appeared in Windows Notification Center. A notification with a tag consisting of 73 x characters failed: Electron logged a Windows error, The size of the notification tag is too large. (HRESULT: -2143420138). Windows' notification-history API contained the short-tag notification and no long-tag notification. Do not disturb was On for both tests; I was checking storage in Notification Center, not whether a banner appeared.
  • With the patch and Do not disturb Off, real Codex completion notifications displayed banners. Electron logged the shortened app tag 828d2b52f503fb00. The first test had no audible sound. After reloading T3 with Ctrl+R, both banner and sound worked in two further tests: one with T3 minimized, and one with T3 unminimized behind another application. I have not established why sound was absent on the first attempt.
  • Automated checks: before applying the patch, 16 notification-coordinator tests passed. With the patch, all 19 tests in the coordinator and tag-helper test files passed. Lint on the changed files and the web package's typecheck also passed.

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 Notification clicked, but that does not prove T3's click-handling code ran. I had to click T3's taskbar icon. The agent then appeared to continue for about one second before the final response became visible. I do not know whether the agent was still working or the interface was catching up. This PR does not directly change the code that handles notification clicks. I haven't established whether the click failure existed before this patch.

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: gpt-6-astra; reasoning effort: medium. Codex assisted with test setup, running automated checks, interpreting diagnostic logs, and drafting this report. I performed the manual Windows checks, reported the visible and audible outcomes, and reviewed the report before posting.

@satyalyadav

Copy link
Copy Markdown
Contributor Author

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.

@ScottN-PV

Copy link
Copy Markdown
Contributor

Thanks for the suggested checks. I ran an automated native Windows follow-up using the same isolated source build: PR 13ebd2589a2970ecd759fc90061e2e3d2d226f30 applied to baseline 5781b5240bd5d2e21c651f6b228975ac40cbd67b, Electron 44.4.2.

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:

  • First completion in conversation A: one entry for A.
  • Completion in conversation B: two entries, A and B, with distinct tags.
  • Second completion in A: still two entries. A reused its original tag and replaced its previous entry; B's tag and notification XML were unchanged. I changed A's conversation title before the second turn so the replacement could be distinguished in the stored notification body.

The application tags were 7fd41c906aca1aa6 for A and 1436fb8f1893e7c7 for B. Electron's log also recorded the old notification being hidden when A completed again. A local coordinator test covering A → B → A passed alongside the 16 existing coordinator tests.

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 c26119ada3565dda55bc3e86fdf47c7f9088e4b5, in a separate worktree and copied disposable profile, with Electron 44.4.2. A real turn completed while the renderer was unfocused and notification permission was granted. Electron attempted the original 73-character application tag, and Windows rejected it with The size of the notification tag is too large. (HRESULT: -2143420138). Notification history was unchanged: no entry for that completion appeared. Consequently there was no new notification to click, so this does not establish click-to-open behavior on unpatched main. I agree that the earlier click observation is not evidence of a regression from this PR. No Windows notification settings were changed for these checks.

AI assistance: OpenAI Codex; harness: Codex CLI 0.155.1 app-server integration; host/interface: T3 Code desktop app; model: gpt-6-astra; reasoning/effort: medium. Contribution: local API test setup, automated test execution, Windows notification-history inspection, evidence comparison, and drafting this reply. Human involvement: requested the follow-up and automatic testing, and confirmed that the model, effort, runtime version, and host remained the same as in the earlier verification. These follow-up results are automated observations; no new manual Windows checks were performed. Attribution settings are user-confirmed; runtime metadata was not independently captured again for this follow-up.

@satyalyadav

Copy link
Copy Markdown
Contributor Author

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.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 1, 2026 07:36

Dismissing prior approval to re-evaluate 13ebd25

AdEx-Partners-DE added a commit to AdEx-Partners-DE/t3code that referenced this pull request Oct 10, 2026
…indows toast limit

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

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.

@maria-rcks maria-rcks closed this Oct 11, 2026
@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Reopening, this was closed by mistake. Sorry for the noise!

@maria-rcks maria-rcks reopened this Oct 11, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Completion notifications never appear on Windows: 73-character notification tag exceeds the platform tag limit

5 participants