Repository navigation
feat(vscode): add a motion budget for the chat animations - #15361
gouravkargwal wants to merge 9 commits into
Conversation
github-actions
Bot
added
area:ui
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| "full" the default, no rule matches | ||
| "reduced" looping animations play once and hold their end state | ||
| "paused" whatever is running stops where it is and resumes later | ||
| "off" nothing animates and nothing transitions |
There was a problem hiding this comment.
SUGGESTION: off/paused only stop CSS animations, not the JS-driven motion
These selectors only match CSS animations and transitions, but the chat also plays Web Animations API animations that this stylesheet cannot reach: the tool-row height glide (packages/kilo-ui/src/components/tool-motion.ts:147), the dock pill flight (packages/kilo-vscode/webview-ui/src/components/chat/SessionDock.tsx:420), and the todo chip animations (packages/kilo-vscode/webview-ui/src/components/chat/todo/TodoChip.tsx). They keep running with data-animations="off" or "paused", so the contract above ("nothing animates", "stops where it is") only holds for CSS-driven motion. useReducedMotion also still reads prefers-reduced-motion alone, so the new budget never reaches those code paths.
If the budget is meant to be authoritative, consider having applyAnimations expose the resolved mode to useReducedMotion (or check it at those call sites); otherwise the comment and the "Turn off chat animations entirely" description could be qualified. Non-blocking.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
The incremental commit resolves the previous Previous Review Summaries (4 snapshots, latest commit 63d79a4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 63d79a4)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
The incremental commit replaces the source-text grep with real happy-dom assertions: Fix these issues in Kilo Cloud Previous review (commit 6a6ec96)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
The incremental commit resolves the earlier warning: Fix these issues in Kilo Cloud Previous review (commit 6ede287)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
The new Fix these issues in Kilo Cloud Previous review (commit 7e42d5b)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (11 files)
No memory or listener leaks found in the new code: the Reviewed by deepseek-flash-latest · Input: 91.2K · Output: 3.3K · Cached: 259.8K Review guidance: REVIEW.md from base branch |
|
Good catch, and it was the right thing to point at: the contract as written only reached the CSS. Fixed in
So the four One deliberate boundary: "reduced" still allows the one-shot script-driven motion, since the budget only shortens the looping animations. "paused" and "off" stop everything. |
| const mode = resolveAnimations(user, focused, document.visibilityState === "hidden") | ||
| if (mode === "full") delete root.dataset.animations | ||
| else root.dataset.animations = mode | ||
| document.body?.classList.toggle("vscode-reduce-motion", reduceMotion(mode)) |
There was a problem hiding this comment.
WARNING: Toggling vscode-reduce-motion can remove the host's own reduce-motion marker
Nothing in this extension writes this class; the webview only reads it as a reduce-motion escape hatch (SessionDock.tsx:412, TodoChip.tsx:50, todo-dock.css:444, prompt-input.css:740), which points to it being the class VS Code puts on webview bodies when the user has reduced motion enabled.
classList.toggle(..., false) removes it on the first applyAnimations call (the default budget is full) and whenever the budget resolves to full/reduced, so a user who enabled reduced motion in VS Code loses it on exactly the JS paths that check the class.
Consider a Kilo-owned class (for example kilo-reduce-motion) added alongside the existing checks, or at least avoid removing a class the host may own. Non-blocking, but it is an accessibility regression for those users.
|
Confirmed, and it was a real regression rather than a theoretical one: VS Code's webview bootstrap ( Fixed in
The budget still stops everything it stopped before; it just stops it with a class Kilo owns. |
| // Writing on it would take that setting away from the user. | ||
| it("is only read, never written", () => { | ||
| const source = fs.readFileSync(path.join(ROOT, "webview-ui/src/animations.ts"), "utf-8") | ||
| expect(source).not.toContain('toggle("vscode-reduce-motion"') |
There was a problem hiding this comment.
[SUGGESTION]: Assert the behaviour instead of the source text
This guard greps animations.ts for literal strings, so it still passes if the host class is written differently (single quotes, a variable, or a helper in another file) and fails on unrelated refactors, while never checking the actual outcome. The package already has a happy-dom harness (tests/unit/diff-keyboard-focus.test.ts:20), so you could install document/body, call applyAnimations("off", true) and applyAnimations("paused", true), and assert that kilo-reduce-motion is toggled on while a pre-existing vscode-reduce-motion class is left intact. That covers the regression this commit fixes without pinning the file's text. Non-blocking.
|
Right, the source-text guard was the wrong tool. Replaced it with the happy-dom behaviour test in The test now installs a DOM, puts the host's
I checked that it is a real guard: putting the old |
| import { applyAnimations, reduceMotion, resolveAnimations } from "../../webview-ui/src/animations" | ||
|
|
||
| /** Install the happy-dom globals the webview code relies on, and return a restore function. */ | ||
| function useDom(window: Window, names: string[]) { |
There was a problem hiding this comment.
[SUGGESTION]: useDom is a verbatim copy of an existing test helper
This helper, including the doc comment, is identical to useDom in tests/unit/diff-keyboard-focus.test.ts:21. Moving it into a shared test utility (for example tests/unit/happy-dom.ts) keeps the global install/restore logic in one place, so a future change to how happy-dom globals are installed or torn down doesn't need to be made twice. Non-blocking.
|
Agreed, the helper belongs in one place. Done in
The fixture naming follows the |
Why
The chat animates while a session works: the status shimmer, the spinner, the tool icon breathe, the agent avatar pulse. Each of those ticks on the main thread, so the webview renderer keeps working at the display refresh rate for as long as the session runs. On a laptop that is the difference between a quiet machine and a hot one, and it gets worse the more chat windows are open.
#14792 measures this on Windows and on a 144 Hz display. The same cost shows up on macOS: with one window running a single subagent, the renderer for that window sat at 0.6-0.7 cores and
kilo serveat roughly 15% of a core, while the rest of the machine was idle.The shimmer is a
background-positionanimation on abackground-clip: textelement, so it cannot be composited and it repaints on every frame. The other busy indicators animate opacity and transform, which are cheap on their own, but there are always a few of them running and each one keeps the animation tick alive.What changed
Motion is now a budget instead of a fixed behaviour.
packages/kilo-ui/src/styles/motion.cssdefines the contract. A client writesdata-animationson the root element and the stylesheet reacts:full- the default, no rule matchesreduced- looping animations play once and hold their end statepaused- whatever is running stops where it is and resumes lateroff- nothing animates and nothing transitionskilo-code.new.uiAnimationssetting picksfull,reduced, oroff, and the window state is reported alongside it: a chat in a window that is not focused is paused, because the user is not looking at it.The values are picked so that components which clean up in an
animationendortransitionendhandler still get their event: a shortened animation still ends, and a paused animation resumes and fires its event once the motion is allowed again. Nothing is left half revealed.The budget is one source of truth
The stylesheet cannot reach the Web Animations API, so the budget does not stop there:
useReducedMotioninpackages/kilo-ui/src/hooks/use-reduced-motion.tsnow honours the budget next to the OS setting, so the script-driven motion intool-motion.ts,grow-box.tsx, androlling-results.tsxstops with it. The hook reads the same root attribute the stylesheet reads, so the two can never disagree.SessionDockandTodoChipmotion read. Kilo sets its ownkilo-reduce-motionmarker for the budget and the existing checks read both markers, becausevscode-reduce-motionbelongs to VS Code: the host puts that class on the webview body when the user asked for reduced motion and removes it again on every style change, so writing on it would take the user's setting away.Scope
This covers the chat webview (the sidebar and the editor panels), which is where the session status is animated. The Agent Manager has its own webview and its own message contract, so it is not included here.
The server side is not touched:
kilo serveused roughly 15% of a core during a session, which is a separate cost from the renderer.Verification
bun run typecheckinpackages/kilo-vscode(host and webview)bun typecheck --filter='!@kilocode/kilo-jetbrains'from the repo root, which is what the pre-push gate runsbun run lint,bun run knipinpackages/kilo-vscodebun test tests/unit/chat-animations.test.tsbun esbuild.jsbuilds the webview, and the new rules are present in the emitted stylesheetThe JetBrains half of the pre-push gate was not run locally: it needs a Java 21 toolchain and pulls about 14 GB of Gradle caches, and this change touches no Kotlin.