Repository navigation
fix: count progress against the set the total describes - #19
Merged
Merged
Conversation
The loading overlay could print `Loaded 5 of 4 resources`. The numerator
and the denominator came from two different sets: `resourcesCount` covered
the pre-cached set (Core + Required), while the client counted every
resource the SW reported — including Optional files served lazily by the
fetch handler, which is unbounded. On the example app a cold load counts
9 resources against a total of 6.
Progress messages now carry `counted` (membership of the set
`resourcesCount` describes) and `swVersion` (which worker spoke, since an
old controller and a new installing worker both post to the same client).
The bootstrap counts only its own worker's counted resources, latches the
denominator, and clamps the rendered number as a last line of defence.
All messages go through one reporter, and the pre-cache and the counted
set are driven by the same category list so they cannot drift apart.
Along the way, the app shell:
- `index.html` was categorised `ignore` and so never reached the manifest,
leaving the fetch handler's `manifest['index.html']` lookups permanently
empty — the shell was neither pre-cached nor reported, and an offline
navigation had nothing to fall back to. It is now `required`, stored
under one canonical cache key shared by precache and navigation.
- Navigations to SPA deep links were dropped: the manifest lookup ran
before the navigate branch and returned early for any route without an
entry of its own.
- `reloadIfForeignController` resolved the SW filename against
`location.href` rather than the document base URL that `register()`
uses, so on `/chat/42` it expected `/chat/sw.js`, judged the real
`/sw.js` controller foreign, unregistered it and reloaded the page.
Fixes DoctorinaAI/doctorina#4770
Review of the previous commit turned up three ways the navigation handler
could stop the app from starting, and one way the manifest could lie.
Every same-origin navigation wrote its response into the app-shell cache
slot. A page the host serves outside the SPA rewrite answers a navigation
with its own HTML, and storing that under `index.html` leaves the app
unable to boot from cache at all. Firebase Hosting excludes `/__/auth/**`
from the rewrite, and Firebase Auth puts its handler and its iframe there
whenever `authDomain` is the app's own domain — so on a deployment that
signs users in, a sign-in replaces the shell. Reproduced in Chromium
against the example app: after one navigation to `offline.html`, the deep
route `/deep/route` renders the offline page instead of the app.
Only a request *for* the shell may refresh the shell now — `/` and
`/index.html`, which is what a host with an SPA rewrite answers with the
shell anyway. Deep links read that cache and never write it. Paths under a
host-reserved prefix (`__/`) are passed straight to the network, so a
sign-in document no longer goes through `fetchWithRetry` — which downgrades
a navigation request's mode — nor falls back to the app shell. And a
redirected response is never stored: browsers refuse to replay one for a
navigation.
`cache.put` sat inside the same `try` as the fetch, so a failed cache write
discarded a page the origin had just served. Quota pressure is routine for
a wasm-sized app, and it turned a 200 into the stale cached copy — or into
the literal `503 Offline` page on a profile with nothing cached yet, while
the network was fine. Cache reads and writes are each contained now, and
nothing after a successful fetch can reject the promise handed to
`respondWith`. In the same spirit `cacheFirst` degrades to a plain fetch
when CacheStorage is unusable rather than failing the resource outright,
and `progress.report` is best-effort — it is telemetry for an overlay, but
it sits on the path that produces a response.
The manifest recorded the hash and size of `index.html` as it was *before*
`cleanup` substituted `{{sw_version}}` into it, so the entry described a
file the origin never serves. Harmless while the shell was `ignore`; not
once it became a manifest entry. The substitution now runs before hashing
and the entry is refreshed. The version stays derived from the
pre-substitution manifest, so it is unchanged and still deterministic.
Finally, a `bootstrap.js` and an `sw.js` from different builds froze the
counter silently, since no message passes the `swVersion` check. Logged
once now, instead of leaving a still bar with nothing to explain it.
…ects with the shell Review follow-ups on the app-shell rework. Three of these are only reachable off the happy path, which is where a service worker does its damage. Resource keys are now relative to `registration.scope`. `getResourceKey` returned the absolute pathname, so under a non-root `<base href>` every manifest lookup missed — and `RESERVED_PATH_PREFIXES` stopped matching, which meant an app served from `/app/` routed Firebase Auth's `/app/__/auth/*` navigations straight through the app-shell branch. The guard added in the previous commit was inert exactly where it was load bearing. `swapCaches` shared the normalisation and so could evict the shell it had just pre-cached. A same-origin navigation the origin redirects is no longer answered with the app shell. Navigations carry redirect mode "manual", so a 3xx arrives as an opaque redirect (status 0, `ok` false) — indistinguishable, to the previous code, from a failure. It is passed back untouched now, which is what lets the browser follow it. Both writers of the shell store a replayable copy. The `!response.redirected` guard covered the network-first refresh but not `precacheResources`, which on a cold profile is the only writer there is: a host that redirects `index.html?v=…` left a shell the browser refuses to serve for any later navigation. Rebuilding the response keeps the body and drops the flag, so the offline path survives a redirecting origin rather than going empty. Also: the shell refresh reports `updated` only when it was actually stored, instead of announcing a write quota pressure had just discarded; the pre-cache progress callback runs outside the block that attributes failures, so a throwing report can no longer be recorded as a failed resource or fail an install over telemetry; and the foreign-`swVersion` report is deferred to teardown and fires only when *every* message was foreign — it was warning on ordinary update loads, where the outgoing controller talks alongside the installing worker, and pointing at a Cache-Control problem that wasn't there. The counter clamp now has a test that reaches it: two builds reporting under one pinned `--version` is the one way a numerator can still outrun a latched denominator. Deleting the clamp previously broke no test at all. The Playwright counter specs wait for the worker to go quiet instead of sleeping on fixed timers — five iterations of `waitForTimeout(4000)` plus ten release-build navigations against a 30s per-test budget was arithmetically set up to flake — and the five-load spec installs its recorder once, since `addInitScript` accumulates and was pushing every message five times by the last run.
Making the shell a manifest entry meant the generator had to substitute
`{{sw_version}}` before hashing it, or the manifest would describe a file
the origin never serves. That substitution was hoisted out of `cleanup` and
run unconditionally, which quietly did two things it should not.
It broke the flag's contract — `--no-cleanup` exists to leave the input tree
alone, and it is the documented in-place dev loop. And because the version
is derived from the manifest the shell now belongs to, rewriting the shell
inside the same run made the derived version depend on a file that run had
just changed: generate twice on one directory and byte-identical content
shipped under a new version, handing every client a spurious update, while
`index.html` kept the version stamped by the first run.
Gating on `noCleanup` closes both. Under the flag nothing is rewritten and
the manifest describes the placeholders as they sit on disk; without it,
`cleanup` removes `flutter_bootstrap.js` in the same run, so a second pass
fails loudly instead of silently deriving something else.
New `test/generator_test.dart` covers `generate()` over a synthetic build
directory — the suites under `test/integration/` need a real `flutter build
web`, so the invariants binding the manifest to the bytes on disk had no
home that runs everywhere. It pins the shell entry against its file, version
stability across in-place re-runs, and that the version still moves for any
changed resource, the shell included: idempotence must not be bought by
making the version blind to `index.html`.
Also corrects the doc comment on `applyIndexHtmlVersion`, which described an
ordering the code does not have, and documents that `--ignore=index.html`
cannot demote the shell — required is matched before ignore, so `--exclude`
is the only opt-out.
Publish the app-shell and counter work as a pre-release first, so it can be exercised against a real deployment before the version that ordinary caret constraints will pick up. Note for consumers: `^0.1.6` does not resolve a pre-release — `0.1.6-dev` sorts before `0.1.6`, so it falls outside `>=0.1.6`. Pin `0.1.6-dev` exactly, or use `^0.1.6-dev` to accept the pre-release now and the release later.
The README still told readers to activate `^0.1.2`, four releases behind, and a caret range would not resolve a pre-release anyway. Documents the exact pin the boot path warrants, and the pre-release ordering rule that makes `^0.1.6` skip `0.1.6-dev`.
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
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.
Fixes DoctorinaAI/doctorina#4770 —
Loaded 5 of 4 resourceson web startup.The bug
The numerator and the denominator came from two different sets of resources.
resourcesCountcounts the pre-cached set (Core+Required), whilebootstrap.jsadded every resource key the SW reported a terminal statusfor — including
Optionalfiles served lazily by the fetch handler, which isunbounded. So the overflow is systematic, and its size is whatever the startup
happens to touch (the field report saw both
5 of 4and6 of 4).Reproduced on the example app at 0.1.5, cold load:
The rendered text stopped at
6 of 6only because the overlay is disposed onfirst frame — the fonts land a moment later. In production, where the app takes
longer to boot, the overflow is on screen.
The fix
sw-progressmessages now carry two fields:counted— whether this resource is one of theresourcesCountones.The SW still reports everything it serves (useful for display); only counted
resources may enter a numerator rendered against that total.
swVersion— which worker spoke. During an update load an old controllerand a new installing worker both post to the same client, with different
manifests and different totals; the bootstrap now ignores anyone but its own
build, so the denominator cannot change mid-load. A mismatch is logged once,
so a
bootstrap.jsand answ.jsfrom different builds can't freeze thecounter silently.
Everything goes through a single
createProgressReporter, andprecacheResourcesnow takes its category list (
COUNTED_CATEGORIES) from the same module, so "whatis pre-cached" and "what is counted" cannot drift apart again. The rendered number
is also clamped — unreachable given the above, but it is what the user reads.
App-shell bugs found on the way
index.htmlwasignore, so it never reached the manifest and the fetchhandler's
manifest['index.html']lookups were permanently empty. The shell wasneither pre-cached nor reported as progress, and on a cold profile an offline
navigation had nothing to fall back to. Now
required, stored under onecanonical
index.htmlcache key shared by precache and navigation.request.mode === 'navigate'branch and returned early for any route without anentry of its own — making that branch unreachable for exactly the routes that
need it.
reloadIfForeignControllerresolvedsw.jsagainstlocation.hrefratherthan the document base URL
register()uses. On/chat/42it expected/chat/sw.js, judged the real/sw.jscontroller foreign, unregistered it andreloaded the page — once per tab session, on every deep link. Caught by the new
deep-route e2e test.
Second pass: what the first cut of the shell fix broke
Making the navigate branch unconditional handed every same-origin navigation to
the shell handler, and the shell handler wrote whatever it got.
Any same-origin page could replace the app shell. A page the host serves
outside the SPA rewrite answers a navigation with its own HTML, and storing
that under
index.htmlleaves the app unable to boot from cache at all.Firebase Hosting excludes
/__/auth/**from the rewrite, and Firebase Authputs its handler and its iframe there whenever
authDomainis the app's owndomain — which is the case in production. Reproduced in Chromium against the
example app: after one navigation to
offline.html,/deep/routerenders theoffline page.
Only a request for the shell refreshes the shell now (
/,/index.html);deep links read that cache and never write it. Paths under a host-reserved
prefix (
RESERVED_PATH_PREFIXES = ['__/']) are passed straight to the network,so a sign-in document no longer goes through
fetchWithRetry— whichdowngrades a navigation request's mode — nor falls back to the app shell.
A redirected response is never stored: browsers refuse to replay one for a
navigation.
A failed cache write discarded a good page.
cache.putsat inside the sametryas the fetch, so quota pressure — routine for a wasm-sized app — turned a200 from the origin into the stale cached copy, or into the literal
503 Offlinepage on a profile with nothing cached yet, while the network wasfine. Cache reads and writes are each contained now, and nothing after a
successful fetch can reject the promise handed to
respondWith. In the samespirit
cacheFirstdegrades to a plain fetch whenCacheStorageis unusable,and
progress.reportis best-effort — telemetry for an overlay should not beable to fail a navigation or a
main.dart.wasm.The manifest described an
index.htmlthe origin never serves.cleanupsubstitutes
{{sw_version}}after the manifest is hashed. Harmless while theshell was
ignore; not once it became a manifest entry. Verified on a realbuild: manifest
0674fe…/1069vs on-disk637e02…/1067. The substitution nowruns before hashing and the entry is refreshed; the version stays derived from
the pre-substitution manifest, so it is unchanged and still deterministic.
Known limitation
A warm load has no install event, so the counter does not reach its total:
measured
cold 7 of 7,warm 2 of 7on the example. The shell is answeredduring the navigation itself, before any page script exists to hear the message;
a JS-build entry point is never requested on a wasm-capable browser; the rest can
come from the browser's HTTP cache without a fetch event. The overflow this PR
fixes is gone, but the count is a floor, not a completion signal — documented as
such in
docs/service-worker.md. Closing that gap needs a protocol addition (theSW replying with the counted resources already in the content cache) and is left
for a follow-up rather than folded into a release whose job is not to break
startup.
Verification
Against the example app (
flutter build web --release --wasm→sw:generate):Loaded 1 of 7…Loaded 7 of 7, monotonic, reaching the totalexactly when the last pre-cached resource settles. Warm load and five consecutive
cold loads: no overflow.
Suites: 247 Vitest, 98 Dart (unit + integration), 16 Playwright — all green.
New tests:
e2e/progress-counter.spec.ts— cold / warm / five-consecutive-cold, asserting onthe SW messages rather than the overlay text (which stops updating once the app
takes over).
e2e/app-shell.spec.ts— the shell is pre-cached, answers an offline navigation,and answers a route the origin 404s.
e2e/app-shell-integrity.spec.ts— visiting a same-origin page outside the SPAleaves the cached shell intact, and a deep link still gets the app afterwards.
Both fail against the first cut of the shell fix.
src/sw/__tests__/progress.test.ts, plus pipeline cases for foreign versions,uncounted resources and a fixed denominator.
src/sw/__tests__/fetch-handler.navigation.test.ts— host-reserved navigationsare passed through; the shell is refreshed only from a request for the shell;
a redirected response is not stored; a page the origin served is still returned
when the cache write fails; precache and the fetch handler agree on the shell
cache key.
Also fixed a pre-existing red test:
bails out safely when sessionStorage throwsspied on
Storage.prototype.setItem, which jsdom does not route sessionStoragethrough, so the spy never fired.
Breaking-ish
resourcesSizenow describes the counted set rather than every cacheable resource,so it agrees with
resourcesCount. Anything reading it as "total bytes on disk"should sum the manifest instead. Nothing in the Doctorina app consumes
sw-progress.Release
Needs a
0.1.6tag to publish;DoctorinaAI/doctorinapinssw ^0.1.6inDoctorinaAI/doctorina#4770's branch and cannot go green until then.