Repository navigation
feat: [SDK-5120] inline notification icons from the service worker so CDN issues preserve the brand icon - #1531
sherwinski wants to merge 7 commits into
Conversation
sherwinski
changed the title
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
A failed fetch falls back to the cache, then the remote URL, and the click path still gets the original URL. A good download is dropped when the cache write throws, and any small 2xx body is stored with its raw content type.
initSaveState writes defaultIcon for every user. The worker already uses that key when a payload has no icon, and nothing in src wrote it before, so those notifications start showing the site icon with the flag off.
refreshFeatureFlags writes localStorage and then db.put. That put resolves without writing when IndexedDB is wedged, and the worker only reads IndexedDB, so a flag turned off on the page can stay on in the worker.
The flag-on service worker test returns the same bytes for every URL and expects the icon and the badge to be the same data URL, so swapping them still passes.
|
Should make a stacked pr and split the feature flag work as the first pr |
Fair enough |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review
Act on (3/3): void refreshFeatureFlags() is not joined before initSaveState reads isFeatureEnabled to persist defaultIcon. get() retries 10s/20s/30s, so the load that turns the flag on typically writes defaultIcon: null while the worker mirror later becomes on. displayNotification then cannot fill a missing icon or treat the Site Setup icon as default when image is present. The off-transition leaves defaultIcon set after the worker flag is cleared. Write defaultIcon from the list refreshFeatureFlags actually stored, or await that refresh (including failure) before the Options write.
Consider (2/3): os_feature_flags and Options.featureFlags are not keyed by app id, and the app-id reset in initSaveState does not clear them. A failed or in-flight features request after an app swap inherits the previous app's rollout (including sdk_identity_verification).
Consider (2/3): With the flag on, showNotification waits up to 5s for icon/badge, and notifications in one push are shown sequentially. A dead CDN — the case this feature targets — delays display by the full timeout, then the browser fetches the remote URL again.
Consider (1/3, verified): readUpTo returns an empty Uint8Array for a 0-length 2xx image response. That value is truthy, so cache.put replaces the last good copy with empty bytes.
Noted: The worker kill switch needs a later page visit. CORS-less hosts stay a silent extra fetch (called out in the PR). icon === defaultIcon is an exact string match (author confirmed payload equality). A late-committing wedged db.put can still diverge the page list from the worker list.
Dismissed: Writing defaultIcon: null when the flag is off (intentional after the last review). ensureImageResourceHttps now returning the original string (intentional, ungated). The comment that data stays as received (title/icon were already mutated before this PR).
Sent by Cursor Automation: PR Reviews
| Log._info(`Set pageTitle to be '${pageTitle}'.`); | ||
| // The worker also uses this key to fill a missing icon, so it is only set | ||
| // while the inline images flag is on. | ||
| const inlineImages = isFeatureEnabled(FeatureFlag._InlineNotificationImages); |
There was a problem hiding this comment.
isFeatureEnabled reads the previous localStorage list here, while refreshFeatureFlags is fire-and-forget from OneSignal.init and get() retries 10s/20s/30s. On the load that turns sdk_web_inline_notification_images on, this write typically stores defaultIcon: null and nothing updates it after the features response lands. The worker then cannot fill a missing icon or treat the Site Setup icon as default when image is present. The off-transition is the reverse: defaultIcon stays set after the worker mirror is cleared.
Write defaultIcon from the flag list refreshFeatureFlags actually stored, or await that refresh (including its failure path) before this put.
| const { ok, result } = await get<FeaturesResponse>(`apps/${appId}/sdk/features/web/${VERSION}`); | ||
| if (!ok || !Array.isArray(result?.features)) return; | ||
| const flags = result.features.filter((f) => typeof f === 'string'); | ||
| await db.put('Options', { |
There was a problem hiding this comment.
This key is not scoped to appId, and neither is os_feature_flags. initSaveState's app-id reset does not clear either store. If the features request for the new app fails or is still in flight, the worker (and defaultIcon) keep the previous app's sdk_web_inline_notification_images / sdk_identity_verification values.
Persist the app id with the list and reject a mismatch, or key both caches by app id and clear them in that migration.
| return undefined; | ||
| } | ||
| const bytes = await readUpTo(response.body, MAX_INLINE_BYTES); | ||
| return bytes && { bytes, type }; |
There was a problem hiding this comment.
A 2xx response with an allowed Content-Type and a 0-length body makes readUpTo return new Uint8Array(0), which is truthy, so this caches empty bytes and returns data:image/png;base64,. That overwrites the last good copy this cache exists to replay.
Treat a 0-length body as a failed fetch so the previous cached image (or the remote URL) is used instead.
d024b51 to
e0c60a6
Compare
ensureImageResourceHttps returned undefined when new URL() threw, which dropped the icon from the notification. Production logs show this TypeError for customer payloads. The browser may still load such a URL, so return it unchanged instead.
…eature flag Chromium fetches notification icons at display time with no fallback to the HTTP cache or Cache Storage. On Android a failed fetch shows a grey letter placeholder in place of the brand icon, so a short outage of the image CDN marks every notification in that window. When the sdk_web_inline_notification_images flag is on, the worker now: 1. Fetches the icon and the badge inside the push handler with a 5 s timeout. 2. Stores the bytes in Cache Storage (10 most recent images) and passes them to showNotification as a data: URL. 3. On a failed fetch, uses the last cached copy. With no cached copy, or for an image over 128 KB, it passes the remote URL as before. The notification object in `data` keeps the original URLs. A cross-origin image host must send Access-Control-Allow-Origin for the worker to read the bytes; without it the fetch rejects and the remote URL is used. The worker has no localStorage, so refreshFeatureFlags now mirrors the active flag list (server list plus local overrides) into the IndexedDB Options store, and isFeatureEnabledInWorker reads it there.
…ge image On Chrome for macOS the server replaces chrome_web_icon with the chrome_web_image URL. That per-notification image should not be inlined or cached, so the worker skips inlining when the icon equals the image. The badge is still inlined. Raises the worker size limit to the measured 12.96 kB.
A real send with both global_image and chrome_web_image set shows that the server can deliver an icon that is one image URL while image is a different one, so an icon === image check misses the macOS swap. The swap only happens when an image is present, so the worker now inlines the icon in that case only when it equals the app default icon from the Options store. A per-notification icon with an image stays a remote URL. Raises the worker size limit to the measured 12.97 kB.
…pped image The worker read Options.defaultIcon but no code wrote it, so the guard that inlines only the app default icon when a notification has an image never matched. The page now copies siteInfo.defaultIconUrl from the server config into the Options store at init, and clears it when the app has no default icon. A real send confirmed the server delivers the Site Setup icon as the exact same string.
Review follow-ups on the inline images change: - Build the data URL before the cache write, and treat the cache write and read as best effort, so a quota error does not drop bytes that were already fetched. - Inline and cache only image/png, image/jpeg, image/gif, and image/webp. Any other response falls back to the cached copy, which also covers a captive portal that answers 200 text/html. - Return before the body when Content-Length is over 128 KB, and stop the stream once the cap is passed instead of buffering the whole body. - Write Options.defaultIcon only while the flag is on, so the existing no-icon fallback in the worker does not turn on for every user. - Give each URL its own bytes in the worker test so a swapped icon and badge would fail.
e0c60a6 to
3593213
Compare
fadi-george
left a comment
fadi-george
left a comment
There was a problem hiding this comment.
Non-blocking: src/shared/libraries/workerMessenger/sw.ts:52 (not in this diff), the DisplayNotification message handler's promise is dropped with no event.waitUntil. That was fine when it showed the notification right away, but it now waits up to 5s on image fetches first. Worth wrapping it so the worker stays alive until it's shown.
| Log._info(`Set pageTitle to be '${pageTitle}'.`); | ||
| // The worker also uses this key to fill a missing icon, so it is only set | ||
| // while the inline images flag is on. | ||
| const inlineImages = isFeatureEnabled(FeatureFlag._InlineNotificationImages); |
There was a problem hiding this comment.
Blocking: refreshFeatureFlags isn't awaited (OneSignal.ts:139), so this reads last visit's localStorage list. The first load after the flag turns on writes null while the worker already inlines, so an icon on an image notification never matches and stays remote. After the flag turns off it can write the URL again. Could we always store defaultIconUrl here and gate its use in the worker instead?
| @@ -584,12 +588,26 @@ async function displayNotification(notification: IMutableOSNotification) { | |||
|
|
|||
| notification.title = notification.title ? notification.title : defaultTitle; | |||
| notification.icon = notification.icon ? notification.icon : defaultIcon ? defaultIcon : undefined; | |||
There was a problem hiding this comment.
Blocking: this fill doesn't check the flag, so the only gate is the page write above, which can be stale. After the flag goes off, icon-less notifications keep getting the default icon until that visitor loads the page again. Can we only use defaultIcon here and in iconIsAppDefault when isFeatureEnabledInWorker is true? That also makes the kill switch work as soon as the mirror updates.
| // over the inline limit, so the caller falls back to the cached copy. | ||
| async function fetchImage(url: string): Promise<ImageBytes | undefined> { | ||
| try { | ||
| const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); |
There was a problem hiding this comment.
Non-blocking: a host without CORS headers fails here on every push, then the browser fetches the same URL again. A slow host can delay display by up to 5s. Worth remembering CORS failures per URL for a while?
| const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); | ||
| const type = imageType(response); | ||
| const declaredLength = Number(response.headers.get('content-length')); | ||
| if (!response.ok || !type || !response.body || declaredLength > MAX_INLINE_BYTES) { |
There was a problem hiding this comment.
Non-blocking: a valid image over the cap takes the same path as a failed fetch, so if an icon is replaced at the same URL with a file over 128 KB, the old cached version keeps showing while the network is fine. Maybe return the URL when the fetch worked but was too big, and only use the cache when it failed or the response wasn't an image.
|
|
||
| ensureNotificationResourcesHttps(notification); | ||
|
|
||
| // The notification object stays as received so `data` and the click |
There was a problem hiding this comment.
Nit: the object isn't quite as received, since the default icon fill and ensureNotificationResourcesHttps have already changed icon/image. Maybe say data keeps the remote URLs rather than the inlined bytes.
Description
1 Line Summary
The worker fetches the notification icon and badge, keeping the last good copy in Cache Storage, and passes the bytes to
showNotificationas adata:URL. For now, this is gated behind thesdk_web_inline_notification_imagesflag. Stacked on #1534, which adds the flag reader.Details
Chromium fetches notification icons at display time and has no fallback when that fetch fails. On Android a failed fetch shows a grey letter placeholder in place of the brand icon, corresponding to the first letter of the host site's URL e.g.
W.Worker change (
src/sw/helpers/inlineImage.ts)When the flag is on,
displayNotificationresolvesiconandbadgethroughinlineImagebeforeshowNotification:fetch(url)with a 5 s timeout. A 2xx response with aContent-Typeofimage/png,image/jpeg,image/gif, orimage/webpand a body of 128 KB or less is handed to the browser asdata:<type>;base64,.... The body is read as a stream and dropped as soon as it passes the cap; aContent-Lengthover the cap skips the body.onesignal-notification-images, 10 most recent entries). The write is best effort: a quota error is logged and the fetched bytes are still used.200 text/html), or a body over the cap, the last cached copy for that URL is used.The original image URL remains in the notification object in
data, so click handlers and webhooks are not affected. The largeimageis not inlined.On Chrome for macOS the server sets
chrome_web_iconto a per-notification image URL at delivery time when an image is set. A notification with bothglobal_imageandchrome_web_imageshowed that the deliverediconandimagecan be two different URLs, so the worker does not compare them. Instead, when the notification has animage, the icon is inlined only if it equals the app default icon from theOptionsstore. A per-notification icon that comes with an image stays a remote URL and is never cached. The badge is still inlined.The worker already read
Options.defaultIcon, but no code wrote it. The page now copiesconfig.siteInfo.defaultIconUrlfrom the server config into theOptionsstore at init (initSaveState) while the flag is on, and writesnullwhen the flag is off or the app has no default icon. The worker also uses that key to fill a missing icon, so the flag covers that fallback too. Testing confirmed that a notification includes the Site Setup icon in the payload as the exact same string asdefaultIconUrl, so the comparison should hold. The cache key is the full URL in every case, so a cached brand icon is never shown in place of a different image.Dependencies
A cross-origin host must send
Access-Control-Allow-Originfor the worker to read the bytes.img.os-content.comandimg.onesignal.comdo not send it today (confirmed by runningcurl -Ion 2026-10-13), so for those hosts the fetch rejects and the behavior is unchanged until we add the header via infra.Feature flag
Stacked on #1534 (
sherwin/sdk-5597), which adds the flag reader and the IndexedDB mirror the worker reads. This PR adds thesdk_web_inline_notification_imagesconstant and theisFeatureEnabledInWorkercall indisplayNotification. Theos_feature_overrideslocalStorage key turns the flag on for local testing, in the page and in the worker.Included fix
ensureImageResourceHttpsreturnedundefinedwhennew URL()threw, which dropped the image from the notification. It now returns the original string.Bundle size
Over the flag reader PR: page 43.49 kB → 43.58 kB (+90 B: the default icon write and its flag check). Worker 12.47 kB → 13.19 kB (+720 B: the fetch, the streamed read with the cap, the type allowlist, and the cache). Limits set to 43.6 kB and 13.2 kB.
Out of scope, needed before the flag can go on
Access-Control-Allow-Origin: *onimg.os-content.comforGET/HEADraster image responses: staging infra#7654, production infra#7615.img.onesignal.comis being decommissioned; The Sun's icon still resolves there until OneSignal#20459 backfills it.sdk_web_inline_notification_imagessoGET apps/{appId}/sdk/features/web/{version}returns it for an app first.Systems Affected
Validation
Tests
Info
vp test run— 64 files, 611 tests pass.vp check— no new errors or warnings.maincurrently has 3 formatting issues and 2 type errors inBell.test.tsfrom the Vite+ 1.3.0 upgrade (chore: upgrade to Vite+ 1.3.0 and TypeScript 7 #1532); they are unchanged here.vp run build:prod— passes with the new limits.src/sw/helpers/inlineImage.test.ts(17),ServiceWorker.test.ts > push event > notification images(8, each URL gets its own bytes),init.test.tsdefault icon write (3),app.test.tsdefault icon mapping (1).Manual test on Chrome for Android
Pixel 9 emulator (Android 17, Chrome 154), dev build from this branch served over
adb reverse, flag on throughos_feature_overrides, worker console read over CDP. Icon host:avatars.githubusercontent.com(sendsAccess-Control-Allow-Origin: *).icon= avatar URLshowNotificationgot a 7,027-chardata:image/jpegURL;data.iconkept the remote URL; Cache Storage holds 1 entry (5,252 bytes).icon= avatar URLblocked), the cached bytes were used, icon shown. Onmainthis is the grey letter.image=chrome_web_imageURL, noiconicon= default icon URL,image=chrome_web_imageURLiconequalssiteInfo.defaultIconUrlexactly.Chrome for macOS (Arc) payload checks: with both image fields set the server delivers
icon=global_imageURL andimage=chrome_web_imageURL; with no image and no default icon it delivers noicon.Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of arraysyntax. PreferforEachor usemapcontextif possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.contextScreenshots
Info
None yet; a device recording will follow the end-to-end run.
Checklist
Related Tickets