Motion: Proof Rail draw, approval lifecycle, scroll reveal (#541) - #542
Merged
Conversation
The rail showed a static text glyph per state and nothing transitioned. The check is now an SVG path present in EVERY state, undrawn until the stage passes. That is the whole trick: a path rendered only for the passed state would be a NEW node at the flip, and a new node cannot transition — the draw would never play on the one occasion it means something. Keeping the node alive through the morph lets its stroke-dashoffset transition instead. The same property gives the honest-motion rule for free. A stage that ARRIVES already passed renders its check statically, because a transition needs a previous value to move from. Only an observed flip animates. No JS guard, no state diffing. pathLength="1" makes the dash resolution-independent — the #525 idiom — so the offset reads as "how much of the check is undrawn". Blocked and failed stages gain colour-and-shadow emphasis. No box or layout property is touched: this surface repaints every few seconds and a size change would shift the page under the reader's cursor. --motion-draw was added by #512 and had no consumer until now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Approving is the one action in the Hub where a human's own decision changes governed state, so the card should show that happening rather than silently appearing in its new colour on the next repaint. Transitions across border colour, opacity and background. Transition rather than animation, so a card that merely renders in its final state on page load stays still — only a real PENDING → APPROVED → CONSUMED move under #495's morph animates. Found while doing it: .approval-card.consumed had NO rule. A spent one-shot override lost its left border entirely and fell back to the bare card, even though .pill.consumed was styled — so the state this epic singles out as deserving a dignified change had the least visual identity of any. It is now muted with a neutral border: a spent credential, deliberately distinct from the green of a live approval. STALE is deliberately absent. The epic child says "CONSUMED / STALE", but annotateApprovalLifecycle emits exactly one lifecycle value and STALE_ARTIFACT_CHANGED appears nowhere in the dashboard — it is a harness-side concept that never reaches the projection. Animating it would paint a state the server cannot produce. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent (#541) Command Center sections reveal as they first reach the fold, once, with no re-trigger on scroll-back. Two properties matter more than the animation: The hidden state is applied by script, never by a static rule. There is deliberately no CSS on [data-reveal] itself — only .reveal-pending hides, and only initScrollReveal adds it. A reader whose JS fails, whose engine lacks IntersectionObserver, or who prefers reduced motion sees every section. A page whose whole job is showing governed state must never withhold it behind a capability check. IntersectionObserver alone is NOT sufficient, which the browser proved: it reports a CHANGE in intersection, so a section carried from below the fold to above it in one jump never has a non-zero ratio, is never reported, and stays hidden until the reader happens to scroll back. Jumping to the page bottom left four panels hidden at y=-1585 and y=-669. A passive scroll sweep now reveals anything that has reached the fold whether or not IO noticed, and detaches itself once nothing is pending. Both are mutation-checked — removing the sweep fails the unit test and the browser journey. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three no-undef lint errors. Worth a note on how they surfaced: the #507 canary runs the REAL eslint binary over a brace glob, so my own lint errors made it report a crash — the canary is downstream of a clean tree. (The crash it actually printed was the genuine #507 signature, from a stale node_modules in this worktree still holding eslint 9.39.5 against a package.json asking for ^10. npm install resolved it; no code change.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe dashboard adds proof-stage SVG transitions, approval lifecycle styling, and one-way scroll reveals. Motion behavior includes reduced-motion and unsupported-browser fallbacks. Browser and unit tests cover transitions, persistence, cleanup, and accessibility. ChangesDashboard motion behaviors
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 motion: Proof Rail check draw, approval lifecycle, scroll reveal
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
+204
to
+225
| motionTest('below-the-fold sections reveal on scroll and never re-hide (#541)', async ({ page, server }) => { | ||
| await open(page, server); | ||
|
|
||
| const pending = await page.evaluate(() => | ||
| document.querySelectorAll('#page-command [data-reveal].reveal-pending').length); | ||
| assert.ok(pending > 0, `something below the fold is waiting to reveal — got ${pending}`); | ||
|
|
||
| // A single jump to the bottom, deliberately: this is the case IO cannot | ||
| // see, because a section that passes from below the fold to above it never | ||
| // has a non-zero intersection ratio. | ||
| await page.evaluate(() => window.scrollTo(0, document.documentElement.scrollHeight)); | ||
| await page.waitForFunction(() => | ||
| document.querySelectorAll('#page-command [data-reveal].reveal-pending').length === 0, | ||
| null, { timeout: PAGE_VISIBLE_TIMEOUT_MS }); | ||
|
|
||
| // Scrolling back must not hide real data again. | ||
| await page.evaluate(() => window.scrollTo(0, 0)); | ||
| await page.waitForTimeout(300); | ||
| const rehidden = await page.evaluate(() => | ||
| document.querySelectorAll('#page-command [data-reveal].reveal-pending').length); | ||
| assert.equal(rehidden, 0, 'nothing re-hides on scroll-back'); | ||
| }); |
There was a problem hiding this comment.
5. Reveal tests scroll window 🐞 Bug ⚙ Maintainability
The new browser tests use window.scrollTo() to trigger reveal, but the app can scroll within the #content container; this may fail to exercise the production scroll path and can be flaky or miss regressions depending on layout. Update the tests to scroll the same container(s) the app actually uses.
Agent Prompt
### Issue description
Browser tests use `window.scrollTo(...)` to drive scroll reveal. If the dashboard’s effective scroll container is `#content` (overflow scroller), these test actions won’t reliably trigger the reveal path they’re intended to validate.
### Issue Context
The app may scroll either `document.scrollingElement` or `#content` (navigation resets both). The tests should scroll the real container to ensure they validate the safety-net behavior.
### Fix Focus Areas
- tests/browser/dashboard-541-motion.test.js[204-225]
- tests/browser/dashboard-541-motion.test.js[242-250]
### Implementation notes
- Replace `window.scrollTo(...)` with something like:
- `const c = document.getElementById('content'); if (c && c.scrollHeight > c.clientHeight) c.scrollTop = c.scrollHeight; else window.scrollTo(0, document.documentElement.scrollHeight);`
- Do the same for the scroll-back step.
- Consider adding an assertion for which scroller moved (optional) to make failures easier to interpret.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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: 2
🤖 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 `@src/observability/dashboard/ui/lib.js`:
- Around line 111-122: Update the reveal fallback setup around onScroll and the
IntersectionObserver to register the handler on the dashboard scroll owner,
document.getElementById('content'), instead of only the global event target.
Ensure the corresponding cleanup removes onScroll from the same content element,
preserving the existing passive listener behavior.
In `@src/observability/dashboard/ui/pages/index.js`:
- Line 44: Update the reveal-state handling for sections such as the
`overview-outcome` element so `.reveal-pending` also disables pointer/touch
interaction and keyboard focus for descendant controls, while preserving the
existing reduced-motion fallback behavior. Re-enable interaction when the
pending state is removed or the section is revealed.
🪄 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: c6593018-9e0d-4481-ac2c-06f70b3b3632
📒 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-541-motion.test.jstests/hub-motion-499.test.js
| const bodies = { | ||
| command: ` | ||
| <section class="overview-decision-surface" id="overview-outcome" aria-labelledby="overview-outcome-title"> | ||
| <section class="overview-decision-surface" data-reveal id="overview-outcome" aria-labelledby="overview-outcome-title"> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'reveal-pending|reveal-in|\[data-reveal\]|inert|visibility|pointer-events|tabindex' \
src/observability/dashboard/uiRepository: richard-devbot/SDLC-rstack
Length of output: 36718
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,150p' src/observability/dashboard/ui/lib.js
sed -n '1780,1845p' src/observability/dashboard/ui/styles.js
sed -n '40,135p' src/observability/dashboard/ui/pages/index.js
sed -n '165,190p' src/observability/dashboard/ui/pages/command-center.jsRepository: richard-devbot/SDLC-rstack
Length of output: 18270
Make pending reveal sections non-interactive.
.reveal-pending only applies opacity: 0 and transform, so interactive children such as the Proof Rail “Open all evidence” button and the other new data-reveal sections remain focused and clickable while hidden. Add CSS or JS state that disables pointer/touch access and keyboard interaction while a section is pending without changing the reduced-motion fallback behavior.
🤖 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/pages/index.js` at line 44, Update the
reveal-state handling for sections such as the `overview-outcome` element so
`.reveal-pending` also disables pointer/touch interaction and keyboard focus for
descendant controls, while preserving the existing reduced-motion fallback
behavior. Re-enable interaction when the pending state is removed or the section
is revealed.
Source: Coding guidelines
…weep (#541) Qodo and CodeRabbit independently flagged that the scroll sweep listened on window only, while #content carries overflow-y:auto. Scroll events do not bubble, so a window listener sees only document scrolling. Measured before changing anything: #content does NOT scroll today — its ancestors use min-height, so the document grows instead. So this is a latent fragility rather than a live bug. Fixed anyway, because the failure mode is invisible governance data and the fix is one flag: the listener now captures on document, so it sees scroll from whichever element ends up being the scroller. Correcting my own PR claim while I am here. I described the scroll sweep as the thing that fixes the jump-past case and called it mutation-checked. The mutation check removed BOTH sweep call sites. Isolating them shows they are redundant: either alone fixes every case reproducible here, and only removing both brings the bug back. They are kept deliberately — the IO call cannot help when IO is never invoked, the scroll call cannot help when content moves without a scroll — but "redundant belt-and-braces" is what they are, and the comment now says so. The new browser test is labelled for what it proves: reveal survives an inner-container layout. It does not isolate the capture flag, because IO fires regardless of which element scrolls — so it passes with capture removed. The flag is asserted structurally in the unit suite instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
Both reviewers found the same real thing — fixed, plus a correction to my own PR bodyQodo (#4, #5) and CodeRabbit independently flagged that the scroll sweep listened on What I measured before changing anything
Fixed anyway — the failure mode is invisible data and the fix is one flag. The listener now captures on Correcting my own PR bodyWhile verifying the fix I found I had over-claimed, so I want to put it right rather than leave it standing. I described the scroll sweep as the thing that fixes the jump-past case and called it mutation-checked. The mutation check removed both sweep call sites. Isolating them:
They are redundant. Either alone fixes every case reproducible here. I have kept both deliberately, because they fail in different directions — the IO call cannot help when IO is never invoked at all, the scroll call cannot help when content moves without a scroll (layout change, page switch, a panel growing) — but "belt-and-braces" is what they are, and the code comment now says exactly that instead of implying the scroll sweep is load-bearing on its own. The new browser test is labelled for what it proves
Verification1965/1965 core, 9/9 in this suite (51 → 52 browser overall), lint 0 errors, typecheck clean. Thanks to both — the finding was correct, and chasing it is what surfaced the redundancy I had mischaracterised. |
8 tasks
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 #541. The three remaining zero-dependency children of epic #499 — Proof Rail transitions, approval gate lifecycle, scroll reveal. One bisected commit each, all TDD. The two open epic decisions (
motionpackage, Rive) are untouched and were not needed.The mechanism, and why it is a correctness property
Everything here uses
transition, never@keyframes animation. That is not a style preference:Which is exactly the epic's iron rule, expressed in CSS:
No JS guard, no state diffing, nothing for a future contributor to forget. Two browser assertions pin both directions, and they are deliberate opposites.
1. Proof Rail draws its check on a real PASS
The check is an SVG path present in every state, undrawn until the stage passes. A path rendered only for
passedwould be a new node at the flip, and a new node cannot transition — so the draw would never play on the one occasion it means something.pathLength="1"makes the dash resolution-independent (the #525 idiom).--motion-drawwas added by #512 and had no consumer until now.Blocked and failed stages gain colour-and-shadow emphasis, with no box or layout property touched — this surface repaints every few seconds and a size change would shift the page under the reader's cursor.
Declined: "rail fill progress" from the epic child. Command Center already carries the #526 subway strip and the readiness KPIs; a third representation of stage progress on one page is noise. Flagged rather than silently dropped.
2. The approval lifecycle is legible when it changes
Transitions across PENDING → APPROVED → CONSUMED on the card that survives the morph.
Found in recon:
.approval-card.consumedhad no rule at all. A spent one-shot override lost its left border entirely and fell back to the bare card, while.pill.consumedwas styled — so the state this epic singles out as deserving "a visible, dignified state change" had the least visual identity of any. It is now muted with a neutral border: a spent credential, deliberately distinct from the green of a live approval.Correction to the epic: there is no STALE approval state in the Hub. The child says "CONSUMED / STALE".
annotateApprovalLifecycleemits exactly one lifecycle value, andSTALE_ARTIFACT_CHANGEDappears nowhere undersrc/observability/dashboard/— it is harness-side and never reaches the projection. Animating it would paint a state the server cannot produce, which is the #533 "approaching cap" prohibition again.3. Scroll reveal — and the real bug it exposed
Two properties matter more than the animation.
The hidden state is applied by script, never by a static rule. There is deliberately no CSS on
[data-reveal]itself. A reader whose JS fails, whose engine lacks IntersectionObserver, or who prefers reduced motion sees every section. A page whose job is showing governed state must never withhold it behind a capability check.IntersectionObserver alone is not sufficient — the browser proved it. IO reports a change in intersection, so a section carried from below the fold to above it in one jump never has a non-zero ratio, is never reported, and stays hidden until the reader happens to scroll back. Jumping to the page bottom left four panels hidden at y=-1585 and y=-669. A passive scroll sweep now reveals anything that has reached the fold whether or not IO noticed, and detaches once nothing is pending.
I hit this because the first version of the browser journey timed out, and the honest read of a timeout is "find out why", not "raise the timeout".
Verification
git diff --checkclean.transition-durationto.01msrather than removing it, so the value still settles on the next frame, and the sampling window is what separates that from the.9sdraw.A note on the one non-obvious failure
The #507 eslint canary failed mid-run with the genuine
expand is not a functionsignature. Two causes stacked: my own threeno-undeferrors (the canary runs the real binary, so it is downstream of a clean tree), and a stalenode_modulesin this worktree still holding eslint 9.39.5 against apackage.jsonasking for^10.npm installresolved the second; no code change was needed for it.Summary by CodeRabbit
New Features
Accessibility