Skip to content

fix(notifications): restore background push notifications and improve SW session recovery#671

Closed
Just-Insane wants to merge 24 commits intoSableClient:devfrom
Just-Insane:fix/sw-push-session-recovery
Closed

fix(notifications): restore background push notifications and improve SW session recovery#671
Just-Insane wants to merge 24 commits intoSableClient:devfrom
Just-Insane:fix/sw-push-session-recovery

Conversation

@Just-Insane
Copy link
Copy Markdown
Contributor

@Just-Insane Just-Insane commented Apr 12, 2026

Description

Restores background push notification delivery and hardens the service worker's session management. Notifications were silently dropped when the app was backgrounded because two upstream mechanisms — the appIsVisible flag in the SW and togglePusher on visibility changes — had been removed.

Notification & visibility fixes

  • Restore appIsVisible flag and setAppVisible handler in sw.ts — the SW now tracks whether the app is visible via messages from the main thread, used alongside clients.matchAll() visibilityState as a dual-signal check before suppressing push notifications.
  • Post setAppVisible from the main thread in ClientNonUIFeatures.tsx — bridges document visibility changes to the service worker.
  • Reconnect togglePusher on visibility changes in useAppVisibility.ts — re-registers the push endpoint when the app returns to the foreground, ensuring the homeserver can deliver pushes.

Service worker session recovery

  • Increase session TTL from 30 min → 24 h — push notifications can arrive hours after last active use; the short TTL caused unnecessary re-fetches and auth errors.
  • requestSessionWithTimeout fallbackPromise.race wrapper so the SW falls back to its cached preloadedSession if the main client is unresponsive, instead of hanging.
  • Reset heartbeat backoff on foreground sync — the exponential backoff counter resets when the user foregrounds the app, allowing immediate session refresh.
  • Warm preloadedSession from push handler — the session fetched during push processing is stored for reuse within the same SW lifetime.

Robustness improvements

  • appEvents.ts → Set-based multi-subscriber pattern — replaces single-callback slots with Set-backed subscriptions that return unsubscribe functions, preventing silent overwrites when multiple consumers subscribe.
  • BackgroundNotifications.tsx retry timer cleanup — tracks setTimeout IDs in a Set and cancels them on effect cleanup, preventing orphaned background clients on unmount.

Experiment / session-sync config types

  • useClientConfig.ts — adds ExperimentConfig, SessionSyncConfig, and ExperimentSelection types plus selectExperimentVariant / useExperimentVariant hooks for feature-flag gating.

DM sidebar fixes

  • RoomNavItem.tsx — fix room topic/status description sticking to the wrong row by replacing useRoomTopic with a per-room getStateEvent + useEffect.
  • useUserPresence.ts — simplify null-safety with optional chaining on the User object.
  • DirectDMsList.tsx — use getDirectRoomAvatarUrl directly instead of falling through getRoomAvatarUrl first.

Fixes #

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

AI disclosure:

  • Partially AI assisted (clarify which code was AI assisted and briefly explain what it does).

The notification/visibility restoration (appIsVisible flag, setAppVisible handler, togglePusher reconnection), the appEvents multi-subscriber refactor, and the BackgroundNotifications retry timer cleanup were developed with AI assistance. The session recovery improvements (requestSessionWithTimeout, backoff reset, TTL increase) were drafted with AI assistance and reviewed against the existing SW session-handshake protocol. The experiment config types, DM sidebar fixes, and presence cleanup were developed with AI assistance.

…t fallback

Matrix access tokens are long-lived and only invalidated on logout or server revocation.
The previous 60s TTL caused iOS push handlers (which restart the SW per push) to reject
cached sessions as stale, resulting in generic 'New Message' notifications.

Also adds a requestSessionWithTimeout fallback in handleMinimalPushPayload that asks
live window clients for a fresh session when neither the in-memory map nor the persisted
cache contains a usable session.
…ssion from push handler

When phase3AdaptiveBackoffJitter is enabled, successful foreground/focus session pushes
(phase1ForegroundResync) now reset heartbeatFailuresRef to 0.  Previously a period of SW
controller absence (e.g. SW update) could inflate the heartbeat interval to its maximum
(30 min) even after the SW became healthy again, reducing session-refresh frequency below
the intended 10-minute rate.

