Repository navigation
fix: sync shared-name storage vars in the writing tab (#7567) - #7637
anishgurjar wants to merge 4 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/units/compiler/state_js.test.mjs">
<violation number="1" location="tests/units/compiler/state_js.test.mjs:758">
P2: This echo check cannot detect a repeated `localStorage.setItem`: the map value remains `"from-cs0"` after another identical write. Record setter calls (or expose a write log) and assert the echoed update leaves the write count unchanged.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
|
||
| // The storage event only fires in other tabs, so the writing tab syncs its | ||
| // own sibling vars sharing the name. | ||
| assert.equal(app.localStorage.items.get("shared"), "from-cs0"); |
There was a problem hiding this comment.
P2: This echo check cannot detect a repeated localStorage.setItem: the map value remains "from-cs0" after another identical write. Record setter calls (or expose a write log) and assert the echoed update leaves the write count unchanged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/units/compiler/state_js.test.mjs, line 758:
<comment>This echo check cannot detect a repeated `localStorage.setItem`: the map value remains `"from-cs0"` after another identical write. Record setter calls (or expose a write log) and assert the echoed update leaves the write count unchanged.</comment>
<file context>
@@ -724,3 +732,64 @@ test("storage names that are Object.prototype properties sync like any other", a
+
+ // The storage event only fires in other tabs, so the writing tab syncs its
+ // own sibling vars sharing the name.
+ assert.equal(app.localStorage.items.get("shared"), "from-cs0");
+ assert.equal(syncs().length, 1, JSON.stringify(app.emits));
+ assert.equal(
</file context>
d0cba9f to
d931987
Compare
d931987 to
344ae42
Compare
 [Medium impact] The change affects browser storage syncing in apps that share names across synced variables. Fix the guard's handling of changed echoes before merging. Findings
SummaryUpdates same-name synced
Reviews (4) · Last reviewed commit: "Guard same-tab sibling sync against unec..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Re-trigger cubic
One delta writing two same-name vars with different values would queue a sync that excluded the earlier writer, leaving it on the old value while storage held the last write. Build the sync set from every synced var with the name, like handleStorage, so the last write converges all of them.
|
Hi @masenf, this is ready for a look when you have a moment. The workflows are waiting on first-time-contributor approval; the new state.js tests pass locally (37/37, and the new ones fail against the template on main). |
Merging this PR will not alter performance1 warning
Comparing 
|
|
The required app-harness workflow now fails I reduced it against the actual
The same-tab sync generated from an older response is being treated as a fresh write. This needs an ordering fix that preserves same-tab propagation and the existing cross-tab echo protection; simply suppressing every pending echo can also suppress legitimate updates whose backend delta was transformed. I have left the synchronization design to the author/reviewers and have not rerun the deterministic failure. |
| if (previous !== String(value) && !pending_syncs.has(name)) { | ||
| const state_keys = Object.keys(client_storage.local_storage).filter( | ||
| (other_key) => | ||
| client_storage.local_storage[other_key].sync && | ||
| (client_storage.local_storage[other_key].name || other_key) === | ||
| name, | ||
| ); | ||
| // Sync only when another var shares the name. | ||
| if (state_keys.some((other_key) => other_key !== state_key)) { | ||
| pending_syncs.set(name, state_keys); | ||
| } |
There was a problem hiding this comment.
This sync can send a value that is older than one this tab already sent for the sibling. The delta is the reply to an earlier event. If the tab sent update_vars_internal for a sibling after that event (from another tab's storage event), the backend applies this sync after that newer value, and the sync's echo overwrites it.
Sequence with SharedNameState from test_hydration_storage.py (synced and plain share one name):
- Click
set-plain-old. The reply is slow. - Another tab stores
sent. The storage event sendssynced: "sent". - The
plain: "old"delta arrives. It writes storage, and this block sendssynced: "old". - The echo of
sentwrites storage back tosent. - The echo of
oldpasses thelast.writes > echo.writescheck and writesold.
Both tabs end on old. On main they end on sent. This is the test_synced_storage_echo_after_own_write_is_written[shared_name] failure in CI. It also reproduces in a browser with a 1.5 s asyncio.sleep before self.plain = value.
Please skip the sibling sync when a synced var with this name has an unechoed entry in sentStorageValues. Its echo is newer than this delta, and when that echo changes storage, this block queues the sibling sync from it with the correct value.
Generated by Claude Code
There was a problem hiding this comment.
Added the guard and the same-tab Playwright regression test. One edge case: if get_delta rewrites or drops a sent value, or its event errors, an unmatched sent entry can suppress later sibling syncs for that name until a matching echo clears it or reconnect/hydrate heals it. While suppressed, the same-tab sibling can remain stale, as on main. I've kept this scoped to your prescribed ordering fix. Is that limitation acceptable here, or should I address it in this PR?
| ); | ||
| }); | ||
|
|
||
| test("a synced storage write syncs the writing tab's sibling vars", async () => { |
There was a problem hiding this comment.
Please add a browser test for the fixed scenario to tests/integration/tests_playwright/test_hydration_storage.py. The synced page already has SharedNameState. Add a second synced var with the shared name, click a button that sets one of them, and expect both texts to update in the same tab.
This harness replays deltas in the order the test writes them. It does not see the backend's event order, which is where the CI regression shows.
Generated by Claude Code
Skip the sibling sync when any synced var sharing the storage name has an unechoed entry in sentStorageValues, so a stale delta cannot resend an older value while a newer update_vars_internal is in flight. Adds the Playwright regression test requested in review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-base/src/reflex_base/.templates/web/utils/state.js">
<violation number="1" location="packages/reflex-base/src/reflex_base/.templates/web/utils/state.js:1457">
P2: This guard can permanently suppress sibling sync after a sent value is reconciled to a different backend value. `takeEcho` removes entries only on exact matches, so an outstanding entry left by a `get_delta` override makes every later delta skip `pending_syncs`; track pending sends by the applied backend update and clear or reconcile nonmatching echoes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| if ( | ||
| state_keys.some((other_key) => other_key !== state_key) && | ||
| !state_keys.some( | ||
| (other_key) => sentStorageValues[other_key]?.length > 0, |
There was a problem hiding this comment.
P2: This guard can permanently suppress sibling sync after a sent value is reconciled to a different backend value. takeEcho removes entries only on exact matches, so an outstanding entry left by a get_delta override makes every later delta skip pending_syncs; track pending sends by the applied backend update and clear or reconcile nonmatching echoes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/reflex-base/src/reflex_base/.templates/web/utils/state.js, line 1457:
<comment>This guard can permanently suppress sibling sync after a sent value is reconciled to a different backend value. `takeEcho` removes entries only on exact matches, so an outstanding entry left by a `get_delta` override makes every later delta skip `pending_syncs`; track pending sends by the applied backend update and clear or reconcile nonmatching echoes.</comment>
<file context>
@@ -1446,8 +1446,17 @@ const applyClientStorageDelta = (client_storage, delta) => {
+ if (
+ state_keys.some((other_key) => other_key !== state_key) &&
+ !state_keys.some(
+ (other_key) => sentStorageValues[other_key]?.length > 0,
+ )
+ ) {
</file context>
| !state_keys.some( | ||
| (other_key) => sentStorageValues[other_key]?.length > 0, | ||
| ) |
There was a problem hiding this comment.
When a get_delta override replaces or drops a sent value, takeEcho leaves its entry in sentStorageValues: it only removes entries that exactly match the returned value. The new guard treats that old entry as a pending reply and skips every later sibling sync for the storage name. Later writes therefore leave the writing tab's vars different.
Track pending replies separately from the value history, and add a shared-name test with a replaced echo.
This branch has not been deployed
3 participants
Fixes #7567
All Submissions:
Type of change
New Feature Submission:
Changes To Core Features:
What
Several
rx.LocalStorage(..., sync=True)vars can share one storagename(the common case is a named synced field on anrx.ComponentStateused more than once). When the backend applies a write to one of them,applyClientStorageDeltaupdateslocalStorage, which raises astorageevent in the other tabs only. The writing tab never hears it, so its sibling vars sharing the name stay stale until a reload.Following the fix sketched in the issue: when a synced var's value is written to its storage name and the write actually changes storage,
applyClientStorageDeltanow also queues the sameSYNC_LOCAL_STORAGEevent for the other synced vars with that name in this tab, once per storage name per delta. The written var's ownsyncflag does not matter, matching the cross-tab path: storage writes raise thestorageevent either way. Deltas are per-session, so other tabs are unaffected and still sync through the real event. The sibling update records its sent values, so the echoing delta is recognized bytakeEchoand neither writeslocalStorageagain nor queues another sync; a genuinely newer backend value overwrites, re-syncs once, and its echo terminates the cycle.Tests
Five new cases in
tests/units/compiler/state_js.test.mjs(run throughtest_state_js_template.py): sibling sync on a synced write with three vars sharing a name; a write from an unsynced var still syncing its synced siblings; one sync per storage name when one delta writes two same-name vars; no writes or syncs from a not-yet-hydrated delta; and a newer backend value overwriting, re-syncing once, then terminating on its echo.All five pass with the change; four of them fail against the template on
main(the hydrate case guards existing behavior). The full file passes 37/37 and both files are prettier-clean. The test harness'slocalStoragemock gainedsetItemand its socket mock recordsemitso the delta path can be driven directly. I kept this to unit coverage since the existingstate_js.test.mjsharness drives the real template with controlled browser APIs; happy to add a Playwright test if you'd like one.