From b3b44ee77d65fa4d850a83d046dc92f9ff31410a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 16 Aug 2026 18:57:25 +0000 Subject: [PATCH] changing route in a preview no longer reloads the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported as "I change routes and a second later the whole app refreshes". The second was mine: the 800ms debounce on writing the location down. `DevProxyFrame`'s session effect had the composed URL in its dependency list. That URL changes every time the app navigates, so the effect re-ran — and its first act is `setReady(false)`, which swaps the frame for "Connecting…" and takes the iframe out of the tree. Coming back mounted a fresh one. A full page load, one debounce after every click. Watching the URL was never right: the cookie is issued with `Path=/api/dev-proxy` and no port in it, so it already covers every path and port the panel can reach. It now depends on entering proxy mode and on an explicit reload, where re-requesting is the whole point. ── A second bug, found while fixing the first, not yet reported ── Freezing `src` at mount stopped the app's navigation from reloading the frame — and also stopped a newly TYPED url from loading, because nothing remounted. The panel would have silently ignored a change of address. The two pull in opposite directions, which is what makes the pair the interesting part rather than either fix. They are told apart by which value changes: `entry` moves only when a person points the panel somewhere else, `target` moves when the app does. So the frame is keyed on `entry` and loads `target`. ── The tests were made to fail first ── Three component tests, and both bugs were re-introduced one at a time to check the tests actually catch them: URL back in the effect deps -> "the app navigating does NOT reload" FAILS key back to reloadToken alone -> "a newly typed url DOES load" FAILS Either bug alone would have been passed by a test written for the other, so they only mean anything as a pair. Both were invisible to reading the component and obvious in the DOM — the assertion is node identity across a rerender, which is the same question as whether the browser re-requested the page. `fetch` is stubbed and restored per test rather than `mock.module('hooks/useClient')`: Bun has one module registry for the whole run, so that mock is process-wide and permanent, and `useDashboardState.test.tsx` already owns it. tsgo clean, 937 pass / 7 fail unchanged, frontend builds. Needs a rebuild — frontend only, so the Rebuild button is enough; no restart. Co-Authored-By: Claude Opus 5 --- .../src/apps/Preview/PreviewApp.test.tsx | 110 ++++++++++++++++++ .../src/apps/Preview/PreviewApp.tsx | 43 +++++-- 2 files changed, 145 insertions(+), 8 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/Preview/PreviewApp.test.tsx diff --git a/src/workspaces/officerdev/src/apps/Preview/PreviewApp.test.tsx b/src/workspaces/officerdev/src/apps/Preview/PreviewApp.test.tsx new file mode 100644 index 00000000..15b1c5b4 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Preview/PreviewApp.test.tsx @@ -0,0 +1,110 @@ +import type { ReactNode } from 'react'; +import { afterEach, describe, expect, test } from 'bun:test'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { render, waitFor } from '@testing-library/react'; +import { DevProxyFrame } from './PreviewApp'; + +/** + * Whether the frame is REMOUNTED, which is the same question as whether the app reloads. + * + * Both bugs this pins were invisible to reading the component and obvious in the DOM. The first shipped: + * changing route inside a preview reloaded the whole app about a second later — the debounce on writing the + * location down, feeding a URL into an effect whose `setReady(false)` tore the frame out of the tree. The + * second was introduced by the fix for the first: with `src` frozen at mount, nothing remounted when a + * person typed a NEW url, so the panel simply ignored it. + * + * They pull in opposite directions, so a test for either alone would have been passed by the other bug. + * + * `fetch` is stubbed rather than `mock.module('hooks/useClient')`: Bun has one module registry for the whole + * run, so that mock is process-wide and permanent, and `useDashboardState.test.tsx` already owns it. A global + * restored in `afterEach` cannot reach across files the same way. + */ +const realFetch = globalThis.fetch; +afterEach(() => { + globalThis.fetch = realFetch; +}); + +const stubFetch = () => { + globalThis.fetch = (async () => + new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'content-type': 'application/json' }, + })) as unknown as typeof fetch; +}; + +const PROXY = '/api/dev-proxy/3001/'; +const frame = (el: HTMLElement) => el.querySelector('iframe'); + +// `useClient` reaches for auth through react-query, so the component needs a provider even though nothing +// here is querying. +const Wrap = ({ children }: { children: ReactNode }) => ( + + {children} + +); + +const mount = (props: { entry: string; target: string; reloadToken?: number }) => + render( + + {}} + /> + , + ); + +describe('DevProxyFrame remounting', () => { + test('the app navigating does NOT reload the frame', async () => { + stubFetch(); + const view = mount({ entry: PROXY, target: PROXY }); + await waitFor(() => expect(frame(view.container)).not.toBeNull()); + const before = frame(view.container); + + // What the location reporter produces a debounce after a route change. + view.rerender( + + {}} /> + , + ); + + // Same DOM node = the browser never re-requested anything. A remount here is the reported bug. + expect(frame(view.container)).toBe(before); + expect(frame(view.container)?.getAttribute('src')).toBe(PROXY); + }); + + test('a newly typed url DOES load — the regression the freeze introduced', async () => { + stubFetch(); + const view = mount({ entry: PROXY, target: PROXY }); + await waitFor(() => expect(frame(view.container)).not.toBeNull()); + const before = frame(view.container); + + const next = '/api/dev-proxy/4000/'; + view.rerender( + + {}} /> + , + ); + + await waitFor(() => expect(frame(view.container)?.getAttribute('src')).toBe(next)); + expect(frame(view.container)).not.toBe(before); + }); + + test('the reload button remounts, and comes back at the current page', async () => { + stubFetch(); + const deep = '/api/dev-proxy/3001/settings/profile'; + const view = mount({ entry: PROXY, target: deep }); + await waitFor(() => expect(frame(view.container)).not.toBeNull()); + const before = frame(view.container); + + view.rerender( + + {}} /> + , + ); + + await waitFor(() => expect(frame(view.container)).not.toBe(before)); + expect(frame(view.container)?.getAttribute('src')).toBe(deep); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Preview/PreviewApp.tsx b/src/workspaces/officerdev/src/apps/Preview/PreviewApp.tsx index 390643bf..81167101 100644 --- a/src/workspaces/officerdev/src/apps/Preview/PreviewApp.tsx +++ b/src/workspaces/officerdev/src/apps/Preview/PreviewApp.tsx @@ -56,7 +56,7 @@ export const PreviewApp = ({ panelId }: { panelId: string }) => { ); } - return ; + return ; }; /** @@ -71,14 +71,39 @@ export const PreviewApp = ({ panelId }: { panelId: string }) => { * * An ordinary URL mounts immediately — nothing to arrange, and no reason to make it wait. */ -type DevProxyFrameProps = { url: string; reloadToken: number; onLocation: (path: string) => void }; +/** + * `entry` is what the panel was pointed at; `target` is that plus wherever the app has navigated to. + * + * Both are needed here and they are not interchangeable. `entry` decides WHEN to start over — a person + * typing a new address — and `target` decides WHAT to load. Keying on `target` would restart the frame + * every time the app moved, which is the whole bug below. + */ +type DevProxyFrameProps = { + entry: string; + target: string; + reloadToken: number; + onLocation: (path: string) => void; +}; -const DevProxyFrame = ({ url, reloadToken, onLocation }: DevProxyFrameProps) => { +export const DevProxyFrame = ({ entry, target, reloadToken, onLocation }: DevProxyFrameProps) => { const { get } = useClient(); - const needsSession = url.startsWith('/api/dev-proxy/'); + const needsSession = entry.startsWith('/api/dev-proxy/'); const [ready, setReady] = useState(!needsSession); const [failed, setFailed] = useState(null); + /** + * The session cookie, and why this effect must NOT watch the URL. + * + * It used to depend on the full composed URL, which changes every time the app navigates. Each change + * re-ran this, and `setReady(false)` tears the frame down to show "Connecting…" — so a route change was + * followed, one debounce later, by the whole app reloading. Reported as "I change routes and a second + * later the page refreshes"; the second was the 800ms debounce on writing the location down. + * + * Watching the URL was never right regardless: the cookie is set with `Path=/api/dev-proxy` and no port + * in it, so it already covers every path and every port the panel might point at. The only things that + * warrant a fresh one are entering proxy mode at all, and an explicit reload — where re-requesting is the + * point, since a preview left open overnight should recover by itself rather than show a wall of 401s. + */ useEffect(() => { if (!needsSession) { setReady(true); @@ -87,15 +112,13 @@ const DevProxyFrame = ({ url, reloadToken, onLocation }: DevProxyFrameProps) => let alive = true; setReady(false); setFailed(null); - // Re-run on every reload: the cookie has a lifetime, and a preview left open overnight should recover - // by itself rather than showing a wall of 401s. get<{ ok: boolean }>('/dev-proxy/session') .then(() => alive && setReady(true)) .catch((err: unknown) => alive && setFailed(err instanceof Error ? err.message : 'could not authorise')); return () => { alive = false; }; - }, [needsSession, reloadToken, url]); + }, [needsSession, reloadToken]); if (failed) { return ( @@ -107,7 +130,11 @@ const DevProxyFrame = ({ url, reloadToken, onLocation }: DevProxyFrameProps) => if (!ready) { return
Connecting…
; } - return ; + // Keyed on the ENTRY url as well as the reload counter. Freezing `src` at mount stopped the app's own + // navigation from reloading the frame — and stopped a newly typed URL from loading at all, since nothing + // remounted. The entry url in the key is what distinguishes the two: it changes only when a person points + // the panel somewhere else, and `setUrl` clears the remembered path when they do. + return ; }; /**