Fix clear vault state on disconnect in Claim page#61
Open
Ehsan-saradar wants to merge 6 commits into
Open
Conversation
- Reset vultisig wallet/attest data eagerly on connect and wagmi disconnect to prevent stale addresses showing when switching vaults - Extract DEFAULT_VULTISIG_WALLET and DEFAULT_ATTEST_DATA constants - Merge step into unified state object - Sort StateProps, destructuring, setState calls, and functions alphabetically - Convert promise chains to async/await with try/catch - Deduplicate isMetaMask, remove no-op setState, add early returns Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReworks the Claim page: adds centralized defaults for attest/vultisig, expands state with connection, tx and loading fields, integrates IOU balance and unclaimed-burns fetching, and implements allowance/approve/burn and claim flows with on-chain polling, receipt parsing, storage hooks, and improved chain-switching. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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
🧹 Nitpick comments (6)
src/pages/Claim.tsx (6)
884-884: MissingsetCurrentPagein dependency array.While
setCurrentPageis likely stable, the exhaustive-deps rule expects it in the array. Consider including it to satisfy linting.Suggested fix
- useEffect(() => setCurrentPage("claim"), []); + useEffect(() => setCurrentPage("claim"), [setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` at line 884, The useEffect call in Claim.tsx currently calls setCurrentPage("claim") with an empty dependency array; add setCurrentPage to the dependency array of that useEffect (i.e., useEffect(() => setCurrentPage("claim"), [setCurrentPage])) so the hook satisfies the exhaustive-deps lint rule and still updates the page when the setter reference changes.
162-197: Missingmessagein dependency array.The callback uses
message.error()butmessageis not included in the dependencies. WhilemessagefromuseCore()is likely stable, explicitly including it satisfies exhaustive-deps and prevents potential stale closure issues.Suggested fix
- }, [vultisigWallet]); + }, [vultisigWallet, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 162 - 197, The useCallback checkAndRegisterVultisigWallet currently references message.error (from useCore) but omits message from its dependency array; update the dependency array for checkAndRegisterVultisigWallet to include message (and any other external vars like registerVultisigWallet if not already stable) so the closure sees the latest message instance and satisfies exhaustive-deps.
569-614: Variable shadowing:attestDataredeclared.Line 571 declares
const attestData = attestBurnResult.data;, which shadows theattestDatafrom component state. While intentional (the API response is needed for the claim), this is confusing since both contain different data (burn attestation vs. wallet registration domain).Consider renaming the local variable for clarity.
Suggested fix
- const attestData = attestBurnResult.data; + const burnAttestData = attestBurnResult.data; if (pollingIntervalRef.current) { clearInterval(pollingIntervalRef.current); pollingIntervalRef.current = null; } pollingStartTimeRef.current = null; setState((prev) => ({ ...prev, - claimAmount: Number(formatEther(BigInt(attestData.amount))), + claimAmount: Number(formatEther(BigInt(burnAttestData.amount))), isPollingAttestBurn: false, })); // ... update remaining usages of attestData to burnAttestData🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 569 - 614, The local const attestData = attestBurnResult.data shadows the component-level attestData state and is confusing; rename the local variable (e.g., attestBurnData or burnAttest) and update all its usages in this block — references in setState (formatEther(BigInt(...))), clearInterval logic, chain switch, and the writeContract call (attestData.domain.verifyingContract, attestData.amount, attestData.baseTxId, attestData.baseEventId, attestData.recipient, attestData.signature) — so the state attestData remains distinct from the API response variable and no other logic changes are made.
231-249: ExtraneousisConnectedin dependency array.
isConnectedis listed in the dependencies but is not used in the function body. The callback only checksaddressandchainId.Suggested fix
- }, [address, chainId, message, isConnected]); + }, [address, chainId, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 231 - 249, The useCallback getIOUVultBalance contains an unused dependency isConnected; remove isConnected from the dependency array so it becomes based only on values actually referenced (e.g., address, chainId, message — and add any other referenced external symbols like wagmiConfig or baseContractAddress if they are not stable) to avoid unnecessary re-creations; update the dependency array for getIOUVultBalance accordingly and ensure ESLint/react-hooks rules are satisfied.
406-416: Event ID calculation may be fragile.Using
logs.length - 1assumes the relevant event is always the last log in the transaction. If additional events are emitted, this could store an incorrect index. However, the claim flow has more robust resolution logic, so this is acceptable for local storage purposes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 406 - 416, The current code sets eventId using logs.length - 1 which can be wrong if multiple events exist; update the logic in the Claim flow (around setClaimTransaction and the variables logs and mergeHash) to locate the correct event index instead of assuming the last log: search the logs array for the entry that matches the transaction/hash or a unique event property (e.g., find index where log.transactionHash === mergeHash or matches the expected event signature) and pass that index as eventId to setClaimTransaction; if no match is found, fall back to logs.length - 1 to preserve existing behavior.
199-229: Missingmessagein dependency array.Same as
checkAndRegisterVultisigWallet—message.error()is called butmessageis not in deps.Suggested fix
- }, [address, burnAmount, attestData.domain.verifyingContract, chainId]); + }, [address, burnAmount, attestData.domain.verifyingContract, chainId, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 199 - 229, The checkTokenAllowance useCallback calls message.error(...) but does not include message in its dependency array, which can lead to stale closures; update the dependency array for checkTokenAllowance to include message (in addition to address, burnAmount, attestData.domain.verifyingContract, chainId) so the callback captures the current message API instance; locate the checkTokenAllowance function in Claim.tsx and add message to the dependencies for the useCallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Claim.tsx`:
- Around line 796-807: When api.attestAddress(vultisigWallet.account) returns
success: false the code never clears the loading flag; update the else branch
(or add a finally) to call setState and set connecting: false (and optionally
ensure isWalletRegistered:false and attestData cleared) so the UI is not left
stuck, referencing the api.attestAddress call and the setState updater that
currently sets attestData/connecting/isWalletRegistered.
---
Nitpick comments:
In `@src/pages/Claim.tsx`:
- Line 884: The useEffect call in Claim.tsx currently calls
setCurrentPage("claim") with an empty dependency array; add setCurrentPage to
the dependency array of that useEffect (i.e., useEffect(() =>
setCurrentPage("claim"), [setCurrentPage])) so the hook satisfies the
exhaustive-deps lint rule and still updates the page when the setter reference
changes.
- Around line 162-197: The useCallback checkAndRegisterVultisigWallet currently
references message.error (from useCore) but omits message from its dependency
array; update the dependency array for checkAndRegisterVultisigWallet to include
message (and any other external vars like registerVultisigWallet if not already
stable) so the closure sees the latest message instance and satisfies
exhaustive-deps.
- Around line 569-614: The local const attestData = attestBurnResult.data
shadows the component-level attestData state and is confusing; rename the local
variable (e.g., attestBurnData or burnAttest) and update all its usages in this
block — references in setState (formatEther(BigInt(...))), clearInterval logic,
chain switch, and the writeContract call (attestData.domain.verifyingContract,
attestData.amount, attestData.baseTxId, attestData.baseEventId,
attestData.recipient, attestData.signature) — so the state attestData remains
distinct from the API response variable and no other logic changes are made.
- Around line 231-249: The useCallback getIOUVultBalance contains an unused
dependency isConnected; remove isConnected from the dependency array so it
becomes based only on values actually referenced (e.g., address, chainId,
message — and add any other referenced external symbols like wagmiConfig or
baseContractAddress if they are not stable) to avoid unnecessary re-creations;
update the dependency array for getIOUVultBalance accordingly and ensure
ESLint/react-hooks rules are satisfied.
- Around line 406-416: The current code sets eventId using logs.length - 1 which
can be wrong if multiple events exist; update the logic in the Claim flow
(around setClaimTransaction and the variables logs and mergeHash) to locate the
correct event index instead of assuming the last log: search the logs array for
the entry that matches the transaction/hash or a unique event property (e.g.,
find index where log.transactionHash === mergeHash or matches the expected event
signature) and pass that index as eventId to setClaimTransaction; if no match is
found, fall back to logs.length - 1 to preserve existing behavior.
- Around line 199-229: The checkTokenAllowance useCallback calls
message.error(...) but does not include message in its dependency array, which
can lead to stale closures; update the dependency array for checkTokenAllowance
to include message (in addition to address, burnAmount,
attestData.domain.verifyingContract, chainId) so the callback captures the
current message API instance; locate the checkTokenAllowance function in
Claim.tsx and add message to the dependencies for the useCallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8479760f-167d-410f-a884-361f8fe5113d
📒 Files selected for processing (1)
src/pages/Claim.tsx
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/pages/Claim.tsx (5)
162-197: Missing dependencies inuseCallback.
registerVultisigWalletis called within this callback but is not included in the dependency array. This can lead to stale closure issues ifregisterVultisigWalletreference changes.♻️ Proposed fix
- }, [vultisigWallet]); + }, [vultisigWallet, registerVultisigWallet]);Note: This may require moving
registerVultisigWalletdefinition before this callback or using a ref pattern to avoid circular dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 162 - 197, The useCallback checkAndRegisterVultisigWallet closes over registerVultisigWallet but doesn’t list it in the dependency array, causing potential stale closures; update the dependencies for checkAndRegisterVultisigWallet to include registerVultisigWallet (or refactor so registerVultisigWallet is defined before this callback or accessed via a stable ref) so the latest registerVultisigWallet reference is used when calling registerVultisigWallet from inside checkAndRegisterVultisigWallet.
231-249: Unused dependency inuseCallback.
isConnectedis included in the dependency array but is not used within the callback body. Consider removing it to accurately reflect the callback's dependencies.♻️ Proposed fix
- }, [address, chainId, message, isConnected]); + }, [address, chainId, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 231 - 249, The useCallback for getIOUVultBalance lists isConnected in its dependency array but never references it; update getIOUVultBalance's dependency array to remove isConnected so it only includes the actual dependencies (address, chainId, message) to accurately reflect what the callback depends on and avoid unnecessary re-creations.
569-614: Variable shadowing:attestDatashadows component state.The local
const attestData = attestBurnResult.dataat line 571 shadows the component-levelattestDatafrom state (line 122). This can cause confusion and potential bugs if the code is modified later.♻️ Proposed fix - rename local variable
if (attestBurnResult.success) { try { - const attestData = attestBurnResult.data; + const burnAttestData = attestBurnResult.data; if (pollingIntervalRef.current) { clearInterval(pollingIntervalRef.current); @@ -580,7 +580,7 @@ pollingStartTimeRef.current = null; setState((prev) => ({ ...prev, - claimAmount: Number(formatEther(BigInt(attestData.amount))), + claimAmount: Number(formatEther(BigInt(burnAttestData.amount))), isPollingAttestBurn: false, }));And update all subsequent usages of
attestDatawithin this block (lines 604-613) toburnAttestData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 569 - 614, The local constant attestData declared from attestBurnResult.data shadows the component-level attestData state; rename the local variable (e.g., to burnAttestData) and update all references inside this block (uses in clearing pollingIntervalRef, setState call that sets claimAmount, the chain switching logic branch, and the writeContract invocation including attestData.domain.verifyingContract and the args list) to use burnAttestData so the component-level state name is not shadowed.
891-891: Missing dependency inuseEffect.
setCurrentPageshould be included in the dependency array per React hooks exhaustive-deps rule. IfsetCurrentPageis stable from context, consider adding it with an eslint-disable comment explaining why, or add it to deps.♻️ Proposed fix
- useEffect(() => setCurrentPage("claim"), []); + useEffect(() => setCurrentPage("claim"), [setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` at line 891, The useEffect call in the Claim component currently omits setCurrentPage from its dependency array; update the effect to either include setCurrentPage in the deps or, if setCurrentPage is guaranteed stable (from context/provider), add an eslint-disable-next-line comment for exhaustive-deps with a brief justification. Locate the useEffect that calls setCurrentPage("claim") and: (a) add setCurrentPage to the dependency array, or (b) add a single-line eslint-disable comment above the useEffect referencing "react-hooks/exhaustive-deps" and a short note like "setCurrentPage is stable from context" to suppress the warning.
419-431: RedundantsetStatecalls forburnLoading.The
burnLoading: falsestate update at line 430 runs unconditionally after the try-catch, causing duplicate calls when the else (line 422) or catch (line 427) branches already set it. Consider consolidating:♻️ Proposed fix using early returns
} else { message.error("Failed to burn tokens"); - - setState((prev) => ({ ...prev, burnLoading: false })); + return setState((prev) => ({ ...prev, burnLoading: false })); } } catch { message.error("Failed to burn tokens"); - - setState((prev) => ({ ...prev, burnLoading: false })); + return setState((prev) => ({ ...prev, burnLoading: false })); } - setState((prev) => ({ ...prev, burnLoading: false })); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 419 - 431, The code redundantly sets burnLoading: false multiple times; consolidate by removing the unconditional setState at the end and ensure burnLoading is cleared in one place—either return early from the else and catch blocks after calling setState((p)=>({...p, burnLoading:false})) or move the single setState((p)=>({...p, burnLoading:false})) into a finally block for the containing function (the handler that calls setState and performs the burn operation), referencing the existing setState and burnLoading updates to locate where to adjust control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Claim.tsx`:
- Around line 719-748: The handler handleSwitchChain assumes window.ethereum
exists when isMetaMask is true, which can throw; before calling
window.ethereum.request in the isMetaMask branch, add a defensive check that
window && window.ethereum are present (and that window.ethereum.request is a
function) and if missing show the same message.error fallback; reference
handleSwitchChain, isMetaMask, window.ethereum and the
"wallet_switchEthereumChain" request so you only call window.ethereum.request
when the object and method are defined, otherwise fall back to the existing
manual switch message or to switchChainAsync as appropriate.
---
Nitpick comments:
In `@src/pages/Claim.tsx`:
- Around line 162-197: The useCallback checkAndRegisterVultisigWallet closes
over registerVultisigWallet but doesn’t list it in the dependency array, causing
potential stale closures; update the dependencies for
checkAndRegisterVultisigWallet to include registerVultisigWallet (or refactor so
registerVultisigWallet is defined before this callback or accessed via a stable
ref) so the latest registerVultisigWallet reference is used when calling
registerVultisigWallet from inside checkAndRegisterVultisigWallet.
- Around line 231-249: The useCallback for getIOUVultBalance lists isConnected
in its dependency array but never references it; update getIOUVultBalance's
dependency array to remove isConnected so it only includes the actual
dependencies (address, chainId, message) to accurately reflect what the callback
depends on and avoid unnecessary re-creations.
- Around line 569-614: The local constant attestData declared from
attestBurnResult.data shadows the component-level attestData state; rename the
local variable (e.g., to burnAttestData) and update all references inside this
block (uses in clearing pollingIntervalRef, setState call that sets claimAmount,
the chain switching logic branch, and the writeContract invocation including
attestData.domain.verifyingContract and the args list) to use burnAttestData so
the component-level state name is not shadowed.
- Line 891: The useEffect call in the Claim component currently omits
setCurrentPage from its dependency array; update the effect to either include
setCurrentPage in the deps or, if setCurrentPage is guaranteed stable (from
context/provider), add an eslint-disable-next-line comment for exhaustive-deps
with a brief justification. Locate the useEffect that calls
setCurrentPage("claim") and: (a) add setCurrentPage to the dependency array, or
(b) add a single-line eslint-disable comment above the useEffect referencing
"react-hooks/exhaustive-deps" and a short note like "setCurrentPage is stable
from context" to suppress the warning.
- Around line 419-431: The code redundantly sets burnLoading: false multiple
times; consolidate by removing the unconditional setState at the end and ensure
burnLoading is cleared in one place—either return early from the else and catch
blocks after calling setState((p)=>({...p, burnLoading:false})) or move the
single setState((p)=>({...p, burnLoading:false})) into a finally block for the
containing function (the handler that calls setState and performs the burn
operation), referencing the existing setState and burnLoading updates to locate
where to adjust control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b10491d-3073-403f-94f2-93f867af6b4c
📒 Files selected for processing (1)
src/pages/Claim.tsx
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pages/Claim.tsx (1)
571-614: Variable shadowing:attestDatashadows the state variable.The local
attestData(line 571) shadows theattestDatadestructured from component state (line 122). While this works correctly here since the local variable is the one needed for the claim, it's confusing and error-prone for future maintenance.♻️ Proposed fix - rename to avoid shadowing
- const attestData = attestBurnResult.data; + const burnAttestData = attestBurnResult.data; if (pollingIntervalRef.current) { clearInterval(pollingIntervalRef.current); pollingIntervalRef.current = null; } pollingStartTimeRef.current = null; setState((prev) => ({ ...prev, - claimAmount: Number(formatEther(BigInt(attestData.amount))), + claimAmount: Number(formatEther(BigInt(burnAttestData.amount))), isPollingAttestBurn: false, })); // ... and update all subsequent references to use burnAttestData const claimHash = await writeContract(wagmiConfig, { chainId: mainnet.id, - address: attestData.domain.verifyingContract as `0x${string}`, + address: burnAttestData.domain.verifyingContract as `0x${string}`, abi: ETHClaimAbi, functionName: "claim", args: [ - attestData.baseTxId, - attestData.baseEventId, - attestData.amount, - attestData.recipient, - attestData.signature, + burnAttestData.baseTxId, + burnAttestData.baseEventId, + burnAttestData.amount, + burnAttestData.recipient, + burnAttestData.signature, ], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 571 - 614, The local const attestData declared from attestBurnResult.data shadows the component-level attestData state variable; rename the local binding (e.g., to attestBurnData or burnAttestData) and update every use in this block (BigInt(...). amount, domain.verifyingContract, baseTxId, baseEventId, amount, recipient, signature, and the writeContract call) so the state attestData remains unshadowed and all references still point to the local attestation result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Claim.tsx`:
- Around line 729-752: The MetaMask switch catch block (around
window.ethereum.request) does not stop execution, so after failing it falls
through to calling switchChainAsync and shows a second error; update the catch
for the window.ethereum.request call to return (or otherwise short-circuit)
immediately after calling message.error so execution does not continue to the
subsequent switchChainAsync({ chainId: currentChainId }) block (references:
window.ethereum.request, currentChainId, base.id, switchChainAsync,
message.error).
- Around line 162-197: The checkAndRegisterVultisigWallet callback uses
registerVultisigWallet but doesn't list it in its dependency array, creating a
stale closure; fix by ensuring registerVultisigWallet is referenced in the
dependency array of checkAndRegisterVultisigWallet (or move the
registerVultisigWallet function definition above checkAndRegisterVultisigWallet
so it can be safely included), or alternatively refactor registerVultisigWallet
to a stable ref (useRef) and call ref.current inside
checkAndRegisterVultisigWallet to avoid circular dependency issues; update the
useCallback dependencies accordingly to include registerVultisigWallet (or the
ref) and vultisigWallet.
---
Nitpick comments:
In `@src/pages/Claim.tsx`:
- Around line 571-614: The local const attestData declared from
attestBurnResult.data shadows the component-level attestData state variable;
rename the local binding (e.g., to attestBurnData or burnAttestData) and update
every use in this block (BigInt(...). amount, domain.verifyingContract,
baseTxId, baseEventId, amount, recipient, signature, and the writeContract call)
so the state attestData remains unshadowed and all references still point to the
local attestation result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db3d30a3-af90-41e9-83c6-e930b75a444a
📒 Files selected for processing (1)
src/pages/Claim.tsx
…ain switch fallthrough Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/pages/Claim.tsx (4)
776-784: Past review issue addressed; minor inconsistency in state reset.The
connecting: falsestate is now correctly reset in the else branch. However,attestDatais set toundefinedhere (line 783) while other locations useDEFAULT_ATTEST_DATAexplicitly (e.g., lines 641, 848). Consider using the constant for consistency.♻️ Proposed fix for consistency
} else { message.error("Failed to verify vultisig wallet"); setState((prev) => ({ ...prev, connecting: false, isWalletRegistered: false, - attestData: undefined, + attestData: DEFAULT_ATTEST_DATA, })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 776 - 784, In the else branch that handles failed vultisig wallet verification, replace the explicit undefined reset of attestData with the predefined DEFAULT_ATTEST_DATA constant for consistency; update the setState call (the one setting connecting, isWalletRegistered, attestData) to use DEFAULT_ATTEST_DATA instead of undefined so it matches other usages (see setState sites around lines where attestData is assigned and the DEFAULT_ATTEST_DATA symbol is used).
899-899: MissingsetCurrentPagein dependency array.ESLint's
exhaustive-depsrule would flag this. If the intent is to run only on mount, consider adding an eslint-disable comment or addingsetCurrentPageto the array (assuming it's stable from context).♻️ Proposed fix
- useEffect(() => setCurrentPage("claim"), []); + useEffect(() => setCurrentPage("claim"), [setCurrentPage]);Or if mount-only behavior is intentional:
+ // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => setCurrentPage("claim"), []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` at line 899, The useEffect in the Claim component currently calls setCurrentPage("claim") but omits setCurrentPage from the dependency array; update the effect by either adding setCurrentPage to the dependency array (useEffect(() => setCurrentPage("claim"), [setCurrentPage])) if setCurrentPage is stable, or if you truly intend mount-only behavior, add an explicit eslint-disable comment (e.g., // eslint-disable-next-line react-hooks/exhaustive-deps) immediately above the useEffect and include a brief justification comment referencing setCurrentPage to silence the rule while preserving intent.
194-212: UnnecessaryisConnectedin dependency array.
isConnectedis listed in the dependency array but isn't used within the function body. The function already guards onaddress(which would be empty when disconnected), making this dependency redundant and causing unnecessary callback recreations.♻️ Proposed fix
- }, [address, chainId, message, isConnected]); + }, [address, chainId, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 194 - 212, The getIOUVultBalance useCallback includes an unused dependency isConnected which causes unnecessary recreations; remove isConnected from the dependency array and keep only the actual used values (address, chainId, message, and any other used refs) so the callback signature in getIOUVultBalance/useCallback matches the variables referenced inside the function body.
382-394: RedundantsetStateforburnLoadingin error paths.The unconditional
setStateat line 393 is necessary for the success path but makes thesetStatecalls in the error branches (lines 385 and 390) redundant. Consider using afinallyblock for cleaner state management.♻️ Proposed fix using finally
try { const mergeHash = await writeContract(wagmiConfig, { // ... contract call }); const { logs, status } = await waitForTransactionReceipt(wagmiConfig, { hash: mergeHash, }); if (status === "success") { message.success("Tokens burned successfully"); setState((prev) => ({ ...prev, burnTxHash: mergeHash })); setClaimTransaction(address, { amount: burnAmount, date: Date.now(), hash: mergeHash, status: "success", isClaimed: false, eventId: logs.length - 1, }); getIOUVultBalance(); loadClaimTransaction(); await new Promise((resolve) => setTimeout(resolve, 2000)); await checkTokenAllowance(); } else { message.error("Failed to burn tokens"); - - setState((prev) => ({ ...prev, burnLoading: false })); } } catch { message.error("Failed to burn tokens"); - - setState((prev) => ({ ...prev, burnLoading: false })); + } finally { + setState((prev) => ({ ...prev, burnLoading: false })); } - - setState((prev) => ({ ...prev, burnLoading: false })); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 382 - 394, The code currently calls setState(..., burnLoading: false) in both error branches and again unconditionally after the try/catch; inside the function containing this snippet (the handler that calls message.error and updates burnLoading) remove the duplicate setState calls from the catch and else branches and move the single setState(..., burnLoading: false) into a finally block so burnLoading is always cleared exactly once; keep the message.error(...) calls in the error branches and ensure you reference setState and the burnLoading state key when implementing the finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Claim.tsx`:
- Around line 549-562: The chain-switch block may fail for non-MetaMask wallets
and currently only logs an error, allowing execution to continue to the
subsequent contract call (e.g. writeContract) on the wrong chain; modify the
catch in the non-MetaMask branch (the switchChainAsync call) so that after
calling message.error and logging the error you return early (or throw) to abort
the flow, ensuring that code after the chain switch (including writeContract)
does not run when chain switching fails; update the logic around
chainId/mainnet.id, isMetaMask, connector.switchChain, and switchChainAsync
accordingly.
---
Nitpick comments:
In `@src/pages/Claim.tsx`:
- Around line 776-784: In the else branch that handles failed vultisig wallet
verification, replace the explicit undefined reset of attestData with the
predefined DEFAULT_ATTEST_DATA constant for consistency; update the setState
call (the one setting connecting, isWalletRegistered, attestData) to use
DEFAULT_ATTEST_DATA instead of undefined so it matches other usages (see
setState sites around lines where attestData is assigned and the
DEFAULT_ATTEST_DATA symbol is used).
- Line 899: The useEffect in the Claim component currently calls
setCurrentPage("claim") but omits setCurrentPage from the dependency array;
update the effect by either adding setCurrentPage to the dependency array
(useEffect(() => setCurrentPage("claim"), [setCurrentPage])) if setCurrentPage
is stable, or if you truly intend mount-only behavior, add an explicit
eslint-disable comment (e.g., // eslint-disable-next-line
react-hooks/exhaustive-deps) immediately above the useEffect and include a brief
justification comment referencing setCurrentPage to silence the rule while
preserving intent.
- Around line 194-212: The getIOUVultBalance useCallback includes an unused
dependency isConnected which causes unnecessary recreations; remove isConnected
from the dependency array and keep only the actual used values (address,
chainId, message, and any other used refs) so the callback signature in
getIOUVultBalance/useCallback matches the variables referenced inside the
function body.
- Around line 382-394: The code currently calls setState(..., burnLoading:
false) in both error branches and again unconditionally after the try/catch;
inside the function containing this snippet (the handler that calls
message.error and updates burnLoading) remove the duplicate setState calls from
the catch and else branches and move the single setState(..., burnLoading:
false) into a finally block so burnLoading is always cleared exactly once; keep
the message.error(...) calls in the error branches and ensure you reference
setState and the burnLoading state key when implementing the finally.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac711cfc-b694-45cb-a9e3-42cdbd315272
📒 Files selected for processing (1)
src/pages/Claim.tsx
- Remove isConnected from getIOUVultBalance deps - Convert getIOUVultBalance to early return pattern - Remove message from getUnclaimedBurns deps - Wrap handleClaimBurnSelect in useCallback with unclaimedBurns dep - Use DEFAULT_ATTEST_DATA instead of undefined for consistency - Move burnLoading: false to finally block to avoid duplicate setState calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Claim.tsx (1)
722-729:⚠️ Potential issue | 🟡 MinorMissing
switchChainAsyncin dependency array.
switchChainAsyncis called on line 711 but not included in the dependency array. This can lead to stale closure issues if theswitchChainAsyncreference changes.🔧 Proposed fix
}, [ isConnected, chainId, message, getIOUVultBalance, currentChainId, connector, + switchChainAsync, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 722 - 729, The useEffect that depends on isConnected, chainId, message, getIOUVultBalance, currentChainId, and connector also calls switchChainAsync (referenced around line where switchChainAsync is invoked) but omits it from the dependency array, risking stale closures; update the dependency array to include switchChainAsync (or ensure switchChainAsync is stable/memoized) so the effect re-runs when its reference changes and the call always uses the current function.
♻️ Duplicate comments (1)
src/pages/Claim.tsx (1)
549-563:⚠️ Potential issue | 🟠 MajorChain switch failure doesn't prevent subsequent contract call.
If
switchChainAsyncfails (lines 553-561), execution continues to thewriteContractcall on line 565, which will likely fail with a confusing error since the user remains on the wrong chain.🛡️ Proposed fix to return early after switch failure
} else { try { await switchChainAsync({ chainId: mainnet.id }); } catch (error) { message.error( "Failed to switch chain to Mainnet. Please switch manually from your wallet.", ); console.error("Error switching chain:", error); + + setState((prev) => ({ + ...prev, + claimLoading: false, + isPollingAttestBurn: false, + })); + + return false; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 549 - 563, The chain-switch block may fail but code continues to call writeContract; update the control flow so that if switching to mainnet fails (the switchChainAsync call or connector.switchChain path), you abort the subsequent contract call — e.g., return early or throw after showing the error message. Locate the chain check using variables and functions chainId, isMetaMask, connector.switchChain, and switchChainAsync, and ensure failure in that try/catch prevents reaching writeContract by exiting the function or skipping the write call.
🧹 Nitpick comments (5)
src/pages/Claim.tsx (5)
832-833: Samemessagedependency consideration applies here.For consistency with the suggestion on
registerVultisigWallet, consider addingmessageto this dependency array as well.🔧 Proposed fix
- }, [vultisigWallet, registerVultisigWallet]); + }, [vultisigWallet, registerVultisigWallet, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 832 - 833, The useEffect that currently depends on vultisigWallet and registerVultisigWallet should also include the message variable in its dependency array to mirror the change suggested for registerVultisigWallet; update the dependency array for the effect referencing vultisigWallet and registerVultisigWallet to include message so the effect reruns when message changes and remains consistent with the other useEffect.
284-311: Minor redundancy in allowance retry logic.Both
checkTokenAllowance()(line 286) and the inlinereadContractcall (lines 289-297) check the allowance. Consider relying on just one approach to reduce redundancy. However, this doesn't affect correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 284 - 311, The allowance check is performed twice: once via checkTokenAllowance() and again with an inline readContract call that sets updatedAllowanceAmount; to remove redundancy, have a single source of truth by updating checkTokenAllowance to return the current allowance (or expose it via a shared variable) and use that returned value inside the retry loop to set allowanceUpdated and compare against burnAmount, removing the inline readContract block (or conversely, move the readContract logic into checkTokenAllowance and call that). Update references to formatEther, burnAmount, allowanceUpdated, and readContract accordingly so the retry loop only calls checkTokenAllowance and uses its result to decide when to stop retrying.
249-250: Consider addingmessageto dependency array for consistency.Similar to other callbacks,
message.erroris used (line 246) butmessageisn't in the dependency array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 249 - 250, The useEffect in the Claim component uses message.error (the message API) but only lists [address] in its dependency array; update that effect to include message (the message API) so it becomes [address, message] to ensure consistent behavior and avoid stale closures when calling message.error inside the effect.
899-899: MissingsetCurrentPagein dependency array.The effect uses
setCurrentPagebut has an empty dependency array. While this is likely intentional for mount-only behavior, ESLint will flag this as a missing dependency.🔧 Proposed fix
- useEffect(() => setCurrentPage("claim"), []); + useEffect(() => setCurrentPage("claim"), [setCurrentPage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` at line 899, The useEffect in Claim.tsx calls setCurrentPage but omits it from the dependency array, which ESLint will flag; either include setCurrentPage in the dependency array (useEffect(() => setCurrentPage("claim"), [setCurrentPage])) to satisfy the rule, or if the mount-only behavior is intentional, add a brief eslint-disable comment above the useEffect (// eslint-disable-next-line react-hooks/exhaustive-deps) with a one-line justification so reviewers know the omission is deliberate; reference the existing useEffect and the setCurrentPage function when making the change.
795-796: Consider addingmessageto dependency array.
message.erroris called within this callback (lines 777, 787) butmessageisn't in the dependency array. If themessagereference fromuseCore()can change, this could cause stale closure issues.🔧 Proposed fix
- }, [vultisigWallet, isWalletRegistered]); + }, [vultisigWallet, isWalletRegistered, message]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Claim.tsx` around lines 795 - 796, The effect using useEffect currently depends on vultisigWallet and isWalletRegistered but calls message.error from the useCore() hook, risking a stale closure if message can change; update the dependency array for the effect that contains the callbacks referencing message.error (the useEffect that closes over vultisigWallet, isWalletRegistered and calls message.error at lines where message.error is invoked) to also include message (or the specific message.error function) so React re-runs the effect when the message object changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Claim.tsx`:
- Around line 368-378: The eventId calculation using logs.length - 1 is fragile;
update the setClaimTransaction call (where mergeHash and logs are used) to
determine eventId by searching the transaction receipt logs for the specific
merge/attest event the same way pollAttestBurn does (match by contract address
and event signature) rather than assuming it's the last log; locate the matching
log index (or other unique identifier) and pass that as eventId so
setClaimTransaction and later loadClaimTransaction reliably reference the
correct event.
---
Outside diff comments:
In `@src/pages/Claim.tsx`:
- Around line 722-729: The useEffect that depends on isConnected, chainId,
message, getIOUVultBalance, currentChainId, and connector also calls
switchChainAsync (referenced around line where switchChainAsync is invoked) but
omits it from the dependency array, risking stale closures; update the
dependency array to include switchChainAsync (or ensure switchChainAsync is
stable/memoized) so the effect re-runs when its reference changes and the call
always uses the current function.
---
Duplicate comments:
In `@src/pages/Claim.tsx`:
- Around line 549-563: The chain-switch block may fail but code continues to
call writeContract; update the control flow so that if switching to mainnet
fails (the switchChainAsync call or connector.switchChain path), you abort the
subsequent contract call — e.g., return early or throw after showing the error
message. Locate the chain check using variables and functions chainId,
isMetaMask, connector.switchChain, and switchChainAsync, and ensure failure in
that try/catch prevents reaching writeContract by exiting the function or
skipping the write call.
---
Nitpick comments:
In `@src/pages/Claim.tsx`:
- Around line 832-833: The useEffect that currently depends on vultisigWallet
and registerVultisigWallet should also include the message variable in its
dependency array to mirror the change suggested for registerVultisigWallet;
update the dependency array for the effect referencing vultisigWallet and
registerVultisigWallet to include message so the effect reruns when message
changes and remains consistent with the other useEffect.
- Around line 284-311: The allowance check is performed twice: once via
checkTokenAllowance() and again with an inline readContract call that sets
updatedAllowanceAmount; to remove redundancy, have a single source of truth by
updating checkTokenAllowance to return the current allowance (or expose it via a
shared variable) and use that returned value inside the retry loop to set
allowanceUpdated and compare against burnAmount, removing the inline
readContract block (or conversely, move the readContract logic into
checkTokenAllowance and call that). Update references to formatEther,
burnAmount, allowanceUpdated, and readContract accordingly so the retry loop
only calls checkTokenAllowance and uses its result to decide when to stop
retrying.
- Around line 249-250: The useEffect in the Claim component uses message.error
(the message API) but only lists [address] in its dependency array; update that
effect to include message (the message API) so it becomes [address, message] to
ensure consistent behavior and avoid stale closures when calling message.error
inside the effect.
- Line 899: The useEffect in Claim.tsx calls setCurrentPage but omits it from
the dependency array, which ESLint will flag; either include setCurrentPage in
the dependency array (useEffect(() => setCurrentPage("claim"),
[setCurrentPage])) to satisfy the rule, or if the mount-only behavior is
intentional, add a brief eslint-disable comment above the useEffect (//
eslint-disable-next-line react-hooks/exhaustive-deps) with a one-line
justification so reviewers know the omission is deliberate; reference the
existing useEffect and the setCurrentPage function when making the change.
- Around line 795-796: The effect using useEffect currently depends on
vultisigWallet and isWalletRegistered but calls message.error from the useCore()
hook, risking a stale closure if message can change; update the dependency array
for the effect that contains the callbacks referencing message.error (the
useEffect that closes over vultisigWallet, isWalletRegistered and calls
message.error at lines where message.error is invoked) to also include message
(or the specific message.error function) so React re-runs the effect when the
message object changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5af5be8c-0fde-4950-ac23-81058520985e
📒 Files selected for processing (1)
src/pages/Claim.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements