Fix showing password when server is unreachable#234
Conversation
WalkthroughAdd server-unreachable detection and a ServerDown page; navigation now routes to that page when aspInfo.unreachable is true. Also update init success to use Changes
Sequence DiagramsequenceDiagram
participant App as App (root)
participant NavEffect as Navigation Effect
participant ASP as aspInfo
participant Router as Router/Page resolver
App->>NavEffect: walletLoaded / initialized / initInfo / aspInfo.unreachable change
NavEffect->>ASP: read aspInfo.unreachable
alt aspInfo.unreachable == true
NavEffect->>Router: navigate(Pages.ServerDown)
Router->>App: render ServerDown component
else
NavEffect->>Router: follow existing navigation flow (wallet/setup/etc.)
Router->>App: render target page
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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-signet with
|
| Latest commit: |
eefe61b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://657cb0af.wallet-23u.pages.dev |
| Branch Preview URL: | https://password-when-server-unavail.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
eefe61b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2622d518.arkade-wallet.pages.dev |
| Branch Preview URL: | https://password-when-server-unavail.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
eefe61b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dd44ae30.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://password-when-server-unavail.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: 0
🧹 Nitpick comments (2)
src/screens/Wallet/ServerDown.tsx (1)
6-14: Consider adding a retry mechanism.The ServerDown screen displays a static error message without providing users a way to retry the connection or take any action. If the network issue is temporary, users might be stuck on this screen without recourse.
Consider adding a retry button that attempts to reconnect or refresh the app state:
import Button from '../../components/Button' import ButtonsOnBottom from '../../components/ButtonsOnBottom' export default function ServerDown() { const handleRetry = () => { window.location.reload() } return ( <> <CenterScreen> <WalletNewIcon /> <Text bigger>Arkade Wallet</Text> <ErrorMessage error text='Ark server unreachable' /> </CenterScreen> <ButtonsOnBottom> <Button onClick={handleRetry} label='Retry' /> </ButtonsOnBottom> </> ) }src/App.tsx (1)
58-62: Consider removing redundant loadingError state update.Since line 65 immediately navigates to
Pages.ServerDownwhenaspInfo.unreachableis true, theloadingErrorstate set in this effect is unlikely to be displayed. TheloadingErroris only used by theLoadingcomponent (line 115), but the navigation effect prevents users from seeing the Loading page when the server is unreachable.If the early navigation is reliable, this effect can be removed:
- useEffect(() => { - if (aspInfo.unreachable) { - setLoadingError('Unable to connect to the server. Please check your internet connection and try again.') - } - }, [aspInfo.unreachable]) -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/App.tsx(1 hunks)src/providers/navigation.tsx(4 hunks)src/screens/Init/Success.tsx(1 hunks)src/screens/Wallet/ServerDown.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 142
File: src/screens/Wallet/Unlock.tsx:18-27
Timestamp: 2025-08-17T11:03:40.893Z
Learning: In the Arkade wallet, the default password ("noah") is intentionally used as part of a progressive security model. New users can onboard without password friction using the default password, and once they accumulate 10,000+ sats, the app nudges them to set a custom password. This is a deliberate UX design decision to reduce onboarding barriers while encouraging security practices when users have meaningful value at stake.
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Success.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 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/App.tsx
🧬 Code graph analysis (3)
src/screens/Wallet/ServerDown.tsx (4)
src/components/CenterScreen.tsx (1)
CenterScreen(9-28)src/icons/WalletNew.tsx (1)
WalletNewIcon(1-3)src/components/Text.tsx (1)
Text(25-79)src/components/Error.tsx (1)
ErrorMessage(10-21)
src/providers/navigation.tsx (1)
src/screens/Wallet/ServerDown.tsx (1)
ServerDown(6-14)
src/App.tsx (1)
src/lib/pwa.ts (1)
pwaIsInstalled(7-7)
⏰ 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 (6)
src/App.tsx (1)
64-71: LGTM! Server unreachability check prevents password screen display.The early return when
aspInfo.unreachableis true correctly prevents navigation to theUnlockpage (line 70), which addresses the PR objective of not showing the password when the server is unreachable.src/providers/navigation.tsx (4)
29-29: LGTM! ServerDown component imported correctly.
54-54: LGTM! ServerDown page added to enum.
92-92: LGTM! Appropriate tab mapping for error screen.Mapping
ServerDowntoTabs.Noneis correct since this is a system error state that shouldn't be associated with any specific tab.
146-147: LGTM! ServerDown component rendering integrated.src/screens/Init/Success.tsx (1)
19-19: Verify whether this backup configuration change was intentional or should be separated from the main server reachability fix.The change from
config.lastBackuptoconfig.nostrBackupon line 19 is indeed unrelated to the PR's stated objective of fixing password display when the server is unreachable. The main fixes in this PR occur insrc/App.tsx(server state navigation check),src/providers/navigation.tsx(new ServerDown page routing), and the newsrc/screens/Wallet/ServerDown.tsxscreen.However, the Success.tsx change appears to be a bug fix:
config.lastBackupdoes not exist anywhere in the codebase, whileconfig.nostrBackupis the correct config field for backup settings. This suggests the original code was using a non-existent field.Please confirm: Was this backup field refactoring intentional as part of this PR, or should it be separated into a distinct fix?
louisinger
left a comment
louisinger
left a comment
There was a problem hiding this comment.
LGTM but can we add a playwright test for this ?
with setOffline it should be easy to simulate unreachable server
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/e2e/serverdown.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 142
File: src/screens/Wallet/Unlock.tsx:18-27
Timestamp: 2025-08-17T11:03:40.893Z
Learning: In the Arkade wallet, the default password ("noah") is intentionally used as part of a progressive security model. New users can onboard without password friction using the default password, and once they accumulate 10,000+ sats, the app nudges them to set a custom password. This is a deliberate UX design decision to reduce onboarding barriers while encouraging security practices when users have meaningful value at stake.
⏰ 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
| test('should show server unreachable', async ({ page }) => { | ||
| await page.goto('/') | ||
| await page.context().setOffline(true) | ||
| await expect(page.getByText('Ark server unreachable')).toBeVisible() | ||
| }) |
There was a problem hiding this comment.
Fix the race condition in test sequence.
The test sets offline mode after navigating to the page, which means the app has already loaded and likely fetched server data successfully. Setting offline mode afterward won't retroactively trigger the server unreachable state.
Apply this diff to set offline mode before navigation:
test('should show server unreachable', async ({ page }) => {
- await page.goto('/')
await page.context().setOffline(true)
+ await page.goto('/')
await expect(page.getByText('Ark server unreachable')).toBeVisible()
})Alternatively, if the app needs to detect the server becoming unreachable after load, add a reload or trigger a re-fetch after setting offline mode.
📝 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.
| test('should show server unreachable', async ({ page }) => { | |
| await page.goto('/') | |
| await page.context().setOffline(true) | |
| await expect(page.getByText('Ark server unreachable')).toBeVisible() | |
| }) | |
| test('should show server unreachable', async ({ page }) => { | |
| await page.context().setOffline(true) | |
| await page.goto('/') | |
| await expect(page.getByText('Ark server unreachable')).toBeVisible() | |
| }) |
🤖 Prompt for AI Agents
In src/test/e2e/serverdown.test.ts around lines 3 to 7 the test sets the browser
offline after navigation which causes a race where the app already fetched
server data; either set page.context().setOffline(true) before page.goto('/') so
the app loads in offline mode, or keep the offline switch after navigation but
add a page.reload() or trigger the app's re-fetch mechanism so the unreachable
state is detected; update the test to perform one of these sequences
accordingly.
* Fix showing password when server is unreachable * new server down e2e test
* add swap e2e tests * fix lint * CI: use_ln: true * Fix showing password when server is unreachable (#234) * Fix showing password when server is unreachable * new server down e2e test * use docker images --------- Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
* Fix showing password when server is unreachable * new server down e2e test
* add swap e2e tests * fix lint * CI: use_ln: true * Fix showing password when server is unreachable (arkade-os#234) * Fix showing password when server is unreachable * new server down e2e test * use docker images --------- Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
* Fix showing password when server is unreachable * new server down e2e test
* Fix showing password when server is unreachable * new server down e2e test
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests