Hub page/tab switch motion — live-DOM fade; View Transitions API rejected on evidence (#521) - #524
Conversation
…cted (#521) The incoming page (and workspace tab panel) fades in through the motion tokens as a PLAIN animation on the live DOM. The View Transitions API was implemented first and rejected on evidence: a root capture suppresses painting and hit-testing of the live page for the whole animation window (~160-250ms measured by elementFromPoint probing after navigation), so every click right after a page switch vanished — the #495 swallowed-click class reintroduced by the platform; pointer-events: none on the overlay cannot restore suppressed content. The full browser suite caught it: the #495 click-straddle and #494 dedup journeys went intermittent under the API build. Evidence and probe data on #521. The fade keeps the page interactive for every frame — pinned by the defining browser contract the API could never pass: a click landing immediately after navigation, mid-fade, works. Only navigation toggles .active/the panel hidden attribute, so live repaints structurally cannot re-trigger the entry animation (pinned); the global reduced-motion guard covers it like any other animation (no JS gate); the rejection is pinned by asserting startViewTransition appears nowhere in the bundle. Also: showPage owns the workspace-section render (deduped from showRunWorkspaceSection), the history write derives from resolvedPage, and the #494 journey's hand-rolled navigations now wait for the page to land — correct under any swap timing. Closes #521. Part of epic #499. 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. |
|
Warning Review limit reached
Next review available in: 52 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 (2)
📝 WalkthroughWalkthroughDashboard navigation now uses synchronous live-DOM page switching with CSS fade-in animation. Workspace sections render through the same path. Route history uses the resolved page and active section. Tests cover timing, reduced motion, repaint behavior, and visibility waits. ChangesDashboard navigation fades
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardNavigation
participant LiveDOM
participant BrowserHistory
User->>DashboardNavigation: select page or workspace section
DashboardNavigation->>LiveDOM: render resolved destination
LiveDOM-->>User: show page-enter fade
DashboardNavigation->>BrowserHistory: write resolved route
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 page/tab switch motion via live-DOM fade (reject View Transitions API)
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/navigation.js`:
- Around line 241-245: Remove the redundant renderRunWorkspaceSection call from
the popstate history-navigation handler, while retaining the call in showPage
for resolved run-workspace routes. Ensure back and forward navigation performs
only the single render triggered by showPage.
In `@tests/browser/dashboard-494-approval-dedup.test.js`:
- Around line 45-46: Update the navigation comment near the visibility wait to
describe the current synchronous live-DOM CSS fade mechanism instead of
attributing it to a View Transitions callback or issue `#521`. Preserve the
existing wait behavior unchanged.
In `@tests/browser/dashboard-521-transitions.test.js`:
- Around line 23-30: Add a browser journey alongside the existing test in
dashboard-521-transitions.test.js that clicks a visible workspace tab, verifies
showRunWorkspaceSection, and targets the corresponding run-workspace-* panel
while its animation is active. Assert the panel is visible and immediately
interactive without a settling wait, preserving the same mid-fade interaction
coverage as the existing navigation test.
🪄 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: bed91a37-e190-47a1-8d56-422d8a3588bf
📒 Files selected for processing (5)
src/observability/dashboard/ui/navigation.jssrc/observability/dashboard/ui/styles.jstests/browser/dashboard-494-approval-dedup.test.jstests/browser/dashboard-521-transitions.test.jstests/hub-view-transitions-521.test.js
| if (resolvedPage === 'run-workspace') { | ||
| ACTIVE_RUN_SECTION = RUN_WORKSPACE_SECTIONS.indexOf(options.section) >= 0 ? options.section : (ACTIVE_PAGE === 'run-workspace' ? ACTIVE_RUN_SECTION : 'summary'); | ||
| } | ||
| syncNavigationState(resolvedPage); | ||
| if (options.history !== false) writeDashboardRoute(ACTIVE_PAGE, SCOPE.run, ACTIVE_PAGE === 'run-workspace' ? ACTIVE_RUN_SECTION : '', 'push'); | ||
| if (options.closeMobile !== false) closeMobileNavigation(); | ||
| if (resolvedPage === 'run-workspace' && typeof renderRunWorkspaceSection === 'function') renderRunWorkspaceSection(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Remove the duplicate workspace render on history navigation.
When a popstate route resolves to run-workspace, showPage renders the section at Line 245. The popstate handler then renders it again at Line 382. Render the section only through showPage so back and forward navigation performs one DOM update.
Proposed fix
window.addEventListener('popstate', function() {
var route = readDashboardRoute();
showPage(route.page || DEFAULT_PAGE, { history: false, closeMobile: false, section: route.section });
restoreScopeFromRoute(route.run);
- if (typeof renderRunWorkspaceSection === 'function') renderRunWorkspaceSection();
});🤖 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/navigation.js` around lines 241 - 245, Remove
the redundant renderRunWorkspaceSection call from the popstate
history-navigation handler, while retaining the call in showPage for resolved
run-workspace routes. Ensure back and forward navigation performs only the
single render triggered by showPage.
| // #521 made the visual swap asynchronous (view-transition callback) — | ||
| // wait for the page to land before reading it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the navigation comment.
Line 45 states that #521 uses a View Transitions callback. This PR uses a synchronous live-DOM CSS fade. Keep the visibility wait, but describe the current mechanism.
🤖 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-494-approval-dedup.test.js` around lines 45 - 46,
Update the navigation comment near the visibility wait to describe the current
synchronous live-DOM CSS fade mechanism instead of attributing it to a View
Transitions callback or issue `#521`. Preserve the existing wait behavior
unchanged.
| function browserTest(name, contextOptions, body) { | ||
| suite.browserTest(name, { seed: seedRichProject, contextOptions, tmpPrefix: 'rstack-browser-521-' }, body); | ||
| } | ||
|
|
||
| browserTest('a click lands immediately after navigation, mid-fade (#521)', {}, async ({ page, server }) => { | ||
| await gotoDashboard(page, server); | ||
| await navigate(page, 'traceability'); | ||
| // No settling wait — this click arrives while the entry fade is running. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -0 -e js . tests | xargs -0 rg -n -C 3 \
'run-workspace-tab|run-workspace-panel|showRunWorkspaceSection'Repository: richard-devbot/SDLC-rstack
Length of output: 2413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate test files =="
git ls-files | rg '(^|/)dashboard-521-transitions\.test\.js$|hub-view-transitions-521\.test\.js$|dashboard-run-workspace\.test\.js$' || true
echo
echo "== dashboard-521-transitions outline/content =="
if [ -f tests/browser/dashboard-521-transitions.test.js ]; then
wc -l tests/browser/dashboard-521-transitions.test.js
cat -n tests/browser/dashboard-521-transitions.test.js
fi
echo
echo "== hub-view-transitions snippet =="
if [ -f tests/hub-view-transitions-521.test.js ]; then
wc -l tests/hub-view-transitions-521.test.js
sed -n '1,80p' tests/hub-view-transitions-521.test.js | cat -n
fi
echo
echo "== dashboard-run-workspace relevant snippets =="
if [ -f tests/dashboard-run-workspace.test.js ]; then
wc -l tests/dashboard-run-workspace.test.js
sed -n '1,130p' tests/dashboard-run-workspace.test.js | cat -n
fi
echo
echo "== source references for workspace section implementation =="
rg -n -C 4 'showRunWorkspaceSection|run-workspace-tab|run-workspace-panel|workspace.*tab|run-workspace-tabs' src . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true
echo
echo "== browser test helpers definitions =="
rg -n -C 4 'function browserTest|suite\.browserTest|gotoDashboard|navigate|page-enter|showRunWorkspaceSection' tests src --glob '*.{js,mjs,ts,tsx}' || trueRepository: richard-devbot/SDLC-rstack
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== precise browser test symbols =="
rg -n -C 3 'browserTest|suite\.browserTest|gotoDashboard|navigate|showRunWorkspaceSection|run-workspace-section|run-workspace-tabs|showRunWorkspace' tests src --glob '*.js' --glob '*.mjs' --glob '*.ts' --glob '*.tsx' || true
echo
echo "== workspace section-related selectors =="
rg -n 'run-workspace-section|run-workspace-panel|run-workspace-tab|openDrawer|showRunWorkspaceSection' src tests --glob '*.{js,mjs,ts,tsx}' || trueRepository: richard-devbot/SDLC-rstack
Length of output: 43254
Add a browser journey for workspace-tab motion.
tests/browser/dashboard-521-transitions.test.js only covers /page-traceability page fades. It does not click a visible workspace tab, verify showRunWorkspaceSection, or check the target run-workspace-* panel animation and immediate interaction. Add a browser test for workspace-tab switching to cover the same mid-fade interactivity contract.
🤖 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-521-transitions.test.js` around lines 23 - 30, Add a
browser journey alongside the existing test in dashboard-521-transitions.test.js
that clicks a visible workspace tab, verifies showRunWorkspaceSection, and
targets the corresponding run-workspace-* panel while its animation is active.
Assert the panel is visible and immediately interactive without a settling wait,
preserving the same mid-fade interaction coverage as the existing navigation
test.
Source: Coding guidelines
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}
| syncNavigationState(resolvedPage); | ||
| if (options.history !== false) writeDashboardRoute(ACTIVE_PAGE, SCOPE.run, ACTIVE_PAGE === 'run-workspace' ? ACTIVE_RUN_SECTION : '', 'push'); | ||
| if (options.closeMobile !== false) closeMobileNavigation(); | ||
| if (resolvedPage === 'run-workspace' && typeof renderRunWorkspaceSection === 'function') renderRunWorkspaceSection(); |
There was a problem hiding this comment.
5. Double render on popstate 🐞 Bug ➹ Performance
showPage() now calls renderRunWorkspaceSection() when resolving to run-workspace, but the popstate handler still calls renderRunWorkspaceSection() after showPage(), causing redundant DOM writes on history navigations to run-workspace.
Agent Prompt
### Issue description
`showPage()` now renders the run-workspace section by calling `renderRunWorkspaceSection()`. The `popstate` handler also calls `renderRunWorkspaceSection()` after invoking `showPage()`, resulting in a duplicate render on history navigation to `run-workspace`.
### Issue Context
`renderRunWorkspaceSection()` writes `aria-selected`, `tabindex`, and toggles `panel.hidden` across workspace panels, so calling it twice is redundant work.
### Fix Focus Areas
- src/observability/dashboard/ui/navigation.js[238-251]
- src/observability/dashboard/ui/navigation.js[378-383]
- src/observability/dashboard/ui/pages/run-workspace.js[140-149]
### Suggested fix
Delete the unconditional `renderRunWorkspaceSection()` call inside the `popstate` handler (or guard it so it only runs when `showPage()` did not already handle it).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo (both my own accepted precedents): the #494 journey's navigation waits use the harness's PAGE_VISIBLE_TIMEOUT_MS (#509 convention), and the reduced-motion fade assertion parses durations unit-aware (#514 pattern) instead of assuming seconds serialization. Refs #521 (PR #524 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Closes #521 · third implementation child of epic #499.
The interesting part: what got rejected, and why
The View Transitions API version was fully implemented and its own tests were green — then the full browser suite caught the #495 click-straddle and #494 dedup journeys going intermittent. An
elementFromPointprobe (sampled every 40ms post-navigation) showed hit-testing returning<html>for the entire animation window (~160–250ms): a root capture suppresses painting and hit-testing of the live page, so every click right after a page switch vanished. That is the #495 swallowed-click class reintroduced by the platform itself — and::view-transition { pointer-events: none; }cannot fix it (it stops the overlay capturing; it cannot restore hit-testing to suppressed content). Per the epic's iron rules, the no-swallowed-clicks contract wins. Full probe data on #521.What shipped instead — same visual, zero interactivity cost
.page.activeand.run-workspace-panel:not([hidden])fade in viapage-enterat the fast tier — on the live DOM, interactive every frame..activetoggle site in the bundle, in navigation).startViewTransitionmust appear nowhere in the bundle.showPageowns the workspace-section render, the history write derives fromresolvedPage, and the Approvals projection: one decision renders as two pending items — dedup by (run, task, artifact), not raw queue id #494 journey's hand-rolled navigations wait for the page to land (correct under any swap timing).The defining test
Verification
Core 1829/1829 · browser 23/23 (including the previously-intermittent #494/#495 journeys, stable again) · lint 0 · typecheck 0 · validate 196 · security green · whitespace clean. TDD throughout — every new pin watched failing first, including the
pointer-eventsintermediate attempt that the probe then invalidated.Merging after green checks + reviewer bodies read, per the current working protocol.
🤖 Generated with Claude Code
Summary by CodeRabbit
Enhancements
Bug Fixes