Hub state-driven micro-interactions — epic #499 child 2 (#516) - #520
Merged
Conversation
Zero-dep motion on the two foundations: the #495 morph patches attribute flips on STABLE nodes (so a class change is an animatable property change) and every duration routes through the #512 tokens. - Status pills transition background/color/border at the fast tier — a task flipping PASS→FAIL visibly changes instead of teleporting; the morph's attribute patch IS the trigger, no JS. - Feed rows carry keyed identity (data-key from event identity), so the morph reuses existing rows and inserts only genuinely-new events as new nodes; @starting-style animates the arrival (engines without it simply skip the entry animation). Pinned: existing rows keep identity across quiet ticks AND across a new event's arrival. - KPI numbers count up through animateCount: only when the value actually changed, instant under prefers-reduced-motion (JS motion checks the media query itself), instant on hidden surfaces (throttled rAF must never strand a stale number — found live in an occluded pane during verification), a settle timeout lands the exact final value even if rAF freezes mid-animation, and a newer value cancels the in-flight count. Wired to the sidebar mini rollup and the command KPI grid. - First-load placeholders are quiet skeleton shimmers in the command attention/feed panels — bound to the no-snapshot-yet state, replaced by the first render, frozen by the reduced-motion guard per the motion budget. Browser-pinned on the shared #515 harness: pill transition wiring, keyed feed identity across ticks and arrivals, reduced-motion snap, motion-on settle to the exact target (live ticks held via a neutralized snapshot handler so injected state can't race server truth), and skeletons via a route-held first snapshot. 1825/0 core, 19/19 browser. Closes #516. Part of epic #499. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
Contributor
📝 WalkthroughWalkthroughThe dashboard now animates KPI changes, preserves keyed feed rows during updates, transitions state changes, and displays skeleton placeholders until initial data loads. Reduced-motion handling applies to JavaScript count-ups and CSS motion. ChangesDashboard micro-interactions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Snapshot
participant DashboardPage
participant animateCount
participant KPIElement
Snapshot->>DashboardPage: deliver updated KPI values
DashboardPage->>animateCount: animate changed values
animateCount->>KPIElement: update displayed count
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
PR Summary by QodoDashboard micro-interactions: KPI count-up, keyed feed entries, skeleton first-load
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+88
to
+90
| await page.waitForFunction(() => document.getElementById('side-runs').textContent === '37', null, { timeout: 3000 }); | ||
| assert.ok(true, 'the count settles on the exact target'); | ||
| }); |
There was a problem hiding this comment.
1. Useless assert.ok(true) assertion 📜 Skill insight ▣ Testability
The new browser test includes assert.ok(true, ...), which does not verify any behavior and can mask regressions by always passing. Assertions should validate specific outputs or state changes rather than mere existence/truthiness placeholders.
Agent Prompt
## Issue description
A test assertion always passes (`assert.ok(true, ...)`) and does not check behavior.
## Issue Context
The test already waits for the DOM to reach the expected value; the final assertion should validate the observed value (e.g., read `#side-runs` and assert it equals `37`) or be removed in favor of a meaningful assertion.
## Fix Focus Areas
- tests/browser/dashboard-516-micro.test.js[88-90]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1017
to
+1025
| .skeleton-block { | ||
| height: 14px; | ||
| border-radius: 6px; | ||
| background: linear-gradient(90deg, var(--soft) 25%, var(--line) 50%, var(--soft) 75%); | ||
| background-size: 200% 100%; | ||
| animation: skeleton-shimmer 1.2s var(--motion-ease) infinite; | ||
| margin: 10px 0; | ||
| } | ||
| @keyframes skeleton-shimmer { from { background-position: 200% 0; } to { background-position: -200% 0; } } |
There was a problem hiding this comment.
3. skeleton-shimmer animates background-position 📜 Skill insight ➹ Performance
The new skeleton shimmer animation for .skeleton-block animates background-position, which violates the compliance requirement that CSS animations/transitions only animate transform and/or opacity. This introduces a disallowed animation property and can cause unnecessary paint work during loading states.
Agent Prompt
## Issue description
The skeleton placeholder shimmer currently animates `background-position` via `@keyframes`, but compliance rules require animations/transitions to be limited to `transform` and/or `opacity`.
## Issue Context
Skeleton placeholders were added for the "no snapshot yet" first-load state in #516, and the `.skeleton-block` shimmer is used as a first-load placeholder animation. To stay compliant while preserving a shimmer-like effect, prefer patterns such as using a pseudo-element with a gradient and animating its `transform: translateX(...)` across the block (or switching to an opacity pulse animation).
## Fix Focus Areas
- src/observability/dashboard/ui/styles.js[1017-1025]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| background: linear-gradient(90deg, var(--soft) 25%, var(--line) 50%, var(--soft) 75%); | ||
| background-size: 200% 100%; | ||
| animation: skeleton-shimmer 1.2s var(--motion-ease) infinite; | ||
| margin: 10px 0; |
There was a problem hiding this comment.
4. skeleton-block uses 10px margin 📜 Skill insight ⚙ Maintainability
The new .skeleton-block introduces margin: 10px 0, which does not follow a consistent 4px/8px spacing scale. This can erode design-system consistency over time.
Agent Prompt
## Issue description
A new spacing value `margin: 10px 0` was introduced, which violates the project's consistent 4px/8px spacing scale requirement.
## Issue Context
This margin is part of the new skeleton placeholder styling added in #516.
## Fix Focus Areas
- src/observability/dashboard/ui/styles.js[1023-1023]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // #516: keyed identity — the morph reuses existing rows and inserts only | ||
| // genuinely-new events as new nodes (which @starting-style animates in). | ||
| var feedKey = [item.ts, item.type, item.runId, item.summary].filter(Boolean).join('|'); | ||
| return '<div class="feed-row" data-key="' + esc(feedKey) + '"><div class="feed-icon ' + esc(level) + '">' + icon + '</div><div><div class="feed-summary">' + esc(item.summary || '') + '</div><div class="feed-meta">' + (item.runId ? '<span>' + esc(item.runId.slice(-14)) + '</span>' : '') + (item.projectRoot ? '<span>' + esc(shortName(item.projectRoot)) + '</span>' : '') + (item.type ? '<span>' + esc(item.type) + '</span>' : '') + '</div></div><div class="feed-ts">' + timeHtml(item.ts) + '</div></div>'; |
There was a problem hiding this comment.
6. runid.slice(-14) magic length 📜 Skill insight ⚙ Maintainability
The feed row rendering hardcodes the tail length 14 when truncating runId, which is a magic number embedded in formatting logic. This violates the requirement to replace such literals with named constants for clarity and consistency.
Agent Prompt
## Issue description
`feedRowHtml()` truncates `runId` with `slice(-14)`, embedding `14` as a magic number. Replace it with a named constant (e.g., `RUN_ID_TAIL_CHARS`) to document intent.
## Issue Context
This line was modified in the PR (new keyed identity + re-rendered return string), so it is in-scope for compliance.
## Fix Focus Areas
- src/observability/dashboard/ui/lib.js[325-332]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+328
to
+331
| // #516: keyed identity — the morph reuses existing rows and inserts only | ||
| // genuinely-new events as new nodes (which @starting-style animates in). | ||
| var feedKey = [item.ts, item.type, item.runId, item.summary].filter(Boolean).join('|'); | ||
| return '<div class="feed-row" data-key="' + esc(feedKey) + '"><div class="feed-icon ' + esc(level) + '">' + icon + '</div><div><div class="feed-summary">' + esc(item.summary || '') + '</div><div class="feed-meta">' + (item.runId ? '<span>' + esc(item.runId.slice(-14)) + '</span>' : '') + (item.projectRoot ? '<span>' + esc(shortName(item.projectRoot)) + '</span>' : '') + (item.type ? '<span>' + esc(item.type) + '</span>' : '') + '</div></div><div class="feed-ts">' + timeHtml(item.ts) + '</div></div>'; |
There was a problem hiding this comment.
7. Unstable feed row keys 🐞 Bug ≡ Correctness
feedRowHtml() includes item.summary in the data-key, so rows whose summary text changes across snapshots are treated as different keyed nodes and get replaced/re-animated. This breaks identity preservation for synthesized feed entries like tool_burst where ts/type/runId stay constant but the displayed count (and summary) changes.
Agent Prompt
## Issue description
`feedRowHtml()` builds `data-key` from `[item.ts, item.type, item.runId, item.summary]`. Because `summary` is presentation text (and can change while the underlying feed item identity is the same), keyed morphing will replace existing rows instead of reusing them.
## Issue Context
The activity feed includes synthesized `tool_burst` items where `ts` is pinned to the minute bucket and `summary` embeds a count that can increase as more tool calls arrive that minute. With `summary` in the key, the same logical row will get a different key on the next snapshot, so the morph can’t match/reuse it.
## Fix Focus Areas
- src/observability/dashboard/ui/lib.js[325-332]
- src/observability/dashboard/state/feed.js[14-40]
- src/observability/dashboard/ui/lib.js[249-294]
## Suggested change
- Remove `item.summary` from the key.
- Prefer a key composed only of stable identity fields (e.g., `ts|type|runId|projectRoot`), and use a null/undefined filter (not `Boolean`) so legitimate falsy values like `0` aren’t dropped.
- If collisions are still possible, extend the state model to provide a dedicated stable per-event identifier (e.g., a sequence number when reading `events.jsonl`) and key by that instead.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
Qodo's single finding addressed: the settle buffer is now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/observability/dashboard/ui/lib.js (1)
115-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReduced-motion flag is captured once and never updates.
PREFERS_REDUCED_MOTIONreadsmatchMedia('(prefers-reduced-motion: reduce)').matchesone time when the script loads. If the user's OS-level reduced-motion preference changes while the dashboard tab stays open,animateCountkeeps using the stale value, so KPI counters keep ticking through a full 220ms count-up animation instead of snapping instantly.This differs from the CSS-driven motion in
styles.js, where@media (prefers-reduced-motion: reduce)re-evaluates live. The JS-driven and CSS-driven motion contracts drift out of sync when the preference changes at runtime.Add a
changelistener on the media query and update the flag when it fires.♻️ Proposed fix
-var PREFERS_REDUCED_MOTION = typeof matchMedia === 'function' && matchMedia('(prefers-reduced-motion: reduce)').matches; +var PREFERS_REDUCED_MOTION = typeof matchMedia === 'function' && matchMedia('(prefers-reduced-motion: reduce)').matches; +if (typeof matchMedia === 'function') { + var reducedMotionQuery = matchMedia('(prefers-reduced-motion: reduce)'); + var onReducedMotionChange = function(event) { PREFERS_REDUCED_MOTION = event.matches; }; + if (typeof reducedMotionQuery.addEventListener === 'function') reducedMotionQuery.addEventListener('change', onReducedMotionChange); + else if (typeof reducedMotionQuery.addListener === 'function') reducedMotionQuery.addListener(onReducedMotionChange); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/observability/dashboard/ui/lib.js` around lines 115 - 134, Update the reduced-motion handling near PREFERS_REDUCED_MOTION by retaining the media-query object and registering a change listener that refreshes the flag from the event’s matches value. Keep animateCount using the updated flag so runtime preference changes make counters snap or animate consistently with the current setting.tests/browser/dashboard-516-micro.test.js (1)
20-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuiet-tick assertion doesn't exercise the keyed-morph path it names.
This test calls
window.applyState(window.STATE, {})with the identicalSTATEreference. SincerenderCommandis a pure function of state, the regenerated#command-feedHTML string is byte-identical to the previous one, somorphHTML'sel.__rsHtml === htmlcache check short-circuits before any DOM mutation occurs. ThemarkerSurvivedassertion therefore passes even withoutdata-keywiring, because the DOM node is never touched at all.Only the
rowKeyassertion in this test actually verifies the new keyed-identity feature. To validate that keyed matching (not just the unrelated HTML cache) preserves node identity, force a genuine content change elsewhere in the payload (so the feed HTML string differs) while keeping the marked row's key stable, then assert the marked node survives the re-render.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/browser/dashboard-516-micro.test.js` around lines 20 - 39, Update the test to force a genuine state change that modifies the command-feed HTML elsewhere (so the morphHTML cache short-circuit is bypassed) while keeping the marked row's data-key attribute stable, then verify the markerSurvived assertion validates that keyed matching (not just the HTML cache) preserves the DOM node identity across the re-render. Change the window.applyState call to pass a modified state object instead of the identical window.STATE reference, ensuring the feed HTML string differs before the morphHTML operation occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/browser/dashboard-516-micro.test.js`:
- Around line 92-104: Update the test body in suite.browserTest('skeletons show
before the first snapshot and never after (`#516`)') to explicitly intercept the
/ws connection, wait for the WebSocket handshake, and hold the initial snapshot
message until after the pre-snapshot skeleton assertion. Do not rely on the
/api/state route to block WebSocket delivery; release the WebSocket message
before continuing with the existing post-snapshot assertions.
---
Nitpick comments:
In `@src/observability/dashboard/ui/lib.js`:
- Around line 115-134: Update the reduced-motion handling near
PREFERS_REDUCED_MOTION by retaining the media-query object and registering a
change listener that refreshes the flag from the event’s matches value. Keep
animateCount using the updated flag so runtime preference changes make counters
snap or animate consistently with the current setting.
In `@tests/browser/dashboard-516-micro.test.js`:
- Around line 20-39: Update the test to force a genuine state change that
modifies the command-feed HTML elsewhere (so the morphHTML cache short-circuit
is bypassed) while keeping the marked row's data-key attribute stable, then
verify the markerSurvived assertion validates that keyed matching (not just the
HTML cache) preserves the DOM node identity across the re-render. Change the
window.applyState call to pass a modified state object instead of the identical
window.STATE reference, ensuring the feed HTML string differs before the
morphHTML operation occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a04b7d57-4d3f-4eef-9f8f-dba332e4ca8f
📒 Files selected for processing (7)
src/observability/dashboard/ui/client.jssrc/observability/dashboard/ui/lib.jssrc/observability/dashboard/ui/pages/command-center.jssrc/observability/dashboard/ui/pages/index.jssrc/observability/dashboard/ui/styles.jstests/browser/dashboard-516-micro.test.jstests/hub-micro-interactions-516.test.js
Comment on lines
+92
to
+104
| suite.browserTest('skeletons show before the first snapshot and never after (#516)', { seed: seedRichProject, tmpPrefix: 'rstack-browser-516-' }, async ({ page, server }) => { | ||
| // Hold the state request so the pre-snapshot shell is observable. | ||
| let releaseState; | ||
| const gate = new Promise((resolveGate) => { releaseState = resolveGate; }); | ||
| await page.route('**/api/state**', async (route) => { await gate; await route.continue(); }); | ||
| await page.goto(server.baseUrl, { waitUntil: 'domcontentloaded' }); | ||
| const before = await page.evaluate(() => document.querySelectorAll('#command-feed .skeleton-block').length); | ||
| assert.ok(before > 0, 'the feed panel shows skeleton placeholders before data'); | ||
| releaseState(); | ||
| await page.waitForFunction(() => window.STATE != null, null, { timeout: 10_000 }); | ||
| const after = await page.evaluate(() => document.querySelectorAll('#page-command .skeleton-block').length); | ||
| assert.equal(after, 0, 'skeletons never survive the first snapshot'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the dashboard server pushes an initial snapshot over WebSocket on connect.
rg -nP -C 6 '\bws\.send\b|\bwss?\.on\(.?connection.?' src/observability/dashboard --type=jsRepository: richard-devbot/SDLC-rstack
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)dashboard|connectWS|ws|websocket|server\.|client\.js' || true
echo "== relevant references =="
rg -n -C 4 'connectWS|fetchState|handleGlobalSnapshot|api/state|STATE|websocket|ws|server\.port|server\.baseUrl' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || trueRepository: richard-devbot/SDLC-rstack
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)dashboard|connectWS|ws|websocket|server\.|client\.js' || true
echo "== relevant references =="
rg -n -C 4 'connectWS|fetchState|handleGlobalSnapshot|api/state|STATE|websocket|ws|server\.port|server\.baseUrl' . --glob '!node_modules/**' --Glob '!dist/**' --Glob '!build/**' || trueRepository: richard-devbot/SDLC-rstack
Length of output: 9796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -i 'dashboard|client\.js|server\.' . -t f
echo "== test file section =="
sed -n '80,110p' tests/browser/dashboard-516-micro.test.js 2>/dev/null || true
echo "== search exact send/connection patterns =="
rg -n -C 4 'ws\.send|server\.ws|websocket|connection\s*=|\.upgrade|onupgrade|on\("connection|on'\''connection|ws\.createServer|wss\.createServer' . --glob '!node_modules/**' || trueRepository: richard-devbot/SDLC-rstack
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dashboard server websocket section =="
sed -n '1338,1395p' src/observability/dashboard/server.js
echo "== dashboard client websocket and setup section =="
sed -n '1,130p' src/observability/dashboard/ui/client.js
echo "== handleGlobalSnapshot definitions in dashboard UI =="
rg -n -C 8 'function handleGlobalSnapshot|handleGlobalSnapshot|fetchState|applyState' src/observability/dashboard/ui src/observability/dashboard --glob '!**/*.css' || true
echo "== read harness around dashboard server helpers =="
sed -n '1,220p' tests/browser/helpers/harness.jsRepository: richard-devbot/SDLC-rstack
Length of output: 50382
Wait for the WebSocket handshake before asserting the pre-snapshot view.
The dashboard server sends a full snapshot immediately after accepting a WebSocket connection, and connectWS() is started after the REST fetchState() call. page.route('**/api/state**', ...) does not hold that WebSocket message, so the initial "before" assertion can run after handleGlobalSnapshot has already populated STATE. Open a /ws, wait for the connection, and block that WebSocket message until after the snapshot assertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/browser/dashboard-516-micro.test.js` around lines 92 - 104, Update the
test body in suite.browserTest('skeletons show before the first snapshot and
never after (`#516`)') to explicitly intercept the /ws connection, wait for the
WebSocket handshake, and hold the initial snapshot message until after the
pre-snapshot skeleton assertion. Do not rely on the /api/state route to block
WebSocket delivery; release the WebSocket message before continuing with the
existing post-snapshot assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #516 · second implementation child of epic #499, on the #495 morph + #512 tokens.
What moves, and only when
data-keyidentity — existing rows are reused, never re-animated)@starting-styleentry (graceful no-op on engines without it)animateCountrAF tick at the base tierThe honest bits
matchMediaitself and snaps instantly.animateCountnow sets instantly whendocument.visibilityState !== 'visible'and carries a settle timeout that lands the exact final value even if rAF freezes mid-animation.TDD
Six source-contract pins verified failing first; five browser journeys on the shared #515 harness (pill wiring, keyed identity across quiet ticks AND arrivals, reduced-motion snap, motion-on settle, route-held skeleton lifecycle).
Verification
Core 1825/1825 · browser 19/19 (all suites on the shared harness) · lint 0 · typecheck 0 · validate 196 · security green · live-verified on the fixture-seeded hub (pill transitions at 0.16s, keyed rows, count-up settling on target with ticks held).
Zero-dep; the
motion-package and Rive decisions remain open for later children.Merging after green checks + reviewer bodies read, per the current working protocol.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Tests