Add an app-shell service worker for cold PWA launches (A11) - #1907
Add an app-shell service worker for cold PWA launches (A11)#1907SawyerHood wants to merge 4 commits into
Conversation
SawyerHood
changed the title
3062af4 to
b990843
Compare
a5a1b1b to
742b0df
Compare
A cold launch of the bb PWA on a phone through bb connect paid one tunnel round trip for index.html and then ~40 asset fetches (~450 KB brotli) whenever iOS evicted the HTTP cache or the phone landed on a new edge colo, because the app registered no service worker and only /assets/* was cacheable at all. The build now emits dist/sw.js (vite-service-worker.ts, a post-build step that compiles src/service-worker/sw.ts as one classic script with the precache manifest inlined). The manifest is the entry chunk's static closure, the SplitWorkspaceRoute closure, the CSS those chunks import and the latin Inter woff2, and its build id also covers index.html. The worker: - precaches those assets plus /index.html on install and refuses to install when the shell response is not the app (connect sign-in page) or an asset answers with HTML (captive portal), then skipWaiting; - serves app navigations network-first and falls back to the precached shell only when the fetch itself fails, so the connect gate and offline page keep working; - serves /assets/* cache-first with an on-demand runtime cache for lazy chunks; never stores 404s or HTML under an asset URL; - passes /api, /ws, /internal, /__*, plugin assets, root PWA files and every non-GET or cross-origin request through untouched; - drops the caches of previous builds on activate and claims clients. main.tsx registers it after `load`, only for production builds on secure origins outside the desktop shell, and installs a one-shot reload on `vite:preloadError` so a lazy chunk that 404s after `bb update` rotates the hashes reloads the page instead of leaving a blank route. Co-Authored-By: Claude <noreply@anthropic.com>
Non-asset static files are `no-store`, which is right for the shell but wastes the conditional-request path browsers use for service worker update checks. `no-cache` still guarantees a `bb update` is seen at the next check and keeps the connect edge (which caches only long max-age responses) from pinning an old worker. Co-Authored-By: Claude <noreply@anthropic.com>
A build without a worker script (an older dist, or a future build that drops the worker) answered the browser's service-worker update check with index.html at status 200. The browser rejects that as a worker script and keeps the previously registered worker alive with its stale precache. A 404 retires the registration instead, mirroring the /assets/ guard. Co-Authored-By: Claude <noreply@anthropic.com>
The worker refuses to install a shell response that lacks class="bb-app-shell", so a body-class edit in index.html would ship a worker that silently never installs. Move the marker into sw-manifest.ts (shared by both tsconfig projects), assert it in buildServiceWorkerPrecacheManifest so the build fails loudly, and note the coupling next to <body> in index.html. Co-Authored-By: Claude <noreply@anthropic.com>
742b0df to
8cc9347
Compare
b990843 to
915fc56
Compare
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this service-worker change for security, code quality, architecture, performance, and end-to-end behavior. I have started three parallel reviews. I will post one final review after I verify their findings. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| return; | ||
| } | ||
| const now = deps.now(); | ||
| const lastReloadAt = readLastReloadAt(deps); |
There was a problem hiding this comment.
🚨 slopcop/review — The reload guard disappears when storage is unavailable.
When sessionStorage is unavailable or throws, lastReloadAt stays null. Each reload creates a new in-memory latch. A persistent chunk failure can therefore reload the page forever.
Please require a successful cross-reload guard write before reload(). If the storage read or write fails, let the error reach the error boundary. Add tests for storage: null and throwing storage methods.
| // script instead of a plain 404. Mirrors the /api/v1/* guard above. | ||
| if (urlPath.startsWith("/assets/")) { | ||
| // The service worker script gets the same treatment: a build without | ||
| // one must answer its update check with a 404 (which retires the |
There was a problem hiding this comment.
🚨 slopcop/review — A missing worker script does not retire an active worker.
The 404 response is correct, but this comment and the new test claim too much. The Service Worker update algorithm keeps the current worker when the update fetch fails and a newest worker already exists. A rollback to a build without sw.js therefore leaves the old worker and its caches active.
Please add an explicit retirement signal and a browser lifecycle test. The test must activate this worker, serve a workerless build, navigate, and verify that the registration and bb-app-* caches disappear. The specification is here: https://www.w3.org/TR/service-workers/#update-algorithm
| // handler reloads for it, and it must not be pinned in the cache. | ||
| if (isCacheableAssetResponse(response)) { | ||
| // Fire-and-forget: the response goes to the page first. | ||
| void cache.put(request, response.clone()).catch(() => undefined); |
There was a problem hiding this comment.
🚨 slopcop/review — The browser can stop this runtime cache write early.
respondWith() keeps the worker alive until the response settles. This separate cache.put() can still run after that point, so the browser can stop the worker before the lazy chunk reaches the cache.
Please attach the write promise to event.waitUntil() while you return the network response immediately. Add a test with a deferred cache write and verify that the fetch event records that promise.
| - Caches are named `bb-app-<buildId>`; a new build drops the old ones on | ||
| activate. When a QA session shows stale assets, check Application > Service | ||
| Workers (Chrome) or Develop > Service Workers (Safari) and unregister there. | ||
| - The server serves `/sw.js` with `Cache-Control: no-cache` so an update is |
There was a problem hiding this comment.
🚨 slopcop/review — The shell cache policy is no-cache, not no-store.
STATIC_INDEX_CACHE_CONTROL and its server test both require no-cache. Please update this sentence and the related static-cache.test.ts comment so the QA guide matches the server contract.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This PR adds a browser cache for the main bb interface and code files. It reduces tunnel requests during a cold phone launch. It also reloads a stale tab after an update removes an old code chunk.
I found three defects and one small documentation error.
-
High: The chunk error handler can cause an endless reload loop. The cross-reload guard disappears when
sessionStorageis unavailable or throws. Require a successful guard write before reload. Otherwise, let the error reach the error boundary. -
Medium: A 404 for
/sw.jsdoes not remove an active worker. A rollback can leave the old worker and its caches active. Add an explicit retirement path and a browser lifecycle test. The Service Worker update algorithm confirms this behavior. -
Medium: The runtime cache write does not extend the fetch event lifetime. The browser can stop the worker before a lazy chunk reaches the cache. Attach the write promise to
event.waitUntil()and add a deferred-write test. -
Low: The QA guide says the shell uses
no-store. The server contract usesno-cache.
The security review found no direct exploit in the request routes. The worker passes API, WebSocket, internal, plugin, cross-origin, and non-GET requests through. It also uses a network-first rule for navigation.
The production browser check confirmed registration, activation, cache-first reload, runtime cache growth, and the offline shell. The current install manifest has 89 assets. It transfers approximately 1.16 MB with Brotli and stores 4.33 MB of decoded data. This cache fill starts after page load. I treat its size and request count as a tradeoff, not a confirmed defect.
The code repeats the bundle closure walk and the thread route identity from vite-bundle-stats.ts. A shared helper would reduce future drift. The current build checks catch a missing route, so this refactor does not block the PR.
Verification passed:
- 18 focused application tests.
- The focused server static-cache test.
- The application type check through Turbo.
- The production application build through Turbo.
- The bundle budget check.
- The Chromium service-worker check.
git diff --check.- The current GitHub CI run.
I posted line comments with fixes and test requirements. I used a comment-only review, as required.
|
Closing: the maintainer decided to land only layers 1–22 of the mobile-perf stack (#1880–#1901). The branch stays on the remote for reference; it can be reopened and rebased later if the remaining big rocks (sidebar-bootstrap patching, thread bootstrap include=, composer draft push-down, timeline windowing, persisted cache, service worker) are picked up again. |
1 participant
What was wrong
A cold launch of the bb PWA on a phone through bb connect paid one tunnel round trip for index.html and then ~40 asset fetches (~450 KB brotli boot payload) whenever iOS evicted the HTTP cache or the phone reached a new edge colo. The app registered no service worker; only
/assets/*was cacheable, and the connect edge refusesno-storeHTML. A lazy chunk that 404s afterbb updaterotates hashes also left a blank route with no recovery (audit finding A11).What changed
apps/app/vite-service-worker.ts(new,bb:service-workerplugin,enforce: "post",writeBundle): builds the precache manifest from the bundle graph (entry static closure +src/views/SplitWorkspaceRoute.tsxstatic closure + imported CSS +inter-latin-wght-normal-*.woff2), derives a build id from the URL list and the built index.html, then compilessrc/service-worker/sw.tswith a nested lib-mode Vite build into a single classic scriptdist/sw.js(5 KB raw / 2 KB br, precompressed by the existing step). The build fails if the route module has no chunk.apps/app/src/service-worker/sw-core.ts,sw-routing.ts,sw-manifest.ts,sw.ts(new): install precaches the manifest and/index.html, refuses to install when the shell response lacks theclass="bb-app-shell"marker (connect sign-in page) or an asset answers with HTML (captive portal), thenskipWaiting. Navigations: network-first, precached shell only whenfetchrejects; navigations to/api,/ws,/internal,/__*,/install*are never intercepted./assets/*GET: cache-first, on-demand runtime cache in the same build-scoped cache, never stores 404/HTML/redirects. Everything else passes through (norespondWith). Activate deletesbb-app-*caches of other builds and claims clients.apps/app/src/lib/service-worker-registration.ts(new): registers/sw.jsafterloadonly whenimport.meta.env.PROD && window.isSecureContext && "serviceWorker" in navigator && !window.bbDesktop.apps/app/src/lib/chunk-load-failure-reload.ts(new):vite:preloadErrorhandler reloads once (sessionStorage stamp, 60 s window, in-memory latch), otherwise lets the error reach the boundary.apps/app/src/main.tsx: installs the reload handler and calls the registration.apps/server/src/server.ts:/sw.jsgetsCache-Control: no-cache; index.html and other root files stayno-store.docs/debugging-and-qa.md: short section on the worker and how to unregister it during QA.apps/app/tsconfig.node.json,apps/app/package.json(lint list),apps/app/vite.config.tswiring.How you verified
apps/app/src/service-worker/precache-manifest.test.ts(closure selection, CSS, font pattern, exclusions, deterministic build id that changes with assets or html, missing route module throws),apps/app/src/service-worker/sw-core.test.ts(fake scope + fake Cache Storage: install precache + skipWaiting, sign-in shell refused, missing asset fails install, navigation network-first / shell fallback offline / non-app navigations untouched, asset cache-first + runtime cache + 404 not cached, captive-portal HTML never cached, passthrough classes, activate cleanup + claim),apps/app/src/service-worker/sw-routing.test.ts,apps/app/src/lib/service-worker-registration.test.ts(gating),apps/app/src/lib/chunk-load-failure-reload.test.ts(one-shot + window + concurrent collapse), andapps/server/test/app/static-cache.test.tsextended for/sw.js(200, application/javascript, no-cache).pnpm exec turbo run typecheck --filter=@bb/app --filter=@bb/server: 2 successful.pnpm exec turbo run lint --filter=@bb/app: 0 errors (147 pre-existing warnings, none in changed files).pnpm exec turbo run test --filter=@bb/app -- src/service-worker src/lib/service-worker-registration.test.ts src/lib/chunk-load-failure-reload.test.ts: 5 files, 17 tests passed.pnpm exec turbo run test --filter=@bb/server -- test/app/static-cache.test.ts: passed.pnpm exec turbo run build --filter=@bb/app --force: emitsdist/sw.js(+ .br/.gz); manifest has 77 URLs (~1.4 MB br) and covers every/assets/href/src in the built index.html;node scripts/check-bundle-budget.mjs: 449.2 KB br boot, budget OK.Fixes: part of the mobile / iOS Safari performance program (verified sweep report in the bb thread; no single issue).
Stack context
Layer 28 of 28 in the
bb/mobile-perf/*stack (bottom → top: quick wins first, big rocks last).bb/mobile-perf/persisted-query-cache(Persist an allowlisted query cache slice to IndexedDB behind an experiment (J2) #1906).Cache-Control: no-cache(all other non-/assets/ static files keepno-store). No env var / CLI flag / config knob added.