refactor(choreography): drive overlays from descriptors - #947
Conversation
8c88f49 to
8d7a577
Compare
There was a problem hiding this comment.
Two P2 issues need addressing before merge:
resolveMotionLayer()does not resolve{param}placeholders in animated string endpoints, althoughAnimatableValueSchemaexplicitly permits them. Valid descriptor tracks can therefore pass literal placeholders to Motion.- The Spotlight descriptor now pins the wrapper fade easing, but the video exporter still emits
ease: 'none'. Since it is an active descriptor consumer, this leaves runtime and exported animation behavior inconsistent.
The focused adapter tests, TypeScript, ESLint, and production build otherwise pass.
|
Thanks for the review!! Working on it. |
509c97f to
ea0bf93
Compare
wyuc
left a comment
There was a problem hiding this comment.
Summary
This PR introduces a motion/react adapter for choreography descriptors, moves the in-app Spotlight and Laser overlays onto spotlight.v1 and laser.v1, pins the Spotlight wrapper fade curve, and aligns the Hyperframes Spotlight emitter with that authored easing. Public props and DOM composition remain stable, while duplicated animation constants move out of the React components.
Validation
- Manually exercised the feature in a real classroom and found no functional issue.
- All four GitHub checks are green, including lint/typecheck/unit tests, render-service tests, storage contract, and E2E.
- Ran an independent cross-vendor review against the exact PR diff at
ea0bf93ab272372a5351921f85c3127684c45a77, including a descriptor/Motion/Hyperframes multi-consumer consistency audit. - Codex found no actionable regression.
- Claude independently verified the adapter, both overlays, Motion defaults, descriptor values, prototype-sensitive handling, and runtime equivalence. Its only correctness observation was a pre-existing Laser fade difference between runtime and export; the PR does not introduce or modify that path, so it is a non-blocking follow-up rather than a finding on this change.
The previously requested placeholder resolution and Spotlight runtime/export easing alignment are addressed. I found no remaining actionable issue in the reviewed diff.
Approving this head. The PR currently conflicts with main, so the conflict resolution should preserve the newer video-export dialog and other mainline changes, followed by the focused overlay and export tests.
|
Thank you again for the thorough audit! The previous changes have resolved the issues you pointed out earlier. Since then, the project has merged several other PRs, which introduced some conflicts with this branch. I’ll take care of resolving them shortly :-) |
|
The implementation and current head have been approved after manual verification and cross-review. GitHub currently reports this PR as conflicting with After resolving the conflicts, please rerun the focused Spotlight/Laser tests and the video-export tests. I will take a quick look at the conflict-resolution diff once the branch is updated. |
|
Thanks! I’ve synced the latest The focused Spotlight/Laser and video-export tests pass. The updated head is |
wyuc
left a comment
There was a problem hiding this comment.
Rechecked the conflict-resolution diff at 12e337c. The centered video-export dialog and current mainline behavior are preserved, the merged test keeps both the descriptor easing coverage and the newer subtitle behavior, all checks are green, and no new actionable issue was found.
Summary
Drive the in-app Spotlight and Laser overlays from the existing
spotlight.v1andlaser.v1choreography descriptors through a smallmotion/reactadapter, removing duplicated animation and static-style constants from the components while preserving their runtime behavior.Related Issues
Closes #889
Changes
resolveMotionLayer()to resolve descriptor geometry, parameters, units, lifecycle phases, easing, delay, repeat, and static props into Motion targets and transitions.SpotlightOverlayandLaserOverlayto descriptor-driven layers and z-index values without changing their public props or DOM composition.dimness: 0, authored Laser colors, descriptor defaults, and the legacy unuseddurationprop.[0.25, 0.1, 0.35, 1]) in the descriptor so explicit adapter transitions remain visually equivalent to the previous no-transition component code.Type of Change
Verification
Tests & static checks
tscand Prettier clean; repo-wide ESLint 0 errors (16 pre-existing warnings).chat-storage.test.tsbaseline failure: 303 files / 2732 tests passed.Runtime before/after parity
Since this is not a pure-data refactor, I compared the rendered DOM of
mainvs this branch directly: headless Playwright drove both dev servers with an identical seeded classroom, triggering effects via direct store calls (the lecture-buffer replay path overwrites authored params — pre-existing issue, bypassed here and to be handled separately). Animated values were sampled per frame and fitted against candidate easing curves:[0.25, 0.1, 0.35, 1]on both sides (RMSE ≤ 0.0005); an easeOut fit is ~60× worse — that is exactly the drift the descriptor easing pin prevents.[0.22, 1, 0.36, 1]and its 150ms fade fits easeOut on both sides, matching the source.blackvs#000000, Tailwindtranslatevs inlinetransformwith the dot core centered on the same pixel,rounded-fullcalc(infinity*1px)vs9999px); computed rendering is identical and there are zero console errors on either side.The independent
@openmaic/rendererpackage remains out of scope, as specified in #889.Screenshots
End states, seeded classroom at 1280×800:
main)Checklist