Viem Migration - Connection issues when open in diff tabs#7441
Viem Migration - Connection issues when open in diff tabs#7441brunota20 wants to merge 49 commits intorelease/28-04-2026from
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change modifies iframe-specific storage handling in the Wagmi configuration by monkey-patching Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/wallet/src/wagmi/config.ts`:
- Around line 55-80: Before overriding localStorage for iframe behavior, run a
one-time purge of any existing legacy keys that start with '@appkit/': iterate
through current localStorage keys and remove (or migrate to sessionStorage)
entries whose keys startWith('@appkit/'), then proceed to bind
origSetItem/origGetItem/origRemoveItem and install the iframe redirect logic in
the same block (refer to isEmbeddedInIframe, origSetItem, origGetItem,
origRemoveItem, and the localStorage.setItem/getItem/removeItem overrides).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74f8fbea-9b98-44fa-8a1b-e22c71896761
📒 Files selected for processing (1)
libs/wallet/src/wagmi/config.ts
| if (typeof window !== 'undefined' && isEmbeddedInIframe()) { | ||
| const origSetItem = localStorage.setItem.bind(localStorage) | ||
| const origGetItem = localStorage.getItem.bind(localStorage) | ||
| const origRemoveItem = localStorage.removeItem.bind(localStorage) | ||
|
|
||
| localStorage.setItem = (key: string, value: string) => { | ||
| if (key.startsWith('@appkit/')) { | ||
| sessionStorage.setItem(key, value) | ||
| } else { | ||
| origSetItem(key, value) | ||
| } | ||
| } | ||
| localStorage.getItem = (key: string): string | null => { | ||
| if (key.startsWith('@appkit/')) { | ||
| return sessionStorage.getItem(key) | ||
| } | ||
| return origGetItem(key) | ||
| } | ||
| localStorage.removeItem = (key: string) => { | ||
| if (key.startsWith('@appkit/')) { | ||
| sessionStorage.removeItem(key) | ||
| } else { | ||
| origRemoveItem(key) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Purge legacy @appkit/* state before installing the iframe redirect.
This only stops new iframe writes. Any @appkit/* entries older builds already left in shared localStorage will still be read by the regular tab after upgrade, so affected users can keep hitting the stale reconnect state until they clear storage manually.
Suggested one-time cleanup
if (typeof window !== 'undefined' && isEmbeddedInIframe()) {
+ for (let i = localStorage.length - 1; i >= 0; i -= 1) {
+ const key = localStorage.key(i)
+
+ if (key?.startsWith('@appkit/')) {
+ localStorage.removeItem(key)
+ }
+ }
+
const origSetItem = localStorage.setItem.bind(localStorage)
const origGetItem = localStorage.getItem.bind(localStorage)
const origRemoveItem = localStorage.removeItem.bind(localStorage)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof window !== 'undefined' && isEmbeddedInIframe()) { | |
| const origSetItem = localStorage.setItem.bind(localStorage) | |
| const origGetItem = localStorage.getItem.bind(localStorage) | |
| const origRemoveItem = localStorage.removeItem.bind(localStorage) | |
| localStorage.setItem = (key: string, value: string) => { | |
| if (key.startsWith('@appkit/')) { | |
| sessionStorage.setItem(key, value) | |
| } else { | |
| origSetItem(key, value) | |
| } | |
| } | |
| localStorage.getItem = (key: string): string | null => { | |
| if (key.startsWith('@appkit/')) { | |
| return sessionStorage.getItem(key) | |
| } | |
| return origGetItem(key) | |
| } | |
| localStorage.removeItem = (key: string) => { | |
| if (key.startsWith('@appkit/')) { | |
| sessionStorage.removeItem(key) | |
| } else { | |
| origRemoveItem(key) | |
| } | |
| } | |
| } | |
| if (typeof window !== 'undefined' && isEmbeddedInIframe()) { | |
| for (let i = localStorage.length - 1; i >= 0; i -= 1) { | |
| const key = localStorage.key(i) | |
| if (key?.startsWith('@appkit/')) { | |
| localStorage.removeItem(key) | |
| } | |
| } | |
| const origSetItem = localStorage.setItem.bind(localStorage) | |
| const origGetItem = localStorage.getItem.bind(localStorage) | |
| const origRemoveItem = localStorage.removeItem.bind(localStorage) | |
| localStorage.setItem = (key: string, value: string) => { | |
| if (key.startsWith('@appkit/')) { | |
| sessionStorage.setItem(key, value) | |
| } else { | |
| origSetItem(key, value) | |
| } | |
| } | |
| localStorage.getItem = (key: string): string | null => { | |
| if (key.startsWith('@appkit/')) { | |
| return sessionStorage.getItem(key) | |
| } | |
| return origGetItem(key) | |
| } | |
| localStorage.removeItem = (key: string) => { | |
| if (key.startsWith('@appkit/')) { | |
| sessionStorage.removeItem(key) | |
| } else { | |
| origRemoveItem(key) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/wallet/src/wagmi/config.ts` around lines 55 - 80, Before overriding
localStorage for iframe behavior, run a one-time purge of any existing legacy
keys that start with '@appkit/': iterate through current localStorage keys and
remove (or migrate to sessionStorage) entries whose keys startWith('@appkit/'),
then proceed to bind origSetItem/origGetItem/origRemoveItem and install the
iframe redirect logic in the same block (refer to isEmbeddedInIframe,
origSetItem, origGetItem, origRemoveItem, and the
localStorage.setItem/getItem/removeItem overrides).
|
The problem is still happening, steps to reproduce: 1 - Open the vercel link: https://swap-dev-git-bruno-cow-925-connection-issues-5aeb03-cowswap-dev.vercel.app/#/8453/swap/WETH/0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913, connect with metamask, it will work fine |
* chore: update sdk and simplify deps * chore(i18n): extract i18n strings [automatic] --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Denis Makarov <limitofzero@gmail.com>
brunota20
changed the title
…et-viem # Conflicts: # pnpm-lock.yaml
brunota20
force-pushed
the
bruno/cow-925-connection-issues-when-the-app-is-opened-in-safe-and-in-a
branch
from
712866e to
95095f2
Compare
…allet_revokePermissions When SafeConnectionHandler switched from the injected connector to the safe connector inside a Safe App iframe, it called wagmi's disconnect() which internally calls wallet_revokePermissions on MetaMask. Since MetaMask manages permissions per origin (not per tab), this revoked the eth_accounts permission for the entire localhost:3000 origin, causing any open browser tab at that URL to lose its wallet connection. Fix: write the shimDisconnect storage flag directly instead of calling disconnect(). This prevents the connector from being reconnected on next load without touching MetaMask's global permissions. The connectSafeInIframe effect still runs and makes the safe connector the active connection. Also introduce IS_CROSS_ORIGIN_IFRAME — a one-time check that distinguishes genuine cross-origin iframes (Safe App) from same-origin ones (browser extensions like Loom or 1Password that also run inside iframes). This ensures SafeConnectionHandler only activates in actual cross-origin contexts.
|
Worked here! |
…s-when-the-app-is-opened-in-safe-and-in-a
lgahdl
force-pushed
the
bruno/cow-925-connection-issues-when-the-app-is-opened-in-safe-and-in-a
branch
from
52195b4 to
a30799a
Compare
…in-safe-and-in-a' of https://github.com/cowprotocol/cowswap into bruno/cow-925-connection-issues-when-the-app-is-opened-in-safe-and-in-a
AppKit was being initialised inside the injected widget (cross-origin iframe), causing localStorage key conflicts with the host app — connecting in the widget would auto-connect the standalone app, and disconnecting the app would disconnect the widget. Skip AppKit for all cross-origin iframes (both Safe App and injected widget) instead of only the Safe iframe. Each context already gets a distinct wagmi storage key, so session state remains isolated. The addWagmiConnector modal patch is no longer needed since AppKit doesn't run in the widget. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…or widget Resolve merge conflicts in config.ts — keep widget AppKit with Storage.prototype interception to isolate @appkit/* keys, conditional enableReconnect, and namespaced wagmi storage keys. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dget - Stop clearing @appkit/* keys on widget init — this was wiping the main app's AppKit state, causing two-step disconnect and stale Account view. - Remove plain injected connector from widget — MetaMask is a per-origin singleton, registering it leaked wallet state between widget and app. - Disable EIP-6963 in widget to prevent auto-detecting the main app's injected wallet. - Keep Storage.prototype interception to redirect widget's @appkit/* operations to sessionStorage, preventing cross-tab storage events. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ow-925-connection-issues-when-the-app-is-opened-in-safe-and-in-a
|
https://www.loom.com/share/48f00f73a0ba47a9bd5f81ee28e55cf8 I tested using one safe tab, one regular tab and one widget tab opened, connecting and disconnecting to check the behavior |
|
We just more tests and the issue is still happening between widget and the app. This issue is being harder because of the new package ( |
|
After a few attempts of fixing the widget standalone mode connection, we are still facing the issue when using wallets like metamask or rabby (extensions), since they are using the same window.ethereum on both tabs. I am still investigating on how to do this using appkit. |
…ow-925-connection-issues-when-the-app-is-opened-in-safe-and-in-a
…ross-tab permission revocation
… wallet_revokePermissions fix
…_CONFIG_MISMATCH on Vercel fix(ci): update @wagmi/core patch hash after whitespace normalization
…s-when-the-app-is-opened-in-safe-and-in-a
There was a problem hiding this comment.
Hey @brunota20 ,
Interaction safe app-regular app works great.
Interaction widget-regular app is still not great. Besides, there are many regressions in the widget itself:
- I'm connected to the app to MM, and I cannot connect to standalone more: it shows 'connection declined' error (this also presents on your video)
- widget: when I'm connected to the standalone mode, I switch to the dapp mode --> it appears that I'm connected, but the side panel shows an opposite state
- widget: I'm connected to both modes. When I disconnect the wallet from the standalone mode, navigate to the dapp mode, I see that the widget shows 'connect wallet' state, but the side panel looks like connected.
- widget: I see an error in the console: could you please check what it means?
This is the video how it works now on Prod https://www.loom.com/share/a50e2d2a7d4742108d40e4f56c450967
And this is what I expect to see in the PR:
- I should be able to connect to 3 different wallets in different chains in 3 modes
- connection is not reset when I change it in one of modes (note: in the video connection is reset with MM because their rate limiting issue)
- connection is not reset in any of modes when I connect to a different wallet in another app
- when I'm signing an order from any of modes, it calls the correct wallet to connect
7 participants
Summary
Fixes https://www.notion.so/cownation/Connection-issues-when-the-app-is-opened-in-Safe-and-in-a-regular-tab-3518da5f04ca8056a0bae093b19f3538
Connection issues when the app is opened in Safe and in a regular tab. We could not connect any wallet on the regular tab when the safe tab was opened.
To Test
https://www.loom.com/share/0b505107a5374579ac15311ea9a699c3