From 95dec1bbf352b64d4d1a49e78ca596c2c386c3c8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 22:54:33 +0100 Subject: [PATCH 1/3] fix(page-perf): align perf docs with code; simplify snapshot loader; rename audit test Post-merge review of #825 (maintainability pass). Claude-Session: https://claude.ai/code/session_01FXHA3ZJ5EgoJN4zXTAhBR2 --- CLAUDE.md | 2 +- docs/frontend/performance.md | 6 +++--- ...ll_page_audit.py => test_cli_skill_page_audit.py} | 0 .../simple_module_hosting/_middleware_pipeline.py | 5 +++-- .../background_tasks/endpoints/views.py | 12 ++++-------- .../users/tests/test_users_admin_list_query_count.py | 3 ++- skills/simple-module-page-perf/SKILL.md | 4 ++-- .../scripts/page_audit/__main__.py | 2 +- .../scripts/page_audit/audit_crawl.py | 2 +- .../scripts/page_audit/audit_report.py | 9 +++------ 10 files changed, 20 insertions(+), 25 deletions(-) rename framework/cli/tests/{test_skill_page_audit.py => test_cli_skill_page_audit.py} (100%) diff --git a/CLAUDE.md b/CLAUDE.md index e619b2d84..3e4f5a5a3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -216,7 +216,7 @@ the host), then async `on_startup` / `on_shutdown` (reverse order). Non-hook ove **Middleware** (Starlette `add_middleware` is LIFO). Execution order per request: `(ForwardedHost if trusted proxy + allowlist) → (ProxyHeaders if -SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → CORS → BodyLimit → GZip → SecurityHeaders → Session → InertiaMiddleware → → Tenant +SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → (ServerTiming, development only) → CORS → BodyLimit → GZip → SecurityHeaders → Session → InertiaMiddleware → → Tenant (opt-in) → Locale → InertiaLayoutData → RateLimit → Setup → Maintenance → CommitBeforeResponse → app`. Rules that follow from it: diff --git a/docs/frontend/performance.md b/docs/frontend/performance.md index e51ebe910..8bbd8f225 100644 --- a/docs/frontend/performance.md +++ b/docs/frontend/performance.md @@ -84,7 +84,7 @@ uv run --with playwright python .claude/skills/simple-module-page-perf/scripts/p Run it twice. Against a **development** server you get server time and SQL counts, but dev-server bundle numbers are meaningless. Against a **production -build** (`make build`, then a non-development `SM_ENVIRONMENT`) you get LCP and +build** (`make build`, then an `SM_ENVIRONMENT` other than `development` or `testing`) you get LCP and JS bytes, but no `Server-Timing`. The skill's `SKILL.md` has both recipes and explains each finding. @@ -185,7 +185,7 @@ return await inertia.render( { # The routes and the start command render at once; the fleet fills # in behind a skeleton when the broker answers. - "snapshot": defer(_snapshot_json(request), group="workers"), + "snapshot": defer(lambda: _snapshot_json(request), group="workers"), "queues": queues, "routes": routes, }, @@ -349,7 +349,7 @@ reports horizontal overflow. ### Small things that read as quality One `

