diff --git a/packages/reflex-base/news/+7487.bugfix.md b/packages/reflex-base/news/+7487.bugfix.md new file mode 100644 index 00000000000..1ebdb87569e --- /dev/null +++ b/packages/reflex-base/news/+7487.bugfix.md @@ -0,0 +1 @@ +Use the browser's current route when building the websocket connection's initial hydration event, so navigation during connection setup cannot run the previous page's `on_load` handler or redirect. diff --git a/packages/reflex-base/src/reflex_base/.templates/web/utils/state.js b/packages/reflex-base/src/reflex_base/.templates/web/utils/state.js index aa79aedf662..04ba2cefbdf 100644 --- a/packages/reflex-base/src/reflex_base/.templates/web/utils/state.js +++ b/packages/reflex-base/src/reflex_base/.templates/web/utils/state.js @@ -719,6 +719,12 @@ const withRouterData = (event, params) => { return event; }; +const currentRouteKey = () => { + const loc = locationRef.current ?? window.location; + const liveLoc = env.MOUNT_TARGET ? loc : window.location; + return loc.pathname + (liveLoc.search ?? ""); +}; + /** * Send an event to the server via REST. * @param event The current event. @@ -916,6 +922,7 @@ export const connect = async ( // Get backend URL object from the endpoint. const endpoint = getBackendURL(EVENTURL); + let bootRoute; // The hydrate event rides in the socket.io CONNECT packet, so the backend // starts loading state as soon as the namespace connects instead of after @@ -923,12 +930,13 @@ export const connect = async ( // the backend as CompileVars.CONNECT_AUTH_EVENT and CONNECT_AUTH_STATE_SCHEMA; // the state schema goes along on every (re)connect, since a reconnect can // land on another backend version. - const bootAuth = (first) => { + const bootAuth = (first) => (callback) => { const boot_event = withRouterData(app.initialEvents(first)[0], params); + bootRoute = currentRouteKey(); recordSentStorageValues(boot_event); // Instrumentation hook (installed by reflex-otel): may add a traceparent. window.__reflex_otel?.onEventSend?.(boot_event); - return { event: boot_event, state_schema: app.stateSchema }; + callback({ event: boot_event, state_schema: app.stateSchema }); }; // Create the socket. A new session's token is saved here, once the app has @@ -949,6 +957,8 @@ export const connect = async ( warmSocket ?? createSocket(endpoint, transports, session_token); warmSocket = null; cancelWarmup(); + // Socket.IO calls this when the namespace connection is sent, so route data + // reflects any navigation that happened while the transport was connecting. socket.current.auth = bootAuth(true); socket.current.wait_connect = !socket.current.connected; // Ensure undefined fields in events are sent as null instead of removed @@ -1063,6 +1073,19 @@ export const connect = async ( window.__reflex_otel?.onSocketConnect?.(); window.addEventListener("pagehide", pagehideHandler); window.addEventListener("beforeunload", disconnectTrigger); + // The connection auth event already loads its route, and every queued + // on_load loads the route current when it is sent. Keep the last queued + // on_load, after every navigation's storage update, only if the route + // changed after the auth event was built. + let keepOnLoad = bootRoute !== currentRouteKey(); + for (let i = event_queue.length - 1; i >= 0; i--) { + if (event_queue[i].name !== app.on_load_internal_event) continue; + if (keepOnLoad) { + keepOnLoad = false; + } else { + event_queue.splice(i, 1); + } + } // Drain any initial events from the queue. while (event_queue.length > 0) { await processEvent(socket.current, navigate, params); diff --git a/packages/reflex-base/src/reflex_base/compiler/templates.py b/packages/reflex-base/src/reflex_base/compiler/templates.py index 38072d97b53..4899a9230ad 100644 --- a/packages/reflex-base/src/reflex_base/compiler/templates.py +++ b/packages/reflex-base/src/reflex_base/compiler/templates.py @@ -312,6 +312,7 @@ def context_template( state_str = ( rf""" export const state_name = "{state_name}" +export const on_load_internal_event = '{state_name}.{constants.CompileVars.ON_LOAD_INTERNAL}' export const exception_state_name = "{constants.CompileVars.FRONTEND_EXCEPTION_STATE_FULL}" @@ -366,6 +367,7 @@ def context_template( if state_name else """ export const state_name = undefined +export const on_load_internal_event = undefined export const exception_state_name = undefined @@ -426,6 +428,7 @@ def context_template( initialState, clientStorage, state_name, + on_load_internal_event, stateSchema, exception_state_name, onLoadInternalEvent, diff --git a/tests/integration/tests_playwright/test_navigation_before_connect.py b/tests/integration/tests_playwright/test_navigation_before_connect.py new file mode 100644 index 00000000000..c1b96e39a3c --- /dev/null +++ b/tests/integration/tests_playwright/test_navigation_before_connect.py @@ -0,0 +1,87 @@ +"""Regressions for navigation while the initial websocket is connecting.""" + +from collections.abc import Generator + +import pytest +from playwright.sync_api import Page, WebSocketRoute, expect + +from reflex.testing import AppHarness + + +def NavigationBeforeConnectApp(): + """App whose first route redirects if its stale on_load runs.""" + import reflex as rx + + class NavigationState(rx.State): + loads: list[str] = [] + + @rx.event + def slow_load(self): + self.loads.append("slow") + return rx.redirect("/hijack") + + @rx.event + def other_load(self): + self.loads.append("other") + + app = rx.App() + + def page_content(): + return rx.vstack( + rx.foreach(NavigationState.loads, lambda item: rx.text(item)), + rx.link("Other", href="/other", id="other-link"), + ) + + app.add_page(page_content, route="/slow", on_load=NavigationState.slow_load) + app.add_page(page_content, route="/other", on_load=NavigationState.other_load) + app.add_page(page_content, route="/hijack") + + +@pytest.fixture(scope="module") +def navigation_before_connect_app( + app_harness_env: type[AppHarness], + tmp_path_factory: pytest.TempPathFactory, +) -> Generator[AppHarness, None, None]: + """Start the app in dev or production mode. + + Yields: + The running app harness. + """ + with app_harness_env.create( + root=tmp_path_factory.mktemp("navigation_before_connect"), + app_name=f"navigationbeforeconnect_{app_harness_env.__name__.lower()}", + app_source=NavigationBeforeConnectApp, + ) as harness: + yield harness + + +def test_navigation_before_socket_connect_uses_current_route( + navigation_before_connect_app: AppHarness, page: Page +): + """A delayed connect must not run the old page's on_load or redirect.""" + assert navigation_before_connect_app.frontend_url is not None + frontend_url = navigation_before_connect_app.frontend_url.rstrip("/") + sockets: list[WebSocketRoute] = [] + page_errors: list[str] = [] + page.on("pageerror", lambda error: page_errors.append(str(error))) + + def hold_socket(socket: WebSocketRoute) -> None: + sockets.append(socket) + + page.route_web_socket("**/_event/**", hold_socket) + + page.goto(f"{frontend_url}/slow") + expect(page.get_by_role("link", name="Other")).to_be_visible() + if not sockets: + page.wait_for_event("websocket", timeout=60000) + assert len(sockets) == 1, page_errors + + page.get_by_role("link", name="Other").click() + expect(page).to_have_url(f"{frontend_url}/other") + + sockets[0].connect_to_server() + + expect(page.locator("body")).to_contain_text("other") + expect(page.get_by_text("other", exact=True)).to_have_count(1) + expect(page).to_have_url(f"{frontend_url}/other") + expect(page.locator("body")).not_to_contain_text("slow") diff --git a/tests/units/compiler/state_js.test.mjs b/tests/units/compiler/state_js.test.mjs index 688814a2fde..e3e6dd1486b 100644 --- a/tests/units/compiler/state_js.test.mjs +++ b/tests/units/compiler/state_js.test.mjs @@ -18,6 +18,9 @@ async function setup({ disabled = false, hidden = false, storedToken = "original-token", + deferNamespaceConnect = false, + deferConnectAck = false, + stateName = "root", clientStorage = {}, dropConnects = 0, dropReason = "transport close", @@ -31,6 +34,7 @@ async function setup({ const microtasks = []; const timers = new Map(); const firstHydrates = []; + const onLoadInternalEvent = `${stateName}.test_on_load_internal`; const updates = []; const consoleErrors = []; let connectErrors = []; @@ -77,6 +81,7 @@ async function setup({ connected: false, namespaceConnects: 0, disconnects: 0, + events: [], // Like socket.io-client: the URL path names the namespace. nsp: new URL(url).pathname, // Engine.IO frames of the CONNECT packets sent to the server. @@ -118,30 +123,45 @@ async function setup({ ]) { assert.ok(handlers.has(name), `missing handler: ${name}`); } - // Like Manager._packet: the server receives the frames the encoder returns. - const frames = this.io.encoder.encode({ - type: 0, - nsp: this.nsp, - data: this.auth, - }); - if (frames.length === 0) { - return; - } - this.frames.push(...frames.map((frame) => `4${frame}`)); - this.namespaceConnects++; - if (this.namespaceConnects <= dropConnects) { - // The transport closes before the CONNECT is acknowledged. A - // transport error reports connect_error before the disconnect. - if (dropReason === "transport error") { - handlers.get("connect_error")(new Error("xhr post error")); + this.finishNamespaceConnect = () => { + if (typeof this.auth === "function") { + this.auth((auth) => { + this.auth = auth; + }); } - handlers.get("disconnect")(dropReason); - return; - } - this.connected = true; - handlers.get("connect")(); - handlers.get("new_token")("assigned-token"); - handlers.get("event")({ delta: { child: { count: 7 } } }); + // Like Manager._packet: encode the auth callback's resolved payload. + const frames = this.io.encoder.encode({ + type: 0, + nsp: this.nsp, + data: this.auth, + }); + if (frames.length === 0) { + return; + } + this.frames.push(...frames.map((frame) => `4${frame}`)); + this.namespaceConnects++; + if (this.namespaceConnects <= dropConnects) { + // The transport closes before the CONNECT is acknowledged. A + // transport error reports connect_error before the disconnect. + if (dropReason === "transport error") { + handlers.get("connect_error")(new Error("xhr post error")); + } + handlers.get("disconnect")(dropReason); + return; + } + this.finishConnectAck = () => { + this.connected = true; + const connected = handlers.get("connect")(); + handlers.get("new_token")("assigned-token"); + handlers.get("event")({ delta: { child: { count: 7 } } }); + return connected; + }; + if (!deferConnectAck) this.finishConnectAck(); + }; + if (!deferNamespaceConnect) this.finishNamespaceConnect(); + }, + emit(name, event) { + this.events.push({ name, event }); }, disconnect() { this.disconnects++; @@ -184,7 +204,9 @@ async function setup({ }, initialState: stateful ? { root: {}, child: {} } : {}, onLoadInternalEvent() {}, - state_name: "root", + on_load_internal_event: onLoadInternalEvent, + state_name: stateName, + stateSchema: { version: "test" }, clientStorage, exception_state_name: "exception", }, @@ -222,6 +244,7 @@ async function setup({ sockets, timers, firstHydrates, + onLoadInternalEvent, window, localStorage, socket, @@ -267,6 +290,123 @@ test("warm the transport without hydrating, then reuse it with every handler att assert.equal(app.timers.size, 0); assert.equal(app.window.sessionStorage.getItem("token"), "assigned-token"); assert.equal(app.socket.current.auth.event.router_data.pathname, "/page"); + assert.deepEqual(app.socket.current.auth.state_schema, { version: "test" }); +}); + +test("connection auth uses the route current when the namespace connects", async () => { + const app = await setup({ deferNamespaceConnect: true }); + app.flush(); + await app.connect(); + assert.equal(app.socket.current.namespaceConnects, 0); + app.window.location = new URL("http://localhost:3000/other?query=value"); + app.socket.current.finishNamespaceConnect(); + assert.equal(app.socket.current.namespaceConnects, 1); + assert.equal(app.socket.current.auth.event.router_data.pathname, "/other"); + assert.equal( + app.socket.current.auth.event.router_data.asPath, + "/other?query=value", + ); +}); + +test("queued on_load is kept when navigation follows auth capture", async () => { + const stateName = "reflex___state____root"; + const app = await setup({ deferConnectAck: true, stateName }); + app.flush(); + await app.connect(); + const socket = app.socket.current; + assert.equal(socket.namespaceConnects, 1); + assert.equal(socket.auth.event.router_data.asPath, "/page?query=value"); + + app.window.location = new URL("http://localhost:3000/other?query=value"); + const onLoadEvent = { name: app.onLoadInternalEvent, payload: {} }; + await app.module.namespace.queueEvents( + [onLoadEvent], + app.socket, + false, + () => {}, + { current: {} }, + ); + assert.equal(app.module.namespace.isStateful(), true); + + await socket.finishConnectAck(); + + assert.deepEqual(JSON.parse(JSON.stringify(socket.events)), [ + { + name: "event", + event: { + ...onLoadEvent, + router_data: { + pathname: "/other", + asPath: "/other?query=value", + query: { query: "value" }, + }, + }, + }, + ]); +}); + +test("queued on_loads for a newer route are sent once, after the last storage update", async () => { + const stateName = "reflex___state____root"; + const app = await setup({ deferConnectAck: true, stateName }); + app.flush(); + await app.connect(); + const socket = app.socket.current; + + // Each navigation queues the browser storage it read, then its on_load. + app.window.location = new URL("http://localhost:3000/other?query=value"); + const onLoadEvent = { name: app.onLoadInternalEvent, payload: {} }; + const storageEvent = (value) => ({ + name: `${stateName}.update_vars_internal_state.update_vars_internal`, + payload: { vars: { "root.token": value } }, + }); + await app.module.namespace.queueEvents( + [storageEvent("first"), { ...onLoadEvent }], + app.socket, + false, + () => {}, + { current: {} }, + ); + await app.module.namespace.queueEvents( + [storageEvent("second"), { ...onLoadEvent }], + app.socket, + false, + () => {}, + { current: {} }, + ); + await socket.finishConnectAck(); + + const router_data = { + pathname: "/other", + asPath: "/other?query=value", + query: { query: "value" }, + }; + assert.deepEqual(JSON.parse(JSON.stringify(socket.events)), [ + { name: "event", event: { ...storageEvent("first"), router_data } }, + { name: "event", event: { ...storageEvent("second"), router_data } }, + { name: "event", event: { ...onLoadEvent, router_data } }, + ]); +}); + +test("queued on_load is discarded when only the hash changes after auth capture", async () => { + const stateName = "reflex___state____root"; + const app = await setup({ deferConnectAck: true, stateName }); + app.window.location = new URL("http://localhost:3000/other?query=value#top"); + app.flush(); + await app.connect(); + const socket = app.socket.current; + + app.window.location = new URL("http://localhost:3000/other?query=value"); + const onLoadEvent = { name: app.onLoadInternalEvent, payload: {} }; + await app.module.namespace.queueEvents( + [onLoadEvent], + app.socket, + false, + () => {}, + { current: {} }, + ); + await socket.finishConnectAck(); + + assert.deepEqual(JSON.parse(JSON.stringify(socket.events)), []); }); test("the size check encodes a CONNECT packet once and leaves other packets alone", async () => { diff --git a/tests/units/compiler/test_compiler.py b/tests/units/compiler/test_compiler.py index dc80f257cb8..5c42f69d418 100644 --- a/tests/units/compiler/test_compiler.py +++ b/tests/units/compiler/test_compiler.py @@ -722,13 +722,19 @@ class SchemaContextChild(SchemaContextRoot): schema = SchemaContextRoot._to_frontend_schema() assert f'export const stateSchema = "{schema}"' in code + on_load_internal_event = ( + f"{SchemaContextRoot.get_full_name()}.{constants.CompileVars.ON_LOAD_INTERNAL}" + ) + assert f"export const on_load_internal_event = '{on_load_internal_event}'" in code start = code.index("registerApp({") assert "stateSchema," in code[start : code.index("});", start)] + assert "on_load_internal_event," in code[start : code.index("});", start)] assert SchemaContextChild.get_full_name() in code # Without state there is nothing to compare, and nothing to connect with. _, code = compiler.compile_contexts(None, None) assert "export const stateSchema = null" in code + assert "export const on_load_internal_event = undefined" in code @pytest.mark.parametrize( diff --git a/tests/units/reflex_base/client_state_updates.mjs b/tests/units/reflex_base/client_state_updates.mjs index 4eabef315bf..190d8f1b2d7 100644 --- a/tests/units/reflex_base/client_state_updates.mjs +++ b/tests/units/reflex_base/client_state_updates.mjs @@ -44,6 +44,7 @@ async function createConnection( setItem: (key, value) => session.set(key, String(value)), }; const body = element("body"); + let authPayload; const socket = { connected: true, io: { @@ -54,6 +55,13 @@ async function createConnection( on: (name, handler) => handlers.set(name, handler), emit: (name, value) => sent.push({ name, value }), connect: () => { + if (typeof socket.auth === "function") { + socket.auth((auth) => { + authPayload = auth; + }); + } else { + authPayload = socket.auth; + } socket.connected = true; handlers.get("connect")(); }, @@ -114,6 +122,8 @@ async function createConnection( reloads, session, socket, + authPayload: () => authPayload, + readAuth: () => socket.auth((auth) => (authPayload = auth)), reports: () => sent.filter((event) => event.name === "client_error"), receive: (update) => handlers.get("event")(update), reconnect: () => handlers.get("connect")(), @@ -165,14 +175,15 @@ test("an unknown substate is reported once per connection", async (t) => { test("the state schema rides in the CONNECT auth on connect and reconnect", async (t) => { const c = await createConnection(t); - assert.equal(c.socket.auth.state_schema, FRONTEND_SCHEMA); - assert.equal(c.socket.auth.event.name, "test.hydrate"); - assert.deepEqual(c.socket.auth.event.payload.hashes, []); + c.readAuth(); + assert.equal(c.authPayload().state_schema, FRONTEND_SCHEMA); + assert.equal(c.authPayload().event.name, "test.hydrate"); + assert.deepEqual(c.authPayload().event.payload.hashes, []); c.reconnectSocket(); assert.equal(c.socket.connected, true); - assert.equal(c.socket.auth.state_schema, FRONTEND_SCHEMA); - assert.equal(c.socket.auth.event.payload.hashes, undefined); + assert.equal(c.authPayload().state_schema, FRONTEND_SCHEMA); + assert.equal(c.authPayload().event.payload.hashes, undefined); }); test("a mismatch in prod reloads once per cooldown, then shows a banner", async (t) => {