fix(page-perf): post-merge review fixes for #825

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
 

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 → <module middleware> → Tenant
+SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → (ServerTiming, development only) → CORS → BodyLimit → GZip → SecurityHeaders → Session → InertiaMiddleware → <module middleware> → Tenant
 (opt-in) → Locale → InertiaLayoutData → RateLimit → Setup →
 Maintenance → CommitBeforeResponse → app`. Rules that follow from it:
 

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 `<h1>` per page, an accessible name on every icon-only button, a document
-title per page (`<Head title>`). The audit checks all three. They cost
+title per page (`<Head title>`). The audit checks the first two. They cost
 nothing and make the page faster to *understand*, which is what the user
 perceives.
 

framework/cli/tests/test_skill_page_audit.py → 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"]

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)

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

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.
     """

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<WorkerSnapshot | undefined>(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
             <Button
               variant="outline"
               onClick={refresh}
-              disabled={polling}
+              disabled={loading}
               className="max-lg:min-h-11"
             >
-              <RefreshCw className={polling ? 'animate-spin' : undefined} aria-hidden="true" />
+              <RefreshCw className={loading ? 'animate-spin' : undefined} aria-hidden="true" />
               {t(keys.background_tasks.workers.refresh)}
             </Button>
             <Button variant="outline" asChild className="max-lg:min-h-11">

modules/background_tasks/background_tasks/web/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,

modules/background_tasks/tests-js/background_tasks_WorkersPage.test.tsx

@@ -1,6 +1,6 @@
 import '@testing-library/jest-dom/vitest';
 import { configureI18n, keys } from '@simple-module-py/i18n';
-import { render, screen } from '@testing-library/react';
+import { act, fireEvent, render, screen } from '@testing-library/react';
 import { describe, expect, it, vi } from 'vitest';
 
 configureI18n({ locale: 'en', messages: {} });
@@ -12,8 +12,10 @@ vi.mock('@inertiajs/react', () => ({
     <a href={href}>{children}</a>
   ),
 }));
+vi.mock('../background_tasks/pages/api', () => ({ api: { GET: vi.fn() } }));
 vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } }));
 
+import { api } from '../background_tasks/pages/api';
 import Workers from '../background_tasks/pages/Workers';
 import type { WorkerSnapshot } from '../background_tasks/ui/utils/constants';
 
@@ -45,11 +47,43 @@ describe('background_tasks Workers page', () => {
       'true',
     );
     expect(screen.getByText(w.checking)).toBeInTheDocument();
-    expect(screen.getByRole('button', { name: w.refresh })).toBeDisabled();
+    expect(screen.getByRole('button', { name: w.refresh })).toBeEnabled();
 
     rerender(<Workers {...base} snapshot={snapshot} />);
     expect(screen.queryByRole('status', { name: w.checking_label })).toBeNull();
     expect(screen.getByText('celery@host-a')).toBeInTheDocument();
     expect(screen.getByRole('button', { name: w.refresh })).toBeEnabled();
   });
 });
+
+it('recovers a missing deferred snapshot through Refresh and disables it only during the request', async () => {
+  let resolve!: (value: { data: WorkerSnapshot; response: Response }) => void;
+  vi.mocked(api.GET).mockReturnValueOnce(
+    new Promise((done) => {
+      resolve = done;
+    }),
+  );
+  render(<Workers {...base} />);
+  const refresh = screen.getByRole('button', { name: w.refresh });
+  expect(refresh).toBeEnabled();
+  fireEvent.click(refresh);
+  expect(refresh).toBeDisabled();
+  await act(async () => {
+    resolve({ data: snapshot, response: new Response() });
+  });
+  expect(screen.getByText('celery@host-a')).toBeInTheDocument();
+  expect(screen.queryByRole('status', { name: w.checking_label })).toBeNull();
+  expect(refresh).toBeEnabled();
+});
+
+it('keeps the newer reading when a manual Refresh beats the deferred snapshot', async () => {
+  const later = { ...snapshot, polled_at: '2026-01-01T00:10:00Z', workers: [] };
+  const earlier = { ...snapshot, polled_at: '2026-01-01T00:00:00Z' };
+  vi.mocked(api.GET).mockResolvedValueOnce({ data: later, response: new Response() });
+  const { rerender } = render(<Workers {...base} />);
+  await act(async () => {
+    fireEvent.click(screen.getByRole('button', { name: w.refresh }));
+  });
+  rerender(<Workers {...base} snapshot={earlier} />);
+  expect(screen.queryByText('celery@host-a')).toBeNull();
+});