End-to-End tests #202
End-to-End tests #202
Conversation
WalkthroughAdds Playwright end-to-end tests and config, a GitHub Actions workflow to run them, regtest/Nigiri orchestration and setup scripts, Dockerfile updates, package scripts/devDependency changes, small UI test hooks, .gitignore entries, README e2e/regtest docs, and wallet provider init/reload resilience changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as GitHub Actions
participant Repo as Repository
participant Node as Node / pnpm
participant Nigiri as Nigiri / Docker
participant Setup as setup-arkd.js
participant App as Web App (pnpm start)
participant Tests as Playwright Tests
CI->>Repo: checkout
CI->>Node: setup Node (lts), pnpm install
CI->>Node: playwright install-browsers
CI->>Nigiri: run nigiri/regtest action (bring up regtest)
CI->>Node: pnpm run regtest:setup
Node->>Setup: execute src/test/setup-arkd.js
Setup->>Nigiri: docker exec / faucets / init arkd wallet
Setup-->>Node: setup complete
Node->>App: pnpm start (webServer on port)
Tests->>App: run e2e flows (init/pwa/receive/send)
Tests->>Nigiri: invoke faucets / docker exec funding commands
Tests-->>CI: upload reports & artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 markdownlint-cli2 (0.18.1)README.md76-76: Blank line inside blockquote (MD028, no-blanks-blockquote) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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-signet with
|
| Latest commit: |
82acd3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84e50d57.wallet-23u.pages.dev |
| Branch Preview URL: | https://playwright.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
82acd3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://478c8ebe.arkade-wallet.pages.dev |
| Branch Preview URL: | https://playwright.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
82acd3e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0a2de75.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://playwright.wallet-bitcoin.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: 10
🧹 Nitpick comments (9)
.gitignore (1)
38-45: Consider removing duplicate node_modules/ entry.Line 39 adds
node_modules/which is already ignored on line 6. The duplicate entry is harmless but unnecessary.Apply this diff to remove the duplicate:
# Playwright -node_modules/ /test-results/ /playwright-report/ /blob-report/ /playwright/.cache/ /playwright/.auth/ /dev-dist/src/test/e2e/pwa.test.ts (1)
3-13: LGTM with optional enhancement suggestion.The test correctly verifies the manifest link and accessibility. Consider validating the manifest content as well to ensure it contains required PWA fields (name, short_name, icons, etc.).
Optional enhancement to validate manifest content:
const manifest = await page.evaluate(async () => { const res = await fetch('/manifest.json') - return res.ok + if (!res.ok) return { ok: false } + const json = await res.json() + return { ok: true, data: json } }) - expect(manifest).toBe(true) + expect(manifest.ok).toBe(true) + expect(manifest.data).toHaveProperty('name') + expect(manifest.data).toHaveProperty('short_name') + expect(manifest.data).toHaveProperty('icons')src/test/e2e/init.test.ts (1)
23-23: Reminder: Restore wallet test is pending.The TODO comment indicates that a restore wallet test needs to be implemented to complete test coverage.
Would you like me to generate a test implementation for the wallet restore flow, or should I open an issue to track this task?
.github/workflows/playwright.yml (1)
1-29: LGTM! Consider adding a cleanup step for the regtest environment.The workflow is well-structured and properly sets up the test environment. While GitHub Actions runners are ephemeral, explicitly cleaning up the regtest environment (especially Docker resources) is good practice.
Consider adding a cleanup step at the end:
- name: Cleanup regtest environment if: always() run: pnpm regtest:downsrc/test/e2e/send.test.ts (1)
41-43: Consider extracting the hardcoded recipient address.The hardcoded ARK address could be extracted to a constant at the top of the file for better maintainability, especially if this address needs to be reused or updated.
Apply this diff:
+const TEST_ARK_ADDRESS = 'tark1qr340xg400jtxat9hdd0ungyu6s05zjtdf85uj9smyzxshf98ndah6u2nredqtn0cr4p4zqz53gsmhju4l9t7x47kzleesa9dprx7e56xhzlen' + test('should send offchain funds', async ({ page, isMobile }) => { // ... setup code ... // send page await page .getByLabel('', { exact: true }) - .fill( - 'tark1qr340xg400jtxat9hdd0ungyu6s05zjtdf85uj9smyzxshf98ndah6u2nredqtn0cr4p4zqz53gsmhju4l9t7x47kzleesa9dprx7e56xhzlen', - ) + .fill(TEST_ARK_ADDRESS)src/test/setup-arkd.js (4)
26-39: Consider logging curl errors for better debugging.The empty catch block on line 33 silently suppresses errors. While this is appropriate for retry logic, logging the error type could help debug issues like missing curl or network problems.
Apply this diff to improve observability:
} catch { - console.log(`Waiting for ARK server to be ready (${i + 1}/${maxRetries})...`) + } catch (error) { + console.log(`Waiting for ARK server (${i + 1}/${maxRetries}): ${error.message || 'not ready yet'}`) await sleep(retryDelay) }
92-94: Add delays between faucet calls to avoid race conditions.Calling the faucet 10 times in rapid succession without any delay may cause issues if the faucet service rate-limits requests or if transactions need time to propagate.
Apply this diff to add a small delay:
for (let i = 0; i < 10; i++) { await execCommand(`nigiri faucet ${arkdAddress}`) + await sleep(500) // Small delay to avoid overwhelming the faucet }
96-98: Replace hard-coded sleep with proper confirmation polling.A fixed 5-second sleep may not be reliable across different environments. Consider polling for transaction confirmations instead.
Consider implementing a polling mechanism that checks for confirmed transactions:
// Wait for transactions to be confirmed let confirmed = false for (let i = 0; i < 30 && !confirmed; i++) { const balance = await execCommand(`${arkdExec} arkd wallet balance`) if (parseFloat(balance) > 0) { confirmed = true } else { await sleep(2000) } } if (!confirmed) { throw new Error('Transactions not confirmed after maximum wait time') }
71-118: Consider making configuration values more flexible.Hard-coded values like URLs (
http://localhost:7070), ports, amounts (2000000), and retry counts are scattered throughout. While acceptable for test setup, extracting these to constants at the top of the file would improve maintainability.Example:
const CONFIG = { ARK_SERVER_URL: 'http://localhost:7070', EXPLORER_URL: 'http://chopsticks:3000', FAUCET_COUNT: 10, NOTE_AMOUNT: 2000000, CONFIRMATION_WAIT: 5000, MAX_RETRIES: 30, RETRY_DELAY: 2000 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.github/workflows/playwright.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)arkd.Dockerfile(2 hunks)arkdwallet.Dockerfile(2 hunks)package.json(2 hunks)playwright.config.ts(1 hunks)src/components/InputAmount.tsx(3 hunks)src/components/Keyboard.tsx(1 hunks)src/screens/Wallet/Send/Form.tsx(1 hunks)src/test/e2e/init.test.ts(1 hunks)src/test/e2e/pwa.test.ts(1 hunks)src/test/e2e/receive.test.ts(1 hunks)src/test/e2e/send.test.ts(1 hunks)src/test/e2e/utils.ts(1 hunks)src/test/setup-arkd.js(1 hunks)test.docker-compose.yml(0 hunks)vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test.docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/e2e/send.test.ts (1)
src/test/e2e/utils.ts (1)
readClipboard(3-16)
src/test/e2e/receive.test.ts (1)
src/test/e2e/utils.ts (1)
readClipboard(3-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (20)
vite.config.ts (1)
11-11: LGTM!Excluding test files from ESLint plugin is appropriate for the new Playwright test infrastructure.
src/components/Keyboard.tsx (1)
98-98: LGTM!The
data-testidattribute provides stable selectors for e2e tests without affecting component behavior.arkdwallet.Dockerfile (3)
8-16: LGTM!The consolidated git operations and build steps are cleaner and more maintainable.
19-32: LGTM!The Alpine upgrade and addition of environment variables improve the runtime configuration.
2-2: golang:1.25.3 is a valid and current Go version.Go 1.25.3 was released on 2025-10-13 and the Docker image tag exists on Docker Hub. The original concern appears to be based on outdated information—the latest stable Go release series is not 1.23.x. As of now, Go 1.25.x is the latest series with versions 1.25.1, 1.25.2, and 1.25.3 available, alongside maintained releases in the 1.24 and 1.23 series.
arkd.Dockerfile (3)
8-20: LGTM!The consolidated git operations and improved build organization enhance maintainability.
23-39: LGTM!The Alpine upgrade, environment variable configuration, and volume definitions are well structured.
2-2: No issues found. The golang:1.25.3 version is valid.The latest Go 1.25 point release is go1.25.3 (released October 13, 2025), and the Docker image tag exists on Docker Hub. The Dockerfile is using a current, stable Go version.
src/components/InputAmount.tsx (2)
15-15: LGTM!Adding the optional
nameprop enables proper form field identification for testing and accessibility.
82-82: LGTM!The
nameattribute is correctly passed to the underlyingIonInputcomponent.src/screens/Wallet/Send/Form.tsx (1)
361-361: LGTM!The
nameattribute provides a stable identifier for the amount input field, supporting form handling and e2e testing.README.md (1)
67-93: LGTM! Clear documentation for e2e testing.The documentation clearly explains the regtest setup requirements and provides all necessary commands for running e2e tests in different modes.
src/test/e2e/utils.ts (1)
3-16: LGTM! Well-implemented clipboard utility.The function properly validates Clipboard API availability and uses
Promise.raceto implement a timeout, ensuring tests don't hang indefinitely if clipboard access fails.src/test/e2e/init.test.ts (1)
3-21: LGTM! Comprehensive wallet creation test.The test properly validates the complete wallet creation flow, including onboarding steps and verification of all key UI elements in the wallet view.
package.json (2)
36-47: LGTM! Well-organized test and regtest scripts.The new scripts properly configure Playwright testing and regtest environment management. The script composition (regtest → down → build → up → setup) ensures a clean environment for each run.
68-68: LGTM! Playwright dependency added.The
@playwright/testdependency is correctly added to support the new e2e test infrastructure.playwright.config.ts (1)
3-34: LGTM! Well-configured Playwright setup.The configuration is thoughtfully designed:
- Serial execution (
workers: 1,fullyParallel: false) prevents race conditions with the shared regtest environment- Clipboard permissions enable the
readClipboardutilityreuseExistingServer: !process.env.CIensures clean server startup in CI while allowing local development efficiency- Mobile and desktop projects provide good coverage across form factors
src/test/e2e/send.test.ts (1)
46-54: LGTM! Well-handled mobile keyboard interaction.The conditional mobile keyboard handling properly accounts for different test environments and uses appropriate
data-testidattributes for reliable element selection.src/test/setup-arkd.js (2)
58-69: LGTM!The retry logic and status checking are correctly implemented. Once the issue in
checkWalletStatusis fixed, this function will work as expected.
120-124: LGTM!The error handling and process exit code are appropriate for a setup script.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/e2e/receive.test.ts (2)
28-28: Add error handling for the faucet command.The
execcall lacks error handling and could cause the test to hang if the faucet command fails.This issue was previously flagged in past review comments.
58-58: Add error handling for the faucet command.The Docker exec command lacks error handling and could cause the test to hang if the command fails.
This issue was previously flagged in past review comments.
🧹 Nitpick comments (5)
src/test/e2e/receive.test.ts (5)
15-20: Replace fragile position-based selectors with stable attributes.The selectors rely on
.nth(5)and.nth(4)which are brittle and will break if the UI structure or order changes. Consider addingdata-testidattributes to the relevant UI elements and usingpage.getByTestId()instead.For example, update the UI components to include test IDs:
// In the component <div data-testid="copy-address-button">Copy address</div> <img data-testid="close-receive-modal" />Then refactor the test:
- await page - .locator('div') - .filter({ hasText: /^Copy address$/ }) - .nth(5) - .click() - await page.getByRole('img').nth(4).click() + await page.getByTestId('copy-address-button').click() + await page.getByTestId('close-receive-modal').click()
21-21: ReplacewaitForTimeoutwith deterministic wait.
waitForTimeoutis an anti-pattern that makes tests slower and less reliable. Replace it with a wait for a specific state or condition.If you're waiting for clipboard operations to complete, consider waiting for the clipboard content:
- await page.waitForTimeout(500) - const boardingAddress = await readClipboard(page) + // readClipboard already handles the wait internallyIf there's a specific UI state change, wait for that instead.
45-50: Replace fragile position-based selectors with stable attributes.Same issue as the first test: these selectors are brittle and depend on element order. Consider using
data-testidattributes for stable selection.Apply the same refactoring approach as suggested for the first test (lines 15-20).
51-51: ReplacewaitForTimeoutwith deterministic wait.Same issue as the first test: avoid
waitForTimeoutin favor of waiting for specific conditions or state changes.Apply the same refactoring approach as suggested for line 21 in the first test.
5-63: Consider extracting shared test flow into a helper function.Both tests follow an identical navigation and assertion pattern, differing only in the faucet command and one selector. Extracting the common flow into a helper function would improve maintainability.
Example refactor:
async function testReceiveFlow( page: Page, addressSelectorNth: number, faucetCommand: string ) { await page.goto('/') await page.getByText('Continue').click() await page.getByText('Continue').click() await page.getByText('Continue').click() await page.getByText('Skip for now').click() await page.getByText('+ Create wallet').click() await page.getByText('Go to wallet').click() await page.getByText('Receive').click() await page.getByText('Skip').click() // Copy address (use data-testid in production) await page.locator('div').filter({ hasText: /^Copy address$/ }).nth(5).click() await page.getByRole('img').nth(addressSelectorNth).click() await page.waitForTimeout(500) const address = await readClipboard(page) expect(address).toBeDefined() expect(address).toBeTruthy() // Execute faucet await execPromise(faucetCommand) await page.waitForSelector('text=Payment received!') await expect(page.getByText('Payment received!')).toBeVisible() await expect(page.getByText('SATS received successfully')).toBeVisible() } test('should receive onchain funds', async ({ page }) => { await testReceiveFlow(page, 4, `nigiri faucet \${address} 0.0001`) }) test('should receive offchain funds', async ({ page }) => { await testReceiveFlow(page, 5, `docker exec -t arkd ark send --to \${address} --amount 5000 --password secret`) })Note: This suggestion assumes the
execerror handling is addressed separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/playwright.yml(1 hunks)playwright.config.ts(1 hunks)src/test/e2e/receive.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/playwright.yml
- playwright.config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/e2e/receive.test.ts (1)
src/test/e2e/utils.ts (1)
readClipboard(3-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/playwright.yml (1)
16-19: Consider adding pnpm dependency caching.The workflow could benefit from caching pnpm dependencies to speed up CI runs.
Add this step after the setup-node step:
- uses: pnpm/action-setup@v4 with: version: latest - uses: actions/setup-node@v4 with: node-version: lts/* cache: 'pnpm' - name: Install dependencies run: pnpm installThis leverages the built-in caching mechanism and removes the need to globally install pnpm.
playwright.config.ts (2)
7-7: Consider reducing the retry count in CI.Setting 5 retries in CI is very high and may mask flaky tests rather than addressing their root causes. Consider reducing to 2-3 retries and investing in making tests more reliable.
Apply this diff:
- retries: process.env.CI ? 5 : 0, + retries: process.env.CI ? 2 : 0,
16-17: Timeout values are quite generous.30-second timeouts for actions and navigation may hide performance issues. While this might be reasonable for regtest environments, consider if 15-20 seconds would suffice and better catch performance regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/playwright.yml(1 hunks)playwright.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/App.tsx (1)
70-76: Effect deps omit wallet.pubkey → possible stale navigation.This effect branches on wallet.pubkey but doesn’t depend on it. Pubkey changes (e.g., after import/reset) may not retrigger navigation. Include wallet.pubkey (or a derived boolean) in deps.
Apply:
- }, [walletLoaded, initialized, initInfo]) + }, [walletLoaded, initialized, initInfo, wallet.pubkey])src/providers/wallet.tsx (3)
147-164: Event listener leak: removeEventListener uses a different function reference.handleServiceWorkerMessages is recreated on each init, so removeEventListener won’t detach the old handler. Re-inits accumulate listeners, causing duplicate reloads and memory leaks.
Use a stable handler stored in a ref:
@@ - const handleServiceWorkerMessages = (event: MessageEvent) => { + // keep a stable reference across inits + if (!('swMsgHandlerRef' in window)) { + (window as any).swMsgHandlerRef = { fn: null as ((e: MessageEvent) => void) | null } + } + const swMsgHandlerRef = (window as any).swMsgHandlerRef as { fn: ((e: MessageEvent) => void) | null } + const handleServiceWorkerMessages = (event: MessageEvent) => { if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { reloadWallet(svcWallet) setTimeout(() => reloadWallet(svcWallet), 5000) } } @@ - if (listeningForServiceWorker.current) { - navigator.serviceWorker.removeEventListener('message', handleServiceWorkerMessages) - navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) - } else { - navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) - listeningForServiceWorker.current = true - } + if (listeningForServiceWorker.current && swMsgHandlerRef.fn) { + navigator.serviceWorker.removeEventListener('message', swMsgHandlerRef.fn) + } + navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) + swMsgHandlerRef.fn = handleServiceWorkerMessages + listeningForServiceWorker.current = trueAlternatively, attach once outside init and post messages via a stable dispatcher.
170-179: Status polling interval isn’t cleared → leak and noisy logs after lock/reset.Every successful init sets a new setInterval; none are cleared. After lock/reset, polling continues and may error.
Track and clear the interval:
@@ - // ping the service worker wallet status every 1 second - setInterval(async () => { + // ping the service worker wallet status every 1 second + if (!('statusIntervalRef' in window)) { + (window as any).statusIntervalRef = { id: undefined as number | undefined } + } + const statusIntervalRef = (window as any).statusIntervalRef as { id: number | undefined } + if (statusIntervalRef.id) clearInterval(statusIntervalRef.id) + statusIntervalRef.id = window.setInterval(async () => { try { const { walletInitialized } = await svcWallet.getStatus() setInitialized(walletInitialized) } catch (err) { consoleError(err, 'Error pinging wallet status') } - }, 1_000) + }, 1_000)Also clear on lock/reset:
@@ const lockWallet = async () => { if (!svcWallet) throw new Error('Service worker not initialized') await svcWallet.clear() setInitialized(false) + const ref = (window as any).statusIntervalRef as { id?: number } | undefined + if (ref?.id) clearInterval(ref.id) } @@ const resetWallet = async () => { if (!svcWallet) throw new Error('Service worker not initialized') await clearStorage() await svcWallet.clear() await svcWallet.contractRepository.clearContractData() + const ref = (window as any).statusIntervalRef as { id?: number } | undefined + if (ref?.id) clearInterval(ref.id) }Optionally add a useEffect cleanup on unmount.
84-88: Stale state write: effect closes over an old wallet and can clobber fields.updateWallet({ ...wallet, nextRollover }) uses wallet from the effect’s closure, but wallet isn’t in deps. When wallet changes (e.g., network/pubkey set), this can overwrite those fields.
Use a functional update to merge safely and avoid stale reads:
- const nextRollover = calcNextRollover(vtxos.spendable) - updateWallet({ ...wallet, nextRollover }) + const nextRollover = calcNextRollover(vtxos.spendable) + updateWallet((prev) => (prev.nextRollover === nextRollover ? prev : { ...prev, nextRollover }))And update updateWallet to accept a function:
- const updateWallet = (data: Wallet) => { - setWallet({ ...data }) - saveWalletToStorage(data) - } + const updateWallet = (data: Wallet | ((prev: Wallet) => Wallet)) => { + setWallet((prev) => { + const next = typeof data === 'function' ? (data as (p: Wallet) => Wallet)(prev) : data + if (next === prev) return prev + saveWalletToStorage(next) + return { ...next } + }) + }This prevents accidental overwrites and redundant writes.
🧹 Nitpick comments (2)
src/providers/wallet.tsx (2)
109-122: Parallelize wallet reload for latency.getVtxos, getTxHistory, and getBalance can run in parallel.
Apply:
- const vtxos = await getVtxos(swWallet) - const txs = await getTxHistory(swWallet) - const balance = await getBalance(swWallet) + const [vtxos, txs, balance] = await Promise.all([ + getVtxos(swWallet), + getTxHistory(swWallet), + getBalance(swWallet), + ])
183-211: Backoff logic is solid; consider message brittleness and configurability.Matching by err.message is fragile across browsers. Make the predicate broader or configurable; also allow maxRetries via env/test config for CI flakiness.
Please confirm the actual error text in CI/playwright runs matches 'Service worker activation timed out' so retries trigger reliably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsx(2 hunks)src/providers/wallet.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/App.tsx (1)
src/providers/wallet.tsx (1)
WalletContext(39-53)
src/providers/wallet.tsx (1)
src/lib/logs.ts (1)
consoleError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
src/App.tsx (1)
41-41: Decoupling from svcWallet looks good.App no longer relies on svcWallet; reduces coupling to provider internals.
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 (2)
src/providers/wallet.tsx (2)
186-195: Interval leak and duplicate polling on re‑init.
setIntervalis never cleared; repeated inits will create multiple 1s polls and keep polling after lock/reset.Fix by storing the id in a ref, clearing on re‑init and on lock/reset.
+// near Line 69 +const statusIntervalRef = useRef<number | null>(null) // Lines 186-195 - setInterval(async () => { + if (statusIntervalRef.current) clearInterval(statusIntervalRef.current) + statusIntervalRef.current = window.setInterval(async () => { try { const { walletInitialized } = await svcWallet.getStatus() setInitialized(walletInitialized) } catch (err) { consoleError(err, 'Error pinging wallet status') } - }, 1_000) + }, 1_000)Also clear on lock/reset:
// Lines 244-248 const lockWallet = async () => { if (!svcWallet) throw new Error('Service worker not initialized') await svcWallet.clear() + if (statusIntervalRef.current) { clearInterval(statusIntervalRef.current); statusIntervalRef.current = null } setInitialized(false) } // Lines 250-255 const resetWallet = async () => { if (!svcWallet) throw new Error('Service worker not initialized') await clearStorage() await svcWallet.clear() await svcWallet.contractRepository.clearContractData() + if (statusIntervalRef.current) { clearInterval(statusIntervalRef.current); statusIntervalRef.current = null } }
230-242: Only mark initialized on successful svc wallet init.Guard
setInitialized(true)andupdateWalletwith try/catch aroundinitSvcWorkerWallet. With current flow, a swallowed/returned error (fixed above) would have setinitialized=true.Apply:
const initWallet = async (privateKey: Uint8Array) => { const arkServerUrl = aspInfo.url const network = aspInfo.network as NetworkName const esploraUrl = getRestApiExplorerURL(network) ?? '' const pubkey = hex.encode(secp.getPublicKey(privateKey)) - await initSvcWorkerWallet({ - privateKey: hex.encode(privateKey), - arkServerUrl, - esploraUrl, - }) - updateWallet({ ...wallet, network, pubkey }) - setInitialized(true) + try { + await initSvcWorkerWallet({ + privateKey: hex.encode(privateKey), + arkServerUrl, + esploraUrl, + }) + updateWallet({ ...wallet, network, pubkey }) + setInitialized(true) + } catch (e) { + consoleError(e, 'Wallet init failed') + throw e + } }
🧹 Nitpick comments (3)
src/providers/wallet.tsx (3)
112-134: Parallel loads: good. Prevent stale overwrites and avoid shadowing.
- Multiple reloadWallet invocations (initial, SW events, delayed reload) can race; older responses may clobber newer state. Add a simple last-write-wins guard with a ref-based request id.
- The local names vtxos/txs/balance shadow state vars and increase confusion; rename to explicit result vars.
Apply this diff within this block:
- const [vtxos, txs, balance] = await Promise.allSettled([ + const [vtxosRes, txsRes, balanceRes] = await Promise.allSettled([ getVtxos(swWallet), getTxHistory(swWallet), getBalance(swWallet), ]) - if (vtxos.status === 'rejected') { - consoleError(vtxos.reason, 'Error getting vtxos') + if (vtxosRes.status === 'rejected') { + consoleError(vtxosRes.reason, 'Error getting vtxos') } else { - setVtxos(vtxos.value) + setVtxos(vtxosRes.value) } - if (txs.status === 'rejected') { - consoleError(txs.reason, 'Error getting txs') + if (txsRes.status === 'rejected') { + consoleError(txsRes.reason, 'Error getting txs') } else { - setTxs(txs.value) + setTxs(txsRes.value) } - if (balance.status === 'rejected') { - consoleError(balance.reason, 'Error getting balance') + if (balanceRes.status === 'rejected') { + consoleError(balanceRes.reason, 'Error getting balance') } else { - setBalance(balance.value) + setBalance(balanceRes.value) }Additions outside this range to implement last‑write‑wins:
// near Line 69 const reloadReqId = useRef(0) // at the top of reloadWallet (Line 110-112 area) const reqId = ++reloadReqId.current // right before setting state (once is enough, or before each set) if (reqId !== reloadReqId.current) returnEnsure no callers depend on intermediate state updates ordering.
173-180: Service worker message listener may accumulate; use a stable handler ref.
removeEventListenerreceives a new function identity each init, so the previous listener is not removed, leading to duplicate handlers on re‑init. Keep the handler in a ref and remove the old one before adding a new one.Example:
// near Line 69 const swMsgHandlerRef = useRef<(e: MessageEvent) => void>() // replace Lines 173-180 if (swMsgHandlerRef.current) { navigator.serviceWorker.removeEventListener('message', swMsgHandlerRef.current) } swMsgHandlerRef.current = handleServiceWorkerMessages navigator.serviceWorker.addEventListener('message', swMsgHandlerRef.current)
84-88: Avoid stale merge when updating nextRollover.Use a functional update to prevent losing concurrent wallet updates.
Example:
const nextRollover = calcNextRollover(vtxos.spendable) setWallet((prev) => { const next = { ...prev, nextRollover } saveWalletToStorage(next) return next })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/wallet.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (1)
src/providers/wallet.tsx (2)
src/lib/asp.ts (3)
getVtxos(124-135)getTxHistory(88-122)getBalance(82-86)src/lib/logs.ts (1)
consoleError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
src/providers/wallet.tsx (2)
144-146: Retry params defaults: OK. Consider exposing a constant and validating.Defaults look sane. Minor: export a MAX_RETRIES constant and clamp negatives; reject non-finite values for robustness.
Do any callers pass non-sensical values (e.g., maxRetries=0 or negative)?
Also applies to: 150-152
113-116: Type consistency with asp helpers.You pass
swWallettogetTxHistory/getBalance(typed asIWalletin asp.ts) and togetVtxos(typed asServiceWorkerWallet). This is fine ifServiceWorkerWalletconforms structurally toIWallet, but it diverges from the prior guidance to keep IWallet-only signatures exceptcollaborativeExit.Please confirm asp.ts signatures align with the intended policy. If not, we can adjust overloads or wrappers to keep a consistent public surface. Based on learnings.
louisinger
force-pushed
the
playwright
branch
from
b35cce4 to
5ef4123
Compare
Co-authored-by: João Bordalo <bordalix@users.noreply.github.com> Signed-off-by: louisinger <41042567+louisinger@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
76-76: Blank line inside blockquote
(MD028, no-blanks-blockquote)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
README.md (1)
68-96: Documentation structure and e2e test setup instructions look good.The added documentation clearly explains the regtest environment setup, e2e test prerequisites, and provides concise commands for running tests in different modes (headless, UI, codegen). The progression from basic setup to advanced tooling is well-organized and user-friendly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
This PR configures Playwright and add an extra GH action running e2e tests
To run the tests locally, we need to start and set up the regtest env.
run in headless mode:
run with UI:
The PR includes a set of tests :
init.test.ts: create wallet flowreceive.test.ts: receive boarding utxo or ark vtxosend.test.ts: send offchain vtxo + history checkspwa.test.ts: check service worker registration and PWA manifest@bordalix @altafan @Kukks please review
Summary by CodeRabbit
New Features
Tests
Documentation
Chores