feat(cli): warn when lambda --width/--height conflicts with composition#1022
Conversation
jrusso1020
force-pushed
the
feat/lambda-render-warn-on-dimension-mismatch
branch
from
b67ce68 to
628cce0
Compare
jrusso1020
force-pushed
the
fix/lambda-progress-cost-task-events
branch
from
d8851cf to
3b64fab
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Clean, well-scoped PR. The refactoring of findCompositionDimensions out of resolveCompositionViewportFromHtml is semantically correct — the null-returning variant delegates cleanly and the fallback in the original function is preserved. Edge cases are thoroughly covered: missing index.html, absent composition root, --output-resolution suppression, --json quiet mode. Integration into both render.ts and render-batch.ts threads through the right fields.
No issues found.
jrusso1020
force-pushed
the
fix/lambda-progress-cost-task-events
branch
from
3b64fab to
aa95ee0
Compare
jrusso1020
force-pushed
the
feat/lambda-render-warn-on-dimension-mismatch
branch
from
628cce0 to
370a42f
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
No blockers. _dimensions.ts helper is well-isolated, the --json/--output-resolution suppression is thorough, and the 6 test cases cover all suppression conditions cleanly.
Important
resolveCompositionViewportFromHtml has a subtle behavioral change. Old code defaulted width and height independently (width ?? DEFAULT.width, height ?? DEFAULT.height). New code: findCompositionDimensions(html) ?? DEFAULT_VIEWPORT — if either parsed dimension is null, both fall back to defaults as a pair. Since the CSS selector requires all three attributes ([data-composition-id][data-width][data-height]), the only path to a null parse is a malformed attribute value (e.g., data-width="abc"). The new behavior is arguably more correct (coherent defaults over partial defaulting), but validate and snapshot both call this function in production paths. Please add a test for the partial-invalid-attribute edge case to document and lock in the new semantics. Without it, the semantic change is untested and invisible to future readers.
Nits
fallow-ignore-next-line complexityonwarnOnDimensionMismatchadds noise — the function is ~20 lines and straightforward; consider removing the suppressor.- Warning copy "edit the composition's data-width/data-height in index.html" is a footgun if the user has a build step that generates
index.html. Consider softening to "edit the composition's authored dimensions."
Note: Stacked — Build/Test/Typecheck/Lint did not run on this PR head.
— Vai
jrusso1020
force-pushed
the
feat/lambda-render-warn-on-dimension-mismatch
branch
from
370a42f to
b5bf5dc
Compare
jrusso1020
force-pushed
the
fix/lambda-progress-cost-task-events
branch
2 times, most recently
from
582af7b to
3033d01
Compare
The CDK construct compiles tasks.LambdaInvoke to the optimized arn:aws:states:::lambda:invoke integration, which emits Task* history events with the Lambda response wrapped in .Payload. getRenderProgress was only listening for the older LambdaFunction* events, so every CDK- deployed stack reported $0 total cost and zero invocations on success — a high-visibility regression that only surfaced when we manually walked SFN history during a cost-analysis sweep. Add cases for TaskScheduled (count invocation), TaskSucceeded (parse Payload + accumulate billed duration / frame counts), and TaskFailed (record error). Keep the LambdaFunction* paths so anyone wiring the raw lambda:invokeFunction.sync task type still works. Factor out the shared FramesEncoded-attribution logic so both branches agree on the "only RenderChunk frames count" rule. Tests pin a real-shape regression: replay the inspector-launch 1080p/30fps history (1 Plan + 16 RenderChunks + 1 Assemble) and assert lambdaUsd lands at ~$0.582 — matching the cost-analysis script's direct read against SFN history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`--width 3840 --height 2160` against a composition with `data-width="1920"` silently produces a 1080p output because the runtime lays out the page at the composition's authored dimensions — real footgun we hit during a cost-analysis sweep. Warn early and point at `--output-resolution` (the supersampling escape hatch) so the user doesn't burn a 30-minute render learning the override rule. Skipped when `--output-resolution` is set (the supported supersampling path — the user is opting in), when `--json` is set (machine consumers), or when `index.html` isn't on disk (typical with `--site-id`). Helper lives in a shared module so render + render-batch agree on the parse + message. Tests cover both attribute orders, single/double quotes, the silent paths, and the warning path. Best-effort regex over the canonical attr shape — malformed HTML falls through to no warning rather than blocking the render. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jrusso1020
force-pushed
the
fix/lambda-progress-cost-task-events
branch
from
3033d01 to
36ddd48
Compare
jrusso1020
force-pushed
the
feat/lambda-render-warn-on-dimension-mismatch
branch
from
b5bf5dc to
37ecdad
Compare
jrusso1020
dismissed stale reviews from vanceingalls and miguel-heygen
The base branch was changed.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Approved — no issues found in review.
3 participants
What
Warn early when
hyperframes lambda render/render-batchis passed--width/--heightvalues that disagree with the composition's authoreddata-width/data-height. Points the user at--output-resolution(PR #1020) — the supported supersampling escape hatch.Why
This is a real footgun I hit during the cost-analysis sweep. I passed
--width 3840 --height 2160to render a 1080p composition at 4K, the command succeeded after 8 minutes, and the output was still 1080p. The runtime lays out the page at the composition's authoreddata-width/data-height— Config.width is ignored on disagreement, silently. A 30-minute render later you learn whatdata-*does.The warning catches this at CLI dispatch time (~5ms before any AWS call) and tells the user how to actually get the higher resolution.
How
warnOnDimensionMismatchhelper inpackages/cli/src/commands/lambda/_dimensions.ts, called from bothrunRenderandrunRenderBatch(inrender-batchit fires once before the per-entry loop — a 1000-row batch with wrong dimensions burns the bug across every entry).findCompositionDimensionshelper added topackages/cli/src/utils/compositionViewport.ts(a thin null-returning variant ofresolveCompositionViewportFromHtml, which already powersvalidate+snapshotand uses the same[data-composition-id][data-width][data-height]selector the producer uses to lay out the page). No regex parallel-parser.--output-resolutionis set (user has opted in to supersampling), when--jsonis set (machine consumers parse the manifest), or whenindex.htmlisn't on disk (typical with--site-idpointing at a pre-uploaded site).try { readFileSync }— noexistsSyncTOCTOU.Test plan
--output-resolution· silent under--json· silent whenindex.htmlmissing · silent when composition has nodata-composition-idroot.--width 3840 --height 2160against a 1080p composition prints the warning + suggestion before the render starts.