fix(studio): clean next alpha inspector artifacts#721
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: Comment (not blocking). The cleanup work is right; the square-preset expansion is correct but ships under-tested and silently changes parser behavior. Address the important items in a follow-up before this lands on main.
This is two PRs in one trench coat: (a) a focused QA-artifact + Delete-gate cleanup that directly addresses post-merge follow-ups #1 and #5, and (b) a drive-by union expansion for square render presets. (a) is clean. (b) establishes square / square-4k as first-class CanvasResolution values across studio + core + parser, but stops short of auditing the rest of the call graph.
Blockers
None.
Important
1. Behavior change in parseHtml for square compositions — no migration note.
packages/core/src/parsers/htmlParser.ts:148-160 (and the deleted test at htmlParser.test.ts:278-292)
The previous test was explicitly a pinning test: "Pinning so a future refactor doesn't silently flip it." This PR silently flips it. Any composition with data-composition-width="1080" data-composition-height="1080" and no explicit data-resolution attr now resolves to square instead of portrait.
parseHtml().resolution feeds htmlCompiler, renderOrchestrator, videoFrameExtractor, compilationTester, CLI info, and the studio bundler. None of them have been audited in this PR for the new square / square-4k enum values. If any of those branches has a switch on CanvasResolution without a default, this lands as a runtime regression on next render of an existing 1080×1080 project.
Why this matters: you removed the pinning test that future-you put in place to prevent exactly this. At minimum: (a) call out the behavior change in the PR body, (b) grep every downstream switch/if on CanvasResolution for exhaustiveness, (c) add a parser test that a square composition with an explicit data-resolution="portrait" still respects the override.
2. resolveDeviceScaleFactor and the orientation-mismatch contract are not extended for square.
packages/producer/src/services/renderOrchestrator.test.ts:767-838 (or services/render/shared.ts)
The producer test suite has explicit coverage for landscape-4k and portrait-4k, including an orientation-mismatch rejection at line 812 (landscape comp → portrait-4k). After this PR, square and square-4k are valid outputResolution values that flow through this function — but there is zero test coverage for:
squarecomp →square-4k(scale factor 2? something else?)squarecomp →landscape-4korportrait-4k(orientation mismatch should reject, presumably)landscapecomp →square-4k(definitely orientation mismatch)
This is the audit-all-sites rule: if you add new values to a shared union, every site that switches on the union needs explicit coverage for the new values. Otherwise we discover the behavior in production via a confused render.
3. studio-api/routes/render.test.ts parameterized preset test is now stale.
packages/core/src/studio-api/routes/render.test.ts:102
for (const preset of ["landscape", "portrait", "landscape-4k", "portrait-4k"] as const) {This loop existed precisely to assert that every valid preset forwards correctly through the API. It now skips two of the six valid values. Same reasoning as (2) — extend the loop, don't grow the dead-letter list.
4. CLI init preset table grew silently.
packages/cli/src/commands/init.test.ts:196
applyResolutionPreset is now reachable for square and square-4k via normalizeResolutionFlag ("1080p-square", "4k-square", "square-1080p"), but the init test only covers portrait-4k. Bake in a square case so a future CLI refactor doesn't break it without us noticing.
5. PropertyPanel helper exports have no production caller.
packages/studio/src/components/editor/PropertyPanel.tsx:339-360
isPropertyPanelMediaLikeSelection and getPropertyPanelVisibleSections are exported and consumed only by PropertyPanel.test.ts. The component itself (PropertyPanel.tsx) re-implements the same logic inline (background-image checks at lines 2182/2301/2306, the section ordering lives in the JSX). These helpers test a parallel re-implementation of the logic, not the actual rendered behavior — so a divergence between helper and component would land without any test failing.
Either (a) refactor PropertyPanel to call these helpers (then the tests are real), or (b) delete the helpers and the tests and write component-level tests (RTL or otherwise) against the real surface. The current state is test-shaped scaffolding around dead code. The PR body frames this as "restoring expected exports" — but if no production caller exists, that's exactly the smell.
Nits
6. Native window.confirm is unblocked-but-untestable.
packages/studio/src/App.tsx:160-164
Functional choice is fine for a single-user desktop tool, and the gate covers both the keyboard path (line 2003) and the click path (onDeleteElement at line 4303). Two small UX considerations: (a) clicking "Delete" in a menu and then dismissing a confirm dialog is double-friction — keyboard users benefit more from the gate than mouse users; if there's a future v2, consider an inline confirm/undo toast instead of a modal. (b) window.confirm blocks render-thread message handling; harmless here but a smell to flag if Studio ever moves to a Worker for heavier compute.
7. qa-artifacts/ is in the # Test artifacts block of .gitignore.
.gitignore:66
Cosmetic: the entry comments above are aimed at outputs of test runs (my-video/, examples/, packages/studio/data/). qa-artifacts/ is local browser/QA proof, which is conceptually different. Either rename the comment block to # Local proof / test artifacts or move qa-artifacts/ into its own block with a one-line comment so future contributors know why. Minor.
8. docs/packages/core.mdx documents CanvasResolution as "landscape" | "portrait".
Pre-existing drift (the 4K presets aren't there either), but this PR is the right moment to fix it. One-line doc update.
9. useRenderQueue.ts:17-23 carries a stale comment.
The comment still says "4 string literals tied to a stable enum" but the union is now 6. The PR partially updated this comment ("string literals tied to a stable enum") — finish the cleanup or just say "6 string literals (kept in sync manually with CANVAS_DIMENSIONS)" so the next reader knows the current count.
Praise
- Cleanly addresses post-merge follow-ups #1 (qa-artifacts removed +
.gitignoreentry) and #5 (Delete confirm + post-delete Undo toast). The toast wording ("Use Undo to restore it") is exactly the visible recovery affordance the advisory asked for. - The confirm gate is wired through both the timeline and the DOM-edit delete paths, and into both the keyboard handler and the button handler — that's the right blast-radius.
- The
width === heightbranch inresolveResolutionFromDimensionsis genuinely clearer than the priorw > h ? landscape : portraitternary with a footnote about square corner cases. - The PR body is well-scoped: problem → fix → root cause → verification → notes. Browser proof artifacts are listed but correctly ignored. This is the format I'd hold up as the team standard.
— Review by Vai
26fe3e7 to
005e196
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: Approve. All 5 important items from the prior review are resolved with concrete test artifacts, the consumer audit is now documented in the PR body and independently verifiable, and the fix-up is purely additive (new tests + helper deletion + doc/comment cleanup). No new findings.
Addressed since prior review
Comparing 26fe3e7 (prior reviewed) → 005e196 (current HEAD).
1. Parser pinning + override + consumer audit (Important #1). packages/core/src/parsers/htmlParser.test.ts:200-214 (005e196) adds the explicit-override pin I asked for: data-resolution="portrait" + 1080×1080 dims → portrait. The square / square-4k inference pins were already in place at lines 290-326. The PR body now documents the consumer audit ("only aspect-ratio contract that needs explicit square handling is producer resolveDeviceScaleFactor"). I verified independently: templates/base.ts, generators/hyperframes.ts, studio-api/routes/render.ts, useRenderQueue.ts, renderOrchestrator.ts all either index CANVAS_DIMENSIONS[resolution] or forward the typed value — no switches that would crash on square / square-4k. Audit holds.
2. resolveDeviceScaleFactor square coverage (Important #2). packages/producer/src/services/renderOrchestrator.test.ts:781-789 (005e196) — happy path square 1080p → square-4k returns 2. Lines 829-839 — orientation mismatch square → landscape-4k and square → portrait-4k both throw /aspect ratio/. Lines 841-845 — landscape comp → square-4k rejected. That's the full matrix of square-related branches through this function.
3. Studio-API preset loop parameterized (Important #3). packages/core/src/studio-api/routes/render.test.ts:99-103 now imports VALID_CANVAS_RESOLUTIONS from core.types and loops over the canonical list instead of the hardcoded 4-tuple. Future presets get covered automatically — exactly the right shape for this kind of fan-out test.
4. CLI init preset table (Important #4). packages/cli/src/commands/init.test.ts:210-241 adds explicit square and square-4k cases asserting data-width, data-height, data-resolution, and the CSS dims all flip correctly. CLI init.ts:629 and render.ts:189 descriptions + error messages list the new presets and aliases.
5. PropertyPanel dead helpers (Important #5). Cleanly deleted. The helpers (isPropertyPanelMediaLikeSelection, getPropertyPanelVisibleSections) are gone from PropertyPanel.tsx (verified: rg finds zero hits at 005e196 vs lines 339 + 349 at 26fe3e7). The orphan tests at PropertyPanel.test.ts are also gone (49 lines removed). Component-level tests can come later if needed — no parallel-implementation drift risk anymore.
Nits addressed:
- #7
.gitignorecomment block.# Test artifacts→# Local proof / test artifacts. - #8
docs/packages/core.mdx.CanvasResolutiondoc comment now lists all 6 union values;cli.mdxresolution tables for bothinitandrenderupdated. - #9
useRenderQueue.ts:17. Comment now reads "6 string literals kept in sync manually with CANVAS_DIMENSIONS" — accurate count, explicit drift-management note.
Still open
None.
New findings
None. The fix-up commit is additive only — new tests, helper deletion, comment + doc updates. No new code paths.
Praise
- The override-pin test at
htmlParser.test.ts:200-214is exactly the shape I asked for: positive assertion that explicitdata-resolutionstill wins over dimension inference. Good defensive coverage. - Parameterizing the studio-api preset loop on
VALID_CANVAS_RESOLUTIONSis the right call — turns it from a static list that needs maintenance into a self-updating contract test. Future presets get coverage for free. - The producer aspect-ratio mismatch matrix is complete: square → non-square 4K, landscape → square-4k, and the happy path. No orientation pair untested.
- Deleting the PropertyPanel helpers + their tests (rather than refactoring the component to call them) is the smaller-blast-radius choice. The right call given those helpers had no production caller.
- PR body audit paragraph ("I audited the direct downstream resolution branches with
rgacross core, producer, engine, CLI, and Studio…") is the format that makes union-expansion PRs reviewable. Cite the search, name the only sensitive site, ship the test. Hold this up as the team template.
— Review by Vai
Problem
The post-merge advisory found that
nextstill contained local QA artifacts from the alpha inspector work, including binary screenshots and recordings underqa-artifacts/. It also flagged the destructive Delete shortcut added in the alpha flow as too easy to trigger without a confirmation or visible recovery affordance.While validating the cleanup branch, Studio typecheck also exposed a drifted render-resolution union: the render UI offered
squareandsquare-4k, but the local Studio type and core resolution table onnextdid not consistently accept those values.What this fixes
qa-artifacts/studio-inspector-selection-uxbinaries fromnext.qa-artifacts/to.gitignoreunder a local proof / test artifact comment so future browser proof does not get committed accidentally.Root cause
The inspector QA artifacts were created as local browser proof but were committed into the alpha branch. The Delete shortcut was wired as a direct destructive action, relying on undo behavior that was not visible at the point of deletion. Separately, the render queue UI and core resolution types had drifted around square presets, so typecheck failed once the branch was validated directly.
Square Resolution Behavior
This PR intentionally changes inferred square composition behavior: a composition with equal authored dimensions, such as
data-composition-width="1080" data-composition-height="1080", now resolves tosquareinstead of falling through toportrait. Explicitdata-resolution="portrait"still wins over square dimensions.I audited the direct downstream resolution branches with
rgacross core, producer, engine, CLI, and Studio. The only aspect-ratio contract that needs explicit square handling is producerresolveDeviceScaleFactor; the other direct users either indexCANVAS_DIMENSIONS, forward the typed value, or carry concrete dimensions. This PR adds coverage for the producer square scale/mismatch cases and for the Studio API / CLI preset paths.Verification
Local checks
bun run --filter @hyperframes/studio typecheckbun run --filter @hyperframes/core typecheckbun run --filter @hyperframes/cli typecheckbun run --filter @hyperframes/producer typecheckbun run --filter @hyperframes/studio test -- src/components/editor/manualEditingAvailability.test.ts src/components/editor/domEditing.test.ts src/components/editor/PropertyPanel.test.ts src/components/editor/manualOffsetDrag.test.ts src/player/components/Timeline.test.tsbun run --filter @hyperframes/studio test -- src/components/editor/PropertyPanel.test.tsbun run --filter @hyperframes/core test -- src/index.test.ts src/parsers/htmlParser.test.tsbun run --filter @hyperframes/core test -- src/index.test.ts src/parsers/htmlParser.test.ts src/studio-api/routes/render.test.tsbun run --filter @hyperframes/cli test -- src/commands/init.test.ts src/commands/render.test.tsbun test packages/producer/src/services/renderOrchestrator.test.tsbunx oxlint .gitignore packages/core/src/parsers/htmlParser.test.ts packages/core/src/studio-api/routes/render.test.ts packages/producer/src/services/renderOrchestrator.test.ts packages/cli/src/commands/init.test.ts packages/cli/src/commands/init.ts packages/cli/src/commands/render.ts packages/studio/src/components/editor/PropertyPanel.tsx packages/studio/src/components/editor/PropertyPanel.test.ts packages/studio/src/components/renders/useRenderQueue.ts docs/packages/core.mdx docs/packages/cli.mdxbunx oxfmt --check .gitignore packages/core/src/parsers/htmlParser.test.ts packages/core/src/studio-api/routes/render.test.ts packages/producer/src/services/renderOrchestrator.test.ts packages/cli/src/commands/init.test.ts packages/cli/src/commands/init.ts packages/cli/src/commands/render.ts packages/studio/src/components/editor/PropertyPanel.tsx packages/studio/src/components/editor/PropertyPanel.test.ts packages/studio/src/components/renders/useRenderQueue.ts docs/packages/core.mdx docs/packages/cli.mdxBrowser verification
Studio was run against the local
apple-presentationproject with manual editing, manual dragging, and inspector panels enabled:Using
agent-browser, selected theSlide Core Convictiontimeline clip and pressed Delete. The flow calledwindow.confirmwith:The cancel path did not mutate the project; Undo remained disabled after cancellation.
Local ignored proof artifacts:
qa-artifacts/next-cleanup/delete-confirmation-gate.pngqa-artifacts/next-cleanup/delete-confirmation-gate.webmNotes
I checked the current tree for the advisory's
--widthmention and did not find a tracked--widthfile on currentnext; only the trackedqa-artifacts/binaries were still present and are removed here.