diff --git a/packages/reflex-base/news/+synced-storage-echo.bugfix.md b/packages/reflex-base/news/+synced-storage-echo.bugfix.md new file mode 100644 index 00000000000..7bc41deeb94 --- /dev/null +++ b/packages/reflex-base/news/+synced-storage-echo.bugfix.md @@ -0,0 +1 @@ +`sync=True` `rx.LocalStorage` vars no longer bounce between open tabs forever when the value changes while other tabs are loading, or in several tabs at once: a tab no longer writes a value it sent to the backend back over a newer one another tab stored meanwhile. 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 6916df56371..c0dc568ad56 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 @@ -53,6 +53,23 @@ let backend_state_mismatch = false; // Array holding pending events to be processed. const event_queue = []; +// The event that sends browser storage values to the backend. +const UPDATE_VARS_INTERNAL = + "reflex___state____update_vars_internal_state.update_vars_internal"; +// A frontend event naming one synced localStorage var, turned into an +// UPDATE_VARS_INTERNAL event when it is sent: see syncLocalStorageEvent. +const SYNC_LOCAL_STORAGE = "_sync_local_storage"; + +// Synced localStorage values sent to the backend and not yet echoed back, by +// state key, oldest first, with this tab's localStorage write count at the send. +const sentStorageValues = {}; +// Bounds a key whose echoes a get_delta override changes, so they never match. +const MAX_SENT_STORAGE_VALUES = 256; +// This tab's localStorage write count, and its last write by storage name +// (vars of different state keys may share one). +let localStorageWrites = 0; +const lastLocalStorageWrites = {}; + // Mirrors the data router's location so applyEvent can populate router_data // with the in-widget URL. In embed mode the host page's window.location is // unrelated to the Reflex route, so the backend's on_load and dynamic-route @@ -241,7 +258,8 @@ export const isStateful = () => { return event_queue.some( (event) => typeof event?.name === "string" && - event.name.startsWith("reflex___state"), + (event.name.startsWith("reflex___state") || + event.name === SYNC_LOCAL_STORAGE), ); }; @@ -328,6 +346,19 @@ function urlFrom(string) { return undefined; } +/** + * Build the update_vars_internal event for a synced localStorage var. + * + * The value is read when the event is sent, so it and the write count recorded + * with it come from the same moment. + * @param payload The storage name and state key of the var. + * @returns The update_vars_internal event. + */ +const syncLocalStorageEvent = ({ key, state_key }) => + ReflexEvent(`${app.state_name}.${UPDATE_VARS_INTERNAL}`, { + vars: { [state_key]: localStorage.getItem(key) }, + }); + /** * Handle frontend event or send the event to the backend via Websocket. * @param event The event to send. @@ -513,7 +544,13 @@ export const applyEvent = async (event, socket, navigate, params) => { // Send the event to the server. if (socket) { - const routed_event = withRouterData(event, params); + const routed_event = withRouterData( + event.name == SYNC_LOCAL_STORAGE + ? syncLocalStorageEvent(event.payload) + : event, + params, + ); + recordSentStorageValues(routed_event); // Instrumentation hook (installed by reflex-otel): may add a traceparent. window.__reflex_otel?.onEventSend?.(routed_event); socket.emit("event", routed_event); @@ -702,6 +739,7 @@ export const connect = async ( // the backend as CompileVars.CONNECT_AUTH_EVENT. const bootAuth = (first) => { const boot_event = withRouterData(app.initialEvents(first)[0], params); + recordSentStorageValues(boot_event); // Instrumentation hook (installed by reflex-otel): may add a traceparent. window.__reflex_otel?.onEventSend?.(boot_event); return { event: boot_event }; @@ -1037,6 +1075,48 @@ export const hydrateClientStorage = (client_storage) => { return {}; }; +/** + * Remember the synced localStorage values an event sends, to recognise their echoes. + * + * Only synced vars: the tab sends another tab's newer value only for those. + * @param event The event about to be sent. + */ +const recordSentStorageValues = (event) => { + // ReflexEvent leaves out an empty payload. + const vars = event.payload?.vars; + if ( + vars === undefined || + (event.name !== `${app.state_name}.hydrate_and_load` && + event.name !== `${app.state_name}.${UPDATE_VARS_INTERNAL}`) + ) { + return; + } + const local_storage = app.clientStorage.local_storage; + for (const [state_key, value] of Object.entries(vars)) { + if (!local_storage?.[state_key]?.sync) { + continue; + } + const sent = (sentStorageValues[state_key] ??= []); + sent.push({ value, writes: localStorageWrites }); + if (sent.length > MAX_SENT_STORAGE_VALUES) { + sent.shift(); + } + } +}; + +/** + * Take the sent value that a backend value echoes, and the older ones with it. + * @param state_key The state key of the browser storage var. + * @param value The value the backend sent for it. + * @returns The sent value's entry, or undefined if the value is no echo. + */ +const takeEcho = (state_key, value) => { + const sent = sentStorageValues[state_key]; + const index = sent?.findIndex((entry) => entry.value === value) ?? -1; + // Older values were echoed already, or changed by an override on the way. + return index === -1 ? undefined : sent.splice(0, index + 1)[index]; +}; + /** * Update client storage values based on backend state delta. * @param client_storage The client storage object from context.js @@ -1062,28 +1142,41 @@ const applyClientStorageDelta = (client_storage, delta) => { for (const substate in delta) { for (const key in delta[substate]) { const state_key = `${substate}.${key}`; + const value = delta[substate][key]; if (client_storage.cookies && state_key in client_storage.cookies) { const cookie_options = { ...client_storage.cookies[state_key] }; const cookie_name = cookie_options.name || state_key; delete cookie_options.name; // name is not a valid cookie option - cookies.set(cookie_name, delta[substate][key], cookie_options); + cookies.set(cookie_name, value, cookie_options); } else if ( client_storage.local_storage && state_key in client_storage.local_storage && typeof window !== "undefined" ) { - const options = client_storage.local_storage[state_key]; - localStorage.setItem(options.name || state_key, delta[substate][key]); + const name = client_storage.local_storage[state_key].name || state_key; + const echo = takeEcho(state_key, value); + const last = lastLocalStorageWrites[name]; + // Write an echo only over this tab's own later write, which the backend + // applied before the echoed event. Otherwise storage holds the sent + // value or another tab's newer one. + if ( + !echo || + (last?.writes > echo.writes && + localStorage.getItem(name) === last.value) + ) { + localStorage.setItem(name, value); + lastLocalStorageWrites[name] = { + value, + writes: ++localStorageWrites, + }; + } } else if ( client_storage.session_storage && state_key in client_storage.session_storage && typeof window !== "undefined" ) { const session_options = client_storage.session_storage[state_key]; - sessionStorage.setItem( - session_options.name || state_key, - delta[substate][key], - ); + sessionStorage.setItem(session_options.name || state_key, value); } } } @@ -1265,14 +1358,14 @@ export const useEventLoop = ( // e is StorageEvent const handleStorage = (e) => { - if (storage_to_state_map[e.key]) { - const vars = {}; - vars[storage_to_state_map[e.key]] = e.newValue; - const event = ReflexEvent( - `${app.state_name}.reflex___state____update_vars_internal_state.update_vars_internal`, - { vars: vars }, + const state_key = storage_to_state_map[e.key]; + // Session storage changes in same-origin frames raise storage events too. + if (state_key && e.storageArea === localStorage) { + // The value is read when the event is sent: see syncLocalStorageEvent. + addEvents( + [ReflexEvent(SYNC_LOCAL_STORAGE, { key: e.key, state_key })], + e, ); - addEvents([event], e); } }; diff --git a/tests/integration/tests_playwright/test_hydration_storage.py b/tests/integration/tests_playwright/test_hydration_storage.py index 69bcd1830ec..7a8af6f48d5 100644 --- a/tests/integration/tests_playwright/test_hydration_storage.py +++ b/tests/integration/tests_playwright/test_hydration_storage.py @@ -1,9 +1,10 @@ """Browser regressions for client-storage writes during hydration and assigned defaults.""" -from collections.abc import Generator +import re +from collections.abc import Callable, Generator import pytest -from playwright.sync_api import Page, expect +from playwright.sync_api import Page, WebSocketRoute, expect from reflex.testing import AppHarness, AppHarnessProd @@ -101,6 +102,78 @@ def get_component(cls, initial: str) -> rx.Component: rx.button("Change", on_click=cls.change, id="change-pref"), ) + class SyncState(rx.State): + value: str = rx.LocalStorage("", name="hydrate-sync", sync=True) + + @rx.event + def set_value(self, value: str): + """Store a value that other tabs pick up. + + Args: + value: The value to store. + """ + self.value = value + + class ReplaceState(rx.State): + value: str = rx.LocalStorage("", name="hydrate-replace", sync=True) + plain: str = rx.LocalStorage("", name="hydrate-replace-plain") + _keep: bool = False + + @rx.event + def keep_sent(self): + """Store the value the override replaced at boot, now unreplaced.""" + self._keep = True + self.value = self.plain = "sent" + + @rx.state._override_base_method + def get_delta(self): + """Replace the stored values until a handler stores them on purpose. + + Returns: + The delta, with the values replaced. + """ + delta = super().get_delta() + subdelta = delta.get(self.get_full_name(), {}) + for var in ("value", "plain"): + if not self._keep and subdelta.get(var + FIELD_MARKER) == "sent": + subdelta[var + FIELD_MARKER] = "replaced" + return delta + + class SharedNameState(rx.State): + synced: str = rx.LocalStorage("", name="hydrate-shared", sync=True) + plain: str = rx.LocalStorage("", name="hydrate-shared") + + @rx.event + def set_plain(self, value: str): + """Store a value through the var that shares the synced var's name. + + Args: + value: The value to store. + """ + self.plain = value + + def synced(): + """Display a browser storage var synced across tabs. + + Returns: + The page component. + """ + return rx.box( + rx.text(SyncState.value, id="sync-value"), + rx.button("Old", on_click=SyncState.set_value("old"), id="set-old"), + rx.button("New", on_click=SyncState.set_value("new"), id="set-new"), + rx.text(ReplaceState.value, id="replace-value"), + rx.text(ReplaceState.plain, id="replace-plain"), + rx.button("Keep", on_click=ReplaceState.keep_sent, id="keep-sent"), + rx.text(SharedNameState.synced, id="shared-value"), + rx.button( + "Plain old", + on_click=SharedNameState.set_plain("old"), + id="set-plain-old", + ), + rx.text(rx.cond(rx.State.is_hydrated, "true", "false"), id="hydrated"), + ) + def index(): """Display hydration and normalized storage values. @@ -133,6 +206,7 @@ def assigned(): app.add_page(index) app.add_page(index, route="/loaded", on_load=StorageState.load) app.add_page(assigned, route="/assigned") + app.add_page(synced, route="/synced") @pytest.fixture(scope="module", params=[AppHarness, AppHarnessProd]) @@ -262,3 +336,312 @@ def test_assigned_storage_default_persists( for var in VARS: expect(page.locator(f"#assigned-{var}")).to_have_text("changed") assert page.evaluate("sessionStorage.getItem('assigned-session')") == "changed" + + +# Records the page's own writes of the synced var, and stores values as another +# tab would: its storage event carries `event` (the value stored by default, none +# if null). +SYNC_HELPERS = """ +window.syncWrites = []; +const setItem = Storage.prototype.setItem; +Storage.prototype.setItem = function (key, value) { + if (key === "hydrate-sync") { + window.syncWrites.push(value); + } + return setItem.call(this, key, value); +}; +window.otherTabStores = (value, { key = "hydrate-sync", event = value } = {}) => { + setItem.call(localStorage, key, value); + if (event !== null) { + window.dispatchEvent(new StorageEvent("storage", { + key, newValue: event, storageArea: localStorage, + })); + } +}; +""" + + +def open_synced(app: AppHarness, page: Page) -> str: + """Open the synced page with the helpers above and wait for hydration. + + Args: + app: The running app. + page: The page to open it in. + + Returns: + The URL of the synced page. + """ + assert app.frontend_url is not None + url = f"{app.frontend_url.rstrip('/')}/synced" + page.add_init_script(SYNC_HELPERS) + page.goto(url) + expect(page.locator("#hydrated")).to_have_text("true") + return url + + +def wait_until(page: Page, condition: Callable[[], bool]): + """Wait for a condition while Playwright keeps dispatching websocket messages. + + Args: + page: The page whose event loop dispatches the messages. + condition: The condition to wait for. + + Raises: + TimeoutError: If the condition does not hold within 10 seconds. + """ + for _ in range(100): + if condition(): + return + page.wait_for_timeout(100) + msg = "Condition not met within 10 seconds." + raise TimeoutError(msg) + + +class EventSocket: + """Records a page's event messages and holds back the server's replies on request.""" + + def __init__(self, page: Page, hold: bool = False): + """Route the page's event websocket through this object. + + Args: + page: The page whose websocket to route. + hold: Whether to hold back the replies from the start. + """ + self.page = page + self.sent: list[str] = [] + self.held: list[str | bytes] | None = [] if hold else None + self.route: WebSocketRoute | None = None + page.route_web_socket(re.compile(r".*/_event"), self._connect) + + def _connect(self, route: WebSocketRoute): + """Forward the page's messages and the server's, except held replies. + + Args: + route: The intercepted websocket. + """ + server = route.connect_to_server() + self.route = route + + def to_server(message: str | bytes): + self.sent.append(str(message)) + server.send(message) + + def to_page(message: str | bytes): + if self.held is not None and '"event"' in str(message): + self.held.append(message) + else: + route.send(message) + + route.on_message(to_server) + server.on_message(to_page) + + def wait_sent(self, *parts: str): + """Wait until the page sent a message containing all the parts. + + Args: + parts: The substrings of the message. + """ + wait_until( + self.page, + lambda: any(all(part in m for part in parts) for m in self.sent), + ) + + def hold(self): + """Hold back the server's replies from now on.""" + self.held = [] + + def release(self): + """Deliver the held replies in order and stop holding them back.""" + assert self.route is not None + assert self.held is not None + held, self.held = self.held, None + for message in held: + self.route.send(message) + + +def test_synced_storage_echo_keeps_newer_value( + hydration_storage_app: AppHarness, page: Page +): + """A booting tab does not write back a synced value another tab changed meanwhile. + + The second tab boots with the stored value while its replies are held back, + and the first tab stores a newer value before they arrive. + + Args: + hydration_storage_app: The running app. + page: A fresh browser page. + """ + url = open_synced(hydration_storage_app, page) + page.locator("#set-old").click() + page.wait_for_function("localStorage.getItem('hydrate-sync') === 'old'") + + other = page.context.new_page() + other.add_init_script(SYNC_HELPERS) + socket = EventSocket(other, hold=True) + other.goto(url) + # The boot event is sent once the page's effects, storage listener included, ran. + socket.wait_sent("hydrate_and_load", '"old"') + + page.locator("#set-new").click() + page.wait_for_function("localStorage.getItem('hydrate-sync') === 'new'") + socket.wait_sent("update_vars_internal", '"new"') + + socket.release() + expect(other.locator("#hydrated")).to_have_text("true") + expect(other.locator("#sync-value")).to_have_text("new") + expect(page.locator("#sync-value")).to_have_text("new") + other.wait_for_timeout(500) + assert "old" not in other.evaluate("window.syncWrites") + assert page.evaluate("localStorage.getItem('hydrate-sync')") == "new" + + +def test_synced_storage_event_sends_stored_value( + hydration_storage_app: AppHarness, page: Page +): + """A late storage event syncs the value stored now, not the event's ``newValue``. + + Args: + hydration_storage_app: The running app. + page: A fresh browser page. + """ + open_synced(hydration_storage_app, page) + page.evaluate("window.otherTabStores('stored', { event: 'late' })") + expect(page.locator("#sync-value")).to_have_text("stored") + page.wait_for_timeout(500) + assert "late" not in page.evaluate("window.syncWrites") + assert page.evaluate("localStorage.getItem('hydrate-sync')") == "stored" + + +@pytest.mark.parametrize("notified", [True, False]) +def test_synced_storage_echo_crossed_by_another_tab_is_not_written( + hydration_storage_app: AppHarness, page: Page, notified: bool +): + """An echo of a value another tab replaced after it was sent is not written back. + + The tab may get the echo before the storage event of the newer value. + + Args: + hydration_storage_app: The running app. + page: A fresh browser page. + notified: Whether the tab got the newer value's storage event before the echo. + """ + open_synced(hydration_storage_app, page) + # Another tab stores "sent", then "newer" before the echo of "sent" arrives. + page.evaluate( + """notified => { + window.otherTabStores('sent'); + window.otherTabStores('newer', { event: notified ? 'newer' : null }); + }""", + notified, + ) + if not notified: + expect(page.locator("#sync-value")).to_have_text("sent") + page.evaluate("window.otherTabStores('newer')") + expect(page.locator("#sync-value")).to_have_text("newer") + page.wait_for_timeout(500) + assert "sent" not in page.evaluate("window.syncWrites") + assert page.evaluate("localStorage.getItem('hydrate-sync')") == "newer" + + +@pytest.mark.parametrize( + ("button", "key", "display"), + [ + ("set-old", "hydrate-sync", "sync-value"), + ("set-plain-old", "hydrate-shared", "shared-value"), + ], + ids=["same_var", "shared_name"], +) +def test_synced_storage_echo_after_own_write_is_written( + hydration_storage_app: AppHarness, page: Page, button: str, key: str, display: str +): + """An echo is written over a value the tab itself stored after sending it. + + The handler's reply is older than the echo, whether the handler stored the + synced var or another var of the same name. + + Args: + hydration_storage_app: The running app. + page: A fresh browser page. + button: The button whose handler stores "old". + key: The storage name the handler writes. + display: The id of the element showing the synced var. + """ + socket = EventSocket(page) + open_synced(hydration_storage_app, page) + + socket.hold() + page.locator(f"#{button}").click() + socket.wait_sent('"old"') + page.evaluate("key => window.otherTabStores('sent', { key })", key) + socket.wait_sent("update_vars_internal", '"sent"') + + socket.release() + expect(page.locator(f"#{display}")).to_have_text("sent") + page.wait_for_timeout(500) + assert page.evaluate("key => localStorage.getItem(key)", key) == "sent" + + +@pytest.mark.parametrize("other_tab_stores", [False, True], ids=["alone", "other_tab"]) +@pytest.mark.parametrize("synced", [True, False], ids=["synced", "plain"]) +def test_replaced_storage_echo_does_not_hide_later_change( + hydration_storage_app: AppHarness, page: Page, synced: bool, other_tab_stores: bool +): + """A value sent at boot whose echo was replaced is written when a handler sets it. + + Also after another tab stored a value meanwhile, synced or not. + + Args: + hydration_storage_app: The running app. + page: A fresh browser page. + synced: Whether the var is synced across tabs. + other_tab_stores: Whether another tab stores a value before the handler runs. + """ + key, display = ( + ("hydrate-replace", "#replace-value") + if synced + else ("hydrate-replace-plain", "#replace-plain") + ) + open_synced(hydration_storage_app, page) + page.evaluate("key => localStorage.setItem(key, 'sent')", key) + page.reload() + expect(page.locator("#hydrated")).to_have_text("true") + expect(page.locator(display)).to_have_text("replaced") + page.wait_for_function(f"localStorage.getItem('{key}') === 'replaced'") + + if other_tab_stores: + page.evaluate("key => window.otherTabStores('newer', { key })", key) + expect(page.locator(display)).to_have_text("newer" if synced else "replaced") + + page.locator("#keep-sent").click() + expect(page.locator(display)).to_have_text("sent") + page.wait_for_function(f"localStorage.getItem('{key}') === 'sent'") + page.wait_for_timeout(500) + expect(page.locator(display)).to_have_text("sent") + assert page.evaluate("key => localStorage.getItem(key)", key) == "sent" + + +def test_synced_storage_event_while_disconnected_sends_current_value( + hydration_storage_app: AppHarness, page: Page +): + """A storage event received while disconnected syncs the value stored at send time. + + Args: + hydration_storage_app: The running app. + page: A fresh browser page. + """ + socket = EventSocket(page) + open_synced(hydration_storage_app, page) + # Disconnect as on navigation, then another tab stores twice before the + # reconnect sends the queued event. + page.evaluate( + """() => { + window.dispatchEvent(new Event('pagehide')); + window.otherTabStores('stale'); + window.otherTabStores('fresh', { event: null }); + }""" + ) + socket.wait_sent("update_vars_internal") + page.wait_for_timeout(500) + expect(page.locator("#sync-value")).to_have_text("fresh") + assert not any("update_vars_internal" in m and '"stale"' in m for m in socket.sent) + assert page.evaluate("localStorage.getItem('hydrate-sync')") == "fresh" diff --git a/tests/units/compiler/state_js.test.mjs b/tests/units/compiler/state_js.test.mjs index 9efcc61472b..e1214a109ff 100644 --- a/tests/units/compiler/state_js.test.mjs +++ b/tests/units/compiler/state_js.test.mjs @@ -113,11 +113,14 @@ async function setup({ app: { initialEvents(first) { firstHydrates.push(first); + // Like ReflexEvent, a later hydrate without stored values has no payload. return [ - { - name: "root.hydrate_and_load", - payload: first ? { hashes: ["defaults"] } : {}, - }, + first + ? { + name: "root.hydrate_and_load", + payload: { hashes: ["defaults"] }, + } + : { name: "root.hydrate_and_load" }, ]; }, initialState: stateful ? { root: {}, child: {} } : {},