Pre-flight selector gate, fail-closed highlight overlay, and wire clipsDir - #16
Conversation
Closes #13 and #14. A selector mistake used to surface after every narration had been paid for, as a raw Playwright error naming neither the shot nor the action, having stalled a full 30s locator timeout. The pre-flight gate resolves every declared selector against the page its shot opens, before any TTS spend. The gate resolves with the same engine capture uses, against a page carrying the same injected overlay, in a context as fresh as capture's, and waits for a late-hydrating element. Where its evidence is genuinely weaker than the render's it reports INFO instead of blocking: a selector behind an earlier click or type, an ambiguous hover, an auth-walled live shot, an unsettled page. Reviewers caught an earlier version that would have refused this repo's own demos/proctor script; both classes now carry regression tests. window.__demoHighlight no longer hides its box and returns on a miss, and capture now hands the overlay a rectangle it already resolved with Playwright, so the page never resolves the selector a second time with a different engine. clipsDir is read: a bare filename resolves inside it, a path carrying a directory resolves against the config file's directory, and prebaked runs no longer depend on the working directory. pnpm test 34 files / 201 tests exit 0; typecheck and build exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @OrionArchitekton, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis change adds a fail-closed selector preflight gate, deterministic prebaked clip resolution, strict highlight validation, CLI opt-out support, and preflight results in render reports. Documentation and unit/smoke tests cover configuration, browser resolution, capture behavior, and pipeline ordering. ChangesPreflight and capture reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runPipeline
participant runPreflight
participant Playwright
participant RenderReport
CLI->>runPipeline: load config and preflight setting
runPipeline->>runPreflight: validate manifest selectors
runPreflight->>Playwright: resolve selectors at shot URLs
Playwright-->>runPreflight: selector findings
runPreflight-->>runPipeline: blocking and info findings
runPipeline->>RenderReport: record preflight outcome
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e35e4c8f39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/capture.ts (1)
418-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate clip-existence validation between here and
src/preflight.ts'sstructuralFindings.Both this block and
structuralFindingsindependently callresolveClipPath(shot.clip, config.clipsDir, config.configDir ?? process.cwd())and then run the same!existsSync(...) || !statSync(...).isFile()check with near-identical error text — one throws, the other pushes a finding. Since the whole point of the preflight gate is that its verdict matches what capture will actually do, having this logic maintained twice risks the two checks silently diverging (e.g. a future fix to one exists/isFile edge case not mirrored in the other), which would let the gate pass a clip that capture then rejects, or vice versa.♻️ Suggested consolidation (in clips.ts)
export function resolveAndValidateClipPath(clip: string, clipsDir: string, configDir: string): string { const clipPath = resolveClipPath(clip, clipsDir, configDir); if (!existsSync(clipPath) || !statSync(clipPath).isFile()) { throw new Error(`clip not found at ${clipPath} (clip: ${JSON.stringify(clip)}, clipsDir: ${JSON.stringify(clipsDir)})`); } return clipPath; }
captureShotcan call this directly (wrapping the thrown error with the shot id), andstructuralFindingscan call it inside atry/catchto build its finding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/capture.ts` around lines 418 - 430, Consolidate clip path resolution and file validation into a shared `resolveAndValidateClipPath` helper in `clips.ts`. Update `captureShot` to use it while preserving the shot-id context in thrown errors, and update `structuralFindings` to call it inside its existing finding-building `try/catch`; remove the duplicated `resolveClipPath` and exists/isFile checks so both paths use identical validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 150-157: Label the console-output fenced code block in the README
with the text language identifier so Markdownlint recognizes it as a typed
fence, preserving the existing output unchanged.
In `@specs/preflight-selector-gate-spec.md`:
- Around line 98-106: Update S4 to specify that capture validates the selector
resolves to exactly one element, reports the shot, selector, observed count, and
pre-flight gate on failure, then passes the resolved Playwright rectangle to the
injected overlay API. Remove the requirement for the overlay init script to
receive selectors or perform cardinality checks, while preserving auto-waiting
for selectors that become unique after an initial delay.
In `@src/preflight.ts`:
- Line 57: Update the interaction classification in the preflight logic so a
step with kind "hover" also sets interacted to true alongside "click" and
"type". This ensures subsequent selector highlights are treated as unverified
after hover-driven DOM changes.
- Around line 118-129: Update the navigation validation in preflight.ts to scan
shot.actions in order and identify selector actions occurring before the first
goto, rather than only checking whether any goto exists in the shot. Report the
no-navigation finding when such selectors are present, preserving the existing
finding details and message context, and add a regression test covering a
selector action followed by goto.
In `@src/types.ts`:
- Around line 111-116: Update resolveSelectorWait so a configured
preflightWaitMs of 0 does not pass timeout: 0 to locator.waitFor, which disables
timing out; skip the wait or use a small positive timeout while preserving the
existing behavior for positive values.
In `@tests/preflight.smoke.test.ts`:
- Line 156: Add teardown alongside the existing beforeAll hook in the preflight
smoke test suite to restore FAKE_TTS after tests complete. Capture its prior
environment value before setting it, then have afterAll restore that value or
remove the variable when it was previously unset, preventing leakage to other
test files.
---
Nitpick comments:
In `@src/capture.ts`:
- Around line 418-430: Consolidate clip path resolution and file validation into
a shared `resolveAndValidateClipPath` helper in `clips.ts`. Update `captureShot`
to use it while preserving the shot-id context in thrown errors, and update
`structuralFindings` to call it inside its existing finding-building
`try/catch`; remove the duplicated `resolveClipPath` and exists/isFile checks so
both paths use identical validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be97ad17-b08e-481e-a330-8e52dc2bf631
📒 Files selected for processing (24)
CHANGELOG.mdREADME.mdspecs/preflight-selector-gate-spec.mdsrc/capture.tssrc/cli.test.tssrc/cli.tssrc/clips.test.tssrc/clips.tssrc/config.test.tssrc/config.tssrc/overlay.tssrc/pipeline.tssrc/preflight.test.tssrc/preflight.tssrc/provenance.test.tssrc/provenance.tssrc/sanitize.tssrc/types.tssrc/urls.tstests/capture.smoke.test.tstests/fixtures/late-element.htmltests/fixtures/reveal.htmltests/overlay.smoke.test.tstests/preflight.smoke.test.ts
Bot review (P1, src/preflight.ts): the gate's 3s wait budget was shorter than the render's, so an element arriving between 3s and Playwright's 30s default was blocked here and rendered fine there. The render's budget now lives in src/timeouts.ts, capture passes it EXPLICITLY rather than inheriting the library default, and the gate defaults to the same value. A budget shorter than the render's no longer blocks on absence at all: weaker evidence is reported at INFO instead. Bot review (P2, src/preflight.ts): a prebaked shot declaring no clip at all slipped past the structural check, so every narration was synthesized before capture threw "has no clip path". It is now a blocking finding alongside the declared-but-missing case. Advisory review (grok, while Codex is down) found a real secret spill the first redaction fix missed: redactUrl cleaned the URL we print, but Playwright quotes the full target back inside its own error text, so a bypass token in dashboardBaseUrl survived in an unreachable finding. redactUrlsInText now redacts URLs embedded in free text, with a regression test. Also from that review: hover reveals DOM (submenus) exactly as click does, so it counts as an interaction; and selector actions declared BEFORE a shot's first goto are probed against that goto's page, because openShotPage hoists the navigation ahead of the action loop. Two tests were racy rather than wrong: one asserted an exact finding list that a load-dependent settle warning could join, and one raced the fixture's own 900ms timer. Both now assert what they meant. pnpm test 35 files / 208 tests exit 0; typecheck and build exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Raising the gate's wait budget to match the render's made a cost 10x worse: every absent selector waited the full budget serially, so ten typos in one script cost five minutes before the run was refused. That punishes exactly the badly-broken script the gate exists to reject cheaply. Once one selector on a page has waited the budget out, the page is demonstrably done arriving, so later absences on that same page are counted without waiting again. The preflight smoke suite drops from 52.9s to 32.7s. pnpm test 35 files / 208 tests exit 0; typecheck and build exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeRabbit review, src/types.ts: preflightWaitMs allowed 0, and Playwright reads timeout:0 as WAIT FOREVER rather than "do not wait". An operator setting 0 to disable the wait got an indefinite hang before any narration, with no output at all. Reproduced as a 20s test timeout, now 147ms. Zero means the wait is disabled, matching the capture.settleMs idiom this repo already uses. Also from that review: - specs S4 had drifted from what shipped. It still described the overlay resolving selectors itself; capture now resolves the element and passes the rectangle, which is what makes the cardinality rule single-valued. The selector-taking entry point stays, fail-closed, for callers with no rectangle. - README console fence was unlabelled (MD040). - The preflight suite set FAKE_TTS without restoring it, and vitest reuses a worker across files, so it could leak into whatever ran next. pnpm test 35 files / 209 tests exit 0; typecheck and build exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Brings #16 (pre-flight selector gate, fail-closed highlight overlay, clipsDir wiring) under the fullBleed work so PR #12 stops conflicting. One conflict, src/pipeline.ts, imports only. Both intents kept: deriveSegmentKinds (fullBleed segment classification) and runPreflight/formatPreflightReport (the new gate). The new gate does not reject fullBleed shots: preflight.ts branches on target === "prebaked" and checks the clip resolves instead of resolving selectors, and resolveClipPath sends slash-bearing paths to the config dir. Verified on the merged tree: pnpm typecheck rc 0, pnpm test 35 files / 212 tests passed, pnpm build rc 0.
2 participants
Closes #13. Closes #14.
A correction to #13's premise, up front
#13 says a zero-match highlight selector renders a silent no-op and the pipeline exits 0.
Probed against
master(0d940d5), that is not what happens at thehighlightaction:The action routes through a strict Playwright locator before it ever reaches the overlay.
All ten of the motivating episode's bad selectors were on
target: dashboardshots, sothey would have thrown mid-render rather than rendering silently.
What #13 gets exactly right is the underlying function.
window.__demoHighlightisfail-open, and this is now proven rather than argued: executing the injected overlay in a
real page, both a zero-match and an ambiguous selector resolved silently
(
tests/overlay.smoke.test.tsfails onmaster, passes here).So the defect is real but differently shaped. It is not "renders silently"; it is
"fails late, expensively, and unreadably": after every narration has been paid for,
with a 30s stall per bad selector, reporting a raw Playwright error that names neither the
shot, nor the selector, nor even that a highlight was involved. That is what this PR fixes,
and it is why the pre-flight gate is the load-bearing piece.
What changed
1. Pre-flight selector gate (
src/preflight.ts), fail-closed, before any spend.Runs after the script parses and before TTS. Reports: a selector matching nothing; a
selector matching more than one element; a shot referencing a selector but never
navigating (capture builds a fresh context per shot, so it would run against
about:blank); aprebakedshot declaring selector actions capture never runs; aselector-requiring action declared without a selector; and a prebaked clip missing at its
resolved path. Decline with
preflight: falseor--no-preflight; a declined run says so.2. The gate blocks only on claims it can actually make. It resolves with the same
engine capture uses, against a page carrying the same injected overlay, in a context as
fresh as capture's, and waits for a late-hydrating element. Where its evidence is genuinely
weaker than the render's it reports
INFO: a selector behind an earlierclick/type,an ambiguous
hover(capture resolves hover non-strictly), an auth-walledliveshot,and a page that did not settle.
render-report.jsonrecords ran/declined and which shotswent unadjudicated.
3. The overlay no longer fails open, and no longer re-resolves.
__demoHighlightthrows unless exactly one match. More importantly, capture now hands the overlay a
rectangle it already resolved with Playwright, so the page never resolves the selector a
second time with a different engine. The locator still does the waiting, so an element
that appears after load still works.
4.
clipsDiris read (#14). A bare filename resolves insideclipsDir; a pathcarrying its own directory resolves against the config file's directory; absolute is used
as given. Prebaked runs no longer depend on the working directory.
What review caught, and why it mattered
Three independent reviewers ran against this diff. Two found the same blocking class,
confirmed by inspection: the gate as first written would have refused this repo's own
shipped demo.
demos/proctoruses.toggle-btn >> nth=1, whichdocument.querySelectorAllrejects as invalid, and
.verdict-box, which only exists after a click. It alsofalse-blocked
tests/fixtures/late-element.html, a fixture this same PR adds, because therender's locator auto-waits and the gate counted once instantly.
The suite was green throughout, because the gate and the render were never compared on one
input. Both classes now have regression tests. Also fixed from review: multi-
gotoshotsresolved against the wrong page, shot ORDER changing the verdict via a shared context, a
mislabelled highlight error on non-count failures, URLs logged without redacting query
strings or userinfo, and
./x.mp4silently landing inclipsDir.Breaking
tried, instead of failing later inside ffmpeg.
Test changes worth reviewing
tests/capture.smoke.test.ts"clears a previous run's events file" previously passed whilereferencing
clips/none.mp4, a file that never existed. That is the #14 defect showing upin the suite. It now writes a real clip.
Verification
pnpm test: 34 files, 201 tests, exit 0 (baseline onmaster: 30 files, 175 tests).pnpm typecheck,pnpm build: exit 0.dist/cli.js, not just vitest: a scriptwith an ambiguous selector and a never-navigating shot exits 1 and creates no
out/audio(nothing synthesized); a script exercising Playwright engine syntax, a late-hydrating
element, and a click-revealed selector renders to
final.mp4at exit 0.This repo has no CI: the Actions API reports 0 workflows and 0 runs, and there is no
.github/directory. The local suite is the only gate, so the numbers above are theevidence.
Not in scope
verification here. Left open.
maxDurationSeccomment's TTS band iswrong in both directions.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation