fix: improve timeout error messages and increase service worker timeout#566
fix: improve timeout error messages and increase service worker timeout#566Kukks wants to merge 1 commit into
Conversation
The MessageBus timeout error shows "Message handler timed out after 60000ms (WALLET_UPDATER)" which is cryptic and unhelpful. The handler tag (WALLET_UPDATER) is shown instead of the operation type (SETTLE, SEND, etc.), and the error implies failure when the operation may still complete in the background. Changes: - Increase global MessageBus timeout from 60s to 120s — settlement and send operations can legitimately take over a minute when waiting for an ASP round - Add friendlyError() that detects timeout patterns and translates them to actionable messages: "Settlement is taking longer than expected. It may still complete in the background" - Apply friendlyError in Transaction (settle), Send/Details, and Settings/Vtxos screens - Fix Windows lint script
WalkthroughThis PR adds timeout-aware error handling utilities and updates error handling across multiple UI screens to use friendlier error messages. The service worker message bus timeout is extended from 60s to 120s, and an npm script quote style is normalized. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
e6f9e61
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6fbbaea1.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-timeout-errors.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
e6f9e61
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ae87aafd.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-timeout-errors.arkade-wallet.pages.dev |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approved — clean UX improvement, no protocol-critical changes
Reviewed full diff + all changed files in context + git history (#522 set 60s just 10 days ago) + cross-repo timeout patterns.
Assessment
Not protocol-critical. This PR only changes error display strings and a timeout config. The settlement/send/rollover operations themselves (settleVtxos, settlePreconfirmed, sendOffChain) are called identically — only the catch handler changes from extractError(err) → friendlyError(err, 'Settlement'|'Payment'). No transaction signing, VTXO handling, forfeit paths, or round lifecycle code is touched.
Timeout bump 60s → 120s (wallet-service-worker.ts:33): Justified. Settlement requires ASP round participation and signing, which legitimately exceeds 60s under load. The SDK's withTimeout() doesn't cancel the underlying work, so this just prevents premature error display.
friendlyError() / isTimeoutError() (src/lib/error.ts): Clean. Regex /timed out after \d+ms/i correctly matches the SDK's withTimeout() error format. Falls through to raw extractError() on non-match — safe degradation.
Minor findings (non-blocking)
-
package.json:45— inconsistent quoting:lintchanged to double quotes butlint:fix(line 46) still uses single quotes. Both work on Unix but if the intent is Windows compat,lint:fixshould match. Cosmetic. -
src/lib/error.ts— no unit tests:friendlyError()andisTimeoutError()are pure functions, trivially testable. No test file exists forerror.tsat all (pre-existing gap). Would be good to add coverage for the regex matching, especially edge cases like partial matches or nested error messages. Non-blocking for this PR. -
src/providers/wallet.tsx:479-481— parallel timeout detection: The wallet provider has its own inline timeout check (err.message.includes('Service worker activation timed out') || err.message.includes('MessageBus timed out')) that uses different patterns than the newTIMEOUT_PATTERNregex. These are different error sources (worker activation vs. message handler), so no bug — but worth considering consolidating intoisTimeoutError()in a follow-up to avoid pattern drift. -
isTimeoutErrorexported but unused in this PR: The PR description explains it's for callers that need to branch on timeout vs other failures. Fine as forward-looking API, but consider adding a@seeJSDoc pointing to the intended use case so it doesn't get tree-shaken or removed by a future cleanup. -
No auto-refresh after background completion: The friendly message tells users "check back in a moment" but there's no mechanism to detect when the background operation actually completes and refresh the UI. This is an SDK limitation (acknowledged in the PR body), not a bug here — but worth tracking as a follow-up.
LGTM. Ship it.
There was a problem hiding this comment.
✅ Approved — clean UX improvement, no protocol-critical changes
Reviewed full diff + all changed files in context + git history (#522 set 60s just 10 days ago) + cross-repo timeout patterns.
Assessment
Not protocol-critical. This PR only changes error display strings and a timeout config. The settlement/send/rollover operations themselves (settleVtxos, settlePreconfirmed, sendOffChain) are called identically — only the catch handler changes from extractError(err) → friendlyError(err, 'Settlement'|'Payment'). No transaction signing, VTXO handling, forfeit paths, or round lifecycle code is touched.
Timeout bump 60s → 120s (wallet-service-worker.ts:33): Justified. Settlement requires ASP round participation and signing, which legitimately exceeds 60s under load. The SDK's withTimeout() doesn't cancel the underlying work, so this just prevents premature error display.
friendlyError() / isTimeoutError() (src/lib/error.ts): Clean. Regex /timed out after \d+ms/i correctly matches the SDK's withTimeout() error format. Falls through to raw extractError() on non-match — safe degradation.
Minor findings (non-blocking)
-
package.json:45— inconsistent quoting:lintchanged to double quotes butlint:fix(line 46) still uses single quotes. Both work on Unix but if the intent is Windows compat,lint:fixshould match. Cosmetic. -
src/lib/error.ts— no unit tests:friendlyError()andisTimeoutError()are pure functions, trivially testable. No test file exists forerror.tsat all (pre-existing gap). Would be good to add coverage for the regex matching, especially edge cases like partial matches or nested error messages. Non-blocking for this PR. -
src/providers/wallet.tsx:479-481— parallel timeout detection: The wallet provider has its own inline timeout check (err.message.includes('Service worker activation timed out') || err.message.includes('MessageBus timed out')) that uses different patterns than the newTIMEOUT_PATTERNregex. These are different error sources (worker activation vs. message handler), so no bug — but worth considering consolidating intoisTimeoutError()in a follow-up to avoid pattern drift. -
isTimeoutErrorexported but unused in this PR: The PR description explains it's for callers that need to branch on timeout vs other failures. Fine as forward-looking API, but consider adding a@seeJSDoc pointing to the intended use case so it doesn't get tree-shaken or removed by a future cleanup. -
No auto-refresh after background completion: The friendly message tells users "check back in a moment" but there's no mechanism to detect when the background operation actually completes and refresh the UI. This is an SDK limitation (acknowledged in the PR body), not a bug here — but worth tracking as a follow-up.
LGTM. Ship it.
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
e6f9e61
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d170145d.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-timeout-errors.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
![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 (3)
src/screens/Wallet/Send/Details.tsx (1)
16-16: Send flow error handling is now inconsistent withForm.tsx.
Details.tsxnow usesfriendlyError(err, 'Payment'), butsrc/screens/Wallet/Send/Form.tsx(line 41 import, lines 499-503handleError, plus multiple call sites at 456/459/462/571/578/587) still usesextractError. If the Send flow times out inForm.tsx's paths, users will still see the cryptic raw message — defeating the intent of this PR for part of the Send surface.Consider applying the same
friendlyError(err, 'Payment')swap inForm.tsxfor consistency.Also applies to: 124-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Wallet/Send/Details.tsx` at line 16, Form.tsx still uses extractError in its handleError logic and call sites; switch those to use friendlyError(err, 'Payment') to match Details.tsx: update the handleError function (and any places calling extractError inside Form.tsx, e.g., submit/timeout/error handlers) to call friendlyError(error, 'Payment') instead of extractError(error) and remove or deprecate extractError usage in this component so all Send flow errors are presented consistently with Details.tsx.src/screens/Settings/Vtxos.tsx (1)
148-162: Nit: operation label — "Renewal" vs "Settlement".This path is
handleRolloverand the button reads "Renew Virtual Coins" / "Renew", but the friendly message will render as "Settlement is taking longer than expected...". While rollover is technically a settle operation under the hood, end users in this screen see "Renew", so a label like'Renewal'(or'Coin renewal') would read more naturally and match the surrounding UI copy.- setError(friendlyError(err, 'Settlement')) + setError(friendlyError(err, 'Renewal'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Settings/Vtxos.tsx` around lines 148 - 162, The friendly error label currently uses 'Settlement' in handleRollover which will show "Settlement is taking longer..." but the UI and button use "Renew"/"Renew Virtual Coins"; update the call to friendlyError in handleRollover to use a user-facing label like 'Renewal' (or 'Coin renewal') instead of 'Settlement' so setError displays text consistent with the Renew UI; locate this in the handleRollover function where friendlyError(err, 'Settlement') is called and replace the second argument accordingly.src/lib/error.ts (1)
1-26:TIMEOUT_PATTERNis narrower than the codebase's actual timeout messages.The regex
/timed out after \d+ms/imatches the MessageBus format ("Message handler timed out after 60000ms (TAG)") but misses other timeout strings already in use:
src/providers/wallet.tsx:478-492—"Service worker activation timed out"and"MessageBus timed out"src/lib/nostr.ts—"Publish timeout","Load timeout"src/test/e2e/utils.ts—"Clipboard read timeout"None of the current
friendlyErrorcall sites (Transaction settle, Send/Details, Settings/Vtxos) appear to surface those other variants today, so this isn't a functional bug for this PR. But ifisTimeoutError/friendlyErrorare intended as a general-purpose helper (as the name and export suggest), consider broadening the detection, e.g.:♻️ Suggested broadening
-const TIMEOUT_PATTERN = /timed out after \d+ms/i +const TIMEOUT_PATTERN = /(timed out after \d+ms|timed out\b|\btimeout\b)/iAlternatively, accept an explicit list of known timeout phrases to avoid over-matching (e.g., an error message that legitimately contains "timeout" in a non-timeout context).
Also minor:
extractErroris typed(error: any)and dereferenceserror.messagewithout a null guard, so passingnull/undefinedintofriendlyErrorwould throw insideextractError. Not introduced here, just worth noting for the newunknown-typed callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/error.ts` around lines 1 - 26, TIMEOUT_PATTERN is too narrow and extractError can throw on null/undefined; update TIMEOUT_PATTERN to a broader, word-boundary regex that matches common variants like "timed out", "timeout", "timed out after Xms" (e.g. /\b(timeout|timed out|timed out after \d+ms)\b/i) and use that in isTimeoutError/friendlyError, and change extractError to accept unknown (or guard against null/undefined) and only dereference error.message after confirming error is non-null and typeof error === 'object' (update references in extractError, isTimeoutError, friendlyError accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 45-46: The lint:fix npm script uses single quotes which break on
Windows shells; update the "lint:fix" script to use the same double-quote
pattern as "lint" (change the command for the npm script named lint:fix to use
double quotes around the glob "src/**/*.{ts,tsx,js,jsx}" so it matches the lint
script and works cross-platform).
---
Nitpick comments:
In `@src/lib/error.ts`:
- Around line 1-26: TIMEOUT_PATTERN is too narrow and extractError can throw on
null/undefined; update TIMEOUT_PATTERN to a broader, word-boundary regex that
matches common variants like "timed out", "timeout", "timed out after Xms" (e.g.
/\b(timeout|timed out|timed out after \d+ms)\b/i) and use that in
isTimeoutError/friendlyError, and change extractError to accept unknown (or
guard against null/undefined) and only dereference error.message after
confirming error is non-null and typeof error === 'object' (update references in
extractError, isTimeoutError, friendlyError accordingly).
In `@src/screens/Settings/Vtxos.tsx`:
- Around line 148-162: The friendly error label currently uses 'Settlement' in
handleRollover which will show "Settlement is taking longer..." but the UI and
button use "Renew"/"Renew Virtual Coins"; update the call to friendlyError in
handleRollover to use a user-facing label like 'Renewal' (or 'Coin renewal')
instead of 'Settlement' so setError displays text consistent with the Renew UI;
locate this in the handleRollover function where friendlyError(err,
'Settlement') is called and replace the second argument accordingly.
In `@src/screens/Wallet/Send/Details.tsx`:
- Line 16: Form.tsx still uses extractError in its handleError logic and call
sites; switch those to use friendlyError(err, 'Payment') to match Details.tsx:
update the handleError function (and any places calling extractError inside
Form.tsx, e.g., submit/timeout/error handlers) to call friendlyError(error,
'Payment') instead of extractError(error) and remove or deprecate extractError
usage in this component so all Send flow errors are presented consistently with
Details.tsx.
🪄 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: 1815052b-0687-4e35-b80b-23c57bb50215
📒 Files selected for processing (6)
package.jsonsrc/lib/error.tssrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Send/Details.tsxsrc/screens/Wallet/Transaction.tsxsrc/wallet-service-worker.ts
| "lint": "eslint \"src/**/*.{ts,tsx,js,jsx}\"", | ||
| "lint:fix": "eslint 'src/**/*.{ts,tsx,js,jsx}' --fix", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify quote consistency for lint scripts in package.json
rg -n '"lint"|\"lint:fix\"|eslint .*src/\*\*/\*\.\{ts,tsx,js,jsx\}' package.jsonRepository: arkade-os/wallet
Length of output: 192
🏁 Script executed:
cat -n package.json | sed -n '40,50p'Repository: arkade-os/wallet
Length of output: 798
🌐 Web query:
npm scripts quoting single vs double quotes Windows cmd powershell compatibility
💡 Result:
In npm scripts defined in package.json, use double quotes (escaped as ") for arguments containing spaces or special characters to ensure compatibility with Windows cmd.exe (npm's default shell on Windows) and PowerShell. Single quotes do not work in cmd.exe, as it treats them as literal characters rather than delimiters, leading to arguments being passed with quotes included or commands failing. For cross-platform compatibility (Unix sh/bash and Windows cmd/PowerShell): - Always use double quotes around arguments that need protection from shell interpretation (e.g., paths with spaces, globs like .js). - In package.json, escape double quotes as ": "script": "command "arg with space"". - Example: "scripts": { "lint": "eslint "src//.js"" } This works because: - Unix sh: " becomes ", which quotes the arg. - Windows cmd: " becomes ", which cmd recognizes as quoting. Single quotes ('arg') work in Unix shells but fail in Windows cmd (quotes passed literally) and can cause issues even in Git Bash when invoked via npm (due to cmd.exe default). npm uses cmd.exe by default on Windows (docs confirm), but PowerShell has similar quoting rules (prefers double quotes). Complex cases (nested quotes, variables) may need \" escaping or Node.js helpers (e.g., node -e "require('child_process').spawn(...)"). To change shell (npm 5.1+): npm config set script-shell pwsh (PowerShell Core, cross-platform) or path/to/bash.exe, but stick to double quotes for broad compatibility without config changes. Avoid single quotes for cross-platform scripts; prefer double quotes or cross-platform tools like rimraf, shx.
Citations:
- 1: CLI - npm script failing due to single quote prettier/prettier#9987
- 2: [Readme] add note about windows only supporting double quotes open-cli-tools/concurrently#577
- 3: https://docs.npmjs.com/cli/v9/commands/npm-run-script/?v=true
- 4: https://stackoverflow.com/questions/78820932/windows-node-js-package-json-run-command-with-various-quotes
- 5: https://stackoverflow.com/questions/71595877/package-json-scripts-on-windows-literal-character
- 6: https://stackoverflow.com/questions/23243353/how-to-set-shell-for-npm-run-scripts-in-windows
- 7: https://stackoverflow.com/questions/55300421/how-can-i-make-npm-projects-with-bash-shell-commands-work-on-windows
- 8: https://code-think.eu/blog/npm-shell/
- 9: https://2ality.com/2022/08/npm-package-scripts.html
Make lint:fix quoting consistent for Windows compatibility.
Line 45 is fixed with double quotes, but Line 46 still uses single quotes which will fail on Windows cmd.exe/PowerShell where they are treated as literal characters rather than quote delimiters. This leaves the cross-platform issue partially unresolved.
Proposed fix
- "lint:fix": "eslint 'src/**/*.{ts,tsx,js,jsx}' --fix",
+ "lint:fix": "eslint \"src/**/*.{ts,tsx,js,jsx}\" --fix",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 45 - 46, The lint:fix npm script uses single
quotes which break on Windows shells; update the "lint:fix" script to use the
same double-quote pattern as "lint" (change the command for the npm script named
lint:fix to use double quotes around the glob "src/**/*.{ts,tsx,js,jsx}" so it
matches the lint script and works cross-platform).
|
Better UX although shouldn't be needed since arkade-os/ts-sdk#446 exempted the long-running operations. Good to go from my side @Kukks |
2 participants
Summary
friendlyError()utility that detects timeout patterns and translates them to actionable, user-facing messagesisTimeoutError()for callers that need to branch on timeout vs other failuresProblem
Users see
"Settlement failed: Error: Message handler timed out after 60000ms (WALLET_UPDATER)"when clicking "Complete boarding". This is:WALLET_UPDATERis the handler tag, not the operation (SETTLE). The SDK passes the tag towithTimeout()instead of the message type.withTimeout()does NOT cancel the underlying work. The settlement may actually succeed in the background, but the user sees "failed".Before / After
Before:
Settlement failed: Error: Message handler timed out after 60000ms (WALLET_UPDATER)After:
Settlement is taking longer than expected. It may still complete in the background — check back in a moment.SDK root cause
Filed arkade-os/ts-sdk#448 for the underlying issues:
WALLET_UPDATER) instead of message type (SETTLE)withTimeoutdoesn't cancel underlying work but implies failureTest plan