Skip to content

fix(web): suppress desktop notifications while focused - #11582

Closed
Bil0000 wants to merge 1 commit into
pingdotgg:mainfrom
Bil0000:t3code/check-focused-notifications
Closed

Bil0000 wants to merge 1 commit into
pingdotgg:mainfrom
Bil0000:t3code/check-focused-notifications

Conversation

@Bil0000

@Bil0000 Bil0000 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

What Changed

Skip desktop pop-ups when the app window has focus, using the existing shared web/desktop notification path. Background notifications, opt-in in-app alerts, and notification sounds keep their current behavior.

Why

Completion, approval, input, and failure pop-ups could appear even while the user was working in T3 Code. A single document.hasFocus() check prevents these interruptions while still recording the event, so skipped alerts do not replay after the window loses focus.

Validation

  • All 22 notification tests pass. Six new regression cases failed before the fix and pass after it.
  • Coverage includes focused suppression, no replay after blur, new background alerts, preserved sound, and the active thread with in-app alerts enabled.
  • Targeted lint, formatting, and web typecheck pass.
  • Native OS notification presentation was not manually tested. No layout changes.

Checklist

  • This PR is small and focused
  • I explained what changed and why

Model: GPT-6. Harness: Codex.

Summary by CodeRabbit

  • Bug Fixes
    • Desktop notifications are now suppressed while the document is focused.
    • Notifications appear once when supported events occur while the document is unfocused, with the expected title, message, tag, and sound settings.
    • Prevented duplicate notifications for events already handled while the document remained focused.
    • Focused desktop alerts for the active thread are suppressed when in-app alerts are enabled.

@github-actions github-actions Bot added size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Sep 13, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 13, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 727b2fe

Macroscope's review found this PR approvable — This is a narrowly scoped web notification bug fix: one guard suppresses desktop popups while focused, with sounds, background alerts, and existing in-app behavior preserved. The accompanying regression tests cover the affected event types and no product defaults or static-analysis settings are changed.

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

@coderabbitai

coderabbitai Bot commented Sep 13, 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: 41d862e1-521b-40f0-9c2e-f7f147ebec0f

📥 Commits

Reviewing files that changed from the base of the PR and between 789f41f and 727b2fe.

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

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


📝 Walkthrough

Walkthrough

The coordinator now suppresses desktop notifications while the document has focus. Tests cover thread completion, approval, input-required, and error events, including duplicate prevention and background notification payloads.

Changes

Thread notification behavior

Layer / File(s) Summary
Focus-gated notification flow
apps/web/src/components/ThreadNotificationCoordinator.tsx, apps/web/src/components/ThreadNotificationCoordinator.test.tsx
The coordinator checks document.hasFocus() before creating desktop notifications. Tests cover focused-event suppression, background alerts, replay prevention, cleanup, and notification payloads for thread state changes.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 727b2

Desktop alerts are suppressed while the app is focused, while background alerts and sounds retain their intended behavior. The change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 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 Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary change: suppressing desktop notifications while the window is focused.
Description check ✅ Passed The description includes the required What Changed, Why, and Checklist sections. It also provides validation details and states that there are no layout changes, so the UI evidence section is not appl…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@Bil0000

Bil0000 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed the docstring coverage warning: no documentation change is needed for this one-line guard in an existing private component. The repository guidance keeps discoverable behavior in source and tests; the regression test covers the focus behavior. Keeping the fix scoped as-is.

@Bil0000
Bil0000 force-pushed the t3code/check-focused-notifications branch from 789f41f to 727b2fe Compare September 13, 2026 16:36
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 13, 2026 16:36

Dismissing prior approval to re-evaluate 727b2fe

@Bil0000

Bil0000 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #11569, merged into main at 6e5e986. Its desktop notification guard already suppresses pop-ups while the app is visible and focused. I ran our six regression cases with the merged coordinator and its existing tests: all 22 passed, covering focused suppression, no replay after blur, background delivery, preserved sounds, and the active thread with in-app alerts enabled. Closing this duplicate PR; the branch is preserved.

@Bil0000 Bil0000 closed this Sep 13, 2026
@Bil0000 Bil0000 reopened this Sep 13, 2026
@Bil0000 Bil0000 closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant