Add a fallback polling when the ws connection is lost on mobile#617
Add a fallback polling when the ws connection is lost on mobile#617chedieck merged 3 commits intoPayButton:masterfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds polling and per-file retry logic plus a debounced visibilitychange trigger in WidgetContainer to fetch address history (getAddressDetails), parse opReturn into Transaction objects, and feed them to handleNewTransaction until success or POLL_MAX_RETRY attempts. Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser (visibility)
participant Widget as WidgetContainer
participant AddressAPI as Address API
participant Parser as opReturn Parser
participant TxHandler as Transaction Handler
Browser->>Widget: visibilitychange (becomes visible)
Widget->>Widget: guard checks (destination, paymentId, success)
Widget->>Widget: start retry loop (up to POLL_MAX_RETRY)
loop up to POLL_MAX_RETRY
Widget->>AddressAPI: getAddressDetails(address)
alt address data returned
AddressAPI-->>Widget: address history
Widget->>Parser: parse opReturn -> Transaction
Parser-->>Widget: normalized Transaction(s)
Widget->>TxHandler: handleNewTransaction(Transaction)
TxHandler-->>Widget: processed (may set success)
alt success achieved
Widget->>Widget: stop retry loop
end
else no data / error
AddressAPI-->>Widget: empty / error
Widget->>Widget: wait POLL_REQUEST_DELAY, increment retryCount
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react/lib/components/Widget/WidgetContainer.tsx (1)
274-284:⚠️ Potential issue | 🟠 MajorPolling silently skips confirmed transactions — the primary mobile backgrounding scenario
handleNewTransaction(lines 276-278) only passes the guard whentx.confirmed === false. When the device is backgrounded for more than a few seconds, the payment transaction gets mined and itsconfirmedfield becomestruein the address history API response. As a result, the entire loop incheckForTransactionscallshandleNewTransactionfor each tx, but the guard rejects all of them, making the fallback polling effectively a no-op for the exact scenario the PR is designed to fix.For the narrow window where the tx is still in mempool when the check runs, the polling works. But for any device that has been backgrounded long enough for a block to arrive, it won't.
A fix would be to handle
confirmed: true(orconfirmedbeingundefined) in the polling path, either by relaxing the guard incheckForTransactions's loop or by adding an overload that accepts confirmed transactions.💡 Suggested local fix inside the polling loop
- handleNewTransaction(tx); + // In the polling path, process confirmed txs too; the WS path + // already guards for unconfirmed via handleNewTransaction. + if (isGreaterThanZero(resolveNumber(tx.amount))) { + handlePayment(tx); + }This requires exposing/importing
handlePaymentinto scope, or extracting the confirmed-tx check into a separate path.Also applies to: 297-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 274 - 284, handleNewTransaction currently ignores transactions with tx.confirmed === true which causes polling in checkForTransactions to be a no-op when a backgrounded device sees the tx already mined; update the logic so confirmed === true (and confirmed === undefined) are accepted in the polling path by either relaxing the guard in handleNewTransaction to allow confirmed true/undefined when called from checkForTransactions or by changing checkForTransactions to call handlePayment directly for confirmed transactions; ensure you reference and reuse the existing handlePayment function (or expose it into the polling scope) and keep the isGreaterThanZero(resolveNumber(tx.amount)) check intact.
🧹 Nitpick comments (1)
react/lib/components/Widget/WidgetContainer.tsx (1)
382-406: PrefersetTimeoutoversetInterval, and addcheckForTransactionsto depsTwo independent concerns:
Pattern mismatch:
setIntervalimplies repeated firing, but the cleanup (clearInterval) runs on everyretryCountchange, so the interval fires exactly once per setup.setTimeoutexpresses the one-shot intent more clearly and avoids the misleading API.Missing dep:
checkForTransactionsis used inside the effect but is not in[retryCount, success].tois incheckForTransactions's deps but not in the retry effect's deps. Iftochanges mid-retry (e.g., in a multi-address flow), the retry loop silently continues fetching history for the old address.♻️ Proposed fix
- const intervalId = setInterval(async () => { - if (success) { - // Stop retries upon success - setRetryCount(0); - return; - } - - await checkForTransactions(); - - // Increment retry count for next attempt (regardless of success/error) - setRetryCount(prev => prev + 1); - }, 1000); - - return () => { - clearInterval(intervalId); - }; - }, [retryCount, success]); + const timeoutId = setTimeout(async () => { + if (!success) { + await checkForTransactions(); + setRetryCount(prev => prev + 1); + } + }, 1000); + + return () => { + clearTimeout(timeoutId); + }; + }, [retryCount, success, checkForTransactions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 382 - 406, The retry effect currently uses setInterval but is effectively a one-shot per retryCount change and omits checkForTransactions from its deps; replace the setInterval logic in the useEffect that references retryCount, success and setRetryCount with a setTimeout-based one-shot scheduler (and clearTimeout in the cleanup) so each retry schedules exactly one call to checkForTransactions, then increments retryCount via setRetryCount(prev => prev + 1); also add checkForTransactions to the dependency array of that useEffect so changes to the target address/state (e.g., "to") will be honored mid-retry, preserving the existing early returns (retryCount === 0, success, retryCount >= 5).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 333-334: The effect in WidgetContainer that sets up the
visibilitychange listener resets local vars wasHidden and hiddenTimestamp on
every re-run (deps: [to, thisPaymentId, success, disablePaymentId]) causing a
spurious poll when the page is already hidden; fix it by initializing
hiddenTimestamp to Date.now() (instead of 0) and keep wasHidden set from
document.hidden so the 200ms debounce still blocks immediate polling on mount,
i.e., in the setup that declares let wasHidden = document.hidden; let
hiddenTimestamp = 0; change hiddenTimestamp to Date.now() so the
visibilitychange handler (used in the effect) treats the initial hidden state as
recently hidden.
- Around line 300-311: The parsedOpReturn result can be an array when
apiTx.opReturn is empty, causing parsedOpReturn.paymentId/message/rawMessage to
be undefined and violating the Transaction contract; update the WidgetContainer
to defensively normalize parsedOpReturn after calling
parseOpReturnData(apiTx.opReturn ?? '') (e.g., check
Array.isArray(parsedOpReturn) or typeof parsedOpReturn and replace with a
fallback object like { paymentId: '', message: '', rawMessage: '' }) so that the
Transaction object (created in the tx variable) always receives string values
for paymentId, message, and rawMessage; refer to parseOpReturnData,
parsedOpReturn, apiTx.opReturn, and the Transaction construction to locate where
to add the normalization/defaulting.
- Around line 286-320: The useCallback for checkForTransactions currently omits
handleNewTransaction and apiBaseUrl causing a stale-closure; update the
useCallback dependency array to include handleNewTransaction and apiBaseUrl (in
addition to success and to) so checkForTransactions always captures the latest
handleNewTransaction/thisPaymentId and runtime apiBaseUrl; after this change,
also add checkForTransactions to the dependency arrays of the visibility effect
and the retry effect (where they currently call it) so those effects re-run when
the function identity updates.
---
Outside diff comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 274-284: handleNewTransaction currently ignores transactions with
tx.confirmed === true which causes polling in checkForTransactions to be a no-op
when a backgrounded device sees the tx already mined; update the logic so
confirmed === true (and confirmed === undefined) are accepted in the polling
path by either relaxing the guard in handleNewTransaction to allow confirmed
true/undefined when called from checkForTransactions or by changing
checkForTransactions to call handlePayment directly for confirmed transactions;
ensure you reference and reuse the existing handlePayment function (or expose it
into the polling scope) and keep the isGreaterThanZero(resolveNumber(tx.amount))
check intact.
---
Nitpick comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 382-406: The retry effect currently uses setInterval but is
effectively a one-shot per retryCount change and omits checkForTransactions from
its deps; replace the setInterval logic in the useEffect that references
retryCount, success and setRetryCount with a setTimeout-based one-shot scheduler
(and clearTimeout in the cleanup) so each retry schedules exactly one call to
checkForTransactions, then increments retryCount via setRetryCount(prev => prev
+ 1); also add checkForTransactions to the dependency array of that useEffect so
changes to the target address/state (e.g., "to") will be honored mid-retry,
preserving the existing early returns (retryCount === 0, success, retryCount >=
5).
| const checkForTransactions = useCallback(async (): Promise<boolean> => { | ||
| if (success) { | ||
| // Payment already succeeded, stop checking | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| const history = await getAddressDetails(to, apiBaseUrl); | ||
| // Save time by only checking the last few transactions | ||
| const recentTxs = history.slice(0, 5); | ||
|
|
||
| for (const apiTx of recentTxs) { | ||
| // FIXME: the getAddressDetails API returns an object that is NOT | ||
| // the same as the Transaction[] type, so we need to convert it | ||
| const parsedOpReturn = parseOpReturnData(apiTx.opReturn ?? ''); | ||
|
|
||
| const tx: Transaction = { | ||
| hash: apiTx.hash, | ||
| amount: apiTx.amount, | ||
| paymentId: parsedOpReturn.paymentId, | ||
| confirmed: apiTx.confirmed, | ||
| message: parsedOpReturn.message, | ||
| timestamp: apiTx.timestamp, | ||
| address: (apiTx.address as unknown as { address: string }).address, | ||
| rawMessage: parsedOpReturn.rawMessage, | ||
| opReturn: apiTx.opReturn, | ||
| }; | ||
| handleNewTransaction(tx); | ||
| } | ||
| return true; | ||
| } catch (error) { | ||
| // Failed to fetch history, there is no point in retrying | ||
| return false; | ||
| } | ||
| }, [success, to]); |
There was a problem hiding this comment.
checkForTransactions has stale closure risk due to missing handleNewTransaction and apiBaseUrl deps
The useCallback dep array at line 320 is [success, to], but the function closes over both handleNewTransaction and apiBaseUrl:
handleNewTransaction→handlePayment→ closes overthisPaymentId. WhenthisPaymentIdrotates (new payment cycle),handlePaymentandhandleNewTransactionboth get new refs, butcheckForTransactionsretains the oldhandleNewTransactionthat still matches against the previous payment ID. This can cause missed matches or false positives across payment cycles.apiBaseUrlis a prop, making it a runtime value that can change; omitting it means fetch calls silently use a stale base URL.
🔧 Proposed fix
- }, [success, to]);
+ }, [success, to, handleNewTransaction, apiBaseUrl]);Note: after adding
handleNewTransactionhere, both the visibility effect (line 380) and the retry effect (line 406) should also addcheckForTransactionsto their dep arrays.
📝 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.
| const checkForTransactions = useCallback(async (): Promise<boolean> => { | |
| if (success) { | |
| // Payment already succeeded, stop checking | |
| return true; | |
| } | |
| try { | |
| const history = await getAddressDetails(to, apiBaseUrl); | |
| // Save time by only checking the last few transactions | |
| const recentTxs = history.slice(0, 5); | |
| for (const apiTx of recentTxs) { | |
| // FIXME: the getAddressDetails API returns an object that is NOT | |
| // the same as the Transaction[] type, so we need to convert it | |
| const parsedOpReturn = parseOpReturnData(apiTx.opReturn ?? ''); | |
| const tx: Transaction = { | |
| hash: apiTx.hash, | |
| amount: apiTx.amount, | |
| paymentId: parsedOpReturn.paymentId, | |
| confirmed: apiTx.confirmed, | |
| message: parsedOpReturn.message, | |
| timestamp: apiTx.timestamp, | |
| address: (apiTx.address as unknown as { address: string }).address, | |
| rawMessage: parsedOpReturn.rawMessage, | |
| opReturn: apiTx.opReturn, | |
| }; | |
| handleNewTransaction(tx); | |
| } | |
| return true; | |
| } catch (error) { | |
| // Failed to fetch history, there is no point in retrying | |
| return false; | |
| } | |
| }, [success, to]); | |
| const checkForTransactions = useCallback(async (): Promise<boolean> => { | |
| if (success) { | |
| // Payment already succeeded, stop checking | |
| return true; | |
| } | |
| try { | |
| const history = await getAddressDetails(to, apiBaseUrl); | |
| // Save time by only checking the last few transactions | |
| const recentTxs = history.slice(0, 5); | |
| for (const apiTx of recentTxs) { | |
| // FIXME: the getAddressDetails API returns an object that is NOT | |
| // the same as the Transaction[] type, so we need to convert it | |
| const parsedOpReturn = parseOpReturnData(apiTx.opReturn ?? ''); | |
| const tx: Transaction = { | |
| hash: apiTx.hash, | |
| amount: apiTx.amount, | |
| paymentId: parsedOpReturn.paymentId, | |
| confirmed: apiTx.confirmed, | |
| message: parsedOpReturn.message, | |
| timestamp: apiTx.timestamp, | |
| address: (apiTx.address as unknown as { address: string }).address, | |
| rawMessage: parsedOpReturn.rawMessage, | |
| opReturn: apiTx.opReturn, | |
| }; | |
| handleNewTransaction(tx); | |
| } | |
| return true; | |
| } catch (error) { | |
| // Failed to fetch history, there is no point in retrying | |
| return false; | |
| } | |
| }, [success, to, handleNewTransaction, apiBaseUrl]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 286 - 320, The
useCallback for checkForTransactions currently omits handleNewTransaction and
apiBaseUrl causing a stale-closure; update the useCallback dependency array to
include handleNewTransaction and apiBaseUrl (in addition to success and to) so
checkForTransactions always captures the latest
handleNewTransaction/thisPaymentId and runtime apiBaseUrl; after this change,
also add checkForTransactions to the dependency arrays of the visibility effect
and the retry effect (where they currently call it) so those effects re-run when
the function identity updates.
| const parsedOpReturn = parseOpReturnData(apiTx.opReturn ?? ''); | ||
|
|
||
| const tx: Transaction = { | ||
| hash: apiTx.hash, | ||
| amount: apiTx.amount, | ||
| paymentId: parsedOpReturn.paymentId, | ||
| confirmed: apiTx.confirmed, | ||
| message: parsedOpReturn.message, | ||
| timestamp: apiTx.timestamp, | ||
| address: (apiTx.address as unknown as { address: string }).address, | ||
| rawMessage: parsedOpReturn.rawMessage, | ||
| opReturn: apiTx.opReturn, |
There was a problem hiding this comment.
parseOpReturnData('') returns an array, leaving required Transaction fields as undefined
When apiTx.opReturn is undefined or '', parseOpReturnData('') ends up returning the result of parseStringToArray('') (an array), not an object. Accessing .paymentId, .message, and .rawMessage on an array yields undefined, yet the Transaction interface declares both paymentId: string and message: string as required (non-optional) fields. This is a silent type violation.
At minimum, add a null-check or default:
🛡️ Proposed fix
- const parsedOpReturn = parseOpReturnData(apiTx.opReturn ?? '');
+ const parsedOpReturn = apiTx.opReturn
+ ? parseOpReturnData(apiTx.opReturn)
+ : {};
const tx: Transaction = {
hash: apiTx.hash,
amount: apiTx.amount,
- paymentId: parsedOpReturn.paymentId,
+ paymentId: parsedOpReturn.paymentId ?? '',
confirmed: apiTx.confirmed,
- message: parsedOpReturn.message,
+ message: parsedOpReturn.message ?? '',
timestamp: apiTx.timestamp,
address: (apiTx.address as unknown as { address: string }).address,
- rawMessage: parsedOpReturn.rawMessage,
+ rawMessage: parsedOpReturn.rawMessage, // already optional
opReturn: apiTx.opReturn,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 300 - 311, The
parsedOpReturn result can be an array when apiTx.opReturn is empty, causing
parsedOpReturn.paymentId/message/rawMessage to be undefined and violating the
Transaction contract; update the WidgetContainer to defensively normalize
parsedOpReturn after calling parseOpReturnData(apiTx.opReturn ?? '') (e.g.,
check Array.isArray(parsedOpReturn) or typeof parsedOpReturn and replace with a
fallback object like { paymentId: '', message: '', rawMessage: '' }) so that the
Transaction object (created in the tx variable) always receives string values
for paymentId, message, and rawMessage; refer to parseOpReturnData,
parsedOpReturn, apiTx.opReturn, and the Transaction construction to locate where
to add the normalization/defaulting.
| let wasHidden = document.hidden; | ||
| let hiddenTimestamp = 0; |
There was a problem hiding this comment.
wasHidden resets to document.hidden on every effect re-run, risking a spurious check
wasHidden and hiddenTimestamp are local variables initialised at effect setup time. Whenever any dep in [to, thisPaymentId, success, disablePaymentId] changes (e.g., thisPaymentId is set while the device is already backgrounded), the old listener is torn down and a new one is registered with wasHidden = document.hidden (i.e., true) and hiddenTimestamp = 0.
On the next visibilitychange to visible, the debounce check is !wasHidden || Date.now() - hiddenTimestamp < 200. With wasHidden = true and hiddenTimestamp = 0, this evaluates to false || (Date.now() - 0 < 200) which is false, so the check proceeds — even though no hide event occurred while this component was mounted. This triggers an unintended immediate poll.
A simple fix is to initialise hiddenTimestamp to the current timestamp so the 200 ms window still applies, or to not set wasHidden = true during initial setup when the page is already hidden:
🔧 Proposed fix
- let wasHidden = document.hidden;
- let hiddenTimestamp = 0;
+ // Only track hides that happen after this listener is registered.
+ let wasHidden = false;
+ let hiddenTimestamp = 0;📝 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.
| let wasHidden = document.hidden; | |
| let hiddenTimestamp = 0; | |
| // Only track hides that happen after this listener is registered. | |
| let wasHidden = false; | |
| let hiddenTimestamp = 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 333 - 334, The
effect in WidgetContainer that sets up the visibilitychange listener resets
local vars wasHidden and hiddenTimestamp on every re-run (deps: [to,
thisPaymentId, success, disablePaymentId]) causing a spurious poll when the page
is already hidden; fix it by initializing hiddenTimestamp to Date.now() (instead
of 0) and keep wasHidden set from document.hidden so the 200ms debounce still
blocks immediate polling on mount, i.e., in the setup that declares let
wasHidden = document.hidden; let hiddenTimestamp = 0; change hiddenTimestamp to
Date.now() so the visibilitychange handler (used in the effect) treats the
initial hidden state as recently hidden.
|
The CI failed but I have the same error on master, so it's not related to this PR |
Fabcien
force-pushed
the
poll_for_txs_mobile
branch
from
7ffd4c3 to
02fa7b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react/lib/components/Widget/WidgetContainer.tsx (1)
274-284:⚠️ Potential issue | 🟠 MajorPolling fallback will miss transactions that are already confirmed — the most likely scenario it targets.
handleNewTransaction(line 277) only processes transactions withconfirmed === false. However, the primary use case for this fallback is when the user returns after backgrounding the browser — precisely the scenario where enough time may have elapsed for the transaction to be confirmed on-chain. In that case, the poll fetches the transaction but silently discards it, and the user never sees their payment acknowledged.The PR objectives note this as a known limitation, but it significantly reduces the effectiveness of the fallback. Consider relaxing the
confirmedfilter specifically for the polling path (e.g., pass the transactions directly tohandlePaymentinstead of throughhandleNewTransaction), or add a separate code path for polled results that doesn't requireconfirmed === false.Also applies to: 297-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 274 - 284, handleNewTransaction currently ignores already-confirmed transactions (it requires confirmed === false) which causes polled transactions that matured while the app was backgrounded to be dropped; modify the polling path to call handlePayment directly for polled results or add a separate handler (e.g., handlePolledTransaction or extend handleNewTransaction with a source/polled flag) that skips the confirmed === false guard and forwards the Transaction to handlePayment, ensuring the polling code uses this new path so confirmed-on-chain transactions are processed and acknowledged.
🧹 Nitpick comments (1)
react/lib/components/Widget/WidgetContainer.tsx (1)
392-403:setIntervalis used likesetTimeout— only one tick ever fires before cleanup.Since
setRetryCount(prev => prev + 1)triggers a re-render that tears down and re-creates the effect, each interval only fires once. UsingsetTimeoutwould express the intent more clearly and avoid any risk of overlapping calls if the async work takes longer than the interval period.Proposed fix
- const intervalId = setInterval(async () => { + const timeoutId = setTimeout(async () => { if (success) { // Stop retries upon success setRetryCount(0); return; } await checkForTransactions(); // Increment retry count for next attempt (regardless of success/error) setRetryCount(prev => prev + 1); }, 1000); return () => { - clearInterval(intervalId); + clearTimeout(timeoutId); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 392 - 403, The effect in WidgetContainer uses setInterval which, combined with setRetryCount(prev => prev + 1) causing a re-render, results in only one tick; change to a recursive setTimeout pattern so each next tick is scheduled after the async work completes to avoid overlapping calls: replace the setInterval block with a function that awaits checkForTransactions(), handles success by resetting retry count (setRetryCount(0)) and returning early, otherwise increments retry count (setRetryCount(prev => prev + 1)) and schedules another setTimeout for 1000ms; ensure you clear the timeout in the effect cleanup and reference the same identifiers (checkForTransactions, setRetryCount, success) so the behavior matches the original intent without interval re-creation issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 286-290: The async closure for checkForTransactions and the
visibility handler capture a stale success value causing unnecessary retries and
repeated onSuccess calls; fix this by introducing a mutable ref (e.g.,
successRef) that you update whenever setSuccess(true) is called (or via a
useEffect syncing success to successRef.current) and then read
successRef.current inside checkForTransactions (before early returns and inside
the for-loop) and inside the visibility handler where you currently call
setRetryCount(1); also ensure handleNewTransaction/handlePayment update the ref
when they set success so the async loop and visibility logic see the latest
value immediately.
- Line 382: The effects that register visibility and retry behavior are missing
checkForTransactions in their dependency arrays, causing them to keep calling a
stale closure when checkForTransactions' identity changes (which happens when
handleNewTransaction/handlePayment dependencies like currencyObj or thisPrice
change); update both useEffect dependency arrays that currently list [to,
thisPaymentId, success, disablePaymentId] and its retry counterpart to include
checkForTransactions so the effects re-subscribe when the matcher logic changes
(you may also remove redundant items like to/success if they are already
captured transitively via checkForTransactions, but adding checkForTransactions
is required).
- Line 311: The code is unsafely casting apiTx.address to an object and
accessing .address; update the logic around where apiTx is mapped (the code
using apiTx.address in WidgetContainer or the function that builds the
transaction payload) to defensively handle both shapes: if typeof apiTx.address
=== "string" use that string, else if it's an object and has a string .address
property use that, otherwise fall back to a safe default or throw a clear error;
replace the double-cast expression "(apiTx.address as unknown as { address:
string }).address" with a runtime check that extracts the address accordingly
and documents the expected shapes (string | { address: string }) so the code no
longer relies on unsafe casting.
---
Outside diff comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 274-284: handleNewTransaction currently ignores already-confirmed
transactions (it requires confirmed === false) which causes polled transactions
that matured while the app was backgrounded to be dropped; modify the polling
path to call handlePayment directly for polled results or add a separate handler
(e.g., handlePolledTransaction or extend handleNewTransaction with a
source/polled flag) that skips the confirmed === false guard and forwards the
Transaction to handlePayment, ensuring the polling code uses this new path so
confirmed-on-chain transactions are processed and acknowledged.
---
Duplicate comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 335-336: The issue is that wasHidden is initialized from
document.hidden which can cause the next visibility change to bypass debounce if
the effect re-registers while the page is already hidden; change the
initialization of wasHidden to false (leave hiddenTimestamp as 0) so the
visibility-tracking logic in the WidgetContainer effect only treats hides that
occur after listener registration; update any references to wasHidden and
hiddenTimestamp in the visibility change handler to rely on the new
initialization.
---
Nitpick comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 392-403: The effect in WidgetContainer uses setInterval which,
combined with setRetryCount(prev => prev + 1) causing a re-render, results in
only one tick; change to a recursive setTimeout pattern so each next tick is
scheduled after the async work completes to avoid overlapping calls: replace the
setInterval block with a function that awaits checkForTransactions(), handles
success by resetting retry count (setRetryCount(0)) and returning early,
otherwise increments retry count (setRetryCount(prev => prev + 1)) and schedules
another setTimeout for 1000ms; ensure you clear the timeout in the effect
cleanup and reference the same identifiers (checkForTransactions, setRetryCount,
success) so the behavior matches the original intent without interval
re-creation issues.
| return () => { | ||
| document.removeEventListener('visibilitychange', handleVisibilityChange); | ||
| }; | ||
| }, [to, thisPaymentId, success, disablePaymentId]); |
There was a problem hiding this comment.
Stale closure: checkForTransactions missing from dependency arrays of both effects.
Both the visibility effect (line 382) and the retry effect (line 408) call checkForTransactions but omit it from their dependency arrays. When checkForTransactions gets a new identity (e.g., because handleNewTransaction → handlePayment deps like currencyObj or thisPrice change), these effects continue to invoke the stale version that closes over outdated payment parameters. This can cause missed payment matches or false negatives after a price/currency update mid-session.
Proposed fix
}, [to, thisPaymentId, success, disablePaymentId]);
+ // ↑ add checkForTransactions to deps
// Retry mechanism: check every second if payment hasn't succeeded yet
useEffect(() => {
...
- }, [retryCount, success]);
+ }, [retryCount, success, checkForTransactions]);For the visibility effect, adding checkForTransactions will cause re-registration when payment parameters change, which is the correct behavior. You may want to also remove to and success from that array if they're already captured transitively through checkForTransactions, but keeping them explicit is fine for clarity.
Also applies to: 408-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@react/lib/components/Widget/WidgetContainer.tsx` at line 382, The effects
that register visibility and retry behavior are missing checkForTransactions in
their dependency arrays, causing them to keep calling a stale closure when
checkForTransactions' identity changes (which happens when
handleNewTransaction/handlePayment dependencies like currencyObj or thisPrice
change); update both useEffect dependency arrays that currently list [to,
thisPaymentId, success, disablePaymentId] and its retry counterpart to include
checkForTransactions so the effects re-subscribe when the matcher logic changes
(you may also remove redundant items like to/success if they are already
captured transitively via checkForTransactions, but adding checkForTransactions
is required).
|
Test failure |
|
Fixed in #618 |
…oint This returns a view of the raw database but the client expects a simplified and preparsed output, which makes more sense. This updates the interface accordingly, using the existing function that is exactly designed for this. This only break the coded added in PayButton/paybutton#617 since the output of the api call was unused before. Test Plan: The api tests are still green.
|
Added commit 7de14ca to fix the getAddressDetails interface bug. Note that PayButton/paybutton-server#1108 is required for this PR to work when this commit is included. |
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)
react/lib/components/Widget/WidgetContainer.tsx (1)
273-283:⚠️ Potential issue | 🟠 MajorPolling will miss confirmed transactions — the primary mobile scenario.
handleNewTransactionguards ontx.confirmed === false(line 276). When a user backgrounds the app and returns after the payment has been mined,getAddressDetailsreturns confirmed transactions and every call tohandleNewTransaction(tx)is silently dropped. The fallback polling therefore only catches payments that are still in the mempool at the exact moment of the check, which is a very narrow window.The PR description notes this as a known gap, but it directly undermines the feature's stated goal. Two options to consider:
- Extend
handleNewTransactionto also accept confirmed transactions (with appropriate deduplication to avoid double-firing for txs already processed by the WebSocket path).- Inline the
handlePaymentcall insidecheckForTransactionswith an explicitconfirmed-agnostic path for the polling-only code path, so the existing WebSocket path is not affected.Also applies to: 296-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 273 - 283, handleNewTransaction currently ignores confirmed transactions (tx.confirmed === false), causing polling via checkForTransactions/getAddressDetails to miss mined payments; modify logic so polling can trigger handlePayment for confirmed txs while avoiding double-firing from the WebSocket path: either (A) relax handleNewTransaction to accept confirmed transactions and add deduplication (track processed tx hashes/ids in a Set or map keyed by transaction.hash and check before calling handlePayment) or (B) keep handleNewTransaction unchanged and call handlePayment directly inside checkForTransactions for any tx (confirmed or not) but still perform the same dedupe check there; reference functions: handleNewTransaction, handlePayment, checkForTransactions, getAddressDetails.
🧹 Nitpick comments (1)
react/lib/components/Widget/WidgetContainer.tsx (1)
374-385: UsesetTimeoutinstead ofsetIntervalfor a one-shot per-cycle delay.Each time this effect runs, it creates a
setIntervalthat fires exactly once beforesetRetryCount(prev => prev + 1)triggers a re-render, which clears the interval and re-runs the effect. This is semantically asetTimeout— usingsetIntervalhere is misleading and risks multiple firings if a re-render is unexpectedly delayed.♻️ Proposed refactor
- const intervalId = setInterval(async () => { + const timerId = setTimeout(async () => { if (success) { setRetryCount(0); return; } await checkForTransactions(); setRetryCount(prev => prev + 1); }, 1000); return () => { - clearInterval(intervalId); + clearTimeout(timerId); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/components/Widget/WidgetContainer.tsx` around lines 374 - 385, The effect currently uses setInterval (creating intervalId) to wait 1s then call checkForTransactions, update setRetryCount and bail out on success, but because the effect re-renders after setRetryCount the interval behaves like a one-shot and risks duplicate firings; replace setInterval with setTimeout so each cycle schedules a single delayed run: use setTimeout to call the async checkForTransactions, then call setRetryCount(prev => prev + 1) (and clear the timeout in cleanup), keeping the same success check and cleanup behavior around the timeout identifier instead of intervalId.
🤖 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 `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 273-283: handleNewTransaction currently ignores confirmed
transactions (tx.confirmed === false), causing polling via
checkForTransactions/getAddressDetails to miss mined payments; modify logic so
polling can trigger handlePayment for confirmed txs while avoiding double-firing
from the WebSocket path: either (A) relax handleNewTransaction to accept
confirmed transactions and add deduplication (track processed tx hashes/ids in a
Set or map keyed by transaction.hash and check before calling handlePayment) or
(B) keep handleNewTransaction unchanged and call handlePayment directly inside
checkForTransactions for any tx (confirmed or not) but still perform the same
dedupe check there; reference functions: handleNewTransaction, handlePayment,
checkForTransactions, getAddressDetails.
---
Duplicate comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 353-356: The visibility and retry effects are using stale closures
because checkForTransactions (and derived functions like handleNewTransaction)
isn't included in their dependency arrays and success can be stale; add
checkForTransactions to both effect deps so the effects re-subscribe when its
identity changes, and replace the stale boolean check at the retry guard by
reading a synchronized ref (e.g., successRef) that you keep updated via a small
useEffect mirroring setSuccess; then use successRef.current in the conditional
before calling setRetryCount(1) to avoid triggering retries when a recent check
already set success.
- Around line 285-304: The issue is that checkForTransactions captures a stale
success closure so multiple transactions in recentTxs can each trigger
handleNewTransaction/handlePayment/onSuccess; update checkForTransactions to
stop processing further txs once success becomes true by either (a) using a
mutable ref (e.g., successRef) that's updated when setSuccess(true) and checking
successRef.current inside the loop, or (b) replace recentTxs.forEach with a
for...of loop and break immediately if success (or successRef.current) becomes
true; ensure the function references handleNewTransaction and setSuccess to
update the ref or to break so only the first matching tx triggers onSuccess.
- Around line 317-318: The effect's initial state uses wasHidden =
document.hidden and hiddenTimestamp = 0 which causes a false-positive poll when
the page is already hidden; change the initialization so that if document.hidden
is true you also set hiddenTimestamp to Date.now() (e.g., hiddenTimestamp =
document.hidden ? Date.now() : 0) so the visibilitychange debounce check in the
visibility handler correctly treats prior backgrounded state and prevents the
spurious poll; update references to wasHidden and hiddenTimestamp in the
visibilitychange handler accordingly.
---
Nitpick comments:
In `@react/lib/components/Widget/WidgetContainer.tsx`:
- Around line 374-385: The effect currently uses setInterval (creating
intervalId) to wait 1s then call checkForTransactions, update setRetryCount and
bail out on success, but because the effect re-renders after setRetryCount the
interval behaves like a one-shot and risks duplicate firings; replace
setInterval with setTimeout so each cycle schedules a single delayed run: use
setTimeout to call the async checkForTransactions, then call setRetryCount(prev
=> prev + 1) (and clear the timeout in cleanup), keeping the same success check
and cleanup behavior around the timeout identifier instead of intervalId.
…oint (#1108) This returns a view of the raw database but the client expects a simplified and preparsed output, which makes more sense. This updates the interface accordingly, using the existing function that is exactly designed for this. This only break the coded added in PayButton/paybutton#617 since the output of the api call was unused before. Test Plan: The api tests are still green.
It is common on mobile to move the widget to the background, which causes the DNS to fail. Other issues might also happen, including the even already being fired when the widget is back to foreground. To cover all these use cases this commit adds a polling phase when the widget become visible (event triggered when the browser page goes to foreground on mobile). It will poll for the last 5 transactions from the history once and check if there is a matching payment tx, otherwise it retries every second for 5s which should be plenty for the tx to be discovered. The retry stops as soon as the payment is marked successful. In practice the mobile apps (either Marlin or Cashtab) add a modal window and a small delay before returning to the browser so the retry is not needed most of the time. This has been tested with a remote console, logs and tweaks to make sure all the scenarios are covered properly. Note that this commit does NOT fix the existing bugs. This should be done in other commits/PRs and reference the appropriated issue. More specifically the following bugs are worked around but not fixed: - The returned object for `getAddressDetails` does not match its interface. This needs to be dealed with on server side. - There is no handling of the chronik ws connection closing when the browser goes to background (which cuts network on mobile). No attempt is made to reconnect the lost ws connection either, so polling takes over. - The `shouldTriggerOnSuccess` function doesn't filter out the payment ID when it's undefined, meaning it will match any tx that looks like the payment one but has no payment ID associated with it. This commit avoids this by exiting early and avoiding the call in this case. - Confirmed transactions are ignored. I have no idea why this is the case but it is consistent across the codebase, so any transaction being mined before it entered the chronik instance mempool will be missed.
This require PayButton/paybutton-server#1108 to work properly. The server PR fixes the returned type of getAddressDetails so we don't need to convert anymore. Test Plan: Run against the updated server, add some logs if desired, and check the payment is successful on mobile.
Fabcien
force-pushed
the
poll_for_txs_mobile
branch
from
7de14ca to
de4b439
Compare
|
Rebased to fix the test, no code change |
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
In mobile, if I
- open up a paybutton in a browser, (testing localhost:10001/single.html)
- switch to another app,
- make the payment
- go back to that paybutton on the browser
it will not fetch the tx after 5s or so, as expected. Of course, switching back to the browser before making the payment makes it work.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react/lib/util/constants.ts (1)
42-44: Minor consistency nits in the new polling constants block.A few small inconsistencies worth tidying up:
POLL_MAX_RETRYhas no inline comment while the other two new constants do — a brief// 5 attemptskeeps things uniform.POLL_REQUEST_DELAYomits the_MSsuffix used by the existingAUTO_CLOSE_DEFAULT_MS, making the unit implicit (though the inline comment covers it).- No grouping comment above the block; the rest of the file uses doc-style comments above logically related constants (e.g., line 16).
✨ Proposed consistency fix
+// Polling fallback: fetch address history on visibility restore (mobile) -export const POLL_TX_HISTORY_LOOKBACK = 5 // request last 5 txs -export const POLL_REQUEST_DELAY = 1000 // 1s -export const POLL_MAX_RETRY = 5 +export const POLL_TX_HISTORY_LOOKBACK = 5 // request last 5 txs +export const POLL_REQUEST_DELAY_MS = 1000 // 1s +export const POLL_MAX_RETRY = 5 // 5 attemptsNote: if renaming
POLL_REQUEST_DELAY→POLL_REQUEST_DELAY_MS, update all call sites inWidgetContainer.tsxaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react/lib/util/constants.ts` around lines 42 - 44, Add a doc-style grouping comment above the polling constants, rename POLL_REQUEST_DELAY to POLL_REQUEST_DELAY_MS (and update all call sites, e.g., in WidgetContainer.tsx) so units are explicit, and add a short inline comment to POLL_MAX_RETRY (e.g., // 5 attempts) to match the other constants; update references to the renamed constant across the codebase to avoid breakage and keep naming/comments consistent with AUTO_CLOSE_DEFAULT_MS and surrounding constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react/lib/util/constants.ts`:
- Around line 42-44: Add a doc-style grouping comment above the polling
constants, rename POLL_REQUEST_DELAY to POLL_REQUEST_DELAY_MS (and update all
call sites, e.g., in WidgetContainer.tsx) so units are explicit, and add a short
inline comment to POLL_MAX_RETRY (e.g., // 5 attempts) to match the other
constants; update references to the renamed constant across the codebase to
avoid breakage and keep naming/comments consistent with AUTO_CLOSE_DEFAULT_MS
and surrounding constants.
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
Never mind, server was not properly set up.
Works well, just created some constants for hardcoded values
3 participants
It is common on mobile to move the widget to the background, which causes the DNS to fail. Other issues might also happen, including the even already being fired when the widget is back to foreground.
To cover all these use cases this commit adds a polling phase when the widget become visible (event triggered when the browser page goes to foreground on mobile). It will poll for the last 5 transactions from the history once and check if there is a matching payment tx, otherwise it retries every second for 5s which should be plenty for the tx to be discovered.
The retry stops as soon as the payment is marked successful. In practice the mobile apps (either Marlin or Cashtab) add a modal window and a small delay before returning to the browser so the retry is not needed most of the time.
This has been tested with a remote console, logs and tweaks to make sure all the scenarios are covered properly.
Note that this commit does NOT fix the existing bugs. This should be done in other commits/PRs and reference the appropriated issue. More specifically the following bugs are worked around but not fixed:
The returned object forNow fixed by Return the expected type from the address/transactions/<address> endp… paybutton-server#1108 and commit 7de14cagetAddressDetailsdoes not match its interface. This needs to be dealed with on server side.shouldTriggerOnSuccessfunction doesn't filter out the payment ID when it's undefined, meaning it will match any tx that looks like the payment one but has no payment ID associated with it. This commit avoids this by exiting early and avoiding the call in this case.Related to #616
Summary by CodeRabbit
Bug Fixes
Chores