Hub motion tokens + reduced-motion pin — epic #499 child 1 (#512)

[richard-devbot]

Closes #512 · first implementation child of epic #499 (unblocked by #495/PR #504).

What

1. Motion tokens on :root — --motion-fast/.16s, --motion-base/.22s, --motion-slow/.4s, --motion-draw/.9s, --motion-ease, --motion-ease-emphatic — and all eight existing transition sites migrated to them. One tuned scale for every later motion child. The two 0.15s sites normalize to the fast tier (10ms delta, under a 60fps frame); otherwise zero visual change — the full browser suite (now 14 journeys) is the parity net.
2. Motion budget documented in the stylesheet header: motion exists to make an observed state change legible; decorative loops on quiet data are prohibited; the one sanctioned continuous motion is a pulse indicating an observed ongoing state (live dot, blocked gate).
3. Reduced-motion guard pinned — the global prefers-reduced-motion block already existed (readiness wave; honest scope note on Hub motion tokens + global reduced-motion policy (epic #499, child 1) #512): it's now locked by a source test and a real-browser test with reducedMotion: 'reduce' emulation proving transitions AND animations collapse to ≤0.01ms and infinite pulses stop looping.

TDD

Verified failing first: token/budget/migration tests RED on the old stylesheet (the guard pin correctly passed — it existed). Browser: tokens resolving via getComputedStyle + driving the nav's real transition durations; reduced-motion collapse.

Verification

Core 1818/1818 · browser 14/14 (all merged waves' tests together) · lint 0 · typecheck 0 · validate 196 · security green · whitespace clean.

Zero-dep per the epic's recommended posture — the motion-package and Rive decisions stay open for the micro-interaction children.

Merging after green checks + reviewer bodies read, per the current working protocol.

🤖 Generated with Claude Code

[strix-security]

Strix Security Review

No security issues found.

_{Updated for 5e1f45f.}

---

Reviewed by Strix
_{Re-run review · Configure security review settings}

[coderabbitai]

Warning

Review limit reached

@richard-devbot, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 33 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ad4ccead-7d36-49e9-9b4b-80ce8260aa6c

📥 Commits

Reviewing files that changed from the base of the PR and between 844cb8c and 7cf9e5a.

📒 Files selected for processing (3)

• src/observability/dashboard/ui/styles.js
• tests/browser/dashboard-512-motion.test.js
• tests/hub-motion-tokens-512.test.js

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Dashboard motion tokens + pinned reduced-motion coverage

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add global CSS motion tokens and a written motion budget policy for the Hub dashboard.
• Migrate all existing transition timings/easings to the new --motion-* tokens for consistency.
• Pin prefers-reduced-motion behavior with source and real-browser tests to prevent regressions.

Diagram

graph TD
  A["Dashboard styles (styles.js)"] --> B["Motion tokens (:root)"] --> C["Transition sites (nav/cards/charts)"]
  A --> D["Reduced-motion guard (@media reduce)"]
  E["Source tests (hub-motion-tokens-512)"] --> A
  F["Browser tests (dashboard-512-motion)"] --> G["Dashboard server (server.js)"] --> A
  F --> H["Chromium (playwright-core)"]
  D --> F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt a shared design-token package for motion

• ➕ Enforces consistency across multiple surfaces/apps automatically
• ➕ Allows versioned motion scale changes with clear upgrade paths
• ➖ Introduces dependency/versioning overhead the epic currently avoids
• ➖ Harder to iterate quickly on token values during early rollout

2. Drive motion settings from JS (feature flag / runtime preferences)

• ➕ Can support per-user preferences beyond prefers-reduced-motion
• ➕ Makes A/B testing or gradual rollout straightforward
• ➖ Adds runtime complexity and more moving parts than CSS-only tokens
• ➖ Risk of divergence between JS state and CSS behavior if not tightly tested

Recommendation: The PR’s CSS-only tokenization + explicit reduced-motion pinning is the right first child for the epic: it centralizes timings without adding dependencies and immediately locks accessibility behavior with both unit and real-browser assertions. A shared token package can be revisited once more motion children land and cross-surface reuse becomes concrete.

Files changed (3) +202 / -8

Enhancement (1) +22 / -8

styles.jsIntroduce --motion-* tokens and migrate all transitions +22/-8

Introduce --motion-* tokens and migrate all transitions

• Adds a motion budget policy comment and defines motion duration/easing tokens on :root. Replaces literal transition timings/easings across the dashboard UI with var(--motion-*) tokens (including normalizing 0.15s sites to the fast tier).

src/observability/dashboard/ui/styles.js

Tests (2) +180 / -0

dashboard-512-motion.test.jsAdd Playwright browser checks for token resolution and reduced motion +133/-0

Add Playwright browser checks for token resolution and reduced motion

• Introduces a real-Chromium harness that boots the dashboard server on an ephemeral port and validates --motion-* token values via getComputedStyle. Verifies prefers-reduced-motion emulation collapses transitions/animations to ~0 and stops infinite pulses by forcing a single iteration.

tests/browser/dashboard-512-motion.test.js

hub-motion-tokens-512.test.jsPin motion token policy and forbid literal transition durations +47/-0

Pin motion token policy and forbid literal transition durations

• Adds source-level assertions that motion tokens exist on :root and that the stylesheet documents the motion budget. Guards against regressions by failing if any transition declarations contain literal duration values and by asserting the reduced-motion guard remains present.

tests/hub-motion-tokens-512.test.js

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (2)

Context used

✅ Compliance rules (platform): 300 rules

✅ Skills: 17 invoked

  code-review-pr

  claude-api

  documentation-writing

  pptx

  docx

  performance-monitoring

  security-compliance

  cso

  plan-eng-review

  design-review

  prompt-engineering

  mcp-builder

  qa-testing

  code-patterns

  xlsx

  security-owasp

  testing-qa

1. Duration unit mismatch ✓ Resolved 🐞 Bug ≡ Correctness

Description

The reduced-motion browser test parses transition/animation durations with parseFloat() and compares
them as seconds, but the stylesheet sets reduced-motion durations in milliseconds (".01ms"), which
can make the assertion incorrect if the computed style preserves the "ms" unit.

Code

tests/browser/dashboard-512-motion.test.js[R113-132]

+browserTest('reduced motion collapses every transition and animation (#512)', { reducedMotion: 'reduce' }, async ({ page }) => {
+  const collapsed = await page.evaluate(() => {
+    const nav = document.querySelector('nav button');
+    const probe = document.createElement('div');
+    probe.style.animation = 'gantt-pulse 1.6s ease-in-out infinite';
+    document.body.appendChild(probe);
+    const probeStyle = getComputedStyle(probe);
+    const result = {
+      navDuration: nav ? parseFloat(getComputedStyle(nav).transitionDuration) : null,
+      animationDuration: parseFloat(probeStyle.animationDuration),
+      iterations: probeStyle.animationIterationCount,
+    };
+    probe.remove();
+    return result;
+  });
+  assert.ok(collapsed.navDuration !== null && collapsed.navDuration < 0.001,
+    `transitions collapse under reduced motion — got ${collapsed.navDuration}s`);
+  assert.ok(collapsed.animationDuration < 0.001,
+    `animations collapse under reduced motion — got ${collapsed.animationDuration}s`);
+  assert.equal(collapsed.iterations, '1', 'infinite pulses stop looping');

Relevance

●● Moderate

No historical evidence on ms-vs-s computedStyle duration parsing expectations in Playwright tests.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The stylesheet’s reduced-motion guard sets durations using an ms unit, while the test compares the
parseFloat() output against a seconds-based threshold, which is only correct if the computed style
string is guaranteed to be serialized in seconds.

src/observability/dashboard/ui/styles.js[1703-1709]
tests/browser/dashboard-512-motion.test.js[113-132]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The reduced-motion test treats parsed durations as seconds, but `getComputedStyle(...).transitionDuration` / `animationDuration` may serialize in `ms` (and the CSS explicitly sets `.01ms`). Using `parseFloat()` without unit conversion can mis-evaluate the value (e.g., `0.01ms` becomes `0.01`), breaking the `< 0.001` seconds assertion.

### Issue Context
The reduced-motion CSS guard sets `animation-duration` and `transition-duration` to `.01ms !important`, while the browser test asserts the numeric value is `< 0.001` (seconds).

### Fix Focus Areas
- tests/browser/dashboard-512-motion.test.js[113-132]
- src/observability/dashboard/ui/styles.js[1703-1709]

### Suggested change
In the test, parse durations with unit awareness, e.g.:
- read the raw string
- split by commas (take max or first)
- convert `ms` to seconds (`/1000`)
- then compare in seconds

Alternatively, assert on the raw string matching either `0.01ms` or an equivalent `0.00001s`/`0s` representation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unhandled spawn error event ✓ Resolved 🐞 Bug ☼ Reliability

Description

startServer() doesn’t register a child_process 'error' handler, so a spawn failure can raise an
unhandled 'error' event and crash the test process instead of rejecting the Promise cleanly.

Code

tests/browser/dashboard-512-motion.test.js[R34-67]

+function startServer(root) {
+  return new Promise((resolvePromise, rejectPromise) => {
+    const child = spawn(process.execPath, [SERVER_PATH, '--port', '0', '--no-browser', '--project', root], {
+      cwd: root,
+      env: {
+        ...process.env,
+        RSTACK_APPROVAL_TOKEN: undefined,
+        RSTACK_DASHBOARD_READ_TOKEN: undefined,
+        RSTACK_TLS_CERT: undefined,
+        RSTACK_TLS_KEY: undefined,
+        RSTACK_BUSINESS_PORT: undefined,
+        RSTACK_PROJECT_ROOT: undefined,
+        RSTACK_NO_BROWSER: '1',
+        RSTACK_REGISTRY_DIR: join(root, '.registry'),
+      },
+      stdio: ['ignore', 'pipe', 'pipe'],
+    });
+    let out = '';
+    let settled = false;
+    const timer = setTimeout(() => {
+      if (!settled) { settled = true; child.kill('SIGKILL'); rejectPromise(new Error(`server did not boot\n${out}`)); }
+    }, SERVER_BOOT_TIMEOUT_MS);
+    child.stdout.on('data', (chunk) => {
+      out += chunk;
+      const match = out.match(/Dashboard: http:\/\/localhost:(\d+)/);
+      if (match && !settled) {
+        settled = true;
+        clearTimeout(timer);
+        resolvePromise({ baseUrl: `http://127.0.0.1:${match[1]}`, stop: () => child.kill('SIGKILL') });
+      }
+    });
+    child.stderr.on('data', (chunk) => { out += chunk; });
+    child.on('exit', () => { if (!settled) { settled = true; clearTimeout(timer); rejectPromise(new Error(`server exited\n${out}`)); } });
+  });

Relevance

●● Moderate

No clear historical evidence about requiring child_process 'error' handlers in Promise-based spawn
helpers.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper omits a child 'error' listener, while other repo tests explicitly add one when using
spawn inside a Promise, demonstrating the expected defensive pattern.

tests/browser/dashboard-512-motion.test.js[34-67]
tests/context-cli.test.js[29-42]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `startServer()` helper wraps `spawn()` in a Promise but only listens for stdout/stderr/exit. If `spawn()` fails (e.g., path/permissions), Node emits an `error` event on the ChildProcess; without a listener, this can crash the test run.

### Issue Context
Other Promise-wrapped spawn helpers in the repo attach `child.on('error', rejectPromise)` (often also clearing timeouts) to avoid unhandled exceptions.

### Fix Focus Areas
- tests/browser/dashboard-512-motion.test.js[34-67]
- tests/context-cli.test.js[29-42]

### Suggested change
Add a `child.on('error', (err) => { ... })` handler that:
- checks/sets `settled`
- clears the boot timeout
- rejects the Promise with the error plus buffered output (if any)
This mirrors existing patterns in other tests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. E2E test lacks mocks 📜 Skill insight ▣ Testability

Description

The newly added browser test directly uses external dependencies (filesystem temp dirs, spawning a
real server process, and launching Playwright Chromium) instead of mocking/stubbing them. This
violates the requirement that tests mock external dependencies, and can make the test suite flaky
and environment-dependent.

Code

tests/browser/dashboard-512-motion.test.js[R15-37]

+import { spawn } from 'node:child_process';
+import { mkdtempSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join, dirname } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import { createRequire } from 'node:module';
+
+import { fixtureReadyRun } from '../helpers/dashboard-fixtures.js';
+
+const require = createRequire(import.meta.url);
+const HERE = dirname(fileURLToPath(import.meta.url));
+const SERVER_PATH = join(HERE, '..', '..', 'src', 'observability', 'dashboard', 'server.js');
+
+let chromium = null;
+try { ({ chromium } = require('playwright-core')); } catch { /* dep absent */ }
+
+// Named per the repo's magic-number convention (PR #490).
+const SERVER_BOOT_TIMEOUT_MS = 15_000;
+
+function startServer(root) {
+  return new Promise((resolvePromise, rejectPromise) => {
+    const child = spawn(process.execPath, [SERVER_PATH, '--port', '0', '--no-browser', '--project', root], {
+      cwd: root,

Relevance

● Weak

Team ships real Playwright browser tests that spawn server/temp dirs; mocking FS/process was
rejected (#511).

PR-#360
PR-#504
PR-#511

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1400146 requires tests to mock external dependencies rather than using real
filesystem/process execution. The added test imports and uses spawn, mkdtempSync, and rmSync
and starts a real server process and browser context.

tests/browser/dashboard-512-motion.test.js[15-17]
tests/browser/dashboard-512-motion.test.js[34-37]
tests/browser/dashboard-512-motion.test.js[80-93]
Skill: qa-testing

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tests/browser/dashboard-512-motion.test.js` performs real filesystem operations and spawns a real server process / real browser instead of mocking those dependencies.

## Issue Context
The compliance rule requires tests to mock external dependencies (explicitly including filesystem operations). This test currently imports and uses `node:fs` (`mkdtempSync`, `rmSync`) and `node:child_process` (`spawn`), and launches Playwright Chromium.

## Fix Focus Areas
- tests/browser/dashboard-512-motion.test.js[15-17]
- tests/browser/dashboard-512-motion.test.js[34-37]
- tests/browser/dashboard-512-motion.test.js[80-93]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Transitions not transform/opacity-only 📜 Skill insight ➹ Performance

Description

The updated CSS transitions animate properties beyond transform/opacity (including color,
background, box-shadow, right, stroke-dasharray, and width). This violates the requirement
that animations/transitions be limited to transform/opacity and can trigger unnecessary
layout/paint work, causing performance issues.

Code

src/observability/dashboard/ui/styles.js[97]

+  transition: color var(--motion-fast) var(--motion-ease), background var(--motion-fast) var(--motion-ease), border-color var(--motion-fast) var(--motion-ease), box-shadow var(--motion-fast) var(--motion-ease);

Relevance

● Weak

Repo already transitions color/box-shadow/right/width; no evidence enforcing transform/opacity-only
(e.g., styles work in #312).

PR-#312
PR-#427

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance IDs 1400011 and 1399699 restrict animated/transitioned properties to transform and
opacity only. In src/observability/dashboard/ui/styles.js, the modified transitions include
multiple non-compliant properties—such as color/background/border-color/box-shadow (paint-heavy)
and right/width (explicitly layout-triggering per the rule), along with
stroke-dasharray—demonstrating that the stylesheet changes violate the stated compliance
constraint.

src/observability/dashboard/ui/styles.js[97-97]
src/observability/dashboard/ui/styles.js[1105-1105]
src/observability/dashboard/ui/styles.js[1459-1459]
src/observability/dashboard/ui/styles.js[1471-1471]
src/observability/dashboard/ui/styles.js[1437-1437]
Skill: performance-monitoring
Skill: design-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
CSS transitions in `src/observability/dashboard/ui/styles.js` animate properties beyond `transform` and `opacity` (including layout/paint-triggering ones like `right` and `width`, plus paint-heavy properties like `background`/`box-shadow`, and SVG-related properties like `stroke-dasharray`). Compliance requires that transitions/animations be limited to `transform` and `opacity` only.

## Issue Context
These transitions were modified as part of the migration to motion tokens, so they are in-scope for compliance. Current examples include animating `right` for the drawer, `width` for bar fills, and `stroke-dasharray` for SVG arcs; animating layout/paint-heavy properties can degrade performance.

## Fix Focus Areas
- src/observability/dashboard/ui/styles.js[97-97]
- src/observability/dashboard/ui/styles.js[1105-1105]
- src/observability/dashboard/ui/styles.js[1437-1437]
- src/observability/dashboard/ui/styles.js[1459-1459]
- src/observability/dashboard/ui/styles.js[1471-1471]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[richard-devbot]

Both Qodo bugs addressed in the follow-up commit: (1) reduced-motion duration parsing is unit-aware (ms→s) instead of assuming seconds serialization — Chromium serializes seconds (the test was green), but the assertion shouldn't be engine-dependent; (2) startServer now registers a child error handler and rejects cleanly. The same spawn-handler gap exists in every copied browser harness — a shared tests/browser helper extraction is the durable fix and follows immediately as its own small PR under #502. Motion tests 2/2, lint clean. — Claude Main