Skip to content

fix(#4753): notify background desktop tabs without hiding streams#4797

Closed
rodboev wants to merge 2 commits into
nesquena:masterfrom
rodboev:pr/4753-background-tab-notifications
Closed

fix(#4753): notify background desktop tabs without hiding streams#4797
rodboev wants to merge 2 commits into
nesquena:masterfrom
rodboev:pr/4753-background-tab-notifications

Conversation

@rodboev

@rodboev rodboev commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • The issue is not a service-worker or backend problem. The only broken seam is the notification gate, which still treats document.hidden as the only background signal.
  • The repo already has several stream safety paths tied to real Page Visibility state, so the desktop host needs a second, notification-only background signal instead of spoofing hidden state.
  • That makes the right fix small: add the host setter, widen the notification gate, and prove that stream visibility logic stays untouched.

What Changed

  • static/messages.js: added a desktop-owned notification-only background flag and setter, then used it only in the browser-notification gate.
  • tests/test_pwa_notification_controls.py: kept the existing notification control coverage and added guards that the new desktop flag stays out of SSE hidden-state helpers.
  • tests/test_issue4753_desktop_background_notifications.py: added focused runtime coverage for backgrounded visible tabs, focused visible tabs, real hidden tabs, and forceHidden.

Why It Matters

Background desktop tabs can now fire approval, clarify, and completion notifications without pretending the page is hidden. That keeps the desktop notification gap closed without risking the live SSE behavior that background tabs depend on.

Verification

pytest tests/test_pwa_notification_controls.py tests/test_issue4753_desktop_background_notifications.py -v --timeout=60

Full-suite CI context, not a required local check unless requested: pytest tests/ -v --timeout=60.

Upstream

Closes #4753.

Model Used

GPT 5.5 via Codex CLI

@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes #4753 by introducing a desktop-shell-owned background signal that is entirely separate from the Page Visibility API, so desktop tabs can receive approval, clarify, and completion notifications while remaining visibly open. The stream-visibility and SSE reconnection paths are untouched.

  • static/messages.js: adds window.__hermesSetBackgrounded, _isBackgroundedForBrowserNotification(), and _shouldForceCompletionNotification which atomically reads and clears both tracker entries; all non-done terminal stream paths now call _clearStreamNotificationBackground.
  • tests/test_issue4753_desktop_background_notifications.py: Node.js-backed runtime tests covering backgrounded-visible, foreground-silent, real-hidden, forceHidden, and late-done-after-return scenarios.
  • tests/test_pwa_notification_controls.py: updated assertions to match refactored names and adds an isolation guard verifying notification-only symbols never appear in stream-visibility code.

Confidence Score: 5/5

Safe to merge — the change introduces an additive, desktop-shell-gated notification path with no modifications to stream visibility, SSE, or reconnection logic.

The new flag and its setter are entirely isolated from document.hidden checks that govern stream behavior. The STREAM_NOTIFICATION_BACKGROUND tracker mirrors the well-tested STREAM_WAS_HIDDEN pattern and is cleaned up on every terminal path. Runtime tests confirm gate logic end-to-end.

No files require special attention.

Important Files Changed

Filename Overview
static/messages.js Adds desktop-notification-only flag and setter, refactors completion gate into _shouldForceCompletionNotification, adds _clearStreamNotificationBackground on all terminal paths.
tests/test_issue4753_desktop_background_notifications.py New runtime tests for all notification-gate cases plus sticky wasBackgrounded semantics.
tests/test_pwa_notification_controls.py Updated assertions to match refactored names and added isolation guard for notification-only symbols.

Reviews (2): Last reviewed commit: "fix(#4753): hoist background notificatio..." | Re-trigger Greptile

Comment thread static/messages.js
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in v0.51.612 🚀 — thanks @rodboev. Background desktop-app tabs now fire OS notifications without closing their live stream. Codex+Opus SAFE (the notification-only flag leaves all document.hidden stream/SSE logic untouched) + full suite 10307.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Merged into master + shipped in v0.51.612 (release PR #4802). Closing — GitHub didn't auto-detect the merge because the release stage's CHANGELOG rebase changed the commit. Thanks again @rodboev!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fire OS notifications for visible-but-unfocused (background) tabs without closing their SSE stream

2 participants