Also captures the loadPersistedSession() result in onPushNotification and assigns it to
preloadedSession, avoiding a redundant second cache read in handleMinimalPushPayload when
the SW is restarted by iOS for a push event.
@Just-Insane Just-Insane requested review from 7w1 and hazre as code owners April 12, 2026 16:13
When iOS backgrounds the PWA, the WKWebView JS thread can be frozen before
visibilitychange fires.  This leaves appIsVisible stuck at true and
clients.matchAll() returning a stale 'visible' state — both signals stale
simultaneously — causing the dual AND gate to wrongly suppress push
notifications for backgrounded apps.

Replace the stale-flag check with checkLiveVisibility(): ping each window
client via postMessage and require a response within 500 ms to confirm the
app is genuinely in the foreground.  A frozen/backgrounded page cannot
respond, so the timeout causes checkLiveVisibility to return false and the
notification is shown correctly.

The encrypted-event path already uses this pattern (requestDecryptionFromClient
acts as the live check) and is unaffected.  Also added the matching
checkVisibility/visibilityCheckResult message pair to HandleDecryptPushEvent
so the page can respond to the new ping.
Copilot AI review requested due to automatic review settings April 12, 2026 20:20
Comment thread src/sw.ts Fixed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves service worker (SW) session recovery and media authentication behavior to reduce missed/incorrect push notifications and 401s when the SW restarts or a tab is unavailable, and adds an app↔SW “live visibility” handshake to decide when to suppress OS notifications.

Changes:

  • Persist SW session with a timestamp and enforce a 24h TTL; warm SW preloadedSession during push handling.
  • Add SW↔client live visibility ping and use it to suppress OS notifications only when a tab confirms it’s visible.
  • Expand authenticated media interception and add retry logic to recover from token refresh races.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
src/sw.ts Session persistence/TTL, push visibility ping, media auth path expansion, media fetch retry, and Workbox precache call change.
src/app/pages/client/ClientNonUIFeatures.tsx Responds to SW visibility pings from the client page.
src/app/hooks/useAppVisibility.ts Adds foreground/focus/heartbeat-driven session push behavior (plus experiment/config wiring).
.changeset/sw-push-session-recovery.md Changeset entry for the SW reliability fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/sw.ts Outdated
Comment on lines +108 to +114
const age = typeof s.persistedAt === 'number' ? Date.now() - s.persistedAt : Infinity;
const MAX_SESSION_AGE_MS = 24 * 60 * 60 * 1000; // 24 hours
if (age > MAX_SESSION_AGE_MS) {
console.debug('[SW] loadPersistedSession: session expired', {
age,
accessToken: s.accessToken.slice(0, 8),
});
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

loadPersistedSession() no longer validates that the cached JSON has string accessToken/baseUrl before using them. As written, a corrupted cache entry can throw (e.g. s.accessToken.slice(...)) and/or propagate non-string values into fetch/auth, and the debug log leaks part of the access token. Please restore strict type checks (and avoid logging token material) before returning a SessionInfo.

Copilot uses AI. Check for mistakes.
Comment thread src/sw.ts Outdated
Comment on lines +277 to +281
const checkId = `vis-${Date.now()}-${idx}`;

const promise = new Promise<boolean>((resolve) => {
visibilityCheckPendingMap.set(checkId, resolve);
});
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

checkLiveVisibility() builds checkId from Date.now() + idx, which can collide across overlapping calls (e.g. multiple push events close together), overwriting entries in visibilityCheckPendingMap and resolving the wrong promise. Use a collision-resistant id (e.g. crypto.randomUUID() or a monotonic counter) for the map key.

Copilot uses AI. Check for mistakes.
Comment thread src/sw.ts Outdated
Comment on lines +490 to +493
const result = await Promise.race(
Array.from(windowClients).map((c) => requestSessionWithTimeout(c.id, 1500))
);
session = result ?? undefined;
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

In handleMinimalPushPayload(), using Promise.race over per-client requestSessionWithTimeout calls can return undefined as soon as the first client responds with no session (or a fast failure), even if another client would return a valid session shortly after. This defeats the intended fallback behavior; prefer selecting the first defined session (e.g. Promise.all + find, or a Promise.any-style helper that ignores undefined).

Suggested change
const result = await Promise.race(
Array.from(windowClients).map((c) => requestSessionWithTimeout(c.id, 1500))
);
session = result ?? undefined;
const results = await Promise.all(
Array.from(windowClients).map((c) => requestSessionWithTimeout(c.id, 1500))
);
session = results.find((result) => result != null) ?? undefined;

Copilot uses AI. Check for mistakes.
Comment thread src/sw.ts
Comment on lines +943 to +946
// Capture the persisted session result into preloadedSession so that
// getAnyStoredSession() returns it in handleMinimalPushPayload without a
// second cache read.
const [, persistedSession, clients] = await Promise.all([
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

The comment says persisting the session into preloadedSession makes getAnyStoredSession() return it, but getAnyStoredSession() only reads from the sessions Map. As a result, handleMinimalPushPayload() will still re-read the cache (and won’t benefit from preloadedSession). Either update the comment or wire preloadedSession into getAnyStoredSession()/handleMinimalPushPayload() so the warm-up actually avoids the extra cache read.

Copilot uses AI. Check for mistakes.
Comment thread src/sw.ts Outdated
if (self.__WB_MANIFEST) {
precacheAndRoute(self.__WB_MANIFEST);
}
precacheAndRoute(self.__WB_MANIFEST);
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

precacheAndRoute(self.__WB_MANIFEST) is now unguarded. If __WB_MANIFEST is undefined in any build mode (e.g. dev, tests, or if injectManifest fails), this will throw at SW startup. Safer is to keep the guard or default to an empty array when the manifest isn’t injected.

Suggested change
precacheAndRoute(self.__WB_MANIFEST);
precacheAndRoute(self.__WB_MANIFEST ?? []);

Copilot uses AI. Check for mistakes.
Comment on lines +773 to +780
if (data.type === 'checkVisibility') {
const { id } = data as { id: string };
navigator.serviceWorker.controller?.postMessage({
type: 'visibilityCheckResult',
id,
visible: document.visibilityState === 'visible',
});
return;
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

The visibility ping response uses navigator.serviceWorker.controller?.postMessage(...). Since the SW uses clients.matchAll({ includeUncontrolled: true }), some visible clients may be uncontrolled and have controller === null, so they’ll never respond and the SW will incorrectly treat the app as not visible. Consider posting via (await navigator.serviceWorker.ready).active (or the registration) when controller is null so uncontrolled-but-visible tabs can respond.

Copilot uses AI. Check for mistakes.
Comment on lines +7 to 9
import { useClientConfig, useExperimentVariant } from './useClientConfig';
import { useSetting } from '../state/hooks/settings';
import { settingsAtom } from '../state/settings';
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

useAppVisibility imports useExperimentVariant from ./useClientConfig, but src/app/hooks/useClientConfig.ts does not export it, and ClientConfig also doesn’t declare the sessionSync property used below. This file will not typecheck/compile as-is; either add the missing export/types to useClientConfig.ts or remove/guard the experiment + sessionSync usage here.

Copilot uses AI. Check for mistakes.
Comment thread src/app/hooks/useAppVisibility.ts Outdated
Comment on lines +22 to +29
export function useAppVisibility(mx: MatrixClient | undefined, activeSession?: Session) {
const clientConfig = useClientConfig();
const [usePushNotifications] = useSetting(settingsAtom, 'usePushNotifications');
const pushSubAtom = useAtom(pushSubscriptionAtom);
const isMobile = mobileOrTablet();

const sessionSyncConfig = clientConfig.sessionSync;
const sessionSyncVariant = useExperimentVariant('sessionSyncStrategy', activeSession?.userId);
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

useAppVisibility now relies on activeSession (baseUrl/accessToken/userId) for pushSessionNow/heartbeat, but no call sites pass it (e.g. ClientRoot still calls useAppVisibility(mx) only). With activeSession undefined, the new session-sync logic will always be skipped and the backoff reset/heartbeat won’t take effect. Either make activeSession required or update the callers to pass the current session.

Suggested change
export function useAppVisibility(mx: MatrixClient | undefined, activeSession?: Session) {
const clientConfig = useClientConfig();
const [usePushNotifications] = useSetting(settingsAtom, 'usePushNotifications');
const pushSubAtom = useAtom(pushSubscriptionAtom);
const isMobile = mobileOrTablet();
const sessionSyncConfig = clientConfig.sessionSync;
const sessionSyncVariant = useExperimentVariant('sessionSyncStrategy', activeSession?.userId);
function getActiveSessionFromClient(mx: MatrixClient | undefined): Session | undefined {
if (!mx) return undefined;
const baseUrl = mx.getHomeserverUrl?.();
const accessToken = mx.getAccessToken?.();
const userId = mx.getUserId?.();
if (!baseUrl || !accessToken || !userId) return undefined;
return {
baseUrl,
accessToken,
userId,
} as Session;
}
export function useAppVisibility(mx: MatrixClient | undefined, activeSession?: Session) {
const clientConfig = useClientConfig();
const [usePushNotifications] = useSetting(settingsAtom, 'usePushNotifications');
const pushSubAtom = useAtom(pushSubscriptionAtom);
const isMobile = mobileOrTablet();
const resolvedActiveSession = activeSession ?? getActiveSessionFromClient(mx);
const sessionSyncConfig = clientConfig.sessionSync;
const sessionSyncVariant = useExperimentVariant('sessionSyncStrategy', resolvedActiveSession?.userId);

Copilot uses AI. Check for mistakes.
The postMessage round-trip ping (checkLiveVisibility) introduced a new
race: iOS can background the app without immediately freezing the JS
thread, so the page can still respond 'visible' in the brief window
before the freeze — causing the notification to be suppressed.

client.visibilityState from clients.matchAll() is updated by the browser
engine when the OS signals a visibility transition, independently of the
page JS thread, making it immune to this race.

When matchAll() returns zero clients (an iOS Safari PWA quirk) we default
to showing the notification rather than silently dropping it.

Removes checkLiveVisibility(), visibilityCheckPendingMap, the
visibilityCheckResult message handler, and the checkVisibility handler
in ClientNonUIFeatures.
…ndler

Restores the dual-signal visibility check in the service worker (appIsVisible
flag OR clients.matchAll visibilityState) and the setAppVisible message handler.
Also restores the visibilitychange listener in ClientNonUIFeatures that posts
visibility state to the SW.

These were removed in f79b75e which broke background notification delivery,
particularly on iOS Safari where clients.matchAll() can return stale results
after SW suspension.
…imers

1. appEvents.ts: Replace single-callback onVisibilityChange/onVisibilityHidden
   slots with Set-based multi-subscriber pattern. Subscriptions return an
   unsubscribe function, preventing silent overwrites.

2. useAppVisibility.ts: Update to use emitVisibilityChange/emitVisibilityHidden
   for dispatching and onVisibilityChange() subscription for togglePusher.

3. BackgroundNotifications.tsx: Track retry setTimeout IDs in a Set and cancel
   them on effect cleanup, preventing orphaned background clients on unmount.
@Just-Insane Just-Insane changed the title fix(sw): improve push session recovery — 24h TTL, timeout fallback, backoff reset fix(notifications): restore background push notifications and improve SW session recovery Apr 13, 2026
Just-Insane added a commit to Just-Insane/Sable that referenced this pull request Apr 13, 2026
…e handler

- sw.ts: add type validation in loadPersistedSession before accessing fields
- sw.ts: remove access token leak from debug log
- sw.ts: replace Promise.race with Promise.all+find in handleMinimalPushPayload
  to avoid returning undefined from first fast-failing client
- sw.ts: fix misleading comment about preloadedSession/getAnyStoredSession
- sw.ts: add ?? [] fallback for precacheAndRoute(self.__WB_MANIFEST)
- ClientRoot: pass activeSession to useAppVisibility
- index.tsx: add controllerchange listener to re-push session when SW updates
  via skipWaiting — fixes notifications stopping after SW replacement
Just-Insane added a commit to Just-Insane/Sable that referenced this pull request Apr 13, 2026
…e handler

- sw.ts: add type validation in loadPersistedSession before accessing fields
- sw.ts: remove access token leak from debug log
- sw.ts: replace Promise.race with Promise.all+find in handleMinimalPushPayload
  to avoid returning undefined from first fast-failing client
- sw.ts: fix misleading comment about preloadedSession/getAnyStoredSession
- sw.ts: add ?? [] fallback for precacheAndRoute(self.__WB_MANIFEST)
- ClientRoot: pass activeSession to useAppVisibility
- index.tsx: add controllerchange listener to re-push session when SW updates
  via skipWaiting — fixes notifications stopping after SW replacement
…e handler

- sw.ts: add type validation in loadPersistedSession before accessing fields
- sw.ts: remove access token leak from debug log
- sw.ts: replace Promise.race with Promise.all+find in handleMinimalPushPayload
  to avoid returning undefined from first fast-failing client
- sw.ts: fix misleading comment about preloadedSession/getAnyStoredSession
- sw.ts: add ?? [] fallback for precacheAndRoute(self.__WB_MANIFEST)
- ClientRoot: pass activeSession to useAppVisibility
- index.tsx: add controllerchange listener to re-push session when SW updates
  via skipWaiting — fixes notifications stopping after SW replacement
@Just-Insane Just-Insane force-pushed the fix/sw-push-session-recovery branch from 5ecd51f to 350a54a Compare April 13, 2026 14:55
Just-Insane added a commit to Just-Insane/Sable that referenced this pull request Apr 13, 2026
…e handler

- sw.ts: add type validation in loadPersistedSession before accessing fields
- sw.ts: remove access token leak from debug log
- sw.ts: replace Promise.race with Promise.all+find in handleMinimalPushPayload
  to avoid returning undefined from first fast-failing client
- sw.ts: fix misleading comment about preloadedSession/getAnyStoredSession
- sw.ts: add ?? [] fallback for precacheAndRoute(self.__WB_MANIFEST)
- ClientRoot: pass activeSession to useAppVisibility
- index.tsx: add controllerchange listener to re-push session when SW updates
  via skipWaiting — fixes notifications stopping after SW replacement
iOS PWA freezes the page thread before visibilitychange fires, leaving
appIsVisible stuck at true and suppressing push notifications. Replace
the unreliable OR of appIsVisible / matchAll().visibilityState with a
live checkVisibility round-trip: the SW posts a ping to every window
client and only suppresses if a client confirms visible within 500 ms.
Frozen or killed pages cannot respond, so the timeout resolves false
and the OS notification fires correctly.
Comment thread src/sw.ts Fixed
… on this branch

Removes imports and usages of usePresenceAutoIdle, presenceAutoIdledAtom,
useInitBookmarks, presenceMode setting, and presenceAutoIdleTimeoutMs config
that were accidentally merged from other feature branches but don't exist
on fix/sw-push-session-recovery. Restores PresenceFeature to upstream dev
shape.
…atchAll

The checkLiveVisibility approach (postMessage ping with 500ms timeout)
was causing false-positive suppression on iOS: the push event itself
can briefly wake a suspended page, allowing it to respond with
visibilityState='visible' even when the user is not looking at the app.
This caused background notifications to silently stop after a period of
inactivity.

Revert to upstream/dev's approach: OR of appIsVisible flag (set via
visibilitychange listener) and clients.matchAll() visibilityState.
Remove the checkLiveVisibility function, visibilityCheckPendingMap,
and the client-side checkVisibility responder.
- Fix preloadedSession comment: only media fetch handlers use it, not handleMinimalPushPayload
- Fix changeset frontmatter: '@sable/client': patch → default: patch
…Visibility

Removes the Session dependency from useAppVisibility by deriving the
userId directly from the MatrixClient instance.
Just-Insane and others added 6 commits April 15, 2026 09:06
retryImmediately() is a no-op on SlidingSyncSdk — it returns true
without touching the polling loop.  Call slidingSync.resend() on
foreground/focus to abort a stale long-poll and start a fresh one.

Also fixes activeSession references that should use mx methods
(getHomeserverUrl/getAccessToken/getUserId).
…dling - Use getEffectiveEvent()?.type for decrypted event type in BackgroundNotifications - Fix isEncryptedRoom flag in pushNotification.ts (was hardcoded false) - Add isEncryptedRoom: true to relay payload when decryption succeeds - Wrap push handlers in try/catch with fallback notifications (prevents silent drops on iOS) - Parallelize requestDecryptionFromClient with Promise.any + shared timeout (was sequential)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cations

Add sessionSync.phase1ForegroundResync and phase2VisibleHeartbeat to
config.json so the service worker session stays fresh on iOS.  Without
these flags useAppVisibility disables both foreground resync (phase1)
and the 10-min visible heartbeat (phase2), leaving the CacheStorage
session to age out after 24 h with no refresh.  When iOS kills the SW
while backgrounded and the session has gone stale, push decryption
fails and notifications are silently dropped.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
onPushNotification already fetches the persisted session and stores it in
preloadedSession.  Thread that through handleMinimalPushPayload's fallback
chain so we skip the second cache.match() call on iOS restarts where the
in-memory sessions Map is empty.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When backgrounded, the service worker manages the badge from push
payloads. The app's local unread state may be stale before sync catches
up, causing the badge to flash on then immediately off. Guard
clearAppBadge() with a visibility check so the SW badge persists.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Just-Insane Just-Insane marked this pull request as draft April 17, 2026 11:55
@Just-Insane Just-Insane deleted the fix/sw-push-session-recovery branch April 17, 2026 19:06
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.

3 participants