Repository navigation
Remix 3 SSR: server-rendered, hydrated SPA for all HTML pages - #601
Merged
Merged
Conversation
… run() hydration, injected router location, session + loader-data embedding for home/privacy/community
…d embedded loader data; add /reset-password HTML route
…ned immutable assets, modulepreload), SSR test coverage, and URL-scoped one-time embedded loader data consumption
…roke SPA refetch) and cover secrets SPA navigation in smoke E2E
|
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 a shared SSR rendering path, typed loader-data contracts, request-scoped auth/session resolution, and router-aware client hydration. Worker handlers and client routes now use shared loaders and embedded SSR data instead of direct window reads. ChangesSSR, loaders, and router-aware app runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 09:23
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
🔎 Preview deployed: https://kody-pr-601.kentcdodds.workers.dev Worker: Mocks:
|
…retry storm exhausted auth rate limit in CI)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
packages/worker/client/routes/community-detail.tsx (1)
96-139: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInfinite retry loop for a 404/errored community listing.
lastLoadedListingIdis only updated on the success path (Line 128). The 404 branch (Lines 111-116) and catch branch (Lines 130-138) leave it unset, so the render check at Line 230-233 (listingId && listingId !== lastLoadedListingId) stays true forever, repeatedly re-queuingloadCommunityDetailfor a listing that will keep 404ing.🐛 Proposed fix: stamp lastLoadedListingId on all terminal outcomes
async function loadCommunityDetail() { const listingId = getCurrentListingId(handle) if (!listingId) { status = 'error' message = 'Community package not found.' handle.update() return } const requestId = ++loadRequestId try { const response = await fetch(buildDetailApiPath(listingId), { headers: { Accept: 'application/json' }, }) if (requestId !== loadRequestId) return if (response.status === 404) { status = 'error' message = 'Community package not found.' + lastLoadedListingId = listingId handle.update() return } ... } catch (error) { if (requestId !== loadRequestId) return status = 'error' message = error instanceof Error ? error.message : 'Unable to load community package.' + lastLoadedListingId = listingId handle.update() } }Also applies to: 227-240
🤖 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 96 - 139, The community detail loader can retry forever because loadCommunityDetail only updates lastLoadedListingId on success, leaving 404 and catch paths to re-trigger the render guard in the community-detail route. Update loadCommunityDetail so terminal failures also stamp lastLoadedListingId (or otherwise mark the listing as handled) in the 404 branch and the catch block, keeping the retry check from re-queuing the same listing. Use the existing identifiers loadCommunityDetail, lastLoadedListingId, and the render condition around listingId !== lastLoadedListingId to locate the fix.packages/worker/client/routes/admin-roles.tsx (1)
33-67: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInfinite retry loop when the admin-roles fetch fails.
lastLoadedHrefis only assigned on the success path (Line 58). The 403 branch (Lines 45-50) and the catch branch (Lines 60-66) leave it at''. Since the render check at Line 92 (status === 'loading' || currentHref !== lastLoadedHref) stays true forever after an error, everyhandle.update()re-triggersapplyEmbeddedLoaderData→handle.queueTask(loadAdminRoles)again, forming an unbounded fetch loop against/admin/roles.jsonwhenever the user lacks permission or the network fails.🐛 Proposed fix: stamp lastLoadedHref before starting the request
async function loadAdminRoles() { const requestId = ++loadRequestId + const href = readCurrentRouterHref(handle) try { const response = await fetch(adminRolesApiPath, { headers: { Accept: 'application/json' }, credentials: 'include', }) if (requestId !== loadRequestId) return if (response.status === 401) { window.location.assign('/login') return } if (response.status === 403) { status = 'error' message = 'You do not have permission to view admin roles.' + lastLoadedHref = href handle.update() return } const payload = await readJson<AdminRolesLoaderData>(response) if (!response.ok || !payload?.ok) { throw new Error('Unable to load admin roles.') } roles = payload.roles status = 'ready' message = null - lastLoadedHref = readCurrentRouterHref(handle) + lastLoadedHref = href handle.update() } catch (error) { if (requestId !== loadRequestId) return status = 'error' message = error instanceof Error ? error.message : 'Unable to load admin roles.' + lastLoadedHref = href handle.update() } }Also applies to: 89-99
🤖 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 33 - 67, The admin roles loader can retrigger forever after a failed fetch because lastLoadedHref is only set on the success path in loadAdminRoles. Update loadAdminRoles in admin-roles.tsx so lastLoadedHref is stamped before issuing the request (or otherwise set on every attempt), ensuring the render guard comparing currentHref to lastLoadedHref does not keep scheduling applyEmbeddedLoaderData after 401/403/error paths. Keep the existing status/message handling intact, and make sure the load request flow around loadRequestId and handle.update() still works.packages/worker/client/routes/admin-users.tsx (1)
75-123: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winSame infinite retry loop bug as
admin-roles.tsxon load failure.
lastLoadedHrefis set only on the success path (Line 103). The 403 branch (Lines 88-93) and catch branch (Lines 116-122) never set it, so the render check at Line 223 (status === 'loading' || currentHref !== lastLoadedHref) remains true after every failed load, causingloadAdminUsersto be re-queued and re-fail indefinitely.🐛 Proposed fix: stamp lastLoadedHref on all outcomes
async function loadAdminUsers() { const href = readCurrentRouterHref(handle) const requestId = ++loadRequestId try { const response = await fetch( `${adminUsersApiPath}${readRouterSearch(handle)}`, { headers: { Accept: 'application/json' }, credentials: 'include' }, ) if (requestId !== loadRequestId) return if (response.status === 401) { window.location.assign('/login') return } if (response.status === 403) { status = 'error' message = 'You do not have permission to view admin users.' + lastLoadedHref = href handle.update() return } ... } catch (error) { if (requestId !== loadRequestId) return status = 'error' message = error instanceof Error ? error.message : 'Unable to load admin users.' + lastLoadedHref = href handle.update() } }Also applies to: 217-230
🤖 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 75 - 123, `loadAdminUsers` can get stuck in an infinite retry loop because `lastLoadedHref` is only updated on the success path, so failed loads never satisfy the render guard that compares `currentHref` to `lastLoadedHref`. Update `lastLoadedHref` for every terminal outcome in `loadAdminUsers`—including the 403 branch and the catch path—so the loader doesn’t re-queue after a failure. Keep the existing success behavior, and make sure the render condition that checks `status === 'loading' || currentHref !== lastLoadedHref` can settle after any completed request.
🧹 Nitpick comments (12)
e2e/ssr-hydration.spec.ts (1)
56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic analysis flags dynamic RegExp construction.
alphaListing.listingIdis a fixed test constant, not attacker-controlled input, so the ReDoS/injection risk here is negligible in practice.🤖 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 `@e2e/ssr-hydration.spec.ts` around lines 56 - 59, The URL assertion in ssr-hydration.spec.ts uses dynamic RegExp construction with alphaListing.listingId, which triggers static analysis even though the value is a fixed test constant. Update the page.toHaveURL check in the relevant test to avoid constructing the pattern dynamically, and use a safer direct assertion or a static regex shape in the assert around alphaListing and its listingId.Source: Linters/SAST tools
packages/worker/client/loader-data-types.ts (1)
77-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
AccountIntegrationListItem/flowtype risks drifting from the shared contract.This file's
AccountIntegrationListItem.flowis an inline'pkce' | 'confidential'union, whileroutes/account-integrations.tsxindependently defines a same-named type usingflow: IntegrationFlow. The file's own doc comment states these loader types are meant to be shared with route components and JSON API shapes — having two separately maintained definitions undermines that.Consider importing and reusing the shared
IntegrationFlowtype here instead of re-declaring the union.🤖 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/loader-data-types.ts` around lines 77 - 96, `AccountIntegrationListItem.flow` is duplicating the shared `IntegrationFlow` contract, which can drift from the route/component type definitions. Update `AccountIntegrationListItem` in `loader-data-types.ts` to reuse the existing `IntegrationFlow` type instead of the inline union, keeping the loader shape aligned with `routes/account-integrations.tsx` and the shared API contract.packages/worker/src/app/handlers/auth-page.ts (1)
11-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLogin and signup pages will share the same (generic)
<title>.
createAuthPageHandlernow backs both/loginand/signup(viacreateLoginHandler/createSignupHandler), butrenderAppPage({ request, env })at Line 36 passes notitle, so both routes fall back to whatever generic defaultSsrDocumentuses. Now that login/signup are separate factory calls, it's cheap to pass a distinguishing title through.♻️ Proposed fix to differentiate titles
-export function createAuthPageHandler(env: Env) { +export function createAuthPageHandler(env: Env, title: string) { return { middleware: [], async handler({ request }: { request: Request }) { ... - return renderAppPage({ request, env }) + return renderAppPage({ request, env, title }) }, } }-export function createLoginHandler(env: Env) { - return createAuthPageHandler(env) satisfies Action<typeof routes.login> +export function createLoginHandler(env: Env) { + return createAuthPageHandler(env, 'Log in') satisfies Action<typeof routes.login> }🤖 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/src/app/handlers/auth-page.ts` around lines 11 - 36, createAuthPageHandler currently calls renderAppPage({ request, env }) without a page title, so both createLoginHandler and createSignupHandler end up using the same default SsrDocument title. Update the auth-page flow to pass a distinguishing title through renderAppPage for each route, using the createAuthPageHandler / createLoginHandler / createSignupHandler entry points to select the appropriate login or signup title while keeping the existing redirect and session logic unchanged.packages/worker/src/app/handlers/admin-roles.ts (1)
16-27: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the redundant session check.
requireUserWithRole(request, env, 'admin')already rejects unauthenticated requests through the same auth path, so the follow-upreadAuthSessionResult(request)/redirectToLogin(request)branch can be dropped.🤖 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/src/app/handlers/admin-roles.ts` around lines 16 - 27, The admin role handler currently performs a redundant authentication fallback after requireUserWithRole(request, env, 'admin'). Remove the extra readAuthSessionResult(request) and redirectToLogin(request) branch from the admin-roles handler, since requireUserWithRole already handles unauthenticated requests; keep the existing Response passthrough and error rethrow behavior intact.packages/worker/client/routes/login.tsx (1)
47-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent use of the safe
getPathnamewrapper vs rawreadRouterPathname.
getPathname(handle)(defined inclient-router.tsx) wrapsreadRouterPathnamein a try/catch with a window fallback, butgetCurrentAuthMode/getCurrentRedirectTocallreadRouterPathname/readRouterSearchdirectly. SinceLoginRouteonly runs inside the router-provided tree this is likely safe in practice, but the asymmetry is worth a second look in case these helpers are ever reused outside that context.🤖 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/login.tsx` around lines 47 - 64, The auth helper logic in LoginRoute is bypassing the safer router wrappers by calling readRouterPathname and readRouterSearch directly in getCurrentAuthMode and getCurrentRedirectTo. Update these helpers to use the existing getPathname-style fallback approach (or equivalent safe wrappers from client-router.tsx) so pathname/search access stays consistent and resilient if reused outside the router tree.packages/worker/client/routes/admin-roles.tsx (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the
is*Pathhelper pattern.
isAdminRolesPathmirrors near-identical helpers inadmin-users.tsx,account-integrations.tsx,account-remote-connectors.tsx, etc. A small sharedpathnameOf(href)/matchesPathutility would reduce duplication across route files.🤖 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 20 - 24, The isAdminRolesPath helper duplicates the same pathname parsing logic found in other route files, so extract the shared pattern into a common utility such as pathnameOf(href) and/or matchesPath, then update this file and the similar helpers in admin-users.tsx and account-integrations.tsx/account-remote-connectors.tsx to use it. Keep the route-specific path comparison in each caller, but centralize the URL-to-pathname normalization to remove repeated new URL(href, 'http://localhost').pathname logic.packages/worker/client/routes/account-integrations.tsx (1)
150-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated router-hydration boilerplate across seven route files.
This exact
listenToRouterNavigation+applyEmbeddedLoaderData+queueTaskfallback block is copy-pasted nearly verbatim intoaccount.tsx,account-package-invocation-tokens.tsx,account-remote-connectors.tsx,account-secrets.tsx,community.tsx, andadmin-community-reports.tsx. The duplication has already caused drift:admin-community-reports.tsx's listener omits thestatus !== 'loading'guard that every other copy has (harmless today, but shows how easy it is for these copies to diverge).Consider extracting a small shared helper (e.g. in
client-router.tsxorloader-data-context.tsx) that encapsulates: register nav listener → transition to loading → try embedded data → fallback toqueueTask. This would remove ~30 duplicated lines per route and prevent this kind of silent divergence going forward.🤖 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/account-integrations.tsx` around lines 150 - 183, The router-hydration logic is duplicated across multiple route components and has already drifted, so extract the shared navigation/loading/embedded-data fallback flow into a reusable helper. Move the repeated `listenToRouterNavigation`, `applyEmbeddedLoaderData`, and `queueTask(loadIntegrations)` pattern into a common utility such as `client-router.tsx` or `loader-data-context.tsx`, then update `account.tsx`, `account-package-invocation-tokens.tsx`, `account-remote-connectors.tsx`, `account-secrets.tsx`, `community.tsx`, and `admin-community-reports.tsx` to call it. Keep the existing `handle`, `readCurrentRouterHref`, `tryConsumeEmbeddedLoaderData`, and `loadIntegrations` behavior intact while centralizing the `status` transition and embedded-data check.packages/worker/src/app/request-auth-cache.ts (1)
80-89: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider surfacing role-lookup failures beyond
console.error.On
getUserRolesAndPermissionsfailure, the user is still authenticated but silently downgraded to zero roles/permissions. This fail-closed behavior is reasonable, but a transient DB blip could cause admins to see authz-denied pages that look like a permissions bug rather than an outage. A metric/counter here would make this failure mode observable in production.🤖 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/src/app/request-auth-cache.ts` around lines 80 - 89, The role/permission lookup in request-auth-cache.ts only logs to console when getUserRolesAndPermissions fails, so add an observable signal there such as a metric/counter or structured app logger entry in the catch block around the getUserRolesAndPermissions call. Use the existing request auth path in request-auth-cache.ts and the getUserRolesAndPermissions flow to record lookup failures while preserving the current fail-closed empty roles/permissions fallback.packages/worker/src/app/account-secrets-data.ts (2)
320-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoader-data shapes duplicated locally instead of reused from
loader-data.ts.
AccountSecretListItem/AccountSecretDetail(and the analogousSecretApprovalView) are redefined here even though equivalent shapes are already exported from#app/loader-data.ts. Same pattern shows up inadmin-community-reports-data.ts. Importing the shared types would avoid drift if one side is updated without the other. Also, the two trailingexport {}statements (lines 428-431) could be merged into one.🤖 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/src/app/account-secrets-data.ts` around lines 320 - 431, The loader-data types are duplicated in this module instead of reusing the shared exports from loader-data.ts, which can drift over time. Update account-secrets-data.ts to import and use the existing shared shapes for AccountSecretListItem, AccountSecretDetail, and the analogous SecretApprovalView where applicable, and align any matching code in toAccountSecretListItem and related helpers to those shared types. Also consolidate the trailing export statements at the end of the file into a single export block.
77-88: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDead validation branch in
readSelectedSecretIdFromPath.
parsedLegacyIdis computed but doesn't affect the return value: both branches returnlegacySecretIdwhenever it's non-empty, andnullonly when it's empty — identical toreturn legacySecretId || null. This reads as if invalid legacy ids are being rejected, but they aren't.♻️ Proposed simplification (or fix if rejection was intended)
if (pathname.startsWith(`${secretsBasePath}/`)) { const legacySecretId = pathname.slice(`${secretsBasePath}/`.length) - const parsedLegacyId = parseAccountSecretId(legacySecretId) - return parsedLegacyId ? legacySecretId : legacySecretId || null + return legacySecretId || null }🤖 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/src/app/account-secrets-data.ts` around lines 77 - 88, `readSelectedSecretIdFromPath` contains a dead validation branch because `parsedLegacyId` is computed but never changes the returned value. Either remove the unused `parseAccountSecretId` check and simplify the legacy path handling to return the sliced legacy id or null, or, if invalid legacy ids should be rejected, change the `parseAccountSecretId` result to actually gate the return in the `readSelectedSecretIdFromPath` function.packages/worker/src/app/request-cache.ts (1)
18-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSingle-slot cache-lookup store loses information if a route composes multiple cached loaders.
setRequestDataCacheLookupoverwrites any prior value, so if a future handler calls multiple cache-backed loaders for one request, only the last hit/miss status survives into theX-Kody-Cacheheader. Not an issue today (each current handler calls it at most once), but worth keeping in mind before adding multi-loader pages.🤖 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/src/app/request-cache.ts` around lines 18 - 29, The request cache metadata currently stores only one DataCacheLookup via setRequestDataCacheLookup, so later loaders can overwrite earlier cache hits/misses before getRequestDataCacheLookup reads them back for the X-Kody-Cache header. Update the requestMetaStore-backed request metadata to accumulate or merge multiple lookups per request instead of replacing the previous value, and adjust getRequestDataCacheLookup and any header-building code to consume the combined result; use setRequestDataCacheLookup and getRequestDataCacheLookup as the key entry points.packages/worker/src/app/data-cache.ts (1)
65-78: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo de-duplication of concurrent cache misses (thundering herd).
getOrSetDataCachere-invokesload()for every concurrent request that misses the same key, rather than sharing one in-flight promise. Under a burst of requests right after TTL expiry (e.g. a popular community listing), this can cause N duplicate D1 reads instead of one.♻️ Suggested in-flight promise de-duplication
+const inFlight = new Map<string, Promise<unknown>>() + export async function getOrSetDataCache<T>(input: { key: string ttlMs?: number load: () => Promise<T> }): Promise<{ value: T; lookup: DataCacheLookup }> { const cached = peekDataCache<T>(input.key) if (cached !== undefined) { return { value: cached, lookup: 'hit' } } - const value = await input.load() - setDataCache(input.key, value, input.ttlMs) - return { value, lookup: 'miss' } + let pending = inFlight.get(input.key) as Promise<T> | undefined + if (!pending) { + pending = input.load().finally(() => inFlight.delete(input.key)) + inFlight.set(input.key, pending) + } + const value = await pending + setDataCache(input.key, value, input.ttlMs) + return { value, lookup: 'miss' } }🤖 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/src/app/data-cache.ts` around lines 65 - 78, getOrSetDataCache currently only caches completed values, so concurrent misses for the same key all call load() independently. Update getOrSetDataCache to de-duplicate in-flight loads by tracking a per-key pending promise in the data-cache module and having later callers await it instead of starting a new load. Keep the existing cache hit/miss behavior intact and clear the in-flight entry after the promise settles; use getOrSetDataCache, peekDataCache, and setDataCache as the main touchpoints.
🤖 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 `@e2e/d1-utils.ts`:
- Around line 86-135: The upsert in seedCommunityListingInE2eDatabase leaves
owner_user_id stale on reseed because it is not included in the ON CONFLICT DO
UPDATE SET clause. Update the conflict handling so the reseeded record refreshes
owner_user_id alongside the other mutable fields, using the same ownerUserId
derived from input.ownerEmail. Keep the fix within
seedCommunityListingInE2eDatabase and preserve the existing insert/default
behavior for listingId, packageId, sourceId, and kodyId.
In `@packages/worker/client/client-router.tsx`:
- Around line 308-329: The Router render logic currently relies on
handle.props.notFound, which is only set from the initial SSR payload and can
stay stuck on client-side navigation. Update Router and/or
listenToRouterNavigation so the notFound state is cleared or tracked per
pathname change before rendering, and use readRouterPathname plus matchRoute to
decide when to stop showing handle.props.fallback. Keep the fix localized to
Router, handle.update, and the navigation listener so SPA transitions can
recover from an initial 404.
In `@packages/worker/src/app/admin-community-reports-data.ts`:
- Around line 18-47: `loadAdminCommunityReportsData` is returning the raw query
value for `statusFilter`, but `loadAdminCommunityReports` applies normalization
before fetching reports, so the response can disagree with the actual filter
used. Normalize the status once in `loadAdminCommunityReportsData` (using
`readReportStatusFilter` or the same `all`/default handling as
`loadAdminCommunityReports`) and pass that normalized value into
`loadAdminCommunityReports`, then return that same value in the `statusFilter`
field so the loader data matches the displayed reports.
In `@packages/worker/src/app/data-cache.ts`:
- Line 20: The in-memory cache in data-cache.ts is unbounded because `store`
only grows via `setDataCache` and expired entries are removed lazily in
`peekDataCache`, so unique `q` values can accumulate indefinitely. Update the
cache behavior around `store`, `setDataCache`, and `peekDataCache` to enforce a
maximum size with oldest/LRU eviction or add a periodic expired-entry sweep so
entries do not remain past TTL unless they are revisited.
In `@packages/worker/src/app/ssr-document.tsx`:
- Around line 46-47: The SSR document still hardcodes the stylesheet as
/styles.css while clientEntryHref is versioned, so update the ssr-document
rendering to version the stylesheet link too. In ssr-document.tsx, keep the
existing modulepreload behavior but change the stylesheet reference to use the
same build-id/query-param approach as clientEntryHref so deploys don’t serve
stale CSS.
---
Outside diff comments:
In `@packages/worker/client/routes/admin-roles.tsx`:
- Around line 33-67: The admin roles loader can retrigger forever after a failed
fetch because lastLoadedHref is only set on the success path in loadAdminRoles.
Update loadAdminRoles in admin-roles.tsx so lastLoadedHref is stamped before
issuing the request (or otherwise set on every attempt), ensuring the render
guard comparing currentHref to lastLoadedHref does not keep scheduling
applyEmbeddedLoaderData after 401/403/error paths. Keep the existing
status/message handling intact, and make sure the load request flow around
loadRequestId and handle.update() still works.
In `@packages/worker/client/routes/admin-users.tsx`:
- Around line 75-123: `loadAdminUsers` can get stuck in an infinite retry loop
because `lastLoadedHref` is only updated on the success path, so failed loads
never satisfy the render guard that compares `currentHref` to `lastLoadedHref`.
Update `lastLoadedHref` for every terminal outcome in `loadAdminUsers`—including
the 403 branch and the catch path—so the loader doesn’t re-queue after a
failure. Keep the existing success behavior, and make sure the render condition
that checks `status === 'loading' || currentHref !== lastLoadedHref` can settle
after any completed request.
In `@packages/worker/client/routes/community-detail.tsx`:
- Around line 96-139: The community detail loader can retry forever because
loadCommunityDetail only updates lastLoadedListingId on success, leaving 404 and
catch paths to re-trigger the render guard in the community-detail route. Update
loadCommunityDetail so terminal failures also stamp lastLoadedListingId (or
otherwise mark the listing as handled) in the 404 branch and the catch block,
keeping the retry check from re-queuing the same listing. Use the existing
identifiers loadCommunityDetail, lastLoadedListingId, and the render condition
around listingId !== lastLoadedListingId to locate the fix.
---
Nitpick comments:
In `@e2e/ssr-hydration.spec.ts`:
- Around line 56-59: The URL assertion in ssr-hydration.spec.ts uses dynamic
RegExp construction with alphaListing.listingId, which triggers static analysis
even though the value is a fixed test constant. Update the page.toHaveURL check
in the relevant test to avoid constructing the pattern dynamically, and use a
safer direct assertion or a static regex shape in the assert around alphaListing
and its listingId.
In `@packages/worker/client/loader-data-types.ts`:
- Around line 77-96: `AccountIntegrationListItem.flow` is duplicating the shared
`IntegrationFlow` contract, which can drift from the route/component type
definitions. Update `AccountIntegrationListItem` in `loader-data-types.ts` to
reuse the existing `IntegrationFlow` type instead of the inline union, keeping
the loader shape aligned with `routes/account-integrations.tsx` and the shared
API contract.
In `@packages/worker/client/routes/account-integrations.tsx`:
- Around line 150-183: The router-hydration logic is duplicated across multiple
route components and has already drifted, so extract the shared
navigation/loading/embedded-data fallback flow into a reusable helper. Move the
repeated `listenToRouterNavigation`, `applyEmbeddedLoaderData`, and
`queueTask(loadIntegrations)` pattern into a common utility such as
`client-router.tsx` or `loader-data-context.tsx`, then update `account.tsx`,
`account-package-invocation-tokens.tsx`, `account-remote-connectors.tsx`,
`account-secrets.tsx`, `community.tsx`, and `admin-community-reports.tsx` to
call it. Keep the existing `handle`, `readCurrentRouterHref`,
`tryConsumeEmbeddedLoaderData`, and `loadIntegrations` behavior intact while
centralizing the `status` transition and embedded-data check.
In `@packages/worker/client/routes/admin-roles.tsx`:
- Around line 20-24: The isAdminRolesPath helper duplicates the same pathname
parsing logic found in other route files, so extract the shared pattern into a
common utility such as pathnameOf(href) and/or matchesPath, then update this
file and the similar helpers in admin-users.tsx and
account-integrations.tsx/account-remote-connectors.tsx to use it. Keep the
route-specific path comparison in each caller, but centralize the
URL-to-pathname normalization to remove repeated new URL(href,
'http://localhost').pathname logic.
In `@packages/worker/client/routes/login.tsx`:
- Around line 47-64: The auth helper logic in LoginRoute is bypassing the safer
router wrappers by calling readRouterPathname and readRouterSearch directly in
getCurrentAuthMode and getCurrentRedirectTo. Update these helpers to use the
existing getPathname-style fallback approach (or equivalent safe wrappers from
client-router.tsx) so pathname/search access stays consistent and resilient if
reused outside the router tree.
In `@packages/worker/src/app/account-secrets-data.ts`:
- Around line 320-431: The loader-data types are duplicated in this module
instead of reusing the shared exports from loader-data.ts, which can drift over
time. Update account-secrets-data.ts to import and use the existing shared
shapes for AccountSecretListItem, AccountSecretDetail, and the analogous
SecretApprovalView where applicable, and align any matching code in
toAccountSecretListItem and related helpers to those shared types. Also
consolidate the trailing export statements at the end of the file into a single
export block.
- Around line 77-88: `readSelectedSecretIdFromPath` contains a dead validation
branch because `parsedLegacyId` is computed but never changes the returned
value. Either remove the unused `parseAccountSecretId` check and simplify the
legacy path handling to return the sliced legacy id or null, or, if invalid
legacy ids should be rejected, change the `parseAccountSecretId` result to
actually gate the return in the `readSelectedSecretIdFromPath` function.
In `@packages/worker/src/app/data-cache.ts`:
- Around line 65-78: getOrSetDataCache currently only caches completed values,
so concurrent misses for the same key all call load() independently. Update
getOrSetDataCache to de-duplicate in-flight loads by tracking a per-key pending
promise in the data-cache module and having later callers await it instead of
starting a new load. Keep the existing cache hit/miss behavior intact and clear
the in-flight entry after the promise settles; use getOrSetDataCache,
peekDataCache, and setDataCache as the main touchpoints.
In `@packages/worker/src/app/handlers/admin-roles.ts`:
- Around line 16-27: The admin role handler currently performs a redundant
authentication fallback after requireUserWithRole(request, env, 'admin'). Remove
the extra readAuthSessionResult(request) and redirectToLogin(request) branch
from the admin-roles handler, since requireUserWithRole already handles
unauthenticated requests; keep the existing Response passthrough and error
rethrow behavior intact.
In `@packages/worker/src/app/handlers/auth-page.ts`:
- Around line 11-36: createAuthPageHandler currently calls renderAppPage({
request, env }) without a page title, so both createLoginHandler and
createSignupHandler end up using the same default SsrDocument title. Update the
auth-page flow to pass a distinguishing title through renderAppPage for each
route, using the createAuthPageHandler / createLoginHandler /
createSignupHandler entry points to select the appropriate login or signup title
while keeping the existing redirect and session logic unchanged.
In `@packages/worker/src/app/request-auth-cache.ts`:
- Around line 80-89: The role/permission lookup in request-auth-cache.ts only
logs to console when getUserRolesAndPermissions fails, so add an observable
signal there such as a metric/counter or structured app logger entry in the
catch block around the getUserRolesAndPermissions call. Use the existing request
auth path in request-auth-cache.ts and the getUserRolesAndPermissions flow to
record lookup failures while preserving the current fail-closed empty
roles/permissions fallback.
In `@packages/worker/src/app/request-cache.ts`:
- Around line 18-29: The request cache metadata currently stores only one
DataCacheLookup via setRequestDataCacheLookup, so later loaders can overwrite
earlier cache hits/misses before getRequestDataCacheLookup reads them back for
the X-Kody-Cache header. Update the requestMetaStore-backed request metadata to
accumulate or merge multiple lookups per request instead of replacing the
previous value, and adjust getRequestDataCacheLookup and any header-building
code to consume the combined result; use setRequestDataCacheLookup and
getRequestDataCacheLookup as the key entry points.
🪄 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: e0b9665d-ce33-4100-9292-4edcb2f03ee3
📒 Files selected for processing (86)
e2e/admin-rbac.spec.tse2e/d1-utils.tse2e/smoke.spec.tse2e/ssr-hydration.spec.tspackages/worker/client/app-root.tsxpackages/worker/client/app.tsxpackages/worker/client/client-router.tsxpackages/worker/client/entry.tsxpackages/worker/client/loader-data-context.node.test.tspackages/worker/client/loader-data-context.tsxpackages/worker/client/loader-data-types.tspackages/worker/client/router-location.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/login.tsxpackages/worker/client/routes/oauth-authorize.tsxpackages/worker/client/routes/oauth-callback.tsxpackages/worker/client/routes/reset-password.tsxpackages/worker/project.jsonpackages/worker/src/app/account-integrations-data.tspackages/worker/src/app/account-package-invocation-tokens-data.tspackages/worker/src/app/account-profile-data.tspackages/worker/src/app/account-remote-connectors-data.tspackages/worker/src/app/account-secrets-data.tspackages/worker/src/app/admin-community-reports-data.tspackages/worker/src/app/admin-roles-data.tspackages/worker/src/app/admin-users-data.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/client-build-id.tspackages/worker/src/app/community-api.tspackages/worker/src/app/community-data.tspackages/worker/src/app/data-cache.node.test.tspackages/worker/src/app/data-cache.tspackages/worker/src/app/handlers/account-integrations.node.test.tspackages/worker/src/app/handlers/account-integrations.tspackages/worker/src/app/handlers/account-package-invocation-tokens.node.test.tspackages/worker/src/app/handlers/account-package-invocation-tokens.tspackages/worker/src/app/handlers/account-profile.tspackages/worker/src/app/handlers/account-remote-connectors.node.test.tspackages/worker/src/app/handlers/account-remote-connectors.tspackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/handlers/account.tspackages/worker/src/app/handlers/admin-community-reports.tspackages/worker/src/app/handlers/admin-roles.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/handlers/auth-page.tspackages/worker/src/app/handlers/community-detail.node.test.tspackages/worker/src/app/handlers/community-detail.tsxpackages/worker/src/app/handlers/community.node.test.tspackages/worker/src/app/handlers/community.tspackages/worker/src/app/handlers/community.tsxpackages/worker/src/app/handlers/connect-oauth.tspackages/worker/src/app/handlers/home.tspackages/worker/src/app/handlers/login.tspackages/worker/src/app/handlers/privacy.tspackages/worker/src/app/handlers/reset-password.tspackages/worker/src/app/handlers/session.tspackages/worker/src/app/handlers/signup.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/app/request-cache.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/app/session-info.tspackages/worker/src/app/ssr-document.tsxpackages/worker/src/app/ssr-render.node.test.tspackages/worker/src/app/ssr-render.tsxpackages/worker/src/app/ssr-stubs/app-root.tspackages/worker/src/community/service.tspackages/worker/src/index.tspackages/worker/src/oauth-handlers.tspackages/worker/tsconfig-client.jsonpackages/worker/tsconfig-worker-typecheck.jsonpackages/worker/tsconfig-worker.jsonpackages/worker/tsconfig.jsonpackages/worker/wrangler.jsonctsconfig.json
💤 Files with no reviewable changes (1)
- packages/worker/src/app/handlers/community.ts
…alidate session after hydration
…-consume embedded loader data, normalize report status filter, bound data cache, version styles.css, fix e2e seed upsert
…ty-detail embedded data by listing id
… during refetch Remove the currentHref === lastLoadedHref guard from grid render conditions in community and admin-community-reports routes. The navigation listener already sets status to loading on href changes, so stale-while-revalidate behavior keeps existing listings visible during refetch instead of hiding the grid after hydration when hrefs momentarily diverge.
When a session cookie references a deleted or missing user, protected HTML handlers now include the session-destroy Set-Cookie on redirect-to-login responses. The login page also validates the user via loadSessionInfo instead of trusting the raw session cookie, so stale cookies cannot bounce visitors back to protected routes.
…stale content during SPA navigation
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/d1-utils.ts (1)
128-134: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOther derived columns still go stale on reseed.
owner_user_idwas correctly added toDO UPDATE SEThere, fixing the previously flagged staleness issue. However,package_id,source_id,kody_id,license, andpinned_commitare still excluded from the update clause. If a listing is reseeded with a differentpackageId/sourceId/kodyId(or those derived defaults change based oninput.listingId), these columns will remain stale on conflict, same root cause as the earlierowner_user_idbug.🛠️ Proposed fix
ON CONFLICT(id) DO UPDATE SET owner_user_id = excluded.owner_user_id, + package_id = excluded.package_id, + source_id = excluded.source_id, + kody_id = excluded.kody_id, name = excluded.name, description = excluded.description, tags_json = excluded.tags_json, + license = excluded.license, + pinned_commit = excluded.pinned_commit, status = excluded.status, updated_at = CURRENT_TIMESTAMP;`.trim()🤖 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 `@e2e/d1-utils.ts` around lines 128 - 134, The UPSERT in the D1 seeding logic still leaves derived listing columns stale on conflict. Update the existing ON CONFLICT(id) DO UPDATE SET clause in the d1-utils seeding SQL to also refresh package_id, source_id, kody_id, license, and pinned_commit alongside owner_user_id, name, description, tags_json, status, and updated_at. Keep the change in the same seed/listing insert path so reseeding with a different listingId or derived defaults always overwrites every mutable listing field.
🧹 Nitpick comments (1)
packages/worker/src/app/handlers/account.node.test.ts (1)
20-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest-env boilerplate duplicated across three test files.
createStaleSessionTestEnv()andtestCookieSecretare copied verbatim intoauth-page.node.test.tsandauth-redirect.node.test.ts. Consider extracting to a shared test helper module to avoid drift as the D1 mock 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/src/app/handlers/account.node.test.ts` around lines 20 - 56, The same stale-session test setup is duplicated in multiple auth test files, which risks the D1 mock drifting over time. Extract createStaleSessionTestEnv() and the shared testCookieSecret into a reusable helper module, then update account.node.test.ts, auth-page.node.test.ts, and auth-redirect.node.test.ts to import and use that helper instead of maintaining separate copies.
🤖 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/src/app/handlers/account.node.test.ts`:
- Line 1: The account.node.test.ts file imports vitest symbols, but the worker
package manifest does not declare vitest as a dependency. Update the
packages/worker package.json used by the worker workspace to add vitest so the
test imports resolve correctly; make sure the dependency is available to the
test suite that exercises the account.node.test.ts handler.
---
Outside diff comments:
In `@e2e/d1-utils.ts`:
- Around line 128-134: The UPSERT in the D1 seeding logic still leaves derived
listing columns stale on conflict. Update the existing ON CONFLICT(id) DO UPDATE
SET clause in the d1-utils seeding SQL to also refresh package_id, source_id,
kody_id, license, and pinned_commit alongside owner_user_id, name, description,
tags_json, status, and updated_at. Keep the change in the same seed/listing
insert path so reseeding with a different listingId or derived defaults always
overwrites every mutable listing field.
---
Nitpick comments:
In `@packages/worker/src/app/handlers/account.node.test.ts`:
- Around line 20-56: The same stale-session test setup is duplicated in multiple
auth test files, which risks the D1 mock drifting over time. Extract
createStaleSessionTestEnv() and the shared testCookieSecret into a reusable
helper module, then update account.node.test.ts, auth-page.node.test.ts, and
auth-redirect.node.test.ts to import and use that helper instead of maintaining
separate copies.
🪄 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: fefd1886-db6f-4433-b8da-c5e03d42d878
📒 Files selected for processing (32)
e2e/d1-utils.tspackages/worker/client/app.tsxpackages/worker/client/loader-data-context.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/src/app/admin-community-reports-data.tspackages/worker/src/app/auth-redirect.node.test.tspackages/worker/src/app/auth-redirect.tspackages/worker/src/app/client-build-id.tspackages/worker/src/app/data-cache.node.test.tspackages/worker/src/app/data-cache.tspackages/worker/src/app/handlers/account-integrations.tspackages/worker/src/app/handlers/account-package-invocation-tokens.tspackages/worker/src/app/handlers/account-remote-connectors.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/handlers/account.node.test.tspackages/worker/src/app/handlers/account.tspackages/worker/src/app/handlers/auth-page.node.test.tspackages/worker/src/app/handlers/auth-page.tspackages/worker/src/app/handlers/package-app.tspackages/worker/src/app/permissions-server.tspackages/worker/src/app/ssr-document.tsxpackages/worker/src/app/ssr-render.tsxpackages/worker/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/worker/src/app/handlers/auth-page.ts
- packages/worker/src/app/ssr-render.tsx
- packages/worker/client/routes/account-integrations.tsx
- packages/worker/client/loader-data-context.tsx
- packages/worker/src/app/handlers/account.ts
- packages/worker/src/app/admin-community-reports-data.ts
- packages/worker/src/app/ssr-document.tsx
- packages/worker/client/routes/account.tsx
- packages/worker/client/routes/community-detail.tsx
- packages/worker/src/app/data-cache.ts
- packages/worker/client/routes/admin-roles.tsx
- packages/worker/client/routes/account-remote-connectors.tsx
- packages/worker/client/app.tsx
- packages/worker/src/index.ts
- packages/worker/src/app/handlers/account-remote-connectors.ts
- packages/worker/client/routes/community.tsx
- packages/worker/client/routes/admin-users.tsx
- packages/worker/src/app/handlers/account-package-invocation-tokens.ts
- packages/worker/src/app/handlers/account-integrations.ts
- packages/worker/client/routes/admin-community-reports.tsx
- packages/worker/client/routes/account-secrets.tsx
- packages/worker/src/app/handlers/account-secrets.ts
| @@ -0,0 +1,82 @@ | |||
| import { expect, test, vi } from 'vitest' | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a package.json packages/worker | xargs grep -n '"vitest"'Repository: kentcdodds/kody
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package.json files ---'
git ls-files '**/package.json' | sed -n '1,200p'
printf '\n%s\n' '--- vitest declarations ---'
for f in $(git ls-files '**/package.json'); do
if grep -n '"vitest"' "$f" >/dev/null 2>&1; then
echo "FILE: $f"
grep -n '"vitest"' "$f"
fi
doneRepository: kentcdodds/kody
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/worker/package.jsonRepository: kentcdodds/kody
Length of output: 1221
Add vitest to packages/worker/package.json. The test file imports it, but no workspace manifest declares it.
🤖 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/src/app/handlers/account.node.test.ts` at line 1, The
account.node.test.ts file imports vitest symbols, but the worker package
manifest does not declare vitest as a dependency. Update the packages/worker
package.json used by the worker workspace to add vitest so the test imports
resolve correctly; make sure the dependency is available to the test suite that
exercises the account.node.test.ts handler.
…etch on local selection
Extract authorize-info computation into loadOAuthAuthorizeData shared by the JSON endpoint and GET /oauth/authorize HTML. Embed the result under oauthAuthorize in AppLoaderData so streamed HTML shows the client name, scopes, or error state instead of the loading branch until hydration. Consume embedded data in oauth-authorize.tsx via tryConsumeEmbeddedLoaderData before falling back to client fetch, matching the community route pattern.
Run embedded-data consumption and initial loadInfo when status is idle, not only when the search string changes, so empty-query authorize pages still surface server or client error state. Stop overwriting message from URL error query params after a successful embedded oauthAuthorize payload; match loadInfo behavior that clears message on success.
Add a typed frame registry (registerFrame, handleFrameRequest, resolveRegisteredFrameHtml) shared by SSR and route handlers. Promote x-remix-target to frame-constants.ts, migrate the frame-spike route to the new helper, and enable client imports of routes.href() for Frame src. Add kody-custom/no-literal-frame-src oxlint rule and docs/contributing/frames.md.
…ta types, drop dead export
…x-ssr-hydration-c537 # Conflicts: # packages/worker/client/loader-data-types.ts # packages/worker/tsconfig-client.json
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 53915ea. Configure here.
cursor Bot
pushed a commit
that referenced
this pull request
Jul 4, 2026
…st-scoped detail loads, throttled session refresh Post-merge follow-up to the Remix 3 SSR work (#601): - handleRequest builds the app router and parses env once per env object identity instead of on every request (~40 handler factories + schema validation were re-run per request). - loadAdminUsersData issues the COUNT and page queries in parallel. - loadCommunityDetailData memoizes per Request so the HTML handler and the frame renderer share one load during a single SSR response. - Navigation-triggered client session refreshes are throttled to 30s; hydration and explicit refreshes (login/logout/profile) still always hit /session. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Jul 5, 2026
Merged
This was referenced Jul 13, 2026
This was referenced Jul 20, 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.
Remix 3 SSR: server-rendered, hydrated SPA
Converts the browser app from a client-only SPA (spinner shell + client fetch) to a server-rendered, then hydrated SPA. Every HTML route now streams fully rendered HTML (real page content, session-aware nav, embedded route data) via
renderToStreamfromremix/ui/server, and the client hydrates in place viarun()+ a singleclientEntryapp root — after which the existing client router keeps SPA navigation.What changed
Core SSR pipeline
ssr-render.tsx/ssr-document.tsx:renderAppPage()loads the session server-side and streams a full HTML document with the hydratedAppRootclientEntry (/client-entry.js#AppRoot).client/entry.tsxnow boots withrun({ loadModule })(registry pattern for the single esbuild bundle) instead ofcreateRoot().render()+ the oldreplaceChildren()anti-hydration hack.router-location.tsx: current URL is injected via context (SSR) orwindow.location+ navigation events (client) — nowindow.locationreads in render paths./sessionfetch flash. Client-side refresh still happens on SPA navigations.Loader-data convention (no double fetch)
*-data.tsloader shared by the JSON endpoint and the HTML handler — zero duplication.tryConsumeEmbeddedLoaderData), falling back to the existing fetch on SPA navigation./:tokenId, secrets incl. subpaths), admin (users, roles, community reports)./connect/oauthintentionally keeps its client-only multi-step flow behind an SSR-safe shell.Performance & caching
Cache-Control: no-store(every page embeds session — must never be shared-cached).loggedIn,forkPrompt) composed per request, never cached. Observability viaX-Kody-Cache: hit|miss./client-entry.jsand/styles.cssserved withpublic, max-age=31536000, immutablekeyed by a?v=<commitSha>build id, plus<link rel="modulepreload">so hydration JS fetches during HTML streaming.Tests
ssr-render.node.test.ts: SSR HTML contains real content + embedded payloads; auth redirects; 404 SSR; reset-password modes.e2e/ssr-hydration.spec.ts: raw-response SSR assertions, SPA navigation without full reload (window-marker), hydrated search interactivity, 404 SPA recovery.smoke.spec.tsandadmin-rbac.spec.tswith SPA-navigation regressions (same-origin refetch, admin list rendering).data-cache.node.test.ts,loader-data-context.node.test.ts,auth-redirect.node.test.ts,auth-page.node.test.ts,account.node.test.ts,oauth-handlers.workers.test.tsfor the new utilities and auth flows.Screenshots
Raw SSR HTML evidence (curl): community_ssr_raw_html_excerpt.log
Validation
npm run validatefully green: format, lint, typecheck, unit tests, Playwright E2E specs, MCP E2E.Review follow-ups (Bugbot + CodeRabbit)
Four review rounds addressed; the latest Bugbot run passes with zero findings. Notable fixes on top of the original change:
/oauth/authorizenow embeds authorize info (client + scopes, or error state) in the SSR payload instead of streaming a loading shell.Set-Cookie, and the login page validates the user in D1 — fixes a redirect loop.lastLoadedHrefbeforenavigate()to avoid redundant refetches.styles.cssversioned + immutable like the client bundle.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@ed933a5· Head:52530ddClassification: extends — the browser app surface's rendering contract changes from client-rendered shell to streamed SSR + hydration; sessions and community listings gain shared loaders/caching but keep their contracts.
Primitives touched
app-ui/reset-passwordroute, hydration bootapp-sessionssession-info.ts, per-request dedupmcp-oauthcommunity-listingsrbacSystem map
Before / after
createRoot().render()from scratchrun()hydration of server DOM/reset-passwordGET/client-entry.js?v=<commitSha>+ immutable, modulepreloadInvariants
per-user-isolation: preserved — loaders take the authenticated user; only public community payloads are cached (viewer-specific fields composed per request); all HTML isno-store.community-listing-isolation: preserved — public read stays limited to active listings; cache invalidated on publish/unpublish/fork/rate/moderation.Summary by CodeRabbit