diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dc8f1559..b8faf8344 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,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/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 6656ecbaf..5f9672651 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 64% rename from framework/cli/tests/test_skill_page_audit.py rename to framework/cli/tests/test_cli_skill_page_audit.py index 4d26a46f1..e57b7c2d6 100644 --- a/framework/cli/tests/test_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,94 @@ 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_percent_encoding_is_not_a_redirect(self) -> None: + response = SimpleNamespace( + status=200, + url="http://t/caf%C3%A9", + headers={"x-inertia": "true"}, + body=lambda: b'{"props":{}}', + ) + request = SimpleNamespace(get=lambda *a, **kw: response) + result = audit_server.probe(request, "http://t", "/café", {}, 1) + assert not result.error + assert not result.redirected + + 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/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/pages/Workers.tsx b/modules/background_tasks/background_tasks/pages/Workers.tsx index c021ddba6..1d4a443ea 100644 --- a/modules/background_tasks/background_tasks/pages/Workers.tsx +++ b/modules/background_tasks/background_tasks/pages/Workers.tsx @@ -30,13 +30,20 @@ interface Props { routes: TaskRoute[]; } +// The deferred load and a manual Refresh can race; keep whichever reading is +// newer so a slow first poll never overwrites a later one. +function newer(current: WorkerSnapshot | undefined, next: WorkerSnapshot): WorkerSnapshot { + if (!current) return next; + return Date.parse(next.polled_at) >= Date.parse(current.polled_at) ? next : current; +} + function Workers({ snapshot: initial, broker_url_redacted: brokerUrl, queues, routes }: Props) { const { t } = useT(); const [snapshot, setSnapshot] = useState(initial); const [loading, setLoading] = useState(false); // The deferred prop lands after the first render; adopt it when it does. useEffect(() => { - if (initial) setSnapshot(initial); + if (initial) setSnapshot((current) => newer(current, initial)); }, [initial]); // This page is a point-in-time poll that only updates when asked. Left open @@ -51,13 +58,13 @@ 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); const result = await apiResult(api.GET('/api/background_tasks/admin/workers')); if (result.ok) { - setSnapshot(result.data as WorkerSnapshot); + const fresh = result.data as WorkerSnapshot; + setSnapshot((current) => newer(current, fresh)); // Don't wait up to a tick for the staleness to catch up with the new // reading. setNow(Date.now()); @@ -90,10 +97,10 @@ function Workers({ snapshot: initial, broker_url_redacted: brokerUrl, queues, ro