feat(motion): add route transition gate with HUD overlay#47
Merged
Conversation
WalkthroughThe change introduces centralized route preloading and transition coordination, including readiness tracking, loading overlays, reveal animations, transition-aware links and redirects, updated navigation hooks, migrated route call sites, and supporting tests and styles. ChangesRoute transition navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AppLink
participant RouteTransitionProvider
participant RoutePreloader
participant DestinationScreen
User->>AppLink: Click internal route
AppLink->>RouteTransitionProvider: beginTransition(target)
RouteTransitionProvider->>RoutePreloader: preloadRoute(target)
RouteTransitionProvider->>DestinationScreen: navigate and await readiness
DestinationScreen-->>RouteTransitionProvider: reportScreenReady(true)
RouteTransitionProvider-->>User: Reveal destination with transition
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/components/navigation/AppLink.tsx`:
- Around line 34-35: Update the click handling around beginTransition so
transition or preload failures are caught rather than becoming unhandled
rejections, and restore native navigation when the transition fails. Preserve
event.preventDefault() only for successful transition handling and keep the
existing beginTransition(to) flow for successful loads.
- Around line 7-35: Update AppLink to pass replace, state, preventScrollReset,
and relative from LinkProps into beginTransition, and bypass the transition
coordinator when reloadDocument is enabled or target is not "_self". Preserve
native Link behavior for these cases, and handle or absorb the beginTransition
promise so preload failures cannot become unhandled rejections.
In `@src/navigation/AppNavigate.tsx`:
- Around line 9-22: Make the mount-only redirect in the useEffect idempotent
under React Strict Mode by adding a ref-based once guard before invoking
beginTransition or navigate. Ensure the guard prevents duplicate execution for
the same mount while preserving the existing phase-based transition behavior and
mount-only dependency configuration.
In `@src/navigation/routePreloaders.ts`:
- Around line 34-42: Move the preset edit-route regular expression out of
normalizeRoutePath and define it once at module scope, then reuse that
module-level constant in the function while preserving the current matching and
return behavior.
In `@src/navigation/routeTransition.test.ts`:
- Around line 50-52: Strengthen the lazy preloading test around preloadRoute by
spying on or injecting the lazy route loader and asserting it is invoked, rather
than only checking promise resolution. Cover the critical /map route and a
query-bearing path when route normalization is part of the contract, while
preserving the existing no-throw assertion.
In `@src/navigation/RouteTransitionContext.tsx`:
- Around line 96-112: Update the phase guards in beginTransition to treat both
"loading" and "revealing" as active transition phases, preventing a second
concurrent transition. In the active-phase fallback for a different target,
preload the destination with preloadRoute before calling navigate, while
preserving the existing same-target early return and navigation options.
- Around line 118-139: Update beginTransition to catch preloadRoute failures as
non-fatal, then continue with navigate and the existing
waitForScreenReady/revealRouteTransition flow so lazy loading can handle the
chunk failure. Also handle failures from navigation or reveal within the
transition orchestration so discarded beginTransition promises do not become
unhandled rejections, while preserving the finally cleanup that resets
loadingTargetRef and phase.
In `@src/navigation/RouteTransitionOverlay.tsx`:
- Around line 12-16: Add role="status" to the outer overlay div in
RouteTransitionOverlay, and include aria-atomic="true" to ensure the loading
message is announced as a complete status update while preserving the existing
aria-label and aria-live attributes.
In `@src/routes/CreateSession.test.tsx`:
- Around line 103-105: Reset the navigation mock before each test so calls from
earlier tests do not affect later assertions. Add a beforeEach hook near the
useAppNavigate mock that calls navigate.mockReset(), preserving the existing
navigate mock and test behavior.
In `@src/styles/route-transition.css`:
- Line 8: Remove the hardcoded rgb fallback from the background declaration in
route-transition.css and use the existing color design token directly,
preserving the --color-surface-deep styling.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efc01e72-a528-415f-ac59-9bb46cb213bb
📒 Files selected for processing (45)
CHANGELOG.mdsrc/App.tsxsrc/components/billing/PremiumTierCards.tsxsrc/components/legal/LegalDocumentPage.tsxsrc/components/legal/LegalInlineLinks.tsxsrc/components/navigation/AppLink.tsxsrc/components/presets/BundledPresetTree.tsxsrc/components/presets/PresetBrowseLayout.tsxsrc/components/presets/PresetDetailPanel.tsxsrc/components/presets/PresetSearchResults.tsxsrc/components/session/MapFirstRunSheet.tsxsrc/components/tutorial/TutorialHub.tsxsrc/components/tutorial/TutorialSectionWizard.tsxsrc/components/ui/MapErrorBoundary.tsxsrc/components/ui/ScreenHeader.tsxsrc/components/ui/ScreenNav.tsxsrc/domain/device/changelog.tssrc/hooks/useAppNavigate.test.tsxsrc/hooks/useAppNavigate.tssrc/index.csssrc/navigation/AppNavigate.tsxsrc/navigation/RouteReadinessSensor.tsxsrc/navigation/RouteTransitionContext.tsxsrc/navigation/RouteTransitionOverlay.tsxsrc/navigation/revealRouteTransition.tssrc/navigation/routePreloaders.tssrc/navigation/routeTransition.test.tssrc/navigation/routeTransitionContextInstance.tssrc/navigation/useRouteScreenReady.tssrc/navigation/useRouteTransition.tssrc/routes/AdminMapScreen.tsxsrc/routes/AdminPanel.tsxsrc/routes/CreateSession.test.tsxsrc/routes/GamePresets.test.tsxsrc/routes/GamePresets.tsxsrc/routes/HiderMapScreen.tsxsrc/routes/Home.tsxsrc/routes/MapScreen.test.tsxsrc/routes/MapScreen.tsxsrc/routes/ObserverMapScreen.tsxsrc/routes/create-session/PremiumGateSection.tsxsrc/routes/observer-map-screen/ObserverMapScreenChrome.tsxsrc/styles/route-transition.csssrc/test/RouteTransitionTestProvider.tsxsrc/test/renderWithRouter.tsx
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
23-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign both changelog entries with the coordinator’s timeout behavior.
The route transition can proceed after the readiness deadline even when the destination has not reported ready, so both entries overstate the guarantee.
CHANGELOG.md#L23-L23: revise the source bullet to mention the bounded readiness wait.src/domain/device/changelog.ts#L38-L38: regenerate this entry vianpm run release:syncrather than editing it independently.🤖 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 `@CHANGELOG.md` at line 23, Update CHANGELOG.md line 23 to describe the bounded readiness wait, without implying the destination always reports ready before the transition. Then run npm run release:sync to regenerate the corresponding entry in src/domain/device/changelog.ts line 38; do not edit the generated file independently.Source: Path instructions
♻️ Duplicate comments (1)
src/navigation/AppNavigate.tsx (1)
16-23: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnhandled rejection can strand a mount-only redirect.
beginTransitionreturns aPromise<void>and can reject (preload/readiness failures). BecauseranRef.currentis already set totruebefore this call, a rejection here means the redirect never happens and never retries — the user is stuck, plus an unhandled promise rejection surfaces. This mirrors the exact hazard already fixed on theAppLinkclick path in this same file set, which falls back tonavigate(..., { viewTransition: false })on failure.🔒 Proposed fix
if (phase === "idle") { - void beginTransition(to, { - replace, - state, - direction: replace ? "replace" : "forward", - }); - return; + beginTransition(to, { + replace, + state, + direction: replace ? "replace" : "forward", + }).catch(() => { + navigate(to, { replace, state, viewTransition: false }); + }); + return; }The
ranRefguard for Strict Mode idempotency (Lines 8, 11-14) looks correct.🤖 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/navigation/AppNavigate.tsx` around lines 16 - 23, Handle rejection from beginTransition in the phase === "idle" branch of the mount-only redirect by catching failures and falling back to navigate(to, { replace, state, viewTransition: false }). Preserve the existing ranRef Strict Mode guard and transition options for successful beginTransition calls, while preventing an unhandled rejection and ensuring the redirect still completes.
🤖 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/components/navigation/AppLink.tsx`:
- Around line 22-39: Remove the unreachable target === "_blank" branch from the
click handler in AppLink, since bypassTransition already returns for
non-undefined, non-"_self" targets. Preserve the remaining click handling for
targets that reach that path.
In `@src/navigation/routeTransition.test.ts`:
- Around line 55-66: Make spy cleanup in the “invokes the lazy loader for /map
and query-bearing paths” test exception-safe by moving restoration into a
finally block or shared afterEach cleanup. Ensure mapLoader is restored even
when either expectation fails, while preserving the existing preloadRoute
assertions.
In `@src/navigation/RouteTransitionContext.tsx`:
- Around line 103-108: Update the in-flight deduplication in the navigation
transition flow around loadingTargetRef to compare and update using the full
destination, including search and hash, rather than the normalized value from
resolveNavigatePath. Preserve targetPath for preloadRoute, and ensure the fast
path refreshes loadingTargetRef so repeated clicks do not remain tied to the
original destination.
- Around line 110-119: Update the fast-path branch in beginTransition so
navigation to a different target replaces the active transition state before
calling navigate: refresh loadingTargetRef and the reveal/options state using
the new target and transition inputs. Preserve the existing preload behavior,
and ensure repeated clicks on the new target are deduplicated against the
refreshed state.
In `@src/navigation/RouteTransitionOverlay.tsx`:
- Around line 12-19: Update the overlay container in RouteTransitionOverlay to
use a native output element instead of a div. Remove the redundant role,
aria-live, and aria-atomic attributes while preserving aria-busy, aria-label,
className, and the existing overlay content.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Line 23: Update CHANGELOG.md line 23 to describe the bounded readiness wait,
without implying the destination always reports ready before the transition.
Then run npm run release:sync to regenerate the corresponding entry in
src/domain/device/changelog.ts line 38; do not edit the generated file
independently.
---
Duplicate comments:
In `@src/navigation/AppNavigate.tsx`:
- Around line 16-23: Handle rejection from beginTransition in the phase ===
"idle" branch of the mount-only redirect by catching failures and falling back
to navigate(to, { replace, state, viewTransition: false }). Preserve the
existing ranRef Strict Mode guard and transition options for successful
beginTransition calls, while preventing an unhandled rejection and ensuring the
redirect still completes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 59c1c82f-b875-4899-8b6f-568da3f4801b
📒 Files selected for processing (11)
CHANGELOG.mdsrc/components/navigation/AppLink.tsxsrc/domain/device/changelog.tssrc/navigation/AppNavigate.tsxsrc/navigation/RouteTransitionContext.tsxsrc/navigation/RouteTransitionOverlay.tsxsrc/navigation/routePreloaders.tssrc/navigation/routeTransition.test.tssrc/routes/CreateSession.test.tsxsrc/routes/Home.tsxsrc/styles/route-transition.css
Comment on lines
+22
to
+39
| const bypassTransition = | ||
| reloadDocument === true || (target !== undefined && target !== "_self"); | ||
|
|
||
| if (bypassTransition) { | ||
| return ( | ||
| <Link | ||
| {...props} | ||
| to={to} | ||
| target={target} | ||
| replace={replace} | ||
| state={state} | ||
| preventScrollReset={preventScrollReset} | ||
| relative={relative} | ||
| reloadDocument={reloadDocument} | ||
| onClick={onClick} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead _blank check inside the click handler.
bypassTransition already returns early for any target other than undefined/"_self" (Line 23), so by the time execution reaches Line 72 target can only be undefined or "_self". The if (target === "_blank") guard can never be true here and is dead code that may mislead future edits into thinking _blank is handled at this site.
♻️ Proposed cleanup
if (target === "_blank") {
return;
}
event.preventDefault();becomes
event.preventDefault();Other than this, the previously flagged issues (LinkProps passthrough and unhandled beginTransition rejection) look correctly addressed.
Also applies to: 72-74
🤖 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/components/navigation/AppLink.tsx` around lines 22 - 39, Remove the
unreachable target === "_blank" branch from the click handler in AppLink, since
bypassTransition already returns for non-undefined, non-"_self" targets.
Preserve the remaining click handling for targets that reach that path.
Comment on lines
+55
to
+66
| it("invokes the lazy loader for /map and query-bearing paths", async () => { | ||
| const mapLoader = vi.spyOn(routePreloaders.routeImporter, "importMapScreen"); | ||
|
|
||
| await preloadRoute("/map"); | ||
| expect(mapLoader).toHaveBeenCalledTimes(1); | ||
|
|
||
| mapLoader.mockClear(); | ||
| await preloadRoute("/map?session=abc"); | ||
| expect(mapLoader).toHaveBeenCalledTimes(1); | ||
|
|
||
| mapLoader.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Spy cleanup isn't exception-safe.
If either expect call throws, mapLoader.mockRestore() never runs, leaving the spy in place for later tests. Consider vi.restoreAllMocks() in an afterEach, or wrapping the body in try/finally.
🤖 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/navigation/routeTransition.test.ts` around lines 55 - 66, Make spy
cleanup in the “invokes the lazy loader for /map and query-bearing paths” test
exception-safe by moving restoration into a finally block or shared afterEach
cleanup. Ensure mapLoader is restored even when either expectation fails, while
preserving the existing preloadRoute assertions.
Comment on lines
+103
to
+108
| if ( | ||
| phaseRef.current !== "idle" && | ||
| loadingTargetRef.current === targetPath | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## RouteTransitionContext.tsx outline\n'
ast-grep outline src/navigation/RouteTransitionContext.tsx --view expanded || true
printf '\n## routePreloaders.ts outline\n'
ast-grep outline src/navigation/routePreloaders.ts --view expanded || true
printf '\n## Relevant excerpts\n'
sed -n '1,240p' src/navigation/RouteTransitionContext.tsx
printf '\n--- routePreloaders ---\n'
sed -n '1,220p' src/navigation/routePreloaders.tsRepository: gelbh/jetlag
Length of output: 8048
Use a raw dedup key here resolveNavigatePath strips search/hash, so an in-flight /map transition will treat /map?session=xyz as the same target and return early. The fast path also leaves loadingTargetRef stale, so repeated clicks keep comparing against the original target. Keep targetPath for preloadRoute, but dedup/update against the full destination.
🤖 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/navigation/RouteTransitionContext.tsx` around lines 103 - 108, Update the
in-flight deduplication in the navigation transition flow around
loadingTargetRef to compare and update using the full destination, including
search and hash, rather than the normalized value from resolveNavigatePath.
Preserve targetPath for preloadRoute, and ensure the fast path refreshes
loadingTargetRef so repeated clicks do not remain tied to the original
destination.
Comment on lines
+110
to
+119
| if (phaseRef.current !== "idle") { | ||
| try { | ||
| await preloadRoute(targetPath); | ||
| } catch { | ||
| // Warm-up only; the rendered lazy route retries chunk failures itself. | ||
| } | ||
|
|
||
| navigate(to, navigateOptions); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby symbols with line numbers.
git ls-files src/navigation/RouteTransitionContext.tsx
wc -l src/navigation/RouteTransitionContext.tsx
cat -n src/navigation/RouteTransitionContext.tsx | sed -n '1,260p'
# Find the transition-related refs/functions in the repo.
rg -n "loadingTargetRef|revealRouteTransition|beginTransition|pathnameRef|phaseRef|navigateOptions|preloadRoute" src/navigation -SRepository: gelbh/jetlag
Length of output: 8982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the reveal helper and the transition caller.
wc -l src/navigation/revealRouteTransition.ts src/navigation/AppNavigate.tsx src/navigation/routeTransitionContextInstance.ts
cat -n src/navigation/revealRouteTransition.ts
printf '\n--- AppNavigate ---\n'
cat -n src/navigation/AppNavigate.tsx
printf '\n--- context instance ---\n'
cat -n src/navigation/routeTransitionContextInstance.ts
# Inspect tests that cover transition behavior.
printf '\n--- routeTransition tests ---\n'
cat -n src/navigation/routeTransition.test.ts | sed -n '1,260p'Repository: gelbh/jetlag
Length of output: 6207
Fast-path navigation should refresh the active transition state
When beginTransition runs during an in-flight transition to a different target, the new path is navigated immediately but the old transition still owns loadingTargetRef and the reveal options. That leaves the reveal using the first call’s direction/state and lets repeated clicks on the second target bypass dedup.
🤖 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/navigation/RouteTransitionContext.tsx` around lines 110 - 119, Update the
fast-path branch in beginTransition so navigation to a different target replaces
the active transition state before calling navigate: refresh loadingTargetRef
and the reveal/options state using the new target and transition inputs.
Preserve the existing preload behavior, and ensure repeated clicks on the new
target are deduplicated against the refreshed state.
Comment on lines
+12
to
+19
| <div | ||
| className="route-transition-overlay" | ||
| role="status" | ||
| aria-busy="true" | ||
| aria-live="polite" | ||
| aria-atomic="true" | ||
| aria-label="Loading page" | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer <output> over role="status" for native semantics.
Static analysis flags this: <output> carries an implicit ARIA "status" role, so it can replace the manual role/aria-live/aria-atomic combo on the div.
♻️ Proposed fix
- <div
+ <output
className="route-transition-overlay"
- role="status"
- aria-busy="true"
- aria-live="polite"
- aria-atomic="true"
aria-label="Loading page"
>
<div className="route-transition-overlay-content">
<LoadingSpinnerRing size="md" className="text-brand-blue" />
<span className="route-transition-overlay-label">Loading…</span>
</div>
- </div>
+ </output>🧰 Tools
🪛 React Doctor (0.5.8)
[warning] 14-14: Screen reader users get more reliable semantics from <output> than role="status", so use <output> instead.
Use the matching HTML element when one exists so browsers and assistive tech get native semantics.
(prefer-tag-over-role)
🤖 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/navigation/RouteTransitionOverlay.tsx` around lines 12 - 19, Update the
overlay container in RouteTransitionOverlay to use a native output element
instead of a div. Remove the redundant role, aria-live, and aria-atomic
attributes while preserving aria-busy, aria-label, className, and the existing
overlay content.
Source: Linters/SAST tools
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
1 participant
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.
Summary
AppLink,AppNavigate, and an enhanceduseAppNavigate/ edge-swipe back.Test plan
useAppNavigatedelegates withviewTransition: false/mapwaits for play area before revealSummary by CodeRabbit