feat: capture single-route apps in their in-app states - #8
Merged
Merged
Conversation
A route crawler cannot capture a single-route application. Pointed at an app whose dialogs, workspaces and populated views exist only behind interaction, ui-capture produced one screenshot of the boot view and reported the site fully covered. That is worse than useless: it looks like coverage. A scripted state is a named, declarative interaction script performed on a page before capture, and each state yields its own capture set under <route>/states/<name>/. States are peers of routes rather than a phase bolted onto the end of a crawl: same bounded queue, same worker pool, same --concurrency, same results map, same report. Vocabulary: waitFor, wait, click, fill, select, press, request, reload, plus the shared optional / timeoutMs / settleMs modifiers. Admitting a kind requires that it produce committed page state, read as data in a diff, and not be expressible by composing the others -- which is why there is no hover (the viewport loop resizes underneath a cursor), no evaluate, and no variables anywhere in the format. waitFor's minCount polls locator.count() from the driver, so no user-supplied code ever crosses into the page. Determinism: every state starts from a fresh page load in a fresh browser context, since page.goto clears neither cookies nor localStorage. extends is script composition, not page-state carryover -- the child replays the parent's steps from its own clean load, so any state runs on any worker in any order. Failure model: authoring errors (duplicate names, unknown extends, a cycle, an off-host request, a viewport filter naming an unconfigured viewport) abort before Chromium launches. Runtime errors are recorded per state and the run continues, exactly as a failing route does, with stateStatus and failedStepIndex naming the step that broke -- including for a whole-state timeout, which reads the in-flight step from a Ref. A state whose precondition selector is absent is recorded as skipped rather than failed, so "this state does not exist here" stays distinguishable from "this state's script is broken". request is gated behind --allow-state-requests, checked at load time and again in the service so a programmatic caller cannot skip it. The states file carries a required version: 1 discriminant, because the step vocabulary becomes a public JSON format on other people's disks the day it ships. A state containing a request step skips video by default, since video replays the script in a second context and a non-idempotent seed would run twice. Flags: --states, --state-filter, --skip-routes, --state-timeout, --allow-state-requests, --fail-on-state-error. buildInvocation stays synchronous and I/O-free; reading and parsing the states file happens in runFromArgs, at the edge. Additive throughout: states defaults to [], captureRoutes to true, and an integration test asserts a no-states run still produces the pre-feature output tree and report numbers. A new docs test asserts README, USAGE and the schema agree, since documentation drift is the defect class that survives every other test.
processRouteTask's failure was caught in the worker loop, logged to stderr, and never entered into the results map. Because generateReports counts failures out of that map, failedCaptures was structurally 0: a crawl that lost half its routes to navigation errors still reported as fully covered, which is the same disease scripted states were added to treat. The catchAll now writes a CaptureResult carrying the route and the formatted error, the way a failed state does, unless a result for that URL already exists. This changes report content for runs that already had failing routes: failedCaptures becomes accurate, results.size grows by the number of failed routes, and those routes appear under "Failed Captures" in REPORT.md. Runs with no failures are unaffected. Kept as its own commit because it is a behaviour change to existing output rather than part of the additive feature.
Adversarial review of the scripted-states feature confirmed defects in four models the feature had gotten subtly wrong. Each one made a run report something other than what actually happened. Failure model. A video recording that failed discarded the screenshots already written for that viewport and reported the whole capture as a failure that produced nothing; it is now a success that names what it lost, through a `videoErrors` field carried into both reports. A `precondition` that could not be *evaluated* — a typo'd or malformed selector — answered "not present here", which is how a broken selector produced a green run with nothing captured; it now fails, and only a genuinely absent selector skips. A state that timed out named the last step it had started, often one that had already succeeded; it now names the phase it was actually in: navigating, probing, inside step N, or settling after it. Origin model. The pre-launch check and the runtime `request` gate had drifted into two different comparisons, one of them on hostname alone, so a states file could be validated against one rule and run against a wider one. Both now call a single `isAllowedOrigin` — scheme, host and port — applied where each resolution actually happens. Budget model. `--state-timeout` covered capture as well as reaching the state, which made it unsatisfiable: with `--video` on, no default could cover navigation plus a screenshot and a recording per viewport, so every state timed out on a configuration that looks entirely reasonable. It now bounds reaching the state only, its default clears the 30 s navigation ceiling, and the precondition probe gets its own budget and a `--precondition-timeout` flag. Resource model. A browser context was stranded whenever the page created inside it failed to open, because Effect registers a release only once its acquire has completed. The state path and the worker pool now acquire context and page separately, and the one best-effort close helper is shared rather than reimplemented at each call site. Chain composition. `filterStates` inlined an ancestor's steps but not its `allowVideoReplay`, so a chain whose seed step suppresses video recorded it on a full run and silently dropped it under `--state-filter` — one file, two results. Everything the chain contributes is now written back into the flattened state. Also: an empty `viewports` array is rejected rather than capturing nothing and reporting success, and two helpers with no consumer outside their own module stop being exported. README and USAGE track every behaviour change above.
Claims that only a browser can settle, added to the opt-in RUN_INTEGRATION suite so each fix in the preceding commit is held up by something other than a unit test agreeing with itself. A `--state-filter` run captures only the named state and replays its ancestor to reach it rather than capturing it — asserted on the seed endpoint being reached once and not twice, and on the ancestor having no output directory. The ancestor's `allowVideoReplay` survives that flattening, which is the regression that made the same file record video unfiltered and drop it when filtered. A budget that expires inside a step names that step and sets `failedStepIndex` to it, the counterpart to the existing proof that a budget lost during navigation names no step at all. No browser context is alive when the run closes the browser, measured after a state whose script failed and a state interrupted mid-navigation by a 1 ms budget — asserted on the context count at teardown rather than on the absence of an error, which a leak would not have raised anyway. And a state captured under the shipped defaults with video — no overridden timeout, viewport list, wait time or video options — completes in about a minute instead of timing out. That configuration is the one the budget fix exists for, so it is the one worth running.
`npm run lint:knip` could not fail. tsdown's config declares `src/**/*.ts` as its entry list, knip's tsdown plugin adopts that verbatim, and knip does not report the exports of an entry file — so every module in the package counted as an entry and no unused export was reportable anywhere in src. A canary export appended to shared.ts went unreported, which is what confirmed it rather than assumed it. The plugin is disabled and the real entry points named instead: the package main, the bin, and the test files. Entry exports stay exempt, so the public API re-exported through index.ts is not flagged, while an export from an internal module that nothing imports is. Switched on, it reported four. Two belonged to the scripted-states work and are un-exported in the commit that owns that code. The other two are in src/utils/args.ts and predate this branch: `displayHeader` is called from nowhere and `SEPARATOR` exists only to serve it. They are deleted rather than ignored — a gate is worth having only if what it finds gets acted on.
`isAllowedOrigin` was documented as "the origin gate every URL check in this codebase must agree on". It is not: route crawling has never used it. `scheduleRoute` and the link filter both match on `url.hostname` alone, so a crawl seeded at `http://app.test:3000` follows a link to `https://app.test` or `http://app.test:8080`. The claim was the defect, not the behaviour. Crawling navigates and screenshots, and inside one deployment an http->https or cross-port link is an ordinary internal link; the origin gate authorizes *driving* a URL -- a scripted state's entry `url` and a `request` step's POST or DELETE -- which is a stronger act deserving a stronger check. `--allowed-hosts` and `--include-subdomains` are documented as hostname filters, and hostname is what bounds a crawl. So the claim is narrowed to the two decisions it really covers, and the asymmetry is written down where each half is read: on `isAllowedOrigin`, on the crawl filter in `link-discovery.ts`, at `scheduleRoute`, and in the README beside the two flags it qualifies. No behaviour changes. Adds the unit coverage `isAllowedOrigin` never had: that it rejects a bare scheme or port difference, treats 443/80 as equal to writing them out, and widens the host half only for `--allowed-hosts` and `--include-subdomains`.
…inates The test for "which step was running when the budget expired" asserted only the case where naming a step is right: a `waitFor` that hangs. The model it replaced -- an index set when a step *started*, carrying no phase -- satisfies that assertion exactly as well as the fix does, so the test could not fail if the fix were reverted. The case that separates them is a step that already succeeded and whose settle pause then overruns: the old model names it "on step 1 (click "#reveal")", sending the reader to a click that worked. So the state `settles-past-budget` is added alongside the hanging one and asserted as a pair -- the message must say `while settling after step 1`, and must not say `on step 1` -- while `failedStepIndex` stays 1, because the pause belongs to that step's definition. Also narrows `timeoutMs`'s doc. It called itself a "whole-state budget: navigation + script + capture", which capture is not inside: screenshot and video run after it, under their own timeouts. A budget that really covered capture could not be satisfied by any value once `--video` was on. The behaviour is right and already covered; the comment was wrong.
…it captured Status was decided from reaching the end of the viewport loop, not from anything coming out of it. A state whose loop ran zero times therefore finished as `stateStatus: "captured"` with an empty `screenshots` map: a green result, an empty directory on disk, and a run whose success count included a state that produced no image. The schema's `minItems(1)` on a state's `viewports` is what keeps that unreachable today, and it stays the first line of defense. But the status is decided in the service, so that is where "the loop ended" must be distinguished from "something was captured" -- whatever else could put a state in front of the loop with nothing to iterate: a relaxed schema, a filter that intersects to nothing, a future capture axis. The guard fails through `CaptureError`, the channel the caller already maps to `stateStatus: "failed"`. Tested by constructing the state the schema refuses, via `disableValidation`, after first asserting that the schema does refuse it -- so the test covers the service guard without pretending the schema guard is gone. The expected message is imported rather than copied, so the assertion cannot drift from what the service reports.
…not open The existing leak test asserts that every context is closed, but every context in it gets its page, so it holds just as well for an acquire that takes context and page together as for one that takes them in sequence. It cannot reach the case that separates them. Effect registers a release only once its acquire has completed, so an acquire taking both leaks the context whenever `newPage` rejects -- a live Chromium context per bad state, on the one path where something was already going wrong. A rejecting `newPage` is the only stimulus that tells the two shapes apart, since nothing else fails between the two acquisitions. So it is injected at the Playwright boundary rather than in the service: `context.newPage()` rejects once, at a chosen call, and the count comes from `browser.contexts()` -- Playwright's own list -- sampled after the run is finished with the browser and before teardown, the only window in which a stranded context is still distinguishable from a closed one. A context counted there genuinely was not closed. Both paths are covered, since the state path strands one context per bad state and the worker path stranded one for a whole run. Each asserts that its injection actually fired, so a call ordering the test got wrong fails it rather than passing it vacuously.
|
All reports are resolved now. Thanks! ✅
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
status: accepting prsOverview
A route crawler cannot capture a single-route application. Pointed at a Three.js operator console — one URL, whose interesting states (spawn dialog, environment dialog, safety workspace, a populated fleet) exist only after interaction — ui-capture produces one screenshot of the boot view and reports the site fully covered. That is worse than no coverage, because it looks like coverage.
Scripted states are named, declarative interaction scripts performed before capture, so each state yields its own capture set.
{ "version": 1, "states": [ { "name": "spawn-dialog", "precondition": "#operator-shell", "steps": [ { "click": "#btn-spawn-asset" }, { "waitFor": "#spawn-dialog[open]" } ] } ]}Design decisions worth reviewing
No
evalstep. Steps are declarative and serialisable. Arbitrary in-page JS would be unreviewable, unportable, and a footgun in a tool whose job is pointing a browser at a URL someone supplied. A consequence worth keeping: the step engine is unit-testable without a browser, and the test stub makesevaluatethrow so any regression that starts injecting page code fails loudly.Eight step kinds —
waitFor,wait,click,fill,select,press,request,reload— each justified, with omissions justified too (press.repeat/delayMswere cut as expressible by repetition).Failure is per-state. A state whose selector never appears fails that state, records it in the report, and the run continues — matching how a failing route behaves. States start from a clean page load unless explicitly chained.
Backwards compatible. Every default reproduces current behaviour; a run with no states is byte-identical in output.
What the review caught
27 findings, all confirmed, none refuted — several reproduced end-to-end against real Chromium. The ones worth knowing:
--state-filtercaptured states you didn't name. Ancestors pulled in forextendsresolution were folded into the same flat list and captured, re-running their side-effectingrequeststeps.tryPromise, closed only inline on success. Interruption is the likely path, since that is what the state timeout does.goto(with its own 30 s navigation timeout), the script, every screenshot and the video replay, so a default run with--videoalways timed out. The budget is now scoped to reaching the state; capture sits outside it.false, so a malformed selector and an absent element were indistinguishable and the state reportedskipped.knipwas passing vacuously.tsdowndeclared everysrc/**/*.tsan entry, and knip never flags exports of entry files. Proven with a canary export, then fixed — it immediately found four dead exports, two pre-existing.Honest limitations
createWorkercontext leak is fixed by inspection but unproven: makingcontext.newPage()reject on demand against real Chromium was not possible without mocking the thing under test. Stated rather than covered by a test that pretends.captured. Currently unreachable, but a genuine asymmetry — flagged rather than silently widened.requeststeps match on scheme+host+port. The docblock that claimed otherwise has been narrowed to what is true; crawl scope is deliberately unchanged, so this is not a behaviour change.Test plan
tsc,biome,knip,markdownlint,cspell,tsdown build— all clean