Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<img width="571" height="174" alt="Screenshot 2026-04-01 at 20 38 16" src="https://github.com/user-attachments/assets/fe2bf5aa-cba4-4e20-ad8f-93beb6399988" />

https://github.com/user-attachments/assets/230f789a-5d51-4eec-8848-81f96c742863

Desktest is a general computer use CLI for automated end-to-end virtualised testing of desktop applications using LLM-powered agents. Spins up a disposable 🐳 Docker container (Linux), [Tart VM (macOS)](https://tart.run/), or QEMU/KVM VM (Windows) with a desktop environment, deploys any apps, and runs a computer-use agent that interacts with it based on your prompt. Built with coding agents in mind as first-class citizen users of `desktest`.

Once happy -> Convert agent trajectories to deterministic CI code
Expand Down
295 changes: 295 additions & 0 deletions dev-docs/07-05-2026-animation-scenes-code-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,295 @@
# Code Review — animation-scenes

**Date:** 07-05-2026
**Branch:** master (untracked animations/ folder)
**Scope:** All Desktest launch video animation components (Title, Run, Loop, Debug, Codify, QA, Orchestration, Closing, Launch sequencer) and recording script

**Legend:** ✅ valid (fixed) · 🟡 deferred · ❌ invalid

---

## Iteration 1 — /codex:rescue

### ❌ P2 Finding 1.1 — Run animation exit/tagline timings unreachable in composed video

- **File:** `animations/src/DesktestRunAnimation.tsx:42-45,130-132,259`
- **Severity:** P2
- **Category:** correctness

**Codex said:**
> `DesktestRunAnimation` schedules `dr-terminal-exit` and `dr-json-exit` at 3500ms, and the tagline at 4000ms, but the launch sequencer starts fading this scene out at 2700ms and removes it at 3500ms. The exit animations are effectively invisible in the composed launch video, and the tagline is unreachable.

**Relevant code:**
```css
animation: dr-fade-in 300ms ease-out 1400ms forwards,
dr-terminal-exit 1000ms ease-in-out 3500ms forwards;
/* tagline */ animation: dr-fade-in 500ms ease-out 4000ms forwards;
```

**Resolution:** Invalid. Each scene has its own CYCLE_MS for standalone Storybook viewing. The sequencer chooses when to crossfade (at 2700ms). The important content (task.json at 200ms, terminal at 1400ms, typing finishing ~2226ms) all completes within the visible 2700ms window. The exit animations and tagline are only visible in standalone mode, which is by design.

---

### ✅ P1 Finding 1.2 — Loop tagline transform overwrite breaks horizontal centering

- **File:** `animations/src/DesktestLoopAnimation.tsx:563-571,440-443`
- **Severity:** P1
- **Category:** bug

**Codex said:**
> `.dl-tagline` is positioned with `transform: translateX(-50%)`, but it animates with `dl-fade-in`, whose keyframes set `transform: translateY(-50%) scale(...)`. The `forwards` fill mode overwrites horizontal centering.

**Relevant code:**
```css
.dl-tagline {
left: 50%;
transform: translateX(-50%);
animation: dl-fade-in 500ms ease-out 9500ms forwards;
}
@keyframes dl-fade-in {
from { opacity: 0; transform: translateY(-50%) scale(0.97); }
to { opacity: 1; transform: translateY(-50%) scale(1); }
}
```

**Resolution:** The `dl-fade-in` keyframe was designed for vertically-centered elements (like the title with `top: 50%`). The tagline uses `left: 50%; transform: translateX(-50%)` for horizontal centering. After the animation fills forward, the transform becomes `translateY(-50%) scale(1)`, losing the `translateX(-50%)`. Fixed by using `dl-fade-in-flat` (opacity-only) instead.

**Diff:**
```diff
- animation: dl-fade-in 500ms ease-out 9500ms forwards;
+ animation: dl-fade-in-flat 500ms ease-out 9500ms forwards;
```

---

### 🟡 P2 Finding 1.3 — Codify reduced-motion hides .dc-main but sets children visible

- **File:** `animations/src/DesktestCodifyAnimation.tsx:1059-1085`
- **Severity:** P2
- **Category:** correctness

**Codex said:**
> The reduced-motion fallback sets child elements (`.dc-terminal`, `.dc-code-card`, etc.) to `opacity: 1`, but then sets `.dc-main { opacity: 0 !important; }`, hiding the parent and defeating child visibility.

**Relevant code:**
```css
@media (prefers-reduced-motion: reduce) {
.dc-terminal, .dc-desktop, .dc-code-card, /* ... */ {
animation: none !important; opacity: 1 !important;
}
.dc-traj-card, .dc-main {
animation: none !important; opacity: 0 !important;
}
}
```

**Resolution:** Deferred. The intent is to show the CI flow (Phase 4) as the static reduced-motion view, skipping the 4-phase animation entirely. `.dc-main` wrapping phases 1-3 is hidden so only the CI overlay is visible. The child `opacity: 1` rules are redundant but harmless. This is a design choice, not a bug — cleaning it up would be a refactor.

---

### ✅ P3 Finding 1.4 — Nested setTimeout not cleared on unmount in sequencer

- **File:** `animations/src/DesktestLaunchAnimation.tsx:48-50`
- **Severity:** P3
- **Category:** memory leak

**Codex said:**
> The nested `setTimeout(() => { setLayers(...) }, FADE_MS)` inside the outer timer is not stored or cleared. If the component unmounts during a crossfade, the orphaned timer fires `setLayers` on an unmounted component.

**Relevant code:**
```js
const timer = setTimeout(() => {
// ...
setTimeout(() => {
setLayers((prev) => prev.filter((l) => !l.exiting));
}, FADE_MS);
}, dur - FADE_MS);
return () => clearTimeout(timer);
```

**Resolution:** Stored the inner timeout in a variable and clear it in the cleanup function.

**Diff:**
```diff
- const timer = setTimeout(() => {
+ let cleanupTimer: ReturnType<typeof setTimeout>;
+ const timer = setTimeout(() => {
// ...
- setTimeout(() => {
+ cleanupTimer = setTimeout(() => {
setLayers((prev) => prev.filter((l) => !l.exiting));
}, FADE_MS);
}, dur - FADE_MS);
- return () => clearTimeout(timer);
+ return () => {
+ clearTimeout(timer);
+ clearTimeout(cleanupTimer);
+ };
```

---

### ❌ P1 Finding 1.5 — Recording script pre-roll and trim mismatch

- **File:** `animations/scripts/record-animation.mjs:9-12,28-34,49-50`
- **Severity:** P1
- **Category:** correctness

**Codex said:**
> The recorder captures from page creation and includes `goto()` plus `waitForTimeout(500)`, but does not account for the startup pre-roll. `TRIM_DURATION = "00:01:18"` is shorter than the sequencer's total scene time, so the exported MP4 starts too early and cuts off the end.

**Relevant code:**
```js
const RECORD_MS = 79_500;
const TRIM_DURATION = "00:01:18";
await page.goto(STORYBOOK_URL, { waitUntil: "networkidle" });
await page.waitForTimeout(500);
await page.waitForTimeout(RECORD_MS);
```

**Resolution:** Invalid. Total scene durations sum to exactly 79500ms. The `waitForTimeout(500)` gives Storybook a moment to settle after networkidle. The trim at 78s clips ~1.5s from the tail (end of closing scene's idle time), which is intentional. The video output has been verified correct through many recordings this session — the user has reviewed it without timing issues.

---

### 🟡 P3 Finding 1.6 — Recording script lacks try/finally for cleanup

- **File:** `animations/scripts/record-animation.mjs:18-52`
- **Severity:** P3
- **Category:** robustness

**Codex said:**
> Browser/page/context creation and ffmpeg run without `try/finally`. If any step throws, Chromium processes and temp files are left orphaned.

**Relevant code:**
```js
const browser = await chromium.launch();
// ... no try/finally wrapping close() calls
```

**Resolution:** Deferred. This is a one-shot dev tool script run manually. If it fails, the user re-runs it. Adding error handling would be nice but is out of scope for this review pass.

---

### ✅ P2 Finding 1.7 — CSS namespace collision between Closing and Codify animations

- **File:** `animations/src/DesktestClosingAnimation.tsx` and `animations/src/DesktestCodifyAnimation.tsx`
- **Severity:** P2
- **Category:** correctness

**Codex said:**
> `DesktestClosingAnimation` reuses the `dc-` class/keyframe prefix that `DesktestCodifyAnimation` also defines. Both inject global `<style>` tags. If rendered together during a scene crossfade, the later-mounted style block overrides the earlier one.

**Relevant code:**
```jsx
// DesktestClosingAnimation.tsx
<div className="dc-scene" key={cycle}>
/* dc-tagline, @keyframes dc-fade-in, etc. */

// DesktestCodifyAnimation.tsx
<div className="dc-scene" key={cycle}>
/* dc-tagline, @keyframes dc-fade-in, etc. */
```

**Resolution:** Renamed all CSS classes and keyframes in DesktestClosingAnimation from `dc-` to `dcl-` prefix. Although the two scenes are currently never in the DOM simultaneously (separated by QA and Orchestration), the collision is fragile and would break if scenes are reordered.

**Diff:**
```diff
- <div className="dc-scene" key={cycle}>
+ <div className="dcl-scene" key={cycle}>
/* all dc-* → dcl-* throughout the file */
```

---

## Iteration 2 — /codex:rescue (verification pass)

### ✅ P1 Finding 2.1 — Cleanup timer fix introduced regression: exiting layers never removed

- **File:** `animations/src/DesktestLaunchAnimation.tsx:39-57`
- **Severity:** P1
- **Category:** bug (regression)

**Codex said:**
> When `setLayers` adds the next active scene, React detects that `active.index`/`active.key` changed, tears down the previous effect, and calls `clearTimeout(cleanupTimer)`. But `cleanupTimer` is the only code path that removes exiting layers — cancelling it means exiting layers are never cleaned up.

**Relevant code:**
```js
let cleanupTimer;
const timer = setTimeout(() => {
setLayers(/* add new scene, mark old as exiting */);
cleanupTimer = setTimeout(() => {
setLayers((prev) => prev.filter((l) => !l.exiting));
}, FADE_MS);
}, dur - FADE_MS);
return () => { clearTimeout(timer); clearTimeout(cleanupTimer); };
```

**Resolution:** Reverted to the original approach (untracked inner setTimeout). The inner timer fires in 800ms — well before any unmount. The original "leak" from Finding 1.4 is a P3 harmless warning in a one-shot video component, but the regression from tracking it is P1 (broken scene transitions). Original code is correct.

**Diff:**
```diff
- let cleanupTimer: ReturnType<typeof setTimeout>;
const timer = setTimeout(() => {
// ...
- cleanupTimer = setTimeout(() => {
+ setTimeout(() => {
setLayers((prev) => prev.filter((l) => !l.exiting));
}, FADE_MS);
}, dur - FADE_MS);
- return () => {
- clearTimeout(timer);
- clearTimeout(cleanupTimer);
- };
+ return () => clearTimeout(timer);
```

---

Fixes 1 and 3 from iteration 1 verified correct. No new P0/P1/P2 findings beyond the regression (now reverted).

---

## Iteration 3 — /codex:rescue (final verification)

Clean pass. Both remaining fixes (Loop tagline animation, Closing namespace rename) verified correct. Full scan of all animation source files produced zero new P0/P1/P2 findings.

---

## Summary

**Iterations run:** 3 (initial review → regression fix → clean verification)
**Stop reason:** Clean verification pass with no new P0-P2 findings.

**Findings by severity and status:**

| Severity | ✅ Fixed | 🟡 Deferred | ❌ Invalid | Total |
|----------|---------|------------|----------|-------|
| P0 | 0 | 0 | 0 | 0 |
| P1 | 2 | 0 | 1 | 3 |
| P2 | 1 | 2 | 1 | 4 |
| P3 | 0 | 1 | 0 | 1 |
| **Total**| **3** | **3** | **2** | **8** |

**Fixed (✅):**
- P1: Loop tagline transform overwrite (dl-fade-in → dl-fade-in-flat)
- P1: Iteration 2 regression — cleanup timer cancelling exiting layer removal (reverted)
- P2: CSS namespace collision between Closing and Codify (dc- → dcl-)

**Deferred (🟡):**
- P2: Codify reduced-motion hides .dc-main but sets children visible (design choice for showing CI phase as static view)
- P3: Recording script lacks try/finally (one-shot dev tool)
- P3: Nested setTimeout not tracked in sequencer (harmless in one-shot video component)

**Invalid (❌):**
- P1: Recording script pre-roll/trim mismatch (works correctly in practice)
- P2: Run animation exit/tagline unreachable in composed video (by design for standalone viewing)

**Open P0-P2 issues:** 1 deferred P2 (Codify reduced-motion parent opacity) — cosmetic for accessibility edge case, not blocking.

**Files modified:**
- `animations/src/DesktestLoopAnimation.tsx` — tagline animation fix
- `animations/src/DesktestClosingAnimation.tsx` — CSS namespace rename dc- → dcl-

**Uncommitted state:** All changes are unstaged in the working tree. Nothing committed.

**Review log:** `dev-docs/07-05-2026-animation-scenes-code-review.md`
3 changes: 3 additions & 0 deletions media/animations/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
node_modules/
recordings/*.webm
Comment thread
Miyamura80 marked this conversation as resolved.
recordings/*.mp4
9 changes: 9 additions & 0 deletions media/animations/.storybook/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type { StorybookConfig } from "@storybook/react-vite";

const config: StorybookConfig = {
stories: ["../src/**/*.stories.@(ts|tsx)"],
addons: ["@storybook/addon-essentials"],
framework: "@storybook/react-vite",
};

export default config;
6 changes: 6 additions & 0 deletions media/animations/.storybook/preview-head.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<link rel="preconnect" href="https://fonts.googleapis.com" />
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
<link
href="https://fonts.googleapis.com/css2?family=Archivo:wght@400;700&family=JetBrains+Mono:wght@400;700&display=swap"
rel="stylesheet"
/>
9 changes: 9 additions & 0 deletions media/animations/.storybook/preview.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type { Preview } from "@storybook/react";

const preview: Preview = {
parameters: {
layout: "fullscreen",
},
};

export default preview;
Loading
Loading