feat(cli): emit render_preflight_rejected telemetry for P1-3 pre-flight saves#1856
Conversation
…ht saves The P1-3 aspect/alpha/HDR pre-flight (#1843) aborts an incompatible render before any browser/ffmpeg work — but that "save" was invisible on dashboard 1783183, so we couldn't distinguish "caught early by pre-flight" (fix working) from a deep render failure or a user who gave up. `checkRenderResolutionPreflight` now returns `{ message, kind }` (the kind is the existing low-cardinality `OutputResolutionIssueKind` — aspect-mismatch / alpha-incompatible / hdr-incompatible / downsampling / non-integer-scale), and the render command emits `render_preflight_rejected { kind }` before exiting. Tests: the preflight tests now assert the `kind` alongside the message; a new events test locks the emit. No parsers change — the helper already carried kind. Further follow-up (different subsystems, still log-only): encoder-frame-0-exit counter (streaming stage) and a P1-4 doctor env-check event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM — closes the P1-3 telemetry gap cleanly
Follow-up to #1843 (P1-3 pre-flight). Same shape as the #1850 capture-retry counter — reuses the existing trackEvent pipeline, no new PostHog wiring, no parsers churn.
counter placement
Emission is at the top-level rejection sink in render.ts (right before errorBox + process.exit(1)), not inside each rule branch. checkOutputResolutionCompatibility short-circuits on the first failing check (HDR → alpha → aspect → downsampling → non-integer-scale), so exactly one kind is possible per call → one emit per rejection. No double-count risk.
kind low-cardinality
Verified. OutputResolutionIssueKind in packages/parsers/src/outputResolutionCompatibility.ts is a 5-value string-literal union:
"hdr-incompatible" | "alpha-incompatible" | "aspect-mismatch" | "downsampling" | "non-integer-scale"No dynamic-string concat into kind anywhere. Type flows: helper returns typed union → callsite passes typed value into trackRenderPreflightRejected. Nit (non-blocking): the wrapper's public signature is props: { kind: string } — could narrow to OutputResolutionIssueKind for defense-in-depth, but callsites are safe today.
coverage of all rejection kinds
All 5 kinds route through the single resolutionIssue variable → single emission. Complete coverage. Tests exercise aspect-mismatch and alpha-incompatible explicitly; the other three (hdr-incompatible, downsampling, non-integer-scale) share the same sink so they'll emit in prod without dedicated tests. Nit-level.
reachability
Guard is if (outputResolution) — unconditional, no feature flag. Whenever a user passes --resolution, the pre-flight runs and any rejection fires the counter. No memory-verify-new-path-reachable concern.
CI
Typecheck is red on this SHA, but the failure is transient infra — bun install --frozen-lockfile failed to extract the @fontsource/montserrat@5.2.8 tarball (job 84689807599). Not a code issue. Author's stated local gate green is credible; retry the job before merge.
🟠 pr-envelope (heads-up, non-blocking)
Co-Authored-By: Claude Opus 4.8 in the commit message + 🤖 Generated with [Claude Code] in the PR body. Flagging per team convention.
Review by Via
… kinds Addresses Via's non-blocking nits on #1856: - Narrow `trackRenderPreflightRejected`'s param from `{ kind: string }` to `{ kind: OutputResolutionIssueKind }` so the metric dimension can't carry free text (consistent with the other typed domain wrappers in events.ts). - Add classification tests for the three kinds that shared the rejection sink without dedicated coverage: hdr-incompatible, downsampling, non-integer-scale. (The earlier red Typecheck was a transient `bun install` tarball-extraction failure, not code — this push re-runs it clean.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @vanceingalls — both nits addressed in
Good call on the Typecheck red being transient — it was the |
miguel-heygen
left a comment
There was a problem hiding this comment.
Clean R2 delta. The original counter placement stays right: packages/cli/src/commands/render.ts:800 emits render_preflight_rejected at the single top-level preflight rejection sink, before browser/ffmpeg work starts, so each rejected render records exactly one low-cardinality kind.
The new commit closes the two small gaps from review: packages/cli/src/telemetry/events.ts:1 / packages/cli/src/telemetry/events.ts:316 now type the wrapper as OutputResolutionIssueKind, so the metric cannot drift into arbitrary strings, and packages/cli/src/commands/render.test.ts:482 adds classification coverage for the three remaining issue kinds (hdr-incompatible, downsampling, non-integer-scale) without weakening the existing sink tests.
No blockers. CI note: the earlier Typecheck red was an install flake on @fontsource/montserrat; the current head is re-running and had no failed checks at review time.
Verdict: APPROVE
Reasoning: The PR wires the P1-3 preflight-rejection telemetry through the existing CLI telemetry pipeline with bounded cardinality and targeted coverage, without changing parser/render behavior.
— Magi
What & why
Second render-reliability telemetry follow-up (after #1850). The P1-3 aspect/alpha/HDR pre-flight (#1843) aborts an incompatible render before any browser/ffmpeg work — a good outcome — but that "save" was invisible on dashboard 1783183. Without it we can't distinguish "caught early by pre-flight" (the fix doing its job) from a deep render failure or a user who just gave up. This adds that counter.
Changes
checkRenderResolutionPreflightnow returns{ message, kind }instead of a bare string.kindis the existing low-cardinalityOutputResolutionIssueKind(aspect-mismatch/alpha-incompatible/hdr-incompatible/downsampling/non-integer-scale) — no parsers change needed, the helper already carried it.render_preflight_rejected { kind }right before it exits on an incompatible preset.trackRenderPreflightRejectedevent wrapper (trivialtrackEventwrapper, mirrors the existing ones).Tests
kindalongside the message (aspect-mismatch, alpha-incompatible).events.test.tscase locks the emit (render_preflight_rejected+ the kind prop).Full local gate green: build /
tsc --noEmit(cli) / render + events suites (37 tests) / oxlint / oxfmt /fallow audit.Scope / follow-ups
Focused on the highest-value remaining "is the fix working?" signal. Still log-only, left as further follow-ups (different subsystems): the encoder-frame-0-exit counter (streaming capture stage) and a P1-4 doctor
cli_env_checkevent.Measured on PostHog dashboard 1783183.
🤖 Generated with Claude Code