feat(pwa): make push notifications reliable and non-disruptive - #1173
Open
penso wants to merge 10 commits into
Open
feat(pwa): make push notifications reliable and non-disruptive#1173penso wants to merge 10 commits into
penso wants to merge 10 commits into
Conversation
Push notifications had several defects that compounded into an unreliable
and noisy experience. The most visible: the service worker tagged every
notification with the session key but never set `renotify`, so a second
message in a chat silently replaced the first — no sound, no alert, and
the earlier message simply vanished. Notifications without a session key
all collapsed onto one shared tag.
Notification handling:
- Tag per session with `renotify`, and roll up replaced messages into an
"…and N earlier messages" line so nothing is lost silently.
- Title from the session label instead of a fixed "Message received", and
strip markdown from the body — fences and emphasis render literally in
the notification shade.
- Click focuses a window already showing the target chat and routes
in-place instead of reloading the whole document.
Delivery:
- Devices report which session they are viewing (`/api/push/presence`), and
a device that is visible and focused on a session is skipped when that
session replies. Other devices still get notified, so the phone stays
quiet only for the message being read on it.
- Send concurrently rather than serially, set a 6h TTL, high urgency, and a
per-session Topic so an offline device wakes to the latest message per
chat rather than a backlog.
- Drop endpoints on a typed WebPushError instead of matching
`e.to_string().contains("410")`, which discarded working subscriptions
whenever an unrelated error happened to mention that number.
- Handle `pushsubscriptionchange`: browsers rotate endpoints, and without
this push died silently until the user toggled it off and on again. The
re-registration carries `replaces` so the dead endpoint is retired.
Service worker robustness:
- Precache assets individually. `cache.addAll` is atomic, so one missing
generated asset failed the whole install and the worker never activated.
- Stop calling `skipWaiting()` at install. It force-reloaded the app
mid-conversation and left the existing update-prompt machinery dead;
activation and reload now wait until the app is out of view.
- Add an offline fallback page that reloads itself once connectivity is back.
Two calls turned out to crash the renderer outright, which takes down the
whole app rather than just push, and cannot be contained by try/catch:
- `pushManager.getSubscription()` at boot with no reachable push backend.
Guarded behind granted permission — a subscription cannot exist without
it, so the check costs nothing.
- The Badging API called from inside the service worker. The worker now
posts the count to open pages and the page applies it; a fully closed
app therefore picks up its badge when next opened.
Also adds a "Send test notification" action, since push failures are
otherwise invisible across the browser, push service, and network; and
modernises the manifest (`id`, `launch_handler`, unlocked orientation,
Projects/Settings shortcuts).
Push helpers move to `crates/chat/src/channel_push.rs` to keep
`channels.rs` under the 1500-line limit.
Contributor
Greptile SummaryThe PR strengthens PWA push delivery, ordering, recovery, privacy, revocation, and persistence.
Confidence Score: 5/5The PR appears safe to merge. The three previously reported failures are resolved: topics now incorporate the complete session key through a digest, failed rotations retain recovery state for startup reconciliation, and forgotten server subscriptions trigger guarded replacement and registration.
|
| Filename | Overview |
|---|---|
| crates/gateway/src/push.rs | Derives privacy-safe, bounded topics from SHA-256 session-key digests and integrates ordered, filtered, bounded push fanout. |
| crates/web/ui/src/sw.ts | Preserves failed endpoint-rotation state, enforces notification ordering, and maintains notification and badge lifecycle behavior. |
| crates/web/ui/src/push.ts | Adds startup subscription reconciliation, guarded 404 recovery, durable enablement intent, and cross-tab serialization. |
| crates/gateway/src/push_subscriptions.rs | Enforces durable endpoint revocation while allowing revival only through explicitly authorized registration. |
| crates/chat/src/channel_push.rs | Builds privacy-safe notification content and monotonic completion ordering for chat responses. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Agent response completes] --> B[Create ordered push payload]
B --> C[Gateway filters active presence]
C --> D[Derive hashed session topic]
D --> E[Deliver to subscribed endpoints]
E --> F[Service worker applies completion order]
F --> G[Display or replace notification]
H[Endpoint rotation or presence 404] --> I[Persist rotation intent]
I --> J[Startup reconciliation]
J --> K[Register current browser subscription]
K --> C
Reviews (3): Last reviewed commit: "fix(pwa): harden notification recovery a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Badging was moved into the page on the belief that the Badging API cannot be called from a service worker. That diagnosis was wrong. Probing both environments directly: in headed Chrome every setAppBadge/clearAppBadge call from the worker returns normally, and only headless Chromium is broken — there the promise never settles and never rejects, and merely invoking it wedges the worker, which then takes the page down on its next navigation. `typeof navigator.setAppBadge` is "function" in both, so feature detection cannot tell them apart, and nothing throws, so try/catch is equally useless. Badging a closed app is the whole point of the API, so restore it under two rules. Only when the app is installed: the page reports display-mode to the worker, which persists it in a cache kept outside the versioned one so it survives a cache bump and the app being closed. A badge is meaningless in a browser tab anyway, so this is a semantic gate as much as it is what keeps the worker away from the broken path. And never awaited, never inside waitUntil, so a platform call that hangs cannot stop a notification being shown. Also fixes three recovery gaps found in review, each of which left push permanently dead with nothing to signal it: - Topics collided across chats. The Web Push Topic header is capped at 32 characters, and truncating the encoded session key made any two keys sharing a long prefix — `telegram:bot123:chat…`, or nested project/session keys — collapse onto one another, so one chat's pending notification superseded another's while the device was offline. Hash the key first so the whole of it stays significant. - Endpoint rotation could fail silently. `fetch` resolves for 4xx, and the response status was never checked, so a rejected re-registration looked like success and the browser was left holding an endpoint the server never stored. Check it, and record the failure for the page to repair. - Stale subscriptions never reconciled. `/api/push/presence` answers 404 for an endpoint the server does not know, but the client ignored it and its cached payload then suppressed every retry. Worse, reconciliation only ran from the settings page, so a forgotten subscription stayed broken until someone happened to open Settings. Handle the 404 by re-registering (rate limited so a server that keeps rejecting cannot drive a loop), and reconcile at app startup. Adds unit coverage for the new push routes, which had almost none: device-name and client-IP parsing, unconfigured-service behaviour, subscribe/unsubscribe, rotation via `replaces`, and presence accept/reject.
The flag the previous commit added to mark a failed endpoint rotation was never read: initPushState() reconciles the browser's subscription against the server on every page load regardless, so a rotation the server rejected is already repaired there. Keeping it meant write-only state plus the same cache key spelled out in both the worker and the page, free to drift. The part that mattered — checking the re-registration response instead of treating any resolved fetch as success — stays.
Foreground suppression asked whether any visible client URL *contained* `/chats/<session>`. `/chats/main-2` contains `/chats/main`, so a user reading one chat suppressed notifications for a different one they could not see — the same prefix-collision the Topic header had. Compares the parsed pathname exactly instead.
Collaborator
Author
|
@greptile-apps review |
The session name in the chat toolbar sits in a flex row with `min-w-0` and no overflow containment, so a long name spills out of its box to the left, underneath `#projectCombo`. That combo is `position: relative` and therefore paints above the static name: the name looks and reports as visible, enabled and stable, yet every click lands on the project combo instead and renaming is impossible. It only bites once a project exists, because `#projectCombo` is hidden while the project list is empty — which is why it went unnoticed, and why the `channel-bound session can be renamed` e2e test only failed in a full-suite run, after an earlier spec had created a project. Contains the name in its own box and truncates it, which is what `min-w-0` already implied was intended. Two e2e fixes come with it, both for failures that had nothing to do with the code under test: - The e2e gateway now runs exec on the host. The first sandboxed exec provisions a container with the full default package set; on Linux a pre-built image absorbs that, but the Apple Container backend has no pre-built image and installs ~150 packages after the container starts. No test timeout survives it, so any spec whose agent calls exec failed on macOS while passing in CI. Commands come from the specs' own mock providers, and sandbox behaviour is covered by the sandbox specs, which use their own gateways. - The date-bucket test now stubs the session list endpoint. It injects sessions into the client store to control their timestamps, but a background `/api/sessions` refresh replaces the list wholesale (`mergeSessionListPage` with `append=false`) and the server page is capped at 40 by recency — so once earlier specs had created enough sessions, the injected fixtures were evicted and could never return.
Push delivery and subscription recovery still had unsafe endpoint handling, unbounded or racy lifecycle paths, and service-worker behavior that could lose alerts or app state. Harden endpoint validation and delivery deadlines, serialize cross-tab subscription state, aggregate ordered presence, and keep worker updates and click routing non-disruptive.\n\nAdd focused Rust and Playwright coverage for the corrected security, concurrency, badge, rotation, offline, and routing behavior.
Targeted local validation filters in-source tests by filename. Match the file to the Rust module name so the three push concurrency regressions are selected instead of producing a no-tests error.
The web crate exercises gateway session types that unconditionally use moltis-voice, but isolated web tests built gateway without that optional dependency. Enable the required gateway feature so targeted asset tests compile outside the full CLI feature set.
Remote device removal could be undone by automatic reconciliation, detached sends could finish out of order, and subscription persistence could leave corrupt or partially applied state. Worker rotation and cross-tab intent races also allowed stale responses to disable newer subscriptions. Persist explicit revocations, rotate unknown capabilities, order responses at both server and worker boundaries, make push-store updates transactional and atomic, redact endpoint logs, migrate legacy notifications, and restore correct voice feature gating. Add focused Rust and Playwright regressions for each path.
Collaborator
Author
|
@greptile-apps review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes PWA push notifications reliable, private, ordered, and non-disruptive across tabs and devices.
voicefeature gating so isolated web and lightweight CLI builds do not pull voice dependencies.Validation
Completed
just lintjust release-preflightcargo fmt --all -- --checkcargo test -p moltis-gateway push:: -- --nocapture- 48 passedcargo test -p moltis-httpd push_routes::tests -- --nocapture- 19 passedcargo test -p moltis-chat --features push-notifications channel_push::tests -- --nocapture- 14 passedcargo check -p moltis-web --no-default-featurescargo check -p moltis-web --no-default-features --features voicecargo check -p moltis --no-default-features --features lightweightnpx biome check src/push.ts src/pwa.ts src/sw.ts e2e/specs/pwa-push.spec.jscd crates/web/ui && npm run build:allcd crates/web/ui && npx tsc --noEmitcd crates/web/ui && npx playwright test e2e/specs/pwa-push.spec.js- 37 passed./scripts/local-validate.sh 1173- all local statuses published successfully; 66 targeted Playwright tests and real macOS/iOS builds passedRemaining
Manual QA
cargo run, open the web UI, and enable notifications in Settings > Notifications.