Fix: Disable send transaction button while transaction is in progress#94
Open
Rav1Chauhan wants to merge 1 commit intoDjedAlliance:mainfrom
Open
Fix: Disable send transaction button while transaction is in progress#94Rav1Chauhan wants to merge 1 commit intoDjedAlliance:mainfrom
Rav1Chauhan wants to merge 1 commit intoDjedAlliance:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 `@stablepay-sdk/src/widget/TransactionReview.jsx`:
- Around line 220-234: The outer catch is treating any error from the
onTransactionComplete callback as a transaction failure; wrap the
onTransactionComplete invocation in its own try/catch so a thrown error from the
callback does not trigger setMessage("❌ Transaction failed.") or change
transaction state; call onTransactionComplete({ txHash, network:
selectedNetwork, token: selectedToken?.key, tokenSymbol: selectedToken?.symbol,
amount: contextTransactionDetails?.amount, receivingAddress:
contextTransactionDetails?.receivingAddress }) inside a dedicated try block and
in the callback catch only capture/log the callback error (e.g., via setError or
console.error) without flipping success messaging or setIsSending for the main
transaction flow.
Rav1Chauhan
force-pushed
the
fix/disable-send-transaction-button
branch
from
e1cc5e9 to
900189a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stablepay-sdk/src/widget/TransactionReview.jsx (1)
145-151:⚠️ Potential issue | 🟡 MinorUse a bigint fallback for
amountToSendin the ERC-20 transfer path.The
amountToSendmust be consistently typed asbigint. When truthy,parseUnits()returnsbigint, but the fallback"0"is a string. SinceencodeFunctionData()passes this to auint256parameter, abitype strictly expectsbigint(not string), which can cause type errors during ABI encoding. Change the fallback to0n.Suggested fix
const amountToSend = contextTransactionDetails.amount ? parseUnits( String(contextTransactionDetails.amount), contextTransactionDetails.stableCoinDecimals ) - : "0"; + : 0n;Also applies to: line 77 (similar pattern with
tradeDataBuySc || "0").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stablepay-sdk/src/widget/TransactionReview.jsx` around lines 145 - 151, The fallback value for amountToSend must be a bigint, not a string; change the ternary in TransactionReview.jsx so the false branch returns 0n instead of "0" to ensure amountToSend is always a bigint (this affects the ERC-20 transfer path where parseUnits returns bigint and encodeFunctionData expects uint256); apply the same change for the similar variable tradeDataBuySc (replace "0" with 0n) and ensure any downstream usage (e.g., encodeFunctionData calls) receives the bigint-typed value.
♻️ Duplicate comments (1)
stablepay-sdk/src/widget/TransactionReview.jsx (1)
220-234:⚠️ Potential issue | 🟠 MajorIsolate
onTransactionCompletefailures from transaction status.If the completion callback throws synchronously, the outer catch currently flips UI to “Transaction failed” even after
txHashis set. Handle callback errors in a dedicated try/catch.Suggested fix
if (onTransactionComplete) { - onTransactionComplete({ - txHash, - network: selectedNetwork, - token: selectedToken?.key, - tokenSymbol: selectedToken?.symbol, - amount: contextTransactionDetails?.amount, - receivingAddress: - contextTransactionDetails?.receivingAddress, - }); + try { + await onTransactionComplete({ + txHash, + network: selectedNetwork, + token: selectedToken?.key, + tokenSymbol: selectedToken?.symbol, + amount: contextTransactionDetails?.amount, + receivingAddress: + contextTransactionDetails?.receivingAddress, + }); + } catch (callbackError) { + console.error("onTransactionComplete callback failed:", callbackError); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stablepay-sdk/src/widget/TransactionReview.jsx` around lines 220 - 234, The onTransactionComplete callback invocation inside the try block can throw and is currently caught by the outer catch which incorrectly sets the UI to "Transaction failed"; wrap the onTransactionComplete({...}) call in its own try/catch so that errors from onTransactionComplete are handled separately (e.g., catch and setError or console.error) without changing transaction status or calling setMessage("❌ Transaction failed.")/setIsSending(false) that belong to real transaction failures; locate the invocation of onTransactionComplete, txHash, selectedNetwork, selectedToken, contextTransactionDetails and adjust control flow so the outer catch only handles the actual send/transaction errors while the inner catch isolates callback errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@stablepay-sdk/src/widget/TransactionReview.jsx`:
- Around line 145-151: The fallback value for amountToSend must be a bigint, not
a string; change the ternary in TransactionReview.jsx so the false branch
returns 0n instead of "0" to ensure amountToSend is always a bigint (this
affects the ERC-20 transfer path where parseUnits returns bigint and
encodeFunctionData expects uint256); apply the same change for the similar
variable tradeDataBuySc (replace "0" with 0n) and ensure any downstream usage
(e.g., encodeFunctionData calls) receives the bigint-typed value.
---
Duplicate comments:
In `@stablepay-sdk/src/widget/TransactionReview.jsx`:
- Around line 220-234: The onTransactionComplete callback invocation inside the
try block can throw and is currently caught by the outer catch which incorrectly
sets the UI to "Transaction failed"; wrap the onTransactionComplete({...}) call
in its own try/catch so that errors from onTransactionComplete are handled
separately (e.g., catch and setError or console.error) without changing
transaction status or calling setMessage("❌ Transaction
failed.")/setIsSending(false) that belong to real transaction failures; locate
the invocation of onTransactionComplete, txHash, selectedNetwork, selectedToken,
contextTransactionDetails and adjust control flow so the outer catch only
handles the actual send/transaction errors while the inner catch isolates
callback errors.
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.
Addressed Issues:
Fixes #61
Screenshots/Recordings:
Additional Notes:
Problem
The "Send Transaction" button was not disabled while the transaction was being sent to the blockchain.
This allowed users to:
Click the button multiple times
Trigger multiple transaction requests
Potentially pay twice
This is a critical UX and financial safety issue.
Solution
Introduced a new isSending state in TransactionReview.jsx:
Immediately disables the button when transaction starts
Prevents double invocation of handleBuySc
Re-enables only on failure
Keeps disabled after success
Updated logic:
disabled={isSending || txHash !== null}
This ensures safe transaction handling.
Result
Prevents duplicate payments
Prevents duplicate wallet popups
Improves UX safety
Matches expected behavior from Issue #61
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
Bug Fixes
User Experience