hotfix: infinite render of ethereum phantom wallet pop up #250
Conversation
WalkthroughThis PR updates app/hooks/useWallet.ts by removing state setter functions from two useEffect dependency arrays (Ethereum and Starknet wallet effects), so reruns depend only on wallet state values. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant useWallet
participant EthWallet
participant StrkWallet
rect rgb(240,248,255)
note over useWallet: Old flow (before)
useWallet->>useWallet: useEffect(dep: wallet state + setters)
useWallet-->>EthWallet: Trigger connect/auth logic
loop Setter identity changes
useWallet->>useWallet: Effect re-runs
useWallet-->>EthWallet: Re-triggers popup
end
end
rect rgb(245,255,240)
note over useWallet: New flow (after)
useWallet->>useWallet: useEffect(dep: wallet state only)
useWallet-->>EthWallet: Trigger connect/auth logic
note over useWallet,EthWallet: No rerun from setter identity changes
end
sequenceDiagram
participant UI
participant useWallet
participant StrkWallet
rect rgb(245,255,240)
note over useWallet: Starknet effect (after)
useWallet->>useWallet: useEffect(dep: strkWallet state only)
useWallet-->>StrkWallet: Connect/auth as state changes
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/hooks/useWallet.ts (1)
25-34: Fix nonce regex (captures only 1 char) and harden CSRF fetch.
.?matches at most one character; only the first nonce char is captured. This can break auth/replay protection. Also checkcsrffetchokbefore parsing.- const csrfTokenResponse = await fetch("/api/auth/csrf"); - const { csrfToken } = await csrfTokenResponse.json(); + const csrfTokenResponse = await fetch("/api/auth/csrf"); + if (!csrfTokenResponse.ok) { + throw new Error("Failed to fetch CSRF token"); + } + const { csrfToken } = await csrfTokenResponse.json(); - const nonceMatch = message.match(/Nonce:\s*(.?)(?:\n|$)/); + const nonceMatch = message.match(/Nonce:\s*(.+)(?:\n|$)/); const nonce = nonceMatch ? nonceMatch[1].trim() : null;Optionally: pass the generated
nonceas a param toauthenticateWithSignatureto avoid parsing altogether.
🧹 Nitpick comments (4)
app/hooks/useWallet.ts (4)
152-221: Narrow Starknet auth deps to the address.Depending on
accountobject can retrigger when its identity changes even if the address didn’t. Use address + status to limit runs.- }, [strkWallet.account, strkWallet.status]); + }, [strkWallet.account?.address, strkWallet.status]);
274-281: Await Starknet connect if it returns a Promise.Prevents races where platform UI updates before connection completes.
- strkWallet.connectStarknetWallet({ + await strkWallet.connectStarknetWallet({ connector: starknetConnectors[connectorId], });If the API is sync, ignore.
64-73: Avoid capturing the entirestorein callbacks.Pull
setErrorout ofstoreso callbacks don’t depend on the whole object (cuts unnecessary re-creations on unrelated store updates).const { setEthWallet, setStrkWallet, clearError, resetWallet, setWalletPlatform, - ...store + setError, + ...store } = useWalletStore();And update usages/deps below accordingly:
- store.setError(String(error)); + setError(String(error));- [ethWallet, clearError, resetWallet, setWalletPlatform, store] + [ethWallet, clearError, resetWallet, setWalletPlatform, setError]Repeat the same change for STRK connect/disconnect blocks.
36-58: Surface backend error details for easier debugging.When
/api/auth/callback/credentialsfails, try to parse and propagate the error message.- if (!response.ok) { - throw new Error("Authentication failed"); - } + if (!response.ok) { + let detail = "Authentication failed"; + try { detail = (await response.json())?.error ?? detail; } catch {} + throw new Error(detail); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/hooks/useWallet.ts(1 hunks)
🔇 Additional comments (3)
app/hooks/useWallet.ts (3)
84-97: LGTM: removedsetEthWalletfrom deps.Correct to depend only on wallet values; avoids unnecessary effect churn.
144-151: LGTM: removedsetStrkWalletfrom deps.Matches the ETH effect; reduces spurious reruns.
98-143: Remove ethWallet.getSigner from effect deps to prevent re-promptsFile: app/hooks/useWallet.ts — lines 98–143
getSigner function identity may be unstable and retrigger the effect; depend only on ethWallet.address.
- }, [ethWallet.address, ethWallet.getSigner]); + }, [ethWallet.address]);Manual verification: with Phantom (Ethereum) — connect, cancel signature once, confirm no repeated prompts until address changes.
1 participant
Removed the
Zustandsetters (setEthWallet, setStrkWallet) from the dependency arrays in the newly introduceduseEffectsFixes #248
Summary by CodeRabbit