Repository navigation
refactor(ui): use one animation library - #59025
Conversation
There was a problem hiding this comment.
Looks sound. Nothing in packages/ still imports motion. The removed animated branch in BasicTool has no callers. The shell fade keeps the same starting style and holds its end state after el.animate(). I couldn't record animations here, so for the visual side I'm relying on your before/after videos. The new motion-spring browser test (2 pass), session-ui tests (252 pass) and tsc in session-ui and ui are all green.
Public API unchanged: BasicTool's animated prop is removed, but it's internal to session-ui and nothing passed it.
One small point on the new test (inline).
| } | ||
|
|
||
| expect(state.progress()).toBe(1) | ||
| expect(seen.some((value) => value > 0 && value < 1)).toBe(true) |
There was a problem hiding this comment.
This test samples the spring with real 16 ms setTimeout sleeps, and the spring settles in about 100 ms. If CI stalls for that long before the first sample, progress() is already 1 and this some(0 < v < 1) check fails at random. The settle-to-target and <= 1 checks are the useful ones. I'd drop this mid-flight assertion, or drive the frame clock directly instead of sleeping.
There was a problem hiding this comment.
The earlier point about the flaky spring test is addressed: it now records every emitted value instead of sleeping and sampling. The useSpring options fix works. The new test fails when I put back the previous motion-spring.tsx and passes with this one. The motion-spring tests (3 pass, three runs), session-ui tests (252 pass) and tsc in ui and session-ui are green. I couldn't watch the animations themselves here, so the visual side relies on your recordings. Public API unchanged. Nothing new to raise.
3745c7d to
6ed2377
Compare
There was a problem hiding this comment.
This push is a rebase onto newer dev. The pull request's own changes match the version I last reviewed, apart from surrounding unchanged lines. The earlier point about the flaky spring test was addressed in the previous push. After the rebase, the motion-spring tests (3 pass), session-ui tests (268 pass) and tsc in ui and session-ui are green. Public API unchanged. Nothing new to raise.
What it does. The app shipped two animation libraries:
motion12 (with framer-motion and motion-dom 12) and@kitlangton/solid-motion(motion-dom 14). This removesmotionfrompackages/uiandpackages/session-uiand runs everything on solid-motion. No animation looks or feels different. Part of the dependency trim (OCD-194).How it works.
useSpring(motion-spring.tsx, used by the strikethrough) now takesattachSpring/motionValuefrom solid-motion. I checked the spring solver in both versions: for visual durations .15/.25/.3/.35 and starting velocities 0/400/-900, the output is identical sample for sample (max difference 0) and settles at the same time. Retargeting mid-flight in v14 gives the same curve as a fresh v12 spring.ShellSubmessageintool-renderer.tsx): the width spring (visualDuration .25, bounce 0) stays a spring, now on solid-motion'sanimate. The opacity and blur fade is now the browser'sel.animate()with the same 320 ms andcubic-bezier(0.16, 1, 0.3, 1). motion 12 was already running that fade as a Web Animation, so only the library call is gone.basic-tool.tsx): the height spring only ran when theanimatedprop was set, and no caller sets it. I removed that code (the spring,contentRef, theanimatedbranch and the prop) instead of porting it. Expanding and collapsing a tool works exactly as it does today, with no animation.useSpring): re-attaching the spring left the old animation running, becauseattachSpring's cleanup only unsubscribes from the source. This was also true with motion 12 on dev. It now stops the running animation and restarts toward the current target with the new options.basic-tool.tsxalso needed cleanup with no behavior change:isTriggerTitleuses a real type guard instead ofas JSX.Element, and GenericTool'slabel/argsread the input withSchema.decodeUnknownOptioninstead oftypeofchecks. On sample inputs the output is the same as before.Why solid-motion and not plain CSS. The segmented control uses
motion.spananduseMotionValue. All the springs need a solver that can retarget in flight and keep their velocity, and CSSlinear()can't do that. The tool height spring that would have become a plainel.animate()turned out to be dead code (point 3). The fade callsanimate?.()so environments without Web Animations (happy-dom) still get the final styles.Risk. UI only. If something were wrong you'd see it in the segmented control indicator, the strikethrough, or the shell command reveal. Tool expand/collapse can't change, because its spring never ran.
Proof. I recorded before (dev) and after (this branch) in Storybook on mini, stepping the page clock at exactly 60 fps and logging the animated values on every frame.
Checks:
test-browser/motion-spring.test.ts: "springs progress to its target" (records every emitted value) and "new spring options replace the running spring" (fails without the fix): 3 passbasic-tool,session-lifecycle,session-shellandtool-disclosure: 25 passsegmented-control,import-boundariesandsrc/toolstests: passbun typecheckin ui, session-ui and app: passbun script/lint-changed.ts origin/dev: 0 problems