Conversation
Deploying wallet-bitcoin with
|
| Latest commit: |
dbabd54
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://90dd4792.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://prevent-lockup-state.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
dbabd54
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bcff9677.arkade-wallet.pages.dev |
| Branch Preview URL: | https://prevent-lockup-state.arkade-wallet.pages.dev |
|
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:
WalkthroughAdds WalletAuthState and unlockWallet to the Wallet provider, implements a passwordless auto-init/unlock flow and one-time reload in App, refactors the Unlock screen to call unlockWallet, and extends tests/mocks and test matchMedia stubbing. Changes
Sequence DiagramsequenceDiagram
participant App
participant WalletProvider
participant WalletService
participant AppReloader
App->>WalletProvider: read authState, wallet.pubkey, initialized, dataReady
WalletProvider->>WalletService: check private key / noUserDefinedPassword
WalletService-->>WalletProvider: return 'passwordless' | 'locked' | 'authenticated'
alt authState == "passwordless" and not attempted
App->>WalletProvider: unlockWallet(defaultPassword)
WalletProvider->>WalletService: getPrivateKey(defaultPassword)
alt success
WalletProvider-->>App: set authState 'authenticated'
App->>App: proceed to authenticated route
else failure
WalletProvider-->>App: set authState 'locked'
App->>App: set session flag, schedule one-time reload
App->>AppReloader: reload()
end
else authState == "locked"
App->>App: navigate to Unlock screen
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/test/App.test.tsx (2)
22-22: Duplicated constant creates fragile coupling.
PASSWORDLESS_AUTO_RELOAD_KEYis duplicated fromApp.tsx. If the value changes inApp.tsx, these tests will silently fail. Consider exporting the constant fromApp.tsxand importing it here.♻️ Proposed fix
In
src/App.tsx:-const PASSWORDLESS_AUTO_RELOAD_KEY = 'passwordless-auto-reload-attempted' +export const PASSWORDLESS_AUTO_RELOAD_KEY = 'passwordless-auto-reload-attempted'In this test file:
-const PASSWORDLESS_AUTO_RELOAD_KEY = 'passwordless-auto-reload-attempted' +import App, { appReloader, PASSWORDLESS_AUTO_RELOAD_KEY } from '../App'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/App.test.tsx` at line 22, The test duplicates the PASSWORDLESS_AUTO_RELOAD_KEY constant from App.tsx causing fragile coupling; instead export PASSWORDLESS_AUTO_RELOAD_KEY from the App.tsx module (add an export for the constant where it's defined) and update this test file to import PASSWORDLESS_AUTO_RELOAD_KEY from App.tsx (replace the local const with the imported symbol) so the test always uses the canonical value.
150-151: Microtask flushing pattern may be fragile.The sequential
await act(async () => {})followed byawait Promise.resolve()to flush microtasks can be flaky depending on the async chain depth. Consider usingawait waitFor(() => expect(unlockWallet).toHaveBeenCalled())instead, which is more robust for async operations.♻️ Proposed fix
- await act(async () => {}) - await Promise.resolve() - expect(unlockWallet).toHaveBeenCalledWith(defaultPassword) + await waitFor(() => expect(unlockWallet).toHaveBeenCalledWith(defaultPassword))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/App.test.tsx` around lines 150 - 151, Replace the fragile microtask-flushing pattern (await act(async () => {}) followed by await Promise.resolve()) with a robust wait-for assertion: import and use waitFor from `@testing-library/react` and replace those two lines with await waitFor(() => expect(unlockWallet).toHaveBeenCalled()), or similar waitFor-based assertion targeting the mock/function under test (unlockWallet) to reliably wait for async effects.src/test/setup.ts (1)
46-58: Consider consolidating duplicatematchMediamock.The
matchMediamock is defined identically at lines 30-42 (module level) and again inbeforeEach. If test isolation is the goal, the module-level definition at lines 30-42 could be removed, keeping only thebeforeEachversion. Alternatively, if both are intentional, a brief comment explaining why would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/setup.ts` around lines 46 - 58, The file defines identical window.matchMedia mocks twice (module-level and inside beforeEach), causing duplication; remove the redundant module-level Object.defineProperty for matchMedia (or vice versa) and keep a single authoritative mock in beforeEach called by the existing beforeEach block so tests remain isolated, or if you intend both, add a short comment above the module-level mock explaining why both are required; locate the duplicated definitions by searching for Object.defineProperty(window, 'matchMedia') and the beforeEach block to consolidate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/screens/Wallet/Unlock.tsx`:
- Around line 17-31: The handleUnlock function currently sets
setUnlocking(false) only for the 'Invalid password' error and never for other
exceptions, leaving the UI stuck; modify handleUnlock (the try/catch around
await unlockWallet) to ensure setUnlocking(false) is always called for any
failure—either by adding setUnlocking(false) after consoleError in the catch
block or by moving unlocking state cleanup into a finally block that runs after
the try/catch; keep existing behavior for setError('Invalid password') and
setUnlocked(true) intact.
- Line 42: The NeedsPassword component currently types
NeedsPasswordProps.onPassword as (password: string) => void while handleUnlock
is async; update the prop type to (password: string) => void | Promise<void> so
callers can return a Promise and TypeScript will reflect awaiting
semantics—locate the NeedsPasswordProps/interface declaration (and any PropTypes
or FC generic usage) and change the onPassword signature accordingly, then
ensure the NeedsPassword.handleClick caller properly handles the returned
Promise (no behavioral change required if it already calls
onPassword(password)).
---
Nitpick comments:
In `@src/test/App.test.tsx`:
- Line 22: The test duplicates the PASSWORDLESS_AUTO_RELOAD_KEY constant from
App.tsx causing fragile coupling; instead export PASSWORDLESS_AUTO_RELOAD_KEY
from the App.tsx module (add an export for the constant where it's defined) and
update this test file to import PASSWORDLESS_AUTO_RELOAD_KEY from App.tsx
(replace the local const with the imported symbol) so the test always uses the
canonical value.
- Around line 150-151: Replace the fragile microtask-flushing pattern (await
act(async () => {}) followed by await Promise.resolve()) with a robust wait-for
assertion: import and use waitFor from `@testing-library/react` and replace those
two lines with await waitFor(() => expect(unlockWallet).toHaveBeenCalled()), or
similar waitFor-based assertion targeting the mock/function under test
(unlockWallet) to reliably wait for async effects.
In `@src/test/setup.ts`:
- Around line 46-58: The file defines identical window.matchMedia mocks twice
(module-level and inside beforeEach), causing duplication; remove the redundant
module-level Object.defineProperty for matchMedia (or vice versa) and keep a
single authoritative mock in beforeEach called by the existing beforeEach block
so tests remain isolated, or if you intend both, add a short comment above the
module-level mock explaining why both are required; locate the duplicated
definitions by searching for Object.defineProperty(window, 'matchMedia') and the
beforeEach block to consolidate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18a54434-c7c9-433d-af5f-b320172a7019
📒 Files selected for processing (7)
public/wallet-service-worker.mjssrc/App.tsxsrc/providers/wallet.tsxsrc/screens/Wallet/Unlock.tsxsrc/test/App.test.tsxsrc/test/screens/mocks.tssrc/test/setup.ts
|
It doesn't restore swaps in test |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/App.tsx (1)
221-223: Keep the passwordless boot guard stable for the whole attempt.
src/providers/wallet.tsx:429-440flipsauthStateto'authenticated'beforeinitWallet()settles, so Lines 221-223 clearpasswordlessBootAttemptedwhile the first auto-init is still in flight. If the same wallet is reclassified aspasswordlessagain, this effect can start a secondunlockWallet()call.♻️ Make the guard wallet-scoped instead of auth-state-scoped
- useEffect(() => { - passwordlessBootAttempted.current = false - }, [wallet.pubkey, authState]) + useEffect(() => { + passwordlessBootAttempted.current = false + }, [wallet.pubkey])Also applies to: 231-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 221 - 223, The passwordless boot guard passwordlessBootAttempted should be tied to the wallet identity, not cleared when authState flips: stop resetting passwordlessBootAttempted.current in the useEffect that depends on [wallet.pubkey, authState]; instead scope the guard to wallet.pubkey only (or key-derived token) so it persists for the duration of the initWallet()/unlockWallet() attempt; update both places where you reset the ref (the effect with passwordlessBootAttempted and the similar block at lines ~231-248) to only reset when wallet.pubkey actually changes (or on unmount), and ensure unlockWallet() checks and sets passwordlessBootAttempted.current before starting to prevent duplicate in-flight calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Line 219: The top-level routing gate variable shouldHoldOnLoading currently
includes dataReady which prevents mounting wallet-driven restore flows; remove
dataReady from this global gate so shouldHoldOnLoading is computed as
hasStoredWallet && authState !== 'locked' && !initialized (and mirror this
change for the similar logic around the block mentioned at 250-256). Instead,
keep any dataReady-based spinner local to the wallet screen or provider that
triggers hydration/restore (move the dataReady checks into that
component/provider and render a local loading state there rather than blocking
Pages.Loading globally).
---
Nitpick comments:
In `@src/App.tsx`:
- Around line 221-223: The passwordless boot guard passwordlessBootAttempted
should be tied to the wallet identity, not cleared when authState flips: stop
resetting passwordlessBootAttempted.current in the useEffect that depends on
[wallet.pubkey, authState]; instead scope the guard to wallet.pubkey only (or
key-derived token) so it persists for the duration of the
initWallet()/unlockWallet() attempt; update both places where you reset the ref
(the effect with passwordlessBootAttempted and the similar block at lines
~231-248) to only reset when wallet.pubkey actually changes (or on unmount), and
ensure unlockWallet() checks and sets passwordlessBootAttempted.current before
starting to prevent duplicate in-flight calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/App.tsx (1)
192-196: Clear pending auto-reload when recovery happens.The timeout is only cleared on unmount (Line 194). If recovery completes before the 1s timer fires, the app can still reload unexpectedly.
💡 Suggested fix
useEffect(() => { return () => { if (passwordlessReloadTimer.current) clearTimeout(passwordlessReloadTimer.current) } }, []) + useEffect(() => { + if (!initialized || !passwordlessReloadTimer.current) return + clearTimeout(passwordlessReloadTimer.current) + passwordlessReloadTimer.current = undefined + }, [initialized])Also applies to: 210-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 192 - 196, The current cleanup only clears passwordlessReloadTimer.current on unmount; also clear and nullify that timeout immediately when recovery completes to prevent an unexpected reload—add clearTimeout(passwordlessReloadTimer.current) and set passwordlessReloadTimer.current = null in the recovery completion path (e.g., inside the function that marks recovery success or the handler that sets isRecoveryComplete), and apply the same change to the other recovery-related code block referenced around the second location so both places cancel the pending auto-reload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Line 115: The current conditional `if (!initialized && authState === 'locked')
return navigate(Pages.Unlock)` is fragile; change the routing check to rely only
on `authState === 'locked'` so the Unlock screen is always shown when the app is
locked. Update the two occurrences (the one at line with `if (!initialized &&
authState === 'locked') return navigate(Pages.Unlock'` and the similar check
near line 185) to remove the `!initialized` clause, and ensure no other logic
(e.g., `unlockWallet` which sets `authState('locked')`) is forced to reset
`initialized`; use `authState` alone to determine navigation to `Pages.Unlock`.
---
Nitpick comments:
In `@src/App.tsx`:
- Around line 192-196: The current cleanup only clears
passwordlessReloadTimer.current on unmount; also clear and nullify that timeout
immediately when recovery completes to prevent an unexpected reload—add
clearTimeout(passwordlessReloadTimer.current) and set
passwordlessReloadTimer.current = null in the recovery completion path (e.g.,
inside the function that marks recovery success or the handler that sets
isRecoveryComplete), and apply the same change to the other recovery-related
code block referenced around the second location so both places cancel the
pending auto-reload.
|
Hey @pietro909 — this PR was approved by @Kukks 6 days ago. What's blocking the merge? If it's waiting on something specific, let the team know. |
pietro909
force-pushed
the
prevent-lockup-state
branch
from
83267a8 to
90164a5
Compare
Followup Review — New commits since last reviewKey new commits:
Changes assessed: ✅ ✅ Unlock screen simplification: The Unlock component now uses ✅ ✅ Tests: Comprehensive coverage of the state machine — passwordless boot, locked routing, reload guard with sessionStorage. The Security note: The No blocking issues. |
pietro909
force-pushed
the
prevent-lockup-state
branch
from
90164a5 to
f960ab2
Compare
🔍 Arkana PR ReviewOverall: ✅ Solid fix for a real UX problem — passwordless wallets no longer get stuck on the Unlock screen. ArchitectureThe core change introduces a
This is a meaningful improvement to the startup flow. Security Review
Potential Concerns
TestsComprehensive test coverage for the new startup routing logic:
Good coverage of the critical paths. SummaryThe auth state machine is well-designed and the defensive fallbacks are sound. Main concern is the bundled service worker file in VCS. No protocol-level security issues. |
Review — Prevent lockup state (iterative)New commits since last review: removed compiled service worker + unused import. Core auth state refactor unchanged. What changed since last review:
Architecture recap (for reviewers):
Security review:
Observations:
Cross-repo: No external impact — wallet-internal state management only. LGTM. |
pietro909
force-pushed
the
prevent-lockup-state
branch
from
d6b035b to
5b3e764
Compare
🔍 Arkana PR Review —
|
The original condition (!initialized || !dataReady) could flip the page back to Loading after the wallet was already initialized via restore, breaking navigation to other pages. Only hold on Loading when the wallet is not yet initialized and authState is not locked.
pietro909
force-pushed
the
prevent-lockup-state
branch
from
5b3e764 to
f998952
Compare
🔍 Review — Prevent lockup state (iterative, new commits)What changed since last review: Core architectural shift — wallet auth state is now separated from backend initialization. Passwordless wallets auto-boot in the background instead of routing through Unlock. Key Changes
Positives
Observations
Race between
Missing error feedback for non-password errors in Unlock: Cross-repo impactNone — self-contained wallet app change. Overall: Strong improvement to startup reliability. The auth state machine is well-designed and the one-shot reload is a practical recovery mechanism. |
🔍 Arkana PR Review — wallet#421 (Prevent lockup state)Scope: Wallet auth state machine refactor to prevent passwordless wallets from hitting the Unlock screen on startup failure. Architecture Assessment ✅The introduction of Key improvements:
One-Shot Reload RecoveryThe
Potential Issues
Test Coverage ✅Comprehensive tests covering:
Good use of VerdictSolid refactor that correctly solves the lockup state issue. The auth state machine is clean and testable. The three potential issues above are minor but worth reviewing, especially #3 (error path in |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests