Repository navigation
fix(app): use service worker notifications on Android - #58399
Zijian-Wang wants to merge 2 commits into
Conversation
Chrome on Android rejects the Notification constructor. Prefer ServiceWorkerRegistration.showNotification when the offline worker is active, add a notification click handler that reopens the matching session, and keep the constructor only as a fallback.
github-actions
Bot
added
the
needs:compliance
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I couldn't verify this on a real device: it needs Chrome on Android with an active service worker. The two new unit test files pass and tsc -b passes in packages/app and packages/cli. The worker's same-origin and session-route checks match the format sessionHref produces.
Public API: unchanged. The only signature change is an optional href parameter on the app-internal Platform.notify.
The main concern: nothing in this pull request passes the new href. The callers in shell/notifications/notification.tsx still use the three-argument form; the change that passes sessionHref(...) is only in the stacked #58400. On its own, the service-worker path sets no tag and uses the current page as the click target. The in-page onClick (openNotificationSession) is also dropped on that path. Because the worker path is now preferred wherever one is active, desktop users of the installed web app lose the current "click opens the session" behaviour too, not only Android users. Please include the caller change here so this pull request works on its own (details inline).
| const registration = "serviceWorker" in navigator ? await activeRegistration() : undefined | ||
|
|
||
| if (registration?.active) { | ||
| await registration.showNotification(title, { |
There was a problem hiding this comment.
Nothing in this PR passes href yet: both callers in shell/notifications/notification.tsx still call notify(title, description, onClick). So on this path:
tagisundefined, so notifications stack instead of replacing each other.data.urlfalls back tolocation.href, the page that was open when the notification fired. If that is a different session, the click goes back there instead of to the session that finished. If it is any non-session route,destination()innotification.jsrejects it and the click does nothing, not even focusing the window.onClickis never used here. Desktop browsers with an active worker previously ranopenNotificationSessionon click and now lose it.
Could you move the sessionHref(input.key, sessionID) caller change from #58400 into this PR, so the fix works without the push stack?
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
The new commit fixes my earlier point: both session notification calls now pass sessionHref(...), so service-worker notifications are tagged per session and a click opens the session that finished. Unit tests (6) and test-browser/server-notification-state.test.ts (4) pass, and tsc -b passes in packages/app. Real Android delivery is still unverified because it needs a physical device. Public API unchanged. No new concerns.
|
From #62750 (closed as a duplicate of this PR — agreed, this is the right place). Two pieces from it that this PR does not have yet, both small and both fix real failures I reproduced with a raw-CDP loop (Chromium with 1. The plugin One-line fix: 2. Request notification permission from a user gesture ( Implementation reference (both included, plus unit tests at the |
Issue for this PR
Closes #49961
Type of change
What does this PR do?
Chrome on Android rejects
new Notification(...). It requiresServiceWorkerRegistration.showNotification().The web platform now uses an active service worker registration when one exists and keeps the constructor only as a fallback. The generated worker imports
public/notification.js, which handlesnotificationclickfor same-origin session URLs: focus an existing tab, navigate an app window, or open a new one.The session notification callers now pass
sessionHref(server, sessionID), so clicking a notification opens the session that finished instead of whatever page happened to be open. Thetagis the same href, so a newer event replaces the stale notification.How did you verify your code works?
bun typecheckinpackages/appbun test src/runtime/platform/notification.test.ts src/runtime/platform/web.test.ts— 6 passedScreenshots / recordings
Not included: the behavior needs a physical Android device. The notification appears through the Android system UI, not inside the app.
Checklist