` per page, an accessible name on every icon-only button, a document -title per page (``). The audit checks all three. They cost +title per page (``). The audit checks the first two. They cost nothing and make the page faster to *understand*, which is what the user perceives. diff --git a/framework/cli/tests/test_skill_page_audit.py b/framework/cli/tests/test_cli_skill_page_audit.py similarity index 100% rename from framework/cli/tests/test_skill_page_audit.py rename to framework/cli/tests/test_cli_skill_page_audit.py diff --git a/framework/hosting/simple_module_hosting/_middleware_pipeline.py b/framework/hosting/simple_module_hosting/_middleware_pipeline.py index 2b780e8f0..e39ac436e 100644 --- a/framework/hosting/simple_module_hosting/_middleware_pipeline.py +++ b/framework/hosting/simple_module_hosting/_middleware_pipeline.py @@ -88,8 +88,9 @@ def install_middleware( Order matters: last added = first executed. Execution order: (ForwardedHost, if trusted_proxy and forwarded_host_allowlist) → - (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging → CORS - → BodyLimit → GZip → Security → Session → InertiaMiddleware → [module] → + (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging → + (ServerTiming, development only) → CORS → BodyLimit → GZip → Security → + Session → InertiaMiddleware → [module] → (Tenant, if multi_tenant) → Locale → InertiaLayoutData → RateLimit → Setup → Maintenance → CommitBeforeResponse. """ diff --git a/modules/background_tasks/background_tasks/endpoints/views.py b/modules/background_tasks/background_tasks/endpoints/views.py index 716988261..dbf8bc9b2 100644 --- a/modules/background_tasks/background_tasks/endpoints/views.py +++ b/modules/background_tasks/background_tasks/endpoints/views.py @@ -53,13 +53,9 @@ async def _worker_presence(request: Request) -> dict[str, object]: } -def _snapshot_json(request: Request): - """A deferred loader for the Workers page's fleet snapshot.""" - - async def load() -> dict[str, object]: - return (await poll_workers(request)).model_dump(mode="json") - - return load +async def _snapshot_json(request: Request) -> dict[str, object]: + """The Workers page's fleet snapshot, loaded as a deferred prop.""" + return (await poll_workers(request)).model_dump(mode="json") @router.get("/", response_model=None) @@ -167,7 +163,7 @@ async def workers( { # Deferred for the same ~1s inspect timeout: the routes and the # start command render at once, the fleet fills in behind a skeleton. - "snapshot": defer(_snapshot_json(request), group=_WORKERS_GROUP), + "snapshot": defer(lambda: _snapshot_json(request), group=_WORKERS_GROUP), "broker_url_redacted": redact_broker_url(settings.broker_url), "queues": queues, "routes": routes, diff --git a/modules/users/tests/test_users_admin_list_query_count.py b/modules/users/tests/test_users_admin_list_query_count.py index 0dada3956..0c86ae64f 100644 --- a/modules/users/tests/test_users_admin_list_query_count.py +++ b/modules/users/tests/test_users_admin_list_query_count.py @@ -14,6 +14,7 @@ from fastapi import FastAPI from fastapi_users.password import PasswordHelper from simple_module_db.query_stats import install_query_stats, track_queries +from simple_module_test import InertiaPage from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from users.models import Role, User, UserRole @@ -49,7 +50,7 @@ async def _statements(app: FastAPI, client: httpx.AsyncClient) -> int: with track_queries() as stats: resp = await client.get("/admin/users/", headers={"X-Inertia": "true"}) assert resp.status_code == 200 - assert resp.json()["component"] == "Users/Users/Index" + InertiaPage.from_response(resp).assert_component("Users/Users/Index") counts.append(stats.count) return min(counts) diff --git a/skills/simple-module-page-perf/SKILL.md b/skills/simple-module-page-perf/SKILL.md index 78a74ec56..e78a7cb33 100644 --- a/skills/simple-module-page-perf/SKILL.md +++ b/skills/simple-module-page-perf/SKILL.md @@ -46,12 +46,12 @@ when the page is not served by Vite. ```bash make build # the client bundle SM_ENVIRONMENT=staging SM_SECRET_KEY=<32+ chars> \ - uv run uvicorn main:app --port 8001 # any non-development environment serves the build + uv run uvicorn main:app --port 8001 # any environment but development/testing serves the build uv run --with playwright python .claude/skills/simple-module-page-perf/scripts/page_audit \ --base-url http://localhost:8001 --email … --password … --json perf-prod.json ``` -Outside development the session cookie is `Secure`. Browsers accept it on +Outside development and testing the session cookie is `Secure`. Browsers accept it on `http://localhost`; over plain HTTP on any other host, set `SM_SESSION_COOKIE_SECURE=false` for the audit run. diff --git a/skills/simple-module-page-perf/scripts/page_audit/__main__.py b/skills/simple-module-page-perf/scripts/page_audit/__main__.py index 94c9f820d..58596b695 100644 --- a/skills/simple-module-page-perf/scripts/page_audit/__main__.py +++ b/skills/simple-module-page-perf/scripts/page_audit/__main__.py @@ -7,7 +7,7 @@ then for each page measures the Inertia JSON response (server time and SQL statements from the development ``Server-Timing`` header, payload size, the largest props, deferred props), a full document load (FCP, LCP, CLS, bytes), a -client-side navigation from a sidebar link (click -> painted -> deferred +client-side navigation by clicking a link (click -> painted -> deferred props painted) and UX checks (DOM size, table rows, stuck loaders, unnamed icon buttons, headings, horizontal overflow on a phone). Prints a Markdown report, optionally writes JSON, and exits 1 when any check fails. diff --git a/skills/simple-module-page-perf/scripts/page_audit/audit_crawl.py b/skills/simple-module-page-perf/scripts/page_audit/audit_crawl.py index fcd9c91fe..7191ae2ab 100644 --- a/skills/simple-module-page-perf/scripts/page_audit/audit_crawl.py +++ b/skills/simple-module-page-perf/scripts/page_audit/audit_crawl.py @@ -1,4 +1,4 @@ -"""One-level crawl: pages linked from the content of the menu pages. +"""Link crawl: pages linked from the content of the pages already known. Menus only reach list pages. Detail and edit pages (``/admin/users/``) are reached from links in those lists, so after the first pass every link in diff --git a/skills/simple-module-page-perf/scripts/page_audit/audit_report.py b/skills/simple-module-page-perf/scripts/page_audit/audit_report.py index ea265bbb5..6743cea76 100644 --- a/skills/simple-module-page-perf/scripts/page_audit/audit_report.py +++ b/skills/simple-module-page-perf/scripts/page_audit/audit_report.py @@ -115,12 +115,9 @@ def evaluate(server: ServerResult, browser: BrowserResult | None) -> list[Findin out.append(Finding(p, "fail", "failed_requests", ", ".join(browser.failed_requests[:3]))) if browser.mobile_overflow_x: out.append(Finding(p, "warn", "overflow", f"{browser.mobile_overflow_x}px at 390px wide")) - for check, key in ( - ("stuck_loaders", "stuck_loaders"), - ("unnamed_controls", "unnamed_controls"), - ): - if ux.get(key): - out.append(Finding(p, "warn", check, f"{ux[key]} element(s)")) + for check in ("stuck_loaders", "unnamed_controls"): + if ux.get(check): + out.append(Finding(p, "warn", check, f"{ux[check]} element(s)")) if ux.get("h1_count", 1) != 1: out.append(Finding(p, "warn", "h1", f"{ux.get('h1_count')}

