From ac84356ab038e24ef87c6fb99bcd23bd25c9aae1 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 8 Jul 2026 05:34:57 +0000 Subject: [PATCH 1/2] Make route-loader consumption self-correcting tryConsumeRouteLoaderData mutates route closure state mid-render, so any values a route derived earlier in the same render pass were stale with no follow-up render queued -- the bug class behind the empty secrets list (#664) and the stale /admin/users detail panel. A successful consume now schedules one corrective re-render of the consuming component; the scheduler flushes it in the same microtask (before paint) and consume-once semantics guarantee it cannot loop. Also reorder /admin/users derivations after the apply call (last mis-ordered route), assert the auto-selected detail panel after SPA navigation in the admin RBAC e2e journey, and document the safety net in the request lifecycle notes. --- .../architecture/request-lifecycle.md | 6 + e2e/admin-rbac.spec.ts | 4 + .../client/loader-data-context.node.test.ts | 76 ++++++ .../worker/client/loader-data-context.tsx | 31 ++- packages/worker/client/routes/admin-users.tsx | 11 +- .../worker/public/mcp-apps/kody-ui-utils.css | 228 +++++++++--------- 6 files changed, 237 insertions(+), 119 deletions(-) diff --git a/docs/contributing/architecture/request-lifecycle.md b/docs/contributing/architecture/request-lifecycle.md index e898e724a0..5aa67eeca0 100644 --- a/docs/contributing/architecture/request-lifecycle.md +++ b/docs/contributing/architecture/request-lifecycle.md @@ -107,6 +107,12 @@ loader finishes (or is skipped when no loader matches) does the router commit the URL change and notify subscribers. Route components consume that payload synchronously on first render via `tryConsumeRouteLoaderData`, so the UI updates once with data already present instead of swapping routes into a loading state. +Because consumption mutates route closure state mid-render, a successful consume +also schedules one follow-up render of the consuming component (flushed in the +same microtask, before paint); values a route derived before the consume call +therefore cannot persist stale. Routes should still consume before deriving +list/detail state — the follow-up render is a safety net, not the primary +ordering contract. Route loaders are registered in `clientRouteLoaders` (`routes/index.tsx`) and matched with the same Remix route-pattern specificity as `clientRoutes`. Loaders diff --git a/e2e/admin-rbac.spec.ts b/e2e/admin-rbac.spec.ts index 237f1ec08a..7a1a349297 100644 --- a/e2e/admin-rbac.spec.ts +++ b/e2e/admin-rbac.spec.ts @@ -70,6 +70,10 @@ test('admin RBAC controls access, role assignment, and privacy boundaries', asyn await expect(page).toHaveURL(/\/admin\/users\/?$/) await expect(page.getByRole('heading', { name: 'Admin users' })).toBeVisible() await expect(page.getByText('Unable to load admin users.')).toHaveCount(0) + // The detail panel must render the auto-selected user after SPA + // navigation (regression: preloaded loader data consumed mid-render left + // derivations from pre-navigation state on screen). + await expect(page.getByText('Account metadata only')).toBeVisible() expect( await page.evaluate( () => (window as Window & { __e2eMarker?: boolean }).__e2eMarker, diff --git a/packages/worker/client/loader-data-context.node.test.ts b/packages/worker/client/loader-data-context.node.test.ts index 26bc6f7bed..121857ec84 100644 --- a/packages/worker/client/loader-data-context.node.test.ts +++ b/packages/worker/client/loader-data-context.node.test.ts @@ -1,8 +1,15 @@ import { expect, test } from 'vitest' +import { type Handle } from 'remix/ui' import { + AppLoaderDataProvider, hrefMatchesSsrUrl, normalizeRouterHref, + tryConsumeRouteLoaderData, } from './loader-data-context.tsx' +import { + clearPreloadedNavigationData, + setPreloadedNavigationData, +} from './navigation-data.ts' test('loader href helpers normalize URLs and compare pathname and search', () => { expect(normalizeRouterHref('/community?q=beta')).toBe('/community?q=beta') @@ -32,3 +39,72 @@ test('loader href helpers normalize URLs and compare pathname and search', () => ), ).toBe(false) }) + +function createStubHandle() { + const queuedTasks: Array<(signal?: AbortSignal) => unknown> = [] + let updateCount = 0 + const handle = { + context: { + get(provider: unknown) { + if (provider === AppLoaderDataProvider) { + return { loaderData: undefined, consumedKeys: new Set() } + } + // No RouterLocationProvider in the stub tree; consumption + // falls through to the preloaded navigation slot. + throw new Error('context not available') + }, + }, + queueTask(task: (signal?: AbortSignal) => unknown) { + queuedTasks.push(task) + }, + update() { + updateCount++ + return Promise.resolve(new AbortController().signal) + }, + } as unknown as Handle + return { + handle, + queuedTasks, + getUpdateCount: () => updateCount, + } +} + +test('consuming route loader data schedules a corrective re-render exactly once', () => { + clearPreloadedNavigationData() + setPreloadedNavigationData('/account', { + accountProfile: { + ok: true, + email: 'kody@example.com', + emailVerified: true, + username: 'kody', + displayName: 'Kody', + }, + }) + const { handle, queuedTasks, getUpdateCount } = createStubHandle() + + // Routes apply consumed payloads by mutating closure state mid-render, so + // a follow-up render must be scheduled or derivations made before the + // consume call would persist stale. + const consumed = tryConsumeRouteLoaderData( + handle, + 'accountProfile', + '/account', + ) + expect(consumed?.email).toBe('kody@example.com') + expect(queuedTasks).toHaveLength(1) + for (const task of queuedTasks.splice(0)) task() + expect(getUpdateCount()).toBe(1) + + // Consume-once: the follow-up render finds nothing and schedules nothing, + // so the corrective render cannot loop. + const reconsumed = tryConsumeRouteLoaderData( + handle, + 'accountProfile', + '/account', + ) + expect(reconsumed).toBeUndefined() + expect(queuedTasks).toHaveLength(0) + expect(getUpdateCount()).toBe(1) + + clearPreloadedNavigationData() +}) diff --git a/packages/worker/client/loader-data-context.tsx b/packages/worker/client/loader-data-context.tsx index 32d2f3c78c..e49f29767d 100644 --- a/packages/worker/client/loader-data-context.tsx +++ b/packages/worker/client/loader-data-context.tsx @@ -66,9 +66,28 @@ export function tryConsumeEmbeddedLoaderData( return embedded } +/** + * Consumption happens mid-render and the caller applies the payload by + * mutating route closure state, so anything the route derived earlier in the + * same render pass is stale. Schedule one follow-up render so the route + * re-derives everything from the applied data. The scheduler flushes queued + * tasks and the resulting update in the same microtask as the current render + * (before the browser paints), and consume-once semantics guarantee the + * follow-up pass consumes nothing, so this cannot loop. `queueTask` (not a + * direct `update()`) because a component's update scheduling is not wired + * until its first render returns; on the server `queueTask` is a no-op. + */ +function scheduleCorrectiveRender(handle: Handle) { + handle.queueTask(() => { + void handle.update() + }) +} + /** * Returns loader data for `key` from SSR-embedded props or a preloaded SPA - * navigation slot. Each source is consume-once per key. + * navigation slot. Each source is consume-once per key. Successful reads + * schedule a follow-up render of the consuming component so values derived + * before consumption in the same render pass cannot persist stale. */ export function tryConsumeRouteLoaderData( handle: Handle, @@ -76,6 +95,12 @@ export function tryConsumeRouteLoaderData( currentHref: string, ): AppLoaderData[K] | undefined { const embedded = tryConsumeEmbeddedLoaderData(handle, key, currentHref) - if (embedded !== undefined) return embedded - return tryConsumePreloadedLoaderData(key, currentHref) + const data = + embedded !== undefined + ? embedded + : tryConsumePreloadedLoaderData(key, currentHref) + if (data !== undefined) { + scheduleCorrectiveRender(handle) + } + return data } diff --git a/packages/worker/client/routes/admin-users.tsx b/packages/worker/client/routes/admin-users.tsx index 664ae3c7bd..581617f2f4 100644 --- a/packages/worker/client/routes/admin-users.tsx +++ b/packages/worker/client/routes/admin-users.tsx @@ -247,10 +247,9 @@ export function AdminUsersRoute(handle: Handle) { lastSeenHref = currentHref lastFailedHref = null } - const totalPages = Math.max(1, Math.ceil(total / pageSize)) - const selectedUser = getSelectedUser() - const isMutating = actionState !== 'idle' - + // Consume route-loader data before deriving `totalPages` and + // `selectedUser`; deriving first would render this pass from the + // stale pre-navigation closure state. const appliedRouteData = applyRouteLoaderData(currentHref) // A same-path refresh whose loader failed leaves no preload and no // href change; the stale marker forces the fallback refetch. @@ -268,6 +267,10 @@ export function AdminUsersRoute(handle: Handle) { handle.queueTask(loadAdminUsers) } + const totalPages = Math.max(1, Math.ceil(total / pageSize)) + const selectedUser = getSelectedUser() + const isMutating = actionState !== 'idle' + return ( Date: Wed, 8 Jul 2026 05:45:46 +0000 Subject: [PATCH 2/2] Revert dev-server churn in generated kody-ui-utils.css bundle --- .../worker/public/mcp-apps/kody-ui-utils.css | 228 +++++++++--------- 1 file changed, 112 insertions(+), 116 deletions(-) diff --git a/packages/worker/public/mcp-apps/kody-ui-utils.css b/packages/worker/public/mcp-apps/kody-ui-utils.css index 74106bae9a..f9791fb19a 100644 --- a/packages/worker/public/mcp-apps/kody-ui-utils.css +++ b/packages/worker/public/mcp-apps/kody-ui-utils.css @@ -1,176 +1,172 @@ /* packages/worker/client/mcp-apps/kody-ui-utils.css */ :root { - color-scheme: light dark; - --font-body: - ui-sans-serif, - system-ui, - sans-serif; - --font-mono: - ui-monospace, - SFMono-Regular, - Menlo, - Monaco, - Consolas, - monospace; - --spacing-1: 0.25rem; - --spacing-2: 0.5rem; - --spacing-3: 0.75rem; - --spacing-4: 1rem; - --spacing-6: 1.5rem; - --radius-2: 0.5rem; - --radius-3: 0.75rem; - --shadow-1: 0 1px 2px rgb(15 23 42 / 0.08); - --color-bg: #ffffff; - --color-surface: #f8fafc; - --color-fg: #0f172a; - --color-muted: #475569; - --color-border: #dbe2ea; - --color-accent: #2563eb; - --color-accent-contrast: #ffffff; - --color-code-bg: rgb(15 23 42 / 0.06); + color-scheme: light dark; + --font-body: ui-sans-serif, system-ui, sans-serif; + --font-mono: ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, monospace; + --spacing-1: 0.25rem; + --spacing-2: 0.5rem; + --spacing-3: 0.75rem; + --spacing-4: 1rem; + --spacing-6: 1.5rem; + --radius-2: 0.5rem; + --radius-3: 0.75rem; + --shadow-1: 0 1px 2px rgb(15 23 42 / 0.08); + --color-bg: #ffffff; + --color-surface: #f8fafc; + --color-fg: #0f172a; + --color-muted: #475569; + --color-border: #dbe2ea; + --color-accent: #2563eb; + --color-accent-contrast: #ffffff; + --color-code-bg: rgb(15 23 42 / 0.06); } @media (prefers-color-scheme: dark) { - :root { - --color-bg: #0f172a; - --color-surface: #162033; - --color-fg: #e5eef8; - --color-muted: #a5b4c7; - --color-border: #2a3950; - --color-accent: #60a5fa; - --color-accent-contrast: #0f172a; - --color-code-bg: rgb(148 163 184 / 0.16); - } + :root { + --color-bg: #0f172a; + --color-surface: #162033; + --color-fg: #e5eef8; + --color-muted: #a5b4c7; + --color-border: #2a3950; + --color-accent: #60a5fa; + --color-accent-contrast: #0f172a; + --color-code-bg: rgb(148 163 184 / 0.16); + } } :where(html) { - min-height: 100%; - background: var(--color-bg); - color: var(--color-fg); + min-height: 100%; + background: var(--color-bg); + color: var(--color-fg); } :where(body) { - min-height: 100%; - margin: 0; - padding: var(--spacing-4); - background: var(--color-bg); - color: var(--color-fg); - font: 400 14px/1.5 var(--font-body); + min-height: 100%; + margin: 0; + padding: var(--spacing-4); + background: var(--color-bg); + color: var(--color-fg); + font: 400 14px/1.5 var(--font-body); } -:where(body[data-kody-runtime=javascript]) { - padding: 0; +:where(body[data-kody-runtime='javascript']) { + padding: 0; } :where(*, *::before, *::after) { - box-sizing: border-box; + box-sizing: border-box; } :where(a) { - color: var(--color-accent); + color: var(--color-accent); } :where(p, ul, ol, dl, pre, table, blockquote, fieldset) { - margin: 0 0 var(--spacing-4); + margin: 0 0 var(--spacing-4); } :where(h1, h2, h3, h4, h5, h6) { - margin: 0 0 var(--spacing-3); - line-height: 1.2; + margin: 0 0 var(--spacing-3); + line-height: 1.2; } :where(h1) { - font-size: 1.875rem; + font-size: 1.875rem; } :where(h2) { - font-size: 1.5rem; + font-size: 1.5rem; } :where(h3) { - font-size: 1.25rem; + font-size: 1.25rem; } :where(ul, ol) { - padding-left: 1.5rem; + padding-left: 1.5rem; } :where(hr) { - border: 0; - border-top: 1px solid var(--color-border); - margin: var(--spacing-6) 0; + border: 0; + border-top: 1px solid var(--color-border); + margin: var(--spacing-6) 0; } :where(code, kbd, samp) { - font-family: var(--font-mono); - font-size: 0.95em; + font-family: var(--font-mono); + font-size: 0.95em; } :where(code) { - background: var(--color-code-bg); - border-radius: 0.375rem; - padding: 0.1rem 0.35rem; + background: var(--color-code-bg); + border-radius: 0.375rem; + padding: 0.1rem 0.35rem; } :where(pre) { - overflow-x: auto; - padding: var(--spacing-3); - background: var(--color-code-bg); - border: 1px solid var(--color-border); - border-radius: var(--radius-2); + overflow-x: auto; + padding: var(--spacing-3); + background: var(--color-code-bg); + border: 1px solid var(--color-border); + border-radius: var(--radius-2); } :where(pre code) { - background: transparent; - padding: 0; + background: transparent; + padding: 0; } :where(form) { - display: grid; - gap: var(--spacing-4); + display: grid; + gap: var(--spacing-4); } :where(label) { - display: grid; - gap: var(--spacing-2); - font-weight: 600; + display: grid; + gap: var(--spacing-2); + font-weight: 600; } :where(input, button, textarea, select) { - font: inherit; + font: inherit; } -:where(input:not([type=checkbox]):not([type=radio]), textarea, select) { - width: 100%; - padding: var(--spacing-2) var(--spacing-3); - background: var(--color-surface); - color: var(--color-fg); - border: 1px solid var(--color-border); - border-radius: var(--radius-2); - box-shadow: inset 0 1px 2px rgb(15 23 42 / 0.04); +:where(input:not([type='checkbox']):not([type='radio']), textarea, select) { + width: 100%; + padding: var(--spacing-2) var(--spacing-3); + background: var(--color-surface); + color: var(--color-fg); + border: 1px solid var(--color-border); + border-radius: var(--radius-2); + box-shadow: inset 0 1px 2px rgb(15 23 42 / 0.04); } :where(textarea) { - min-height: 7rem; - resize: vertical; + min-height: 7rem; + resize: vertical; } -:where(input[type=checkbox], input[type=radio]) { - accent-color: var(--color-accent); +:where(input[type='checkbox'], input[type='radio']) { + accent-color: var(--color-accent); } :where(button) { - width: fit-content; - padding: var(--spacing-2) var(--spacing-4); - background: var(--color-accent); - color: var(--color-accent-contrast); - border: 1px solid transparent; - border-radius: var(--radius-2); - box-shadow: var(--shadow-1); - cursor: pointer; + width: fit-content; + padding: var(--spacing-2) var(--spacing-4); + background: var(--color-accent); + color: var(--color-accent-contrast); + border: 1px solid transparent; + border-radius: var(--radius-2); + box-shadow: var(--shadow-1); + cursor: pointer; } :where(button:disabled, input:disabled, textarea:disabled, select:disabled) { - opacity: 0.7; - cursor: not-allowed; -} -:where(button:focus-visible, input:focus-visible, textarea:focus-visible, select:focus-visible) { - outline: 2px solid var(--color-accent); - outline-offset: 2px; + opacity: 0.7; + cursor: not-allowed; +} +:where( + button:focus-visible, + input:focus-visible, + textarea:focus-visible, + select:focus-visible +) { + outline: 2px solid var(--color-accent); + outline-offset: 2px; } :where(table) { - width: 100%; - border-collapse: collapse; + width: 100%; + border-collapse: collapse; } :where(th, td) { - padding: var(--spacing-2) var(--spacing-3); - border: 1px solid var(--color-border); - text-align: left; + padding: var(--spacing-2) var(--spacing-3); + border: 1px solid var(--color-border); + text-align: left; } :where(th) { - background: var(--color-surface); + background: var(--color-surface); } :where(blockquote) { - margin-left: 0; - padding-left: var(--spacing-4); - border-left: 3px solid var(--color-border); - color: var(--color-muted); + margin-left: 0; + padding-left: var(--spacing-4); + border-left: 3px solid var(--color-border); + color: var(--color-muted); } :where([data-generated-ui-root]) { - min-height: 100%; + min-height: 100%; }