From 6bd56766a09df5a4ca21efec24fcc9f3c0e29fa4 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 25 Aug 2026 19:55:33 -0400 Subject: [PATCH 1/2] fix(web): the worker no longer takes over a page the old build rendered, and keel.web is strict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in code shipped last week, both found by running a checklist and a type checker against it rather than by reading it again. ── THE WORKER TOOK OVER A PAGE IT DID NOT RENDER ────────────────────────────── `install` called `skipWaiting()` unconditionally, and `activate` called `clients.claim()`. The argument for that -- written into the module -- was that the version-keyed cache made it safe: new worker, new cache, old one deleted. That reasoning is sound about CACHES and silent about the page already on screen. `skipWaiting` + `claim` takes over a document parsed and rendered by the OLD build's JavaScript and answers its later requests from the NEW build's cache. One page, two builds, nothing on screen to say so. keel had a second line of defence that made it hard to notice: a new build means the process restarted, which means a new session token, which means every `/api/*` call from the old page is a 403 the banner reports. The window was narrow and loud rather than wide and quiet. It was still a window, and a hazard covered by an unrelated layer is not a hazard that has been dealt with. Now the new worker installs its cache and STAYS WAITING. `main.js` notices, the footer offers "A newer build is ready -- reload", and only a click sends `SKIP_WAITING`. The reload waits for `controllerchange` rather than firing straight after the message, because the takeover has to land first or the reload fetches the old build again and leaves the offer standing. The offer is in the footer beside the build line, NOT in the engine banner: that banner is the page's one `aria-live` region and it answers "is keel running", so an upgrade notice there would interrupt a screen reader mid-table to say something that can wait indefinitely. ── AND A SECOND BUG, FOUND BY DRIVING THE FIRST FIX ─────────────────────────── The first spelling of `watchForUpdate` returned early when there was no controller -- meaning that on a FIRST visit, the one load where there reliably is none, the `updatefound` listener was never attached at all. The controller check belongs on the OFFER (do not announce a first install as an update), never on the watching. Found in a browser, not in review. ── `keel/web` IS `--strict`, AND WAS TWO ERRORS AWAY ────────────────────────── Measured rather than assumed: `mypy --strict keel/web` reported two errors, both in `build_server`, and both the same defect. Three-argument `type()` is declared to return plain `type`, so it produced an `Any` that flowed out of a function annotated `-> KeelServer` -- under a `# type: ignore[return-value]` that did not name the error mypy was actually reporting (`no-any-return`), a suppression that silences nothing while looking like it silences something. Rewritten as nested subclasses: the types are checked rather than asserted, and `keel.web.*` now carries the strict flag block. It is the layer where that matters most -- everything under it is reachable from a socket. `test_strictly_typed_packages_ship_a_py_typed_marker` caught the consequence immediately, which is the pin working: PEP 561's marker is a promise to whoever INSTALLS a distribution, and `keel.web` is an internal subpackage with no installer of its own. The rule now reads distribution ROOTS, with a mutation guard so the filter cannot quietly empty the list it parametrises over. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- keel/web/server.py | 28 ++++++++++-- keel/web/static/css/keel.css | 18 ++++++++ keel/web/static/index.html | 9 ++++ keel/web/static/js/main.js | 75 ++++++++++++++++++++++++++++++ keel/web/static/sw.js | 39 ++++++++++++---- pyproject.toml | 9 ++++ tests/test_packaging.py | 30 +++++++++++- tests/web/test_pwa.py | 89 ++++++++++++++++++++++++++++++++++++ 8 files changed, 283 insertions(+), 14 deletions(-) diff --git a/keel/web/server.py b/keel/web/server.py index 6533141..7e1c457 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -814,10 +814,30 @@ class KeelServer(ThreadingHTTPServer): def build_server(cfg: ServeConfig) -> KeelServer: - handler = type("BoundKeelHandler", (KeelHandler,), {"cfg": cfg}) - family = socket.AF_INET6 if ":" in cfg.host else socket.AF_INET - server_type = type("BoundKeelServer", (KeelServer,), {"address_family": family}) - return server_type((cfg.host, cfg.port), handler) # type: ignore[return-value] + """A server bound to `cfg`, with the address family its host implies. + + **Nested subclasses rather than three-argument `type()`.** Both bind a per-run config onto a + handler class and an address family onto a server class, and both are correct at runtime -- + but `type(name, bases, namespace)` is declared to return plain `type`, so calling it produced + an `Any` that flowed straight out of a function annotated `-> KeelServer`. The + `# type: ignore[return-value]` that used to sit here did not even name the right error: mypy + reported `no-any-return` and noted the code was not covered, which is a suppression that + silences nothing while looking like it silences something. Written as classes, the types are + checked rather than asserted, and `keel/web/` type-checks under full `--strict`. + + A fresh pair per call is deliberate: `address_family` and `cfg` are class attributes that + `http.server` reads during `__init__`, so binding them to the shared classes would make two + concurrently-built servers overwrite each other's configuration. + """ + bound_cfg = cfg + + class BoundKeelHandler(KeelHandler): + cfg = bound_cfg + + class BoundKeelServer(KeelServer): + address_family = socket.AF_INET6 if ":" in cfg.host else socket.AF_INET + + return BoundKeelServer((cfg.host, cfg.port), BoundKeelHandler) def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int: diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index 1bb56c4..c9d2ee8 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -564,3 +564,21 @@ button.run:disabled { cursor: progress; opacity: 0.7; } it is present on every card from the moment the card is drawn. */ .action-outcome { margin: 0.5rem 0 0; } .action-outcome:empty { margin: 0; } + +/* The "a newer build is ready" offer (#538). Sized like the footer text it sits in rather than + like the action buttons: it is an offer an operator may ignore indefinitely, and a page that + shouts about an available upgrade while somebody is reading a drawdown has its priorities + backwards. `--warn` because it reports a real difference between what is running and what is + installed, without claiming anything is broken. */ +button.update { + background: none; + border: 1px solid var(--control-line); + border-radius: 4px; + color: var(--warn); + font: inherit; + font-size: 0.85rem; + padding: 0.15rem 0.5rem; + cursor: pointer; +} +button.update:hover { color: var(--fg); } +button.update:disabled { cursor: progress; opacity: 0.7; } diff --git a/keel/web/static/index.html b/keel/web/static/index.html index 6ac1615..b401d8c 100644 --- a/keel/web/static/index.html +++ b/keel/web/static/index.html @@ -136,6 +136,15 @@

