Repository navigation
Load-then-render SPA navigation with subtle top progress bar - #608
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds route-loader-driven client navigation, a single-slot preloaded navigation store, intent prefetch, a navigation progress bar, frame prefetch caching, and route-specific loader migrations across account, admin, community, and OAuth client routes. ChangesPreload-then-commit navigation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant ClientRouter
participant RouteLoader
participant NavStore
participant RouteComponent
App->>ClientRouter: registerRouteLoaders(clientRouteLoaders)
ClientRouter->>ClientRouter: navigate(to)
ClientRouter->>RouteLoader: matched loader(url, signal)
RouteLoader-->>ClientRouter: RouteLoaderResult
ClientRouter->>NavStore: setPreloadedNavigationData(href, data)
ClientRouter->>ClientRouter: commit history + notify
RouteComponent->>NavStore: tryConsumeRouteLoaderData(key, href)
NavStore-->>RouteComponent: route-loader data or fallback
ClientRouter->>ClientRouter: dispatch navigationend
Possibly related PRs
🚥 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 |
kentcdodds
marked this pull request as ready for review
July 4, 2026 21:20
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
🔎 Preview deployed: https://kody-pr-608.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/worker/client/routes/admin-community-reports.tsx (1)
227-240: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvalidate stale fallback loads when route-loader data wins.
If an older
loadReports()fallback is still in flight,applyRouteLoaderData()can render the preloaded destination data, then the old fallback can pass the unchangedloadRequestIdcheck and overwrite the UI with stale reports.Proposed fix
function applyRouteLoaderData(href: string) { if (!isAdminCommunityReportsPath(href)) return false const routeData = tryConsumeRouteLoaderData( handle, 'adminCommunityReports', href, ) if (!routeData) return false + loadRequestId += 1 + loadingForHref = null + lastFailedHref = null reports = routeData.reports statusFilter = routeData.statusFilter status = 'ready' message = null lastLoadedHref = href return true }Also applies to: 250-254
🤖 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 `@packages/worker/client/routes/admin-community-reports.tsx` around lines 227 - 240, The applyRouteLoaderData/loadReports flow in adminCommunityReports is allowing an older fallback request to overwrite newer route-loader data. Update applyRouteLoaderData and the loadReports path so that when tryConsumeRouteLoaderData wins, any in-flight fallback request is invalidated (for example by bumping the request token or marking the current load stale) before setting reports/status/message, and ensure the loadRequestId check in loadReports cannot accept the obsolete response afterward.packages/worker/client/routes/admin-roles.tsx (1)
101-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvalidate stale fallback loads when applying route-loader data.
An in-flight
loadAdminRoles()started before navigation can still pass the unchangedloadRequestIdcheck after preloaded role data is applied, then overwrite the committed route-loader result.Proposed fix
function applyRouteLoaderData(href: string) { if (!isAdminRolesPath(href)) return false const routeData = tryConsumeRouteLoaderData(handle, 'adminRoles', href) if (!routeData) return false + loadRequestId += 1 + loadingForHref = null + lastFailedHref = null roles = routeData.roles status = 'ready' message = null lastLoadedHref = href return true }Also applies to: 117-121
🤖 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 `@packages/worker/client/routes/admin-roles.tsx` around lines 101 - 109, The route-loader commit path in applyRouteLoaderData should also invalidate any in-flight loadAdminRoles() fallback request so it cannot overwrite the newly applied data. Update the state handling around applyRouteLoaderData and loadAdminRoles to bump or reset the shared loadRequestId (or equivalent guard) whenever preloaded adminRoles data is consumed, ensuring any older request started before navigation fails its commit check and cannot replace the committed route-loader result.packages/worker/client/routes/admin-users.tsx (1)
212-233: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvalidate stale fallback loads before showing preloaded admin users.
If a previous
loadAdminUsers()fallback finishes after route-loader data is applied, it can still pass the unchangedloadRequestIdguard and overwrite the committed users/page state.Proposed fix
function applyRouteLoaderData(href: string) { if (!isAdminUsersPath(href)) return false const routeData = tryConsumeRouteLoaderData(handle, 'adminUsers', href) if (!routeData) return false + loadRequestId += 1 + loadingForHref = null + lastFailedHref = null users = routeData.users availableRoles = routeData.availableRoles page = routeData.page pageSize = routeData.pageSize total = routeData.totalAlso applies to: 242-246
🤖 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 `@packages/worker/client/routes/admin-users.tsx` around lines 212 - 233, The admin users route can still be overwritten by a late fallback load because applyRouteLoaderData commits preloaded data without invalidating any in-flight loadAdminUsers() request that shares the same loadRequestId guard. Update applyRouteLoaderData in admin-users.tsx to bump or reset the request token/state used by loadAdminUsers() so any older fallback response is treated as stale, and make sure the same invalidation is applied wherever the committed route-loader data is handled to prevent it from reapplying users/page after preloaded data is shown.
🧹 Nitpick comments (4)
packages/worker/client/frame-prefetch.ts (1)
21-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated header-building logic with
entry.tsx.The
Accept: text/html+ optionalREMIX_FRAME_TARGET_HEADERconstruction here duplicates the equivalent block inentry.tsx'sresolveFramefallback. Extracting a shared helper keeps both call sites in sync if the header contract changes.♻️ Proposed extraction
+export function buildFrameHeaders(target: string | undefined): Headers { + const headers = new Headers({ Accept: 'text/html' }) + if (target) { + headers.set(REMIX_FRAME_TARGET_HEADER, target) + } + return headers +} + export async function prefetchFrame( src: string, target: string | undefined, signal: AbortSignal, ): Promise<void> { clearPrefetchedFrames() try { - const headers = new Headers({ Accept: 'text/html' }) - if (target) { - headers.set(REMIX_FRAME_TARGET_HEADER, target) - } + const headers = buildFrameHeaders(target) const response = await fetch(src, { headers, signal })Then in
entry.tsx:- const headers = new Headers({ Accept: 'text/html' }) - if (target) { - headers.set(REMIX_FRAME_TARGET_HEADER, target) - } + const headers = buildFrameHeaders(target)🤖 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 `@packages/worker/client/frame-prefetch.ts` around lines 21 - 41, The header construction in prefetchFrame duplicates the same Accept/REMIX_FRAME_TARGET_HEADER logic used in entry.tsx’s resolveFrame fallback, so extract that shared logic into a reusable helper and have both call sites use it. Update prefetchFrame to delegate header creation to the helper, and adjust the resolveFrame fallback in entry.tsx to use the same helper so any future header contract changes stay consistent in one place.packages/worker/client/routes/oauth-authorize.tsx (1)
51-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fetch/validate logic with
loadInfo.This loader re-implements the same fetch →
.json().catch(() => null)→ok/payload.okvalidation performed inloadInfo(Lines 108-148). Extracting a sharedfetchAuthorizeInfo(query, signal?)helper used by both the route loader and the fallback path would reduce the risk of the two implementations drifting when the/oauth/authorize-infocontract changes.🤖 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 `@packages/worker/client/routes/oauth-authorize.tsx` around lines 51 - 82, The oauthAuthorizeRouteLoader function duplicates the same `/oauth/authorize-info` fetch and response validation logic already implemented in loadInfo. Extract a shared helper such as fetchAuthorizeInfo(query, signal?) that performs the fetch, JSON parsing, and ok/payload.ok handling, then have both oauthAuthorizeRouteLoader and loadInfo call it so the contract stays consistent if the endpoint changes.packages/worker/client/routes/community-detail.tsx (2)
51-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new route loader.
communityDetailRouteLoaderhas several distinct branches (missing listingId, 404, non-ok response/payload, success) but no accompanying unit test in this cohort, unlike the router/navigation-data layers earlier in the stack. Given this sits on the critical navigation path, tests would help lock in the fallback-to-fetch contract.
Want me to draft unit tests for the branches (missing id, 404, non-ok payload, success)?🤖 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 `@packages/worker/client/routes/community-detail.tsx` around lines 51 - 91, Add unit test coverage for communityDetailRouteLoader to lock in its fallback-to-fetch behavior across all branches. Use the communityDetailRouteLoader symbol and cover missing listingId, 404 from the API, non-ok response or payload, and the successful return shape with frame prefetch. Mock getListingIdFromPathname, fetch, readJson, and prefetchFrame so the tests assert the correct errors and returned communityDetailShell data without relying on real network behavior.
51-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared shell-fetch logic.
communityDetailRouteLoaderre-implements the fetch → 404-check →readJson→ok-check sequence thatloadDetailShell(below) already contains. Factoring this into a small shared helper (e.g.fetchCommunityDetailShell(listingId, signal?)) would keep the API contract in one place and prevent the two paths from drifting apart as the endpoint evolves.🤖 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 `@packages/worker/client/routes/community-detail.tsx` around lines 51 - 91, Extract the shared community detail shell loading sequence so it is implemented in one place instead of duplicated between communityDetailRouteLoader and loadDetailShell. Create or reuse a small helper that performs the fetch, 404 handling, readJson parsing, and ok validation, then have communityDetailRouteLoader call that helper before building communityDetailShell. Keep the helper aligned with the existing loadDetailShell behavior so both paths use the same API contract.
🤖 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 `@packages/worker/client/client-router.tsx`:
- Around line 255-259: commitImmediateNavigation currently always emits
navigationstart/navigationend and ignores the suppressStart option, which can
create an extra unpaired navigation event in the hash-only fast path. Update
commitImmediateNavigation in client-router.tsx to accept and honor the same
options used by runNavigationWithLoader, and make navigateInternal pass through
options so redirects from navigateWithRefreshForSamePath and
submitFormThroughRouter with suppressStart true skip dispatchNavigationStart
consistently. Keep the start/end pairing behavior aligned with the existing
navigation contract.
- Around line 330-379: The form submit flow in submitFormThroughRouter is
missing the shared navigation cancellation behavior used by
runNavigationWithLoader, so a stale POST can still redirect after a newer
navigation. Update the POST fetch in submitFormThroughRouter to use the shared
navigation abort controller signal, and before calling
navigateWithRefreshForSamePath on response.redirected or Location-based
redirects, verify the current navigation is still active and bail out if it was
superseded. Keep the abort and stale-navigation checks aligned with the existing
navigation helpers (navigationAbortController, navigateWithRefreshForSamePath,
navigateInternal).
---
Outside diff comments:
In `@packages/worker/client/routes/admin-community-reports.tsx`:
- Around line 227-240: The applyRouteLoaderData/loadReports flow in
adminCommunityReports is allowing an older fallback request to overwrite newer
route-loader data. Update applyRouteLoaderData and the loadReports path so that
when tryConsumeRouteLoaderData wins, any in-flight fallback request is
invalidated (for example by bumping the request token or marking the current
load stale) before setting reports/status/message, and ensure the loadRequestId
check in loadReports cannot accept the obsolete response afterward.
In `@packages/worker/client/routes/admin-roles.tsx`:
- Around line 101-109: The route-loader commit path in applyRouteLoaderData
should also invalidate any in-flight loadAdminRoles() fallback request so it
cannot overwrite the newly applied data. Update the state handling around
applyRouteLoaderData and loadAdminRoles to bump or reset the shared
loadRequestId (or equivalent guard) whenever preloaded adminRoles data is
consumed, ensuring any older request started before navigation fails its commit
check and cannot replace the committed route-loader result.
In `@packages/worker/client/routes/admin-users.tsx`:
- Around line 212-233: The admin users route can still be overwritten by a late
fallback load because applyRouteLoaderData commits preloaded data without
invalidating any in-flight loadAdminUsers() request that shares the same
loadRequestId guard. Update applyRouteLoaderData in admin-users.tsx to bump or
reset the request token/state used by loadAdminUsers() so any older fallback
response is treated as stale, and make sure the same invalidation is applied
wherever the committed route-loader data is handled to prevent it from
reapplying users/page after preloaded data is shown.
---
Nitpick comments:
In `@packages/worker/client/frame-prefetch.ts`:
- Around line 21-41: The header construction in prefetchFrame duplicates the
same Accept/REMIX_FRAME_TARGET_HEADER logic used in entry.tsx’s resolveFrame
fallback, so extract that shared logic into a reusable helper and have both call
sites use it. Update prefetchFrame to delegate header creation to the helper,
and adjust the resolveFrame fallback in entry.tsx to use the same helper so any
future header contract changes stay consistent in one place.
In `@packages/worker/client/routes/community-detail.tsx`:
- Around line 51-91: Add unit test coverage for communityDetailRouteLoader to
lock in its fallback-to-fetch behavior across all branches. Use the
communityDetailRouteLoader symbol and cover missing listingId, 404 from the API,
non-ok response or payload, and the successful return shape with frame prefetch.
Mock getListingIdFromPathname, fetch, readJson, and prefetchFrame so the tests
assert the correct errors and returned communityDetailShell data without relying
on real network behavior.
- Around line 51-91: Extract the shared community detail shell loading sequence
so it is implemented in one place instead of duplicated between
communityDetailRouteLoader and loadDetailShell. Create or reuse a small helper
that performs the fetch, 404 handling, readJson parsing, and ok validation, then
have communityDetailRouteLoader call that helper before building
communityDetailShell. Keep the helper aligned with the existing loadDetailShell
behavior so both paths use the same API contract.
In `@packages/worker/client/routes/oauth-authorize.tsx`:
- Around line 51-82: The oauthAuthorizeRouteLoader function duplicates the same
`/oauth/authorize-info` fetch and response validation logic already implemented
in loadInfo. Extract a shared helper such as fetchAuthorizeInfo(query, signal?)
that performs the fetch, JSON parsing, and ok/payload.ok handling, then have
both oauthAuthorizeRouteLoader and loadInfo call it so the contract stays
consistent if the endpoint changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d9292e3-266c-4e2f-bec6-8117b9b60861
📒 Files selected for processing (22)
docs/contributing/architecture/request-lifecycle.mdpackages/worker/client/app.tsxpackages/worker/client/client-router.node.test.tspackages/worker/client/client-router.tsxpackages/worker/client/entry.tsxpackages/worker/client/frame-prefetch.tspackages/worker/client/loader-data-context.tsxpackages/worker/client/navigation-data.node.test.tspackages/worker/client/navigation-data.tspackages/worker/client/navigation-progress.tsxpackages/worker/client/routes/account-integrations.tsxpackages/worker/client/routes/account-package-invocation-tokens.tsxpackages/worker/client/routes/account-remote-connectors.tsxpackages/worker/client/routes/account-secrets.tsxpackages/worker/client/routes/account.tsxpackages/worker/client/routes/admin-community-reports.tsxpackages/worker/client/routes/admin-roles.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/client/routes/community-detail.tsxpackages/worker/client/routes/community.tsxpackages/worker/client/routes/index.tsxpackages/worker/client/routes/oauth-authorize.tsx
… pending loader on hash navigation
… hash commits, store preloads only at commit
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d44e488. Configure here.
This was referenced Jul 5, 2026
This was referenced Jul 13, 2026
This was referenced Jul 22, 2026
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
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.
What
Changes client-side navigation from "swap UI, then fetch, then update again" to React Router-style preload-then-commit: on navigation the router first runs a client loader that fetches the destination route's data, and only then commits the URL change and UI swap — so the UI updates exactly once, already populated. Adds a subtle 3px progress bar along the very top of the page (spin-delay semantics: appears only after 150ms pending, shown at least 200ms — no spinner). Also adds intent prefetch (like React Router's
prefetch="intent"): hovering, focusing, or touching an internal link speculatively starts its route loader, and the click adopts the in-flight or settled result instead of refetching.Nav across account/secrets/integrations/tokens/connectors/admin/community renders each page fully populated in a single update — no "Loading …" flashes.
Progress bar demo (with a temporary 1.5s artificial loader delay, since local loads complete under the 150ms threshold): the previous page stays visible while the bar trickles, then the new page swaps in complete.
Intent prefetch demo: hovering the "Secrets" nav link fires
secrets.jsonbefore any click (visible in the DevTools network log); the click then navigates instantly and the log still shows exactly one request — the prefetch was reused.How
client-router.tsx: route-loader registry (registerRouteLoaders, matched withremix/route-pattern), asyncnavigate()that aborts superseded navigations (latest wins), runs the matched loader, then commits (pushState+ notify). Applies to link clicks, popstate (old UI stays until data is ready), GET/POST form submissions, and same-path refreshes after POST redirects. Hash-only moves — forward clicks and popstate — commit immediately without running loaders. Newnavigationstart/navigationendevents drive the progress bar. Loader errors commit anyway and let the route's fallback fetch surface the error; the router marks the destination stale so same-path refreshes (no href change) still trigger that fallback refetch.route-loader.ts: loaders now returnRouteLoaderRedirect(viarouteLoaderRedirect) instead of callingwindow.location.assignthemselves; the router performs the full-document redirect at commit time. This keeps speculative prefetch runs side-effect free — hovering a link whose loader hits a 401 can never redirect the page the user is still on.intent-prefetch.ts: single latest-wins prefetch slot. Hover intent is debounced (100ms dwell) so sweeping the cursor across a nav list fires nothing; focus and touch prefetch immediately (and cancel any pending hover timer so it can't fire later and clobber the deliberate prefetch). Navigations adopt the pending or freshly settled promise (30s TTL, monotonic clock); failed prefetches fall back to a normal loader run; hovering a different link aborts the previous prefetch; form POSTs abort pending prefetches so pre-mutation data is never adopted. Navigating anywhere other than the prefetched link aborts and clears the slot, so a speculative result for a link the user never followed can't be adopted later. Prefetch is skipped for the current page and for the path an active navigation is already loading. Opt out per link withdata-prefetch="none".navigation-data.ts: single-slot, consume-once preloaded data store;tryConsumeRouteLoaderDatainloader-data-context.tsxunifies SSR-embedded and navigation-preloaded consumption. Also holds the one-shot stale marker (markNavigationDataStale/consumeStaleNavigationData) routes consume to force a refetch after a failed same-path refresh.navigation-progress.tsx: fixed top bar,colors.primary, trickles to 90%, completes and fades on end; spin-delay-style 150ms show delay + 200ms minimum visibility.navigate()commits — the async navigation made that optimistic set race interim renders and fire spurious fallback fetches for the old URL.frame-prefetch.ts: community pages prefetch their<Frame>HTML in the loader;resolveFrameinentry.tsxconsumes the cache (consume-once, cleared per prefetch) so frame content is instant after commit.Testing
npm run validate— all six checks green (format, lint, typecheck, unit tests, Playwright E2E, MCP E2E).navigationend, and the progress bar's pending flag is a boolean, so the winning navigation's end always clears it.System recap — extends app-ui (medium risk)
Mode: recap · Base:
main@e51fa91· Head:f5cef55Classification: extends — changes the browser app's client navigation contract (preload-then-commit + intent prefetch) and adds a navigation progress indicator; no new primitives, no server route changes.
Primitives touched
app-uiSystem map
Change flow
Before / after
window.location.assigndirectlyInvariants
Per-user isolation unaffected: loaders call the same authenticated JSON APIs the routes already used; 401s still redirect to login at commit time — and never from a speculative prefetch.
Summary by CodeRabbit