elements")) return out From 43d0dd8defe003753eb7f67560106ba44de68e9e Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 5 Nov 2026 23:03:46 +0100 Subject: [PATCH 2/3] fix(page-perf): query tracking stops at exit; Workers refresh recovers a missing snapshot; audit fixes (UTF-8 sizes, unexpected redirects fail, CLS session windows, --max-pages) Correctness findings from a post-merge Codex review of #825. Claude-Session: https://claude.ai/code/session_01FXHA3ZJ5EgoJN4zXTAhBR2 --- CHANGELOG.md | 4 + .../cli/tests/test_cli_skill_page_audit.py | 85 ++++++++++++++++++- framework/db/simple_module_db/query_stats.py | 7 +- framework/db/tests/test_db_query_stats.py | 29 +++++++ .../background_tasks/pages/Workers.tsx | 5 +- .../background_tasks_WorkersPage.test.tsx | 26 +++++- .../scripts/page_audit/__main__.py | 7 +- .../scripts/page_audit/audit_browser.py | 25 +++++- .../scripts/page_audit/audit_server.py | 12 ++- 9 files changed, 184 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ae96e8e7..9b432185d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,10 @@ All notable changes to this project are documented in this file. The format is b response; they arrive in the deferred `workers` group. - Background tasks pages linked to `/admin/background-tasks` without its trailing slash, so every "back to executions" click cost a redirect. +- Query tracking stops at block exit even for inherited child tasks; Workers + Refresh can recover a missing deferred snapshot. Page audits count UTF-8 prop + bytes, fail unexpected redirects, measure CLS with session windows and enforce + a positive page cap on initial paths as well as crawled pages (#825). ## [0.0.43] — 2026-11-04 diff --git a/framework/cli/tests/test_cli_skill_page_audit.py b/framework/cli/tests/test_cli_skill_page_audit.py index 4d26a46f1..24b16278d 100644 --- a/framework/cli/tests/test_cli_skill_page_audit.py +++ b/framework/cli/tests/test_cli_skill_page_audit.py @@ -8,9 +8,13 @@ from __future__ import annotations +import importlib.util +import json import re +import subprocess import sys from pathlib import Path +from types import SimpleNamespace import pytest @@ -22,7 +26,7 @@ import audit_crawl # noqa: E402 import audit_report # noqa: E402 import audit_server # noqa: E402 -from audit_browser import BrowserResult # noqa: E402 +from audit_browser import _RESET, INIT_SCRIPT, BrowserResult # noqa: E402 _UUID = "91a1b900-5c0c-45c8-9e0c-894d78f7c425" @@ -168,3 +172,82 @@ def test_deferred_budget_falls_back_to_the_first_load(self) -> None: nav={"nav_ms": 100, "deferred_done_ms": 400, "cls": 0}, ) assert _checks(audit_report.evaluate(_server(), clicked)) == {} + + +class TestRegressions: + def test_prop_sizes_use_utf8(self) -> None: + assert audit_server._size({"title": "Grüße 🌍"}) == len('{"title":"Grüße 🌍"}'.encode()) + + @pytest.mark.parametrize( + "final_path, level", + [("/login", "fail"), ("/other", "fail"), ("/p/", "warn"), ("/p//", "fail")], + ) + def test_redirect_verdict(self, final_path, level) -> None: + response = SimpleNamespace( + status=200, + url="http://t" + final_path, + headers={"x-inertia": "true"}, + body=lambda: b'{"props":{}}', + ) + request = SimpleNamespace(get=lambda *a, **kw: response) + result = audit_server.probe(request, "http://t", "/p", {}, 1) + assert level in _checks(audit_report.evaluate(result, None)).values() + if level == "fail": + assert result.error + + def test_cls_uses_session_windows_and_resets(self) -> None: + script = r""" +const vm = require('node:vm'); +const callbacks = {}; +const context = {window: {}, document: {addEventListener() {}}, + PerformanceObserver: class { + constructor(fn) { this.fn = fn; } + observe({type}) { callbacks[type] = this.fn; } + }}; +vm.createContext(context); +vm.runInContext(INIT, context); +const shift = (time, value, hadRecentInput = false) => callbacks['layout-shift']({ + getEntries: () => [{startTime: time, value, hadRecentInput}] +}); +shift(0, .1); shift(500, .2); shift(900, 9, true); +shift(1600, .2); // gap exceeds one second, despite the ignored user shift +for (let time = 2400; time <= 6400; time += 800) shift(time, .1); +shift(6600, .2); // five-second session limit +const before = {...context.window.__pa}; +vm.runInContext('(' + RESET + ')()', context); +shift(6700, .05); +process.stdout.write(JSON.stringify({before, after: context.window.__pa})); +""" + script = ( + "const INIT = " + + json.dumps(INIT_SCRIPT) + + "; const RESET = " + + json.dumps(_RESET) + + ";" + + script + ) + result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + metrics = json.loads(result.stdout) + assert metrics["before"]["cls"] == pytest.approx(0.8) + assert metrics["before"]["shifts"] == 10 + assert metrics["after"]["cls"] == pytest.approx(0.05) + assert metrics["after"]["shifts"] == 1 + + @pytest.fixture + def cli(self): + spec = importlib.util.spec_from_file_location("page_audit_cli", _SCRIPTS / "__main__.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + @pytest.mark.parametrize("limit", ["0", "-1"]) + def test_max_pages_must_be_positive(self, cli, limit) -> None: + with pytest.raises(SystemExit) as exc: + cli._args(["--max-pages", limit]) + assert exc.value.code == 2 + + @pytest.mark.parametrize("no_discover", [False, True]) + def test_initial_paths_respect_max_pages(self, cli, no_discover) -> None: + argv = ["--max-pages", "2", "--path", "/a", "--path", "/a", "--path", "/b", "--path", "/c"] + args = cli._args(argv + (["--no-discover"] if no_discover else [])) + assert cli._paths(["/d", "/e"], args) == ["/a", "/b"] diff --git a/framework/db/simple_module_db/query_stats.py b/framework/db/simple_module_db/query_stats.py index b10e9fa10..0076e5ae4 100644 --- a/framework/db/simple_module_db/query_stats.py +++ b/framework/db/simple_module_db/query_stats.py @@ -34,19 +34,21 @@ class QueryStats: count: int = 0 seconds: float = 0.0 + active: bool = True _current: ContextVar[QueryStats | None] = ContextVar("sm_query_stats", default=None) def _before(conn, cursor, statement, parameters, context, executemany) -> None: - if _current.get() is not None and context is not None: + stats = _current.get() + if stats is not None and stats.active and context is not None: setattr(context, _START_KEY, time.perf_counter()) def _after(conn, cursor, statement, parameters, context, executemany) -> None: stats = _current.get() - if stats is None: + if stats is None or not stats.active: return stats.count += 1 started = getattr(context, _START_KEY, None) if context is not None else None @@ -79,4 +81,5 @@ def track_queries() -> Generator[QueryStats]: try: yield stats finally: + stats.active = False _current.reset(token) diff --git a/framework/db/tests/test_db_query_stats.py b/framework/db/tests/test_db_query_stats.py index 42783ca1d..6855588fa 100644 --- a/framework/db/tests/test_db_query_stats.py +++ b/framework/db/tests/test_db_query_stats.py @@ -3,7 +3,9 @@ from __future__ import annotations import asyncio +from types import SimpleNamespace +import simple_module_db.query_stats as query_stats from simple_module_db.query_stats import install_query_stats, track_queries from sqlalchemy import text from sqlalchemy.ext.asyncio import create_async_engine @@ -64,3 +66,30 @@ async def test_engine_without_install_counts_nothing() -> None: assert stats.count == 0 finally: await engine.dispose() + + +async def test_child_task_stops_counting_after_block_exits() -> None: + resume = asyncio.Event() + counted = asyncio.Event() + + async def child() -> None: + context = SimpleNamespace() + query_stats._before(None, None, None, None, context, False) + query_stats._after(None, None, None, None, context, False) + counted.set() + await resume.wait() + context = SimpleNamespace() + query_stats._before(None, None, None, None, context, False) + query_stats._after(None, None, None, None, context, False) + assert not vars(context) + + with track_queries() as stats: + task = asyncio.create_task(child()) + await counted.wait() + assert stats.count == 1 + seconds = stats.seconds + assert seconds > 0 + resume.set() + await task + assert stats.count == 1 + assert stats.seconds == seconds diff --git a/modules/background_tasks/background_tasks/pages/Workers.tsx b/modules/background_tasks/background_tasks/pages/Workers.tsx index 5e34dc560..9d061afe9 100644 --- a/modules/background_tasks/background_tasks/pages/Workers.tsx +++ b/modules/background_tasks/background_tasks/pages/Workers.tsx @@ -46,7 +46,6 @@ function Workers({ snapshot: initial, broker_url_redacted: brokerUrl, queues, ro return () => clearInterval(id); }, []); const stale = snapshot !== undefined && isStale(ageOf(snapshot.polled_at, now)); - const polling = loading || snapshot === undefined; async function refresh() { setLoading(true); @@ -85,10 +84,10 @@ function Workers({ snapshot: initial, broker_url_redacted: brokerUrl, queues, ro