This page needs JavaScript

keel · served from this machine only + +
diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index 57f9d3b..be576e8 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -129,6 +129,8 @@ const contentNode = must("content"); const buildNode = must("build"); /** The header's outbound documentation link (#539); its href gains `?v=` once the build is known. */ const docsNode = /** @type {HTMLAnchorElement} */ (must("docs-link")); +/** Where the "a newer build is ready" offer goes (#538). Empty until there is one. */ +const updateNode = must("update"); /** * This session's write token (#540), read off `/api/setup` whenever the setup view mounts. @@ -761,9 +763,82 @@ function registerWorker(config) { // install an optional cache would be trading a working page for a nicety. void navigator.serviceWorker .register(`${BASE}sw.js?v=${encodeURIComponent(build)}`, { scope: BASE }) + .then((registration) => watchForUpdate(registration)) .catch(() => {}); } +/** + * Offer the operator a new build once one has finished installing (#538, corrected). + * + * **Why there is a prompt at all.** The worker used to call `skipWaiting()` the moment its cache + * was full, which takes over the page currently on screen -- a document rendered by the OLD + * build's JavaScript, whose later requests are then answered from the NEW build's cache. `sw.js` + * carries the full argument. The fix is that the new worker waits, and this is what tells the + * operator it is waiting. + * + * **Three states, because a worker can already be waiting when this runs.** A registration whose + * `waiting` is populated has an update that installed during a previous visit; `updatefound` plus + * `installed` catches one that arrives while the page is open. Both funnel to the same offer. + * + * `navigator.serviceWorker.controller` is the test for "is this an UPDATE or a first install". + * Without it, the very first visit -- where a worker installs and waits with nothing to replace + * -- would offer the operator a reload for a build they are already running. + * + * **It gates the OFFER, not the watching, and the first spelling got that wrong.** Returning + * early when there is no controller meant that on a first visit -- the one load where there + * reliably is none -- the `updatefound` listener was never attached at all, so an update arriving + * later in that same session went unnoticed. The window was narrow (an update needs a server + * restart, which invalidates the session token, which the banner reports) but the code was saying + * something it did not mean. Found by driving the flow in a browser rather than by reading it. + * + * @param {ServiceWorkerRegistration} registration + */ +function watchForUpdate(registration) { + if (registration.waiting && navigator.serviceWorker.controller) { + offerUpdate(registration.waiting); + } + registration.addEventListener("updatefound", () => { + const installing = registration.installing; + if (!installing) return; + installing.addEventListener("statechange", () => { + if (installing.state === "installed" && navigator.serviceWorker.controller) { + offerUpdate(installing); + } + }); + }); +} + +/** + * The offer itself: one line in the footer, and a button that takes it. + * + * In the FOOTER rather than over the view, and not in the engine banner. The banner is the page's + * one `aria-live` region and it answers "is keel running"; an upgrade notice is neither urgent nor + * about the engine's state, and putting it there would interrupt a screen reader mid-table to say + * something that can wait indefinitely. It sits beside the build line it is about to change. + * + * The reload is driven by `controllerchange` rather than fired straight after the message: the + * new worker has to actually take over before a reload gets the new build, and reloading first + * would fetch the old one again and leave the offer standing. + * + * @param {ServiceWorker} waiting + */ +function offerUpdate(waiting) { + if (updateNode.childElementCount !== 0) return; + + const button = document.createElement("button"); + button.className = "update"; + button.setAttribute("type", "button"); + button.append(document.createTextNode("A newer build is ready — reload")); + button.addEventListener("click", () => { + button.disabled = true; + navigator.serviceWorker.addEventListener("controllerchange", () => { + window.location.reload(); + }); + waiting.postMessage("SKIP_WAITING"); + }); + updateNode.replaceChildren(button); +} + /** * The build, read once, and the three things that depend on it. * diff --git a/keel/web/static/sw.js b/keel/web/static/sw.js index 22a5762..c001a1b 100644 --- a/keel/web/static/sw.js +++ b/keel/web/static/sw.js @@ -88,13 +88,26 @@ const PRECACHE = [ ]; /** - * Install: fill this build's cache, then take over immediately. + * Install: fill this build's cache, and then WAIT. * - * `skipWaiting` rather than waiting for every tab to close, and the version key is what makes - * that safe: the new worker serves the new build's cache, the old one is deleted in `activate`, - * and a client that reloads gets a consistent set. Waiting would leave an upgraded engine being - * read by the previous shell for as long as one tab stayed open -- exactly the failure the - * version key exists to prevent. + * ── THIS CALLED `skipWaiting()` UNCONDITIONALLY, AND THAT WAS WRONG ───────────────────────── + * The argument was that the version-keyed cache made it safe: the new worker serves the new + * build's cache, the old one is deleted in `activate`, so nothing stale survives. That reasoning + * is sound about CACHES and silent about the thing that actually breaks -- the page already on + * screen. `skipWaiting()` plus `clients.claim()` takes over a document that was parsed and + * rendered by the OLD build's JavaScript, and every request it makes afterwards is answered from + * the NEW build's cache. One page, two builds, no indication. + * + * keel had a second line of defence that made this hard to notice: a new build means the process + * restarted, which means a new session token, which means every `/api/*` call from the old page + * is a 403 the banner reports. So the window was narrow and loud rather than wide and quiet. It + * was still a window, and "another layer happens to cover it" is not a reason to keep a hazard + * that costs one message to remove. + * + * So the new worker installs its cache and stays in `waiting`. `main.js` notices, tells the + * operator a new build is ready, and only a click sends `SKIP_WAITING` -- at which point the page + * reloads into the build it just accepted. Nothing is ever half-upgraded, and the operator finds + * out an upgrade happened, which they could not before. * * `cache: "reload"` on every request: the server sends `Cache-Control: no-store` on static * assets (`server._STATIC_BASE_HEADERS`), but the HTTP cache is not the only thing between here @@ -105,11 +118,21 @@ self.addEventListener("install", (event) => { event.waitUntil( caches .open(CACHE) - .then((cache) => cache.addAll(PRECACHE.map((path) => new Request(path, { cache: "reload" })))) - .then(() => self.skipWaiting()), + .then((cache) => cache.addAll(PRECACHE.map((path) => new Request(path, { cache: "reload" })))), ); }); +/** + * The one message this worker answers: "the operator accepted the update, take over". + * + * A message rather than a timer or a heuristic, because the decision is not the worker's to make. + * `skipWaiting()` here is safe in the way it was not in `install`: the page that sent it is about + * to reload, so there is no document left running the old build for the new one to serve. + */ +self.addEventListener("message", (event) => { + if (event.data === "SKIP_WAITING") void self.skipWaiting(); +}); + /** * Activate: delete every other keel cache, then claim open clients. * diff --git a/pyproject.toml b/pyproject.toml index 3b2d536..c05e074 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -147,6 +147,15 @@ warn_redundant_casts = true # fails the build when this list and mypy's own `--strict` bundle drift apart. [[tool.mypy.overrides]] module = [ + # `keel.web.*` is not a new package, and it is here for the same reason the new packages are: + # it was ALREADY clean under these flags. Measured rather than assumed -- `mypy --strict + # keel/web` reported two errors, both in one function, and both were the same defect: a + # three-argument `type()` call returning `Any` into a function annotated `-> KeelServer`, + # under a `type: ignore[return-value]` that did not name the error mypy was actually + # reporting. Fixing that left zero, so gating it costs nothing today and is what stops the + # next edit from costing something. It is also the layer where it matters most: everything + # under `keel/web/` is reachable from a socket. + "keel.web.*", "keel_broker_api.*", "keel_broker_alpaca.*", "keel_broker_coinbase.*", diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 834698f..8232601 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -160,11 +160,22 @@ def _mypy_overrides() -> list[dict]: def _strict_modules() -> list[str]: - """Import packages the root `[tool.mypy]` config checks in strict mode. + """DISTRIBUTION ROOTS the root `[tool.mypy]` config checks in strict mode. Read from the config rather than listed here, so that tightening a package (giving it the strict flag block) automatically brings it under the marker rule below instead of requiring someone to remember this file. + + **Dotted patterns are skipped, and the distinction is the whole point of the rule below.** + PEP 561's marker is a promise to whoever INSTALLS a distribution: it tells their type checker + that this package's annotations are real. A submodule of a distribution -- `keel.web.*`, made + strict at the same time as this filter -- has no installer of its own to make that promise to, + and the place a marker would go (`keel/py.typed`) covers `keel.*`, which is still + `ignore_errors`. Shipping it would be exactly what the docstring below warns against: "a + marker on unchecked code promises a guarantee nothing verifies." + + So the rule keeps its teeth where it was aimed -- every distribution under `packages/` -- and + stops misfiring on internal modules that are strict for their own sake. """ modules: list[str] = [] for override in _mypy_overrides(): @@ -172,10 +183,25 @@ def _strict_modules() -> list[str]: continue entry = override["module"] for pattern in [entry] if isinstance(entry, str) else entry: - modules.append(pattern.removesuffix(".*")) + root = pattern.removesuffix(".*") + if "." in root: + continue + modules.append(root) return sorted(modules) +def test_the_strict_module_reader_still_sees_the_distributions() -> None: + """Mutation guard on the filter above: skipping dotted patterns must not skip everything. + + A filter that quietly emptied this list would make `test_strictly_typed_packages_ship_a_ + py_typed_marker` parametrise over nothing and pass by having no cases -- the exact shape of + vacuous green this file exists to prevent elsewhere.""" + roots = _strict_modules() + assert "keel_broker_api" in roots, roots + assert len(roots) >= 6, roots + assert not [r for r in roots if "." in r], roots + + @pytest.mark.parametrize("module", _strict_modules()) def test_strictly_typed_packages_ship_a_py_typed_marker(module): """A package mypy checks strictly must declare that fact to whoever installs it (PEP 561). diff --git a/tests/web/test_pwa.py b/tests/web/test_pwa.py index 8d031f0..b2ab288 100644 --- a/tests/web/test_pwa.py +++ b/tests/web/test_pwa.py @@ -10,6 +10,8 @@ running `keel serve` driven by a real Chromium; the results are in the PR body: * that the worker installs, activates and reaches `controlling` state; + * that a second build leaves the new worker WAITING and the offer appears in the footer; + * that taking the offer swaps the controller and reloads into the new build; * that the precache is populated and holds exactly `PRECACHE`; * that the shell still paints after `keel serve` is killed, with no figures on it; * that a changed build string swaps the cache and deletes the old one; @@ -286,6 +288,93 @@ def test_the_worker_never_writes_to_a_cache_outside_install() -> None: assert code.count(".addAll(") == 1, "the precache should be the only thing written" +def test_the_worker_never_takes_over_a_page_it_did_not_render() -> None: + """**This is a correction: the worker called `skipWaiting()` unconditionally at install.** + + The original argument was that the version-keyed cache made it safe -- new worker, new cache, + old one deleted. That is true about CACHES and silent about the page already on screen: + `skipWaiting()` with `clients.claim()` takes over a document parsed and rendered by the OLD + build, and answers its later requests from the NEW build's cache. One page, two builds. + + keel had a second line of defence that made it hard to notice -- a new build means a restarted + process, a new session token, and a 403 the banner reports -- but a hazard covered by an + unrelated layer is still a hazard. + + So: `skipWaiting` appears only inside the message handler, never in `install`. + """ + source = _sw_source() + code = "\n".join( + line for line in source.splitlines() if not line.lstrip().startswith(("*", "/*", "//")) + ) + # Sliced to the install handler ALONE, not "install up to activate": the message handler now + # sits between them and it is the one place `skipWaiting` legitimately appears, so the wider + # slice would find it there and report the opposite of the truth. + start = code.index('addEventListener("install"') + install = code[start : code.index("self.addEventListener(", start + 1)] + assert "skipWaiting" not in install, ( + "the worker takes over at install again; see this test's docstring before restoring it" + ) + assert 'if (event.data === "SKIP_WAITING") void self.skipWaiting();' in code, ( + "the consent path is gone -- an update could never be applied" + ) + + +def test_the_client_offers_the_update_rather_than_applying_it() -> None: + """The other half: a waiting worker is useless if nothing tells the operator it is waiting. + + Both arrival paths are pinned. `registration.waiting` catches an update that installed during + a previous visit; `updatefound` + `installed` catches one that arrives while the page is open. + A build that handled only the second would leave an update stranded forever for anyone who + closed the tab at the wrong moment.""" + main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8") + assert "registration.waiting" in main, "an update installed on a previous visit is stranded" + assert '"updatefound"' in main, "an update arriving while the page is open is missed" + assert 'postMessage("SKIP_WAITING")' in main + + +def test_the_first_install_is_not_announced_as_an_update() -> None: + """`navigator.serviceWorker.controller` is the test for "update" versus "first install". + + Without it the very first visit -- a worker installing with nothing to replace -- would offer + the operator a reload for the build they are already running, which teaches them the notice + means nothing. + + **And it gates the OFFER, never the watching.** An earlier spelling returned early from + `watchForUpdate` when there was no controller, which meant that on a first visit -- the one + load where there reliably is none -- the `updatefound` listener was never attached. This + asserts the listener is registered unconditionally.""" + main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8") + watcher = main[main.index("function watchForUpdate(") : main.index("function offerUpdate(")] + assert "navigator.serviceWorker.controller" in watcher, ( + "a first install is announced as an update" + ) + assert "return;" not in watcher.split('addEventListener("updatefound"')[0], ( + "watchForUpdate returns before attaching its listener; a first visit would never watch" + ) + + +def test_the_reload_waits_for_the_new_worker_to_take_over() -> None: + """Reloading straight after `postMessage` races: the new worker may not be controlling yet, so + the reload fetches the old build again and leaves the offer standing.""" + main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8") + controller_change = main.index('"controllerchange"') + post = main.index('postMessage("SKIP_WAITING")') + assert controller_change < post, ( + "the reload listener is registered after the message is sent -- the takeover can land first" + ) + + +def test_the_update_offer_is_not_in_the_live_region() -> None: + """The engine banner is the page's one `aria-live` region and it answers "is keel running". + + An upgrade notice is neither urgent nor about the engine, and putting it there would interrupt + a screen reader mid-table to say something that can wait indefinitely.""" + html = _INDEX.read_text(encoding="utf-8") + banner = html[html.index('id="engine"') : html.index('id="content"')] + assert 'id="update"' not in banner + assert 'id="update"' in html, "the offer has nowhere to go" + + def test_the_cache_name_is_keyed_to_the_build() -> None: """A `CacheFirst` shell with a fixed cache name survives an engine upgrade and answers the new API with the old contract -- everything renders, only the fields are wrong.""" From e4ca22cd35aaeb9f5e78538743ff445753ed5f84 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 26 Aug 2026 05:22:05 -0400 Subject: [PATCH 2/2] refactor(web): the write boundary is a module, after the reference client's own shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `keel/web/static/js/actions.js`: this client's `data-manager.js`. ── WHOSE SHAPE, AND WHY IT IS THE RIGHT ONE TO COPY ─────────────────────────── The design spec's reference implementation is youperiod.app, and `js/external/`'s README already names it. Its client is five modules with one job each: `main.js` attaches event listeners, `data-manager.js` owns the storage boundary behind a `get`/`set` pair, `utils.js` holds the helpers. keel's client was built to that shape -- `api.js` the single `fetch` wrapper, `render.js` the only view builder, `format.js` the helpers, `js/external/` deliberately EMPTY where the reference admits three vetted libraries. Then #540 gave the browser a write surface and put all of it in `main.js`: the session write token, the submit handler, the memory of what each action reported, and the choice of which server field to show. That is the module the reference keeps emptiest, and it had quietly grown a second job -- the read path had a boundary module and the write path did not. ── WHAT THE BOUNDARY HIDES ──────────────────────────────────────────────────── A caller needs an action's key and the operator's answers. It no longer needs to know that a write carries a session-scoped HMAC token beside `X-Keel-Client`, that the token arrives on `/api/setup` and nowhere else, that the result distinguishes "done" from "already done" through a `changed` flag rather than an error, or that the sentence comes from `data.message.display` on success and `error.detail` on refusal. `main.js`'s listener is now what that file is for: read the form, show the answer, repaint. ── THE TOKEN HAS ONE HOME, AND THE TEST SAYS SO IN THOSE TERMS ──────────────── Pinned as "where is it HELD", not "where does the word appear". `api.js` names it because it is the parameter it puts in a header -- that module is the only one allowed to open a connection -- but it takes it as an argument and keeps nothing. `actions.js` is the one module with a variable holding it between calls. A credential that acquires a second home is one the next thing added there will read. Two assertions had to be rewritten before they were true rather than merely red: the header name is a string literal, so `_code_only` (which blanks literals) cannot see it, and `actions.js` NAMES the header in a comment listing what it hides -- so the honest check is behavioural, that the boundary assembles no request at all. ── A BUG THE MOVE REMOVED RATHER THAN FIXED ─────────────────────────────────── The outcome memory was a `Map`, and a DOM node can be in one place in a document at a time -- so every restore had to `cloneNode` or the second rebuild would move the only copy out of the map's reach. It is a `Map` now. The bug that invited is not fixed; it can no longer be written. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- keel/web/static/js/actions.js | 149 ++++++++++++++++++++++++++++++++ keel/web/static/js/main.js | 91 +++++-------------- keel/web/static/sw.js | 1 + tests/web/test_client_assets.py | 72 ++++++++++++++- 4 files changed, 242 insertions(+), 71 deletions(-) create mode 100644 keel/web/static/js/actions.js diff --git a/keel/web/static/js/actions.js b/keel/web/static/js/actions.js new file mode 100644 index 0000000..424ef2a --- /dev/null +++ b/keel/web/static/js/actions.js @@ -0,0 +1,149 @@ +// @ts-check +/** + * The write boundary: this session's write token, the actions performed through it, and what each + * one reported. + * + * ── WHY THIS MODULE EXISTS, AND WHOSE SHAPE IT IS ─────────────────────────────────────────── + * The design spec's reference implementation is + * [youperiod.app](https://github.com/getify/youperiod.app), and its client is five modules with + * one job each: `main.js` attaches event listeners, `data-manager.js` owns the storage boundary + * behind a `get`/`set` pair, `utils.js` holds shared helpers. keel's client was built to that + * shape -- `api.js` is the single `fetch` wrapper, `render.js` the only view builder, + * `format.js` the helpers -- and then #540 gave the browser a WRITE surface and put all of it in + * `main.js`: the token, the submit handler, the outcome memory, and the sentence-picking. That + * is the module the reference keeps emptiest, and it had grown a second job. + * + * So this is keel's `data-manager.js`. It owns everything about performing an action except the + * two things that are not its business: the `fetch` (that is `api.js`, still the only module + * that opens a connection) and the DOM (that is `render.js` and the listener in `main.js`). + * + * ── WHAT IT HIDES ─────────────────────────────────────────────────────────────────────────── + * A caller needs to know an action's key and the operator's answers. It does not need to know + * that a write carries a session-scoped HMAC token in `X-Keel-CSRF` beside `X-Keel-Client`, that + * the token arrives on `/api/setup` and nowhere else, that the result document distinguishes + * "done" from "already done" through a `changed` flag rather than an error, or that the sentence + * to show comes from `data.message.display` on success and `error.detail` on refusal. All of + * that is here, and none of it is in the listener. + * + * ── THE MEMORY IS SESSION STATE, AND IT IS THE ONLY STATE THIS CLIENT KEEPS ───────────────── + * Every other thing on screen is a server document re-read on a timer. "What did I just do" is + * the one fact no document can answer: `paint` rebuilds the view from `/api/*` on every poll, + * every tick and after every action, so an outcome written into a card is gone at the next + * rebuild. It is deliberately NOT persisted and dies with the page -- a message about an action + * from an hour ago, presented as current, is the same failure #538 refuses to make with a cached + * balance, in a much milder register. + */ + +import { runAction } from "./api.js"; + +/** + * @typedef {import("./api.js").Reading} Reading + */ + +/** + * This session's write token, from `/api/setup`'s `data.csrf`. + * + * `""` until a setup document has been read, and that is correct rather than a gap: the only + * thing that can submit an action is a form this client drew, and it draws them only on the view + * that just supplied the token. An empty token reaches the server and is refused, which is the + * right answer for a submission that could not have come from a rendered action card. + * + * @type {string} + */ +let token = ""; + +/** + * What each action last reported, by action key. + * @type {Map} + */ +const results = new Map(); + +/** + * Take the write token off a `/api/setup` document. + * + * Called from the view that reads it, once per read. `payload.setup_payload` sends it as a bare + * string rather than a `Field` precisely because it is a credential the client SENDS and never + * displays; treating it as one here means it never reaches `render.js`. + * + * @param {any} data `/api/setup`'s `data`, or `null`. + */ +export function remember(data) { + token = data && typeof data.csrf === "string" ? data.csrf : ""; +} + +/** + * Whether a write can be attempted at all. + * + * Exported so a caller can ask rather than infer from a failure. Nothing uses it to HIDE a + * button -- "a client that hides a button is not a gate" is the spec's own sentence, and the + * server refuses what is not in `keel.commands.setup.ACTIONS` regardless of what this returns. + * + * @returns {boolean} + */ +export function available() { + return token !== ""; +} + +/** + * Perform one declared action and record what it reported. + * + * @param {string} key an action key from `/api/setup`'s `actions`. + * @param {Record} values the declared fields, by name. + * @returns {Promise} the sentence to show. Never rejects. + */ +export async function perform(key, values) { + const reading = await runAction(key, values, token); + const outcome = describe(reading); + results.set(key, outcome); + return outcome; +} + +/** + * What an action last reported, or `""` if it has not been run in this session. + * + * @param {string} key + * @returns {string} + */ +export function outcomeFor(key) { + return results.get(key) ?? ""; +} + +/** + * Every recorded outcome, for a view being rebuilt. + * + * A copy, not the live map: a caller iterating this while an action completes would otherwise be + * iterating a collection that changed underneath it. + * + * @returns {Map} + */ +export function recorded() { + return new Map(results); +} + +/** + * The sentence for a finished action -- the SERVER's own words in every branch. + * + * `data.message.display` when it ran, `error.detail` when it did not. This function chooses which + * field to read and never composes a sentence of its own, which is the same rule `render.js` + * follows for every value on screen, applied to the one outcome that arrives outside a view. + * + * **`changed` is appended because it is not a success flag.** Every action is idempotent, so a + * repeated submission succeeds and reports `already done -- nothing to change`, which is a true + * statement about the deployment rather than a soft failure. Dropping it would make a re-run look + * identical to a first run, and the difference is the whole reason `keel.commands.setup` carries + * the field. + * + * @param {Reading} reading + * @returns {string} + */ +function describe(reading) { + const data = reading.data; + if (data && data.message && typeof data.message.display === "string") { + const changed = data.changed && data.changed.display ? data.changed.display : ""; + return changed ? data.message.display.concat(" — ", changed) : data.message.display; + } + const error = reading.error; + const detail = error && error.detail ? error.detail : ""; + const title = error && error.title ? error.title : reading.engine.display; + return detail ? title.concat(" — ", detail) : title; +} diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index be576e8..caecb4d 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -32,7 +32,8 @@ * it is worth paying. */ -import { read, runAction } from "./api.js"; +import { perform, recorded, remember } from "./actions.js"; +import { read } from "./api.js"; import { indexUrl, rememberVersion } from "./docs.js"; import { available, subscribe } from "./live.js"; import { @@ -132,18 +133,6 @@ const docsNode = /** @type {HTMLAnchorElement} */ (must("docs-link")); /** Where the "a newer build is ready" offer goes (#538). Empty until there is one. */ const updateNode = must("update"); -/** - * This session's write token (#540), read off `/api/setup` whenever the setup view mounts. - * - * `""` until then, and that is correct rather than a gap: the only thing that can submit a form - * is a form this client drew, and it draws them only on the view that just supplied the token. - * An empty token would be refused by the server, which is the right outcome for a submission - * that could not have come from a rendered action card. - * - * @type {string} - */ -let csrf = ""; - /** * An element that `index.html` guarantees. Throwing beats rendering half a page: the two files * ship together in the same directory, so a missing id is a bug in this commit, not a @@ -369,7 +358,7 @@ function mount(route, readings) { // The write token for this session, kept for the submit handler below. It is read off the // document that was just fetched rather than stored anywhere: it dies with the process that // minted it, and a stale one produces a 403 the view shows rather than a silent no-op. - csrf = (data && typeof data.csrf === "string") ? data.csrf : ""; + remember(data); return setupView(data); } if (route.name === "activity") { @@ -569,55 +558,41 @@ contentNode.addEventListener("submit", (event) => { setBusy(form, true); showOutcome(key, "Running…"); - void runAction(key, values, csrf).then((reading) => { - // Recorded BEFORE the repaint, and re-applied after it. Writing straight into the node would - // put the sentence into an element `paint` is about to replace -- which is exactly what the - // first version of this did, and the message was never visible for a single frame. Found by - // clicking the button in a browser; nothing in the suite could have seen it. - outcomes.set(key, outcomeText(reading)); - showOutcome(key, outcomes.get(key)); + // The token, the headers, the idempotency semantics and the sentence all live in `actions.js`. + // What is left here is what is genuinely this file's job: read the form, show the answer, + // repaint. See that module's note on whose shape it is. + void perform(key, values).then((outcome) => { + showOutcome(key, outcome); // Re-read the whole view: a step that ran has changed the deployment this page describes, and - // patching one card would leave the checklist above it saying the opposite. + // patching one card would leave the checklist above it saying the opposite. The outcome + // survives that rebuild because `rebuildInto` re-applies every recorded one. void paint(routeFor(window.location.pathname), true, false); }); }); -/** - * What each action last reported, by action key. - * - * Kept here rather than in the view because the view does not survive: `paint` rebuilds it from - * the server's documents on every poll, every tick and after every action, and an outcome written - * into a card is gone at the next rebuild. This is the small amount of state that belongs to the - * SESSION rather than to the deployment -- "what did I just do", which no document can answer. - * - * Not persisted, and cleared by a reload: it describes this sitting at this machine, and a - * message about an action from an hour ago presented as current is the same failure #538 refuses - * to make with a cached balance, in a much milder register. - * - * @type {Map} - */ -const outcomes = new Map(); - /** * Put an action's outcome into its card, if that card is on screen. * * @param {string} key - * @param {Node | string} text + * @param {string} text */ function showOutcome(key, text) { const form = contentNode.querySelector('form[data-action="'.concat(key, '"]')); const node = form ? form.querySelector(".action-outcome") : null; if (!node) return; - node.replaceChildren(typeof text === "string" ? document.createTextNode(text) : text); + node.replaceChildren(document.createTextNode(text)); } -/** Re-apply every remembered outcome after a rebuild has replaced the cards. */ +/** + * Re-apply every remembered outcome after a rebuild has replaced the cards. + * + * Strings rather than nodes, since the memory moved into `actions.js`. That is not only tidier: + * a `Node` can be in one place in a document at a time, so a map of nodes had to be cloned on + * every restore or the second rebuild would move the only copy out of the map's reach. A string + * has no such property, and the bug it invites cannot be written. + */ function restoreOutcomes() { - for (const [key, text] of outcomes) { - // `cloneNode`: a `Node` can only be in one place in a document, and this map outlives any - // number of rebuilds. Inserting the same node twice would move it out of the map's reach. - showOutcome(key, text.cloneNode(true)); - } + for (const [key, text] of recorded()) showOutcome(key, text); } /** @@ -632,30 +607,6 @@ function setBusy(form, busy) { } } -/** - * What to show for a finished action. - * - * The SERVER's own words in every branch: `data.message.display` when it ran, `error.detail` when - * it did not. This function chooses which field to read and never composes a sentence -- the same - * rule `render.js` follows, applied to the one outcome that arrives outside a view. - * - * @param {import("./api.js").Reading} reading - * @returns {Node} - */ -function outcomeText(reading) { - const data = reading.data; - if (data && data.message && typeof data.message.display === "string") { - const changed = data.changed && data.changed.display ? data.changed.display : ""; - return document.createTextNode( - changed ? data.message.display.concat(" — ", changed) : data.message.display, - ); - } - const error = reading.error; - const detail = error && error.detail ? error.detail : ""; - const title = error && error.title ? error.title : reading.engine.display; - return document.createTextNode(detail ? title.concat(" — ", detail) : title); -} - document.addEventListener("visibilitychange", () => { if (document.visibilityState === "visible") void paint(current, false); }); diff --git a/keel/web/static/sw.js b/keel/web/static/sw.js index c001a1b..97063d0 100644 --- a/keel/web/static/sw.js +++ b/keel/web/static/sw.js @@ -74,6 +74,7 @@ const PRECACHE = [ SHELL, `${BASE}manifest.webmanifest`, `${BASE}css/keel.css`, + `${BASE}js/actions.js`, `${BASE}js/api.js`, `${BASE}js/chart.js`, `${BASE}js/docs.js`, diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py index 7c90e8d..ce9da97 100644 --- a/tests/web/test_client_assets.py +++ b/tests/web/test_client_assets.py @@ -59,12 +59,27 @@ def _P(rest: str) -> str: #: revision of this line predicted ("a fifth module is a design decision (#537 adds `chart`, #: `live`, `docs`, `sw`) and should arrive with the list updated, not silently"). #: +#: `actions.js` is the write boundary, split out of `main.js` after the reference implementation's +#: own shape: youperiod.app keeps `main.js` to "DOM manipulation and event listener attachment" +#: and puts the storage boundary in `data-manager.js` behind a small interface. #540 gave keel a +#: write surface and left all of it -- token, submit handler, outcome memory, sentence-picking -- +#: in `main.js`, which is the module that reference keeps emptiest. +#: #: **`sw` arrived at #538 and is deliberately NOT in this list**, because it is not under `js/`: #: a service worker's scope is its own directory, so `js/sw.js` would be scoped to `/static/js/` #: and could not answer a navigation to `/static/insights`. It sits at the static root instead, #: and `tests/web/test_pwa.py::test_the_worker_is_served_from_the_scope_it_must_control` asserts #: that placement rather than leaving it to whoever next reads the spec's file list. -_MODULES = ("main.js", "api.js", "render.js", "chart.js", "live.js", "format.js", "docs.js") +_MODULES = ( + "main.js", + "api.js", + "actions.js", + "render.js", + "chart.js", + "live.js", + "format.js", + "docs.js", +) #: The modules held to "no arithmetic, no judgement, no derived display string", and the ONLY two. #: @@ -394,6 +409,61 @@ def test_no_client_module_can_write_markup() -> None: assert sink not in _code_only(source), f"{name} can write markup via {sink}" +def test_the_write_token_lives_only_in_the_write_boundary() -> None: + """**The module split this file's `_MODULES` note describes, asserted rather than intended.** + + The design spec's reference implementation keeps `main.js` to "DOM manipulation and event + listener attachment" and puts the storage boundary in a module of its own behind a small + interface. #540 gave keel a write surface and left every part of it in `main.js`: the session + write token, the submit handler, the memory of what each action reported, and the choice of + which server field to show. `actions.js` owns all of that now. + + The token is the sharpest part to pin, and the rule is about where it is HELD rather than + where the word appears. `api.js` names it because it is the parameter it puts in a header -- + that module is the only one allowed to open a connection, so it is the only one that can send + it -- but it takes it as an argument and keeps nothing. `actions.js` is the one module with a + variable holding it between calls. + + Stated that way rather than "the word appears once", because a credential that acquires a + second home is a credential the next thing added there will read, and that is what this + catches. A parameter passed straight into a header is not a home. + """ + # Asserted as BEHAVIOUR, not as prose. `actions.js` names `X-Keel-CSRF` in a comment listing + # what it hides from its callers, and a raw-source search would read that explanation as a + # violation of the thing it explains -- the same trap `_markup_only` exists for on + # `index.html`. What actually matters is that the boundary builds no headers at all: the + # transport is `api.js`'s job and nothing here should be assembling a request. + boundary = _code_only(_source("actions.js")) + assert "headers" not in boundary.lower(), ( + "the write boundary assembles a request; that belongs to api.js, which sends it" + ) + assert "let token" in boundary, "the write boundary holds no token" + + assert "X-Keel-CSRF" in _source("api.js"), "nothing sends the token" + transport = _code_only(_source("api.js")) + assert "let csrf" not in transport and "const csrf" not in transport, ( + "api.js stores the write token; it takes one as an argument and keeps nothing" + ) + + for name in _MODULES: + if name in ("actions.js", "api.js"): + continue + assert "csrf" not in _code_only(_source(name)).lower(), ( + f"{name} names the write token; it is held by actions.js and sent by api.js" + ) + + +def test_the_write_boundary_opens_no_connection_and_builds_no_dom() -> None: + """It sits between the two modules that do, and touches neither's job. + + `api.js` is still the only module that calls `fetch` (pinned below), and the DOM is + `render.js`'s and the listener's. A boundary module that reached into either would be a third + place to look for behaviour that already has two homes.""" + code = _code_only(_source("actions.js")) + for forbidden in ("fetch(", "document.", "querySelector", "createElement", "innerHTML"): + assert forbidden not in code, f"actions.js reaches past its boundary via {forbidden}" + + def test_fetch_appears_in_exactly_one_client_module() -> None: """`api` is the single `fetch` wrapper.