[pietro909]

Summary

• When the service worker is permanently unresponsive (browser stopped it, MessageBus.start() fails silently on wake), all 5 retry attempts in initSvcWorkerWallet time out
• The PR Prevent lockup state #421 one-time reload then reuses the same broken SW registration, so the app stays stuck on Loading forever
• After retries are exhausted, unregister the dead SW so the reload triggers a fresh registration with a clean MessageBus

Test plan

• Reproduce the stuck loading state (stop the service worker via DevTools → Application → Service Workers → Stop)
• Verify the app recovers after the automatic reload instead of staying on the loading screen
• Verify normal startup flow is unaffected (no regressions when SW is healthy)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Adds an early health-check (quick ping/pong) to detect unresponsive background workers before full startup.
  • Attempts an early, best-effort worker registration to improve startup reliability.
  • Separates activation and message-bus timeouts to reduce startup hangs.

• Bug Fixes

  • Lowers default retry attempts and best-effort unregisters unresponsive workers after final timeout to prevent persistent failures.

[coderabbitai]

Warning

Rate limit exceeded

@pietro909 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 27 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 27 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c7075080-82b1-4db9-9435-f20f6703ac01

📥 Commits

Reviewing files that changed from the base of the PR and between 8ab1708 and a6e3701.

📒 Files selected for processing (1)

• src/providers/wallet.tsx

Walkthrough

Adds a fast PING/PONG health check and zombie-worker cleanup before wallet initialization; splits activation vs. message-bus timeouts (activation shorter, message-bus 30s); reduces init retry count; and ensures best-effort service-worker unregister on final timeout failure.

Changes

Cohort / File(s)	Summary
Provider: service-worker lifecycle & timeouts src/providers/wallet.tsx	Split timeouts: renamed activation constant, introduced MESSAGE_BUS_INIT_TIMEOUT_MS (30s); reduced maxRetries default (5 → 2); added concurrent "zombie" check that PINGs active worker (500ms wait) and unregisters unresponsive registration; best-effort unregister on final timeout before rethrow.
Service worker: health-check handler src/wallet-service-worker.ts	Added an early message event listener that responds to PING by replying { type: 'PONG' } on event.ports[0], enabling quick health checks independent of full worker startup.
App entry: eager registration src/index.tsx	Added best-effort early registration of /wallet-service-worker.mjs when navigator.serviceWorker is available; failures are silently ignored.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant App as Client
participant Reg as Navigator.getRegistration()
participant SW as ServiceWorker
participant Repo as WalletRepository
App->>Reg: getRegistration()
alt registration exists
App->>SW: postMessage(PING) via MessageChannel.port1
SW-->>App: port.postMessage({type: 'PONG'}) (within 500ms)
alt PONG received
App->>Repo: getWalletState() (concurrent)
App->>SW: proceed with activation/setup (activation timeout ~5s)
else no PONG
App->>Reg: unregister()
App->>Reg: register(/wallet-service-worker.mjs)
App->>Repo: getWalletState()
end
else no registration
App->>Reg: register(/wallet-service-worker.mjs)
App->>Repo: getWalletState()
end
Note right of App: message-bus init timeout = 30s; reduced retries = 2

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Handle ServiceWorker lifecycle #357 — Modifies Service Worker lifecycle and wallet ping/lock logic; overlaps with health-check and lifecycle changes.
• Upgrade SDK, boltz, use 30s ServiceWorker timeout #371 — Adjusts service worker timeout configuration; directly related to the timeout split and values here.
• Use new Service Worker #316 — Changes worker startup and messaging wiring; overlaps with PING/PONG and initialization flow.

Suggested reviewers

• louisinger
• bordalix

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: handling stuck/unresponsive service workers through unregistration to recover from loading deadlock, which is the core problem addressed across all three modified files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/unregister-stuck-service-worker

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cloudflare-workers-and-pages]

Deploying wallet-bitcoin with    Cloudflare Pages

Latest commit:	a6e3701
Status:	✅  Deploy successful!
Preview URL:	https://cb761a52.wallet-bitcoin.pages.dev
Branch Preview URL:	https://fix-unregister-stuck-service.wallet-bitcoin.pages.dev

View logs

[ghost]

Review: Service Worker Unregister Recovery

Status: ✅ Approved

Summary: Unregister the dead service worker registration after retries exhaust, allowing the reload to trigger a fresh registration with clean .

Security & Logic:

• No protocol impact. Pure app recovery mechanism.
• Service worker lifecycle is correctly handled: after 5 retry timeouts, unregister is called before reload.
• Recovery flow is clean and won't create orphaned registrations.

Test coverage: Test plan items all checked ✅. Good user-facing behavior validation.

Merge ready: Approved pending CI pass. ✅

[cloudflare-workers-and-pages]

Deploying wallet-mutinynet with    Cloudflare Pages

Latest commit:	a6e3701
Status:	✅  Deploy successful!
Preview URL:	https://b7c54263.arkade-wallet.pages.dev
Branch Preview URL:	https://fix-unregister-stuck-service.arkade-wallet.pages.dev

View logs

[ghost]

Code Review: Service Worker Cleanup on Timeout

Summary

✅ Looks good — Solid fix for the stuck loading state when the service worker becomes unresponsive.

Key Points

What it does:

• Reduces max retries from 5 → 2 (faster failure detection)
• Separates timeouts: SW activation (5s) vs MessageBus init (30s)
• On exhausted retries with timeout, unregisters the broken SW so page reload gets a fresh registration

Why this matters:
The PR#421 reload was reusing the same broken SW registration, keeping the app stuck. Unregistering forces a clean slate on reload.

Code Quality ✅

• Clean error handling with try/catch for unregister
• Best-effort cleanup (silent fallback) is appropriate here
• Comments explain the recovery flow clearly

Minor Observations

1. Timeout values: 30s for MessageBus seems generous given 5s for SW activation. Is this intentional? (Probably for complex wallet state initialization, which makes sense.)
2. Test coverage: Manual test plan looks solid. Consider automating SW failure injection in e2e suite for regression prevention.

No Blockers

This is a straightforward timeout+cleanup improvement. Ship it. ✅

[ghost]

Review: Service Worker Zombie Detection & Recovery

✅ Approach is solid. The PR addresses a real failure mode: browsers that keep a SW registered but don't actually respond to messages (observed in Vivaldi). The fix uses MessageChannel ping as a reliable liveness check.

Strengths

1. Correct diagnosis: The PING/PONG via MessageChannel bypasses event.source issues and gives a clean alive/dead signal. Good.
2. Right scope: Detecting zombies at init time (before full wallet setup) prevents cascading failures.
3. Clean recovery: Unregistering only when truly stuck (after retries exhausted) means we don't race the browser's SW lifecycle.
4. Defensive code: The ping listener is registered early in the SW before any other logic could fail.

Questions / Observations

1. Timeout choice (2s for ping): Is 2 seconds the right threshold for a PING response? A slow browser might legitimately take longer to wake the worker. Have you tested this on low-end devices? Consider if this could cause false-positive unregisters.

2. maxRetries reduction (5 → 2): Why drop from 5 to 2 retries? That's a significant change. Is this tied to the zombie detection, or a separate improvement? If the zombie detection works, we'd expect retries to still help with transient failures. The summary doesn't mention this decision.

3. Timeout constant names: Good rename from SERVICE_WORKER_SETUP_TIMEOUT_MS to SERVICE_WORKER_ACTIVATION_TIMEOUT_MS + MESSAGE_BUS_INIT_TIMEOUT_MS (now 30s). Why is messageBus init much longer (30s vs 5s)? This deserves a comment.

4. Unregister timing: If existingReg.active is null (SW exists but not activated yet), we skip the ping. Is that the right call? Can a SW be registered but not activated and still cause Loading deadlock? Worth documenting the assumption.

5. Multiple unregisters: We unregister in two places now — once after zombie detection, once after retries exhausted. Could race with each other or with browser cleanup. Consider:

   Then we don't need the second unregister at the bottom (retries exhausted). One unregister path is safer.

Code Quality

• No syntax issues, types look correct.
• SW message event handler is minimal and safe.
• Error handling on the unregister is appropriate (best-effort).

Recommendation

Post with clarifications on:

• The 2s ping timeout — is this validated on real devices?
• The maxRetries reduction — why?
• Why two unregister sites — can this be consolidated?

Once clarified, this is good to merge. The core fix is right.

[coderabbitai]

Actionable comments posted: 1

🤖 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/providers/wallet.tsx`:
- Around line 324-347: Capture the ServiceWorkerRegistration.active value into a
local variable (e.g., const active = existingReg?.active) before using it so it
can't change between the check and postMessage, and update the MessageChannel
onmessage handler to only resolve true when the received message is the expected
pong (e.g., event.data?.type === 'PONG'); if not PONG, treat as not alive.
Ensure you still close the port and clear the timeout when resolving or timing
out, and call active.postMessage({ type: 'PING' }, [channel.port2]) using the
captured active reference.

🪄 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: da0d35f7-e84d-4a7b-84f7-1fc4245fb01c

📥 Commits

Reviewing files that changed from the base of the PR and between 0d8c3bb and 2745fa1.

📒 Files selected for processing (2)

• src/providers/wallet.tsx
• src/wallet-service-worker.ts

[ghost]

🔍 Code Review

Summary

This PR addresses a critical deadlock in wallet initialization when the service worker becomes unresponsive. The fix is methodical and well-structured across 5 commits:

1. Fix retry exhaustion + unregistration — Clears broken SW on timeout
2. Increase MessageBus timeout — 5s → 30s for cold-start network calls (ARK server)
3. Detect zombie SWs — Pre-flight PING/PONG before setup to catch unresponsive workers
4. Untrack built files — Cleanup after Remove artifact from repo #437
5. Parallelize init — Pre-register SW and run zombie check concurrently with IndexedDB warmup

Protocol/Security Assessment

✅ No protocol impact — Changes are client-side wallet initialization only
✅ No key handling risks — Uses existing SingleKey/storage patterns
✅ Unilateral exit unaffected — SW health is independent of VTXO state
✅ No double-spend vectors introduced — Message-based communication, no new transaction handling

Code Quality

Strengths:

• Clear commit isolation — Each commit addresses one concern, easy to bisect
• Timeout strategy is sound — 5s for activation (no network), 30s for MessageBus init (includes ARK RPC)
• Concurrency done right — IndexedDB warmup + zombie check run in parallel, not sequentially
• Fallback layers — SW unregistration is last resort after retries exhausted + zombie check
• PING/PONG is minimal — Registered before any other code, won't block on SDK init failures

Minor observations:

• Line 357: Message bus timeout now 30s while activation stays 5s — this split is intentional and correct (good comment explains it)
• Zombie detection timeout: 500ms is appropriate for local MessageChannel (vs. 2s in the commit message, which was revised during the PR)
• reduced 5→2 with 30s timeout = 60s total, which is reasonable for ARK cold starts

Testing Coverage

The test plan covers the critical paths:

• ✅ Dead SW recovery (DevTools stop + reload)
• ✅ Normal startup regression check
• ⚠️ Consider also: zombie SW scenario (Vivaldi case) — may need manual testing given browser-specific behavior

Cross-Repo Impact

✅ TS SDK — Uses parameter; now receives 30s instead of 5s. No breaking changes.
✅ Go SDK — Independent implementation, unaffected.
✅ Wallet — Consumer of the timeout logic; this PR fixes its handling.

Recommendation

Approve. This is a well-decomposed fix for a real deadlock that affects users after browser/SW crashes or long idle periods. The timeout tuning is based on observed ARK server cold-start behavior, and the zombie detection is an elegant pre-flight check. No protocol or security risks.

One thing to watch in next release: monitor for regression if any user reports the 30s timeout feeling long — though given this is only for recovery cases, it's justified.

[ghost]

Review: Service Worker Recovery & Cold Start Optimization

Status: Approved for merge ✅

Strengths

1. Race Condition Hardening (Latest commit)

   • Captures registration.active before null check — prevents TOCTOU between guard and postMessage
   • Type-checks event.data.type === 'PONG' — guards against unrelated messages on the MessageChannel
   • Solid defensive pattern for cross-thread communication

2. Zombie SW Detection

   • 500ms health-check ping with MessageChannel is elegant and lightweight
   • Runs in parallel with IndexedDB warmup — good perf optimization
   • Unregisters only if truly dead (no response), avoiding false positives on slow networks

3. Timeout Tuning

   • Increased MessageBus init timeout from 5s → 30s addresses real cold-start issue:
     • DNS, TLS handshake, and server wake-up on days-old idle connections legitimately exceed 5s
     • SDK itself uses 30s default, so alignment makes sense
   • Reduced maxRetries 5 → 2: with 30s timeouts, 2 retries (60s total) is reasonable
   • Kept SW activation timeout at 5s (no network involved) — correct decision

4. Graceful Degradation

   • SW unregistration only after all retries exhausted — preserves the working SW most of the time
   • Best-effort error handling on cleanup
   • Reload recovery now works because fresh registration bypasses zombie SW

5. Early Pre-Registration (Previous commit)

   • Parallelizes SW activation with React bootstrap, auth checks, ASP fetch
   • On cold starts, this removes activation from the critical path
   • Non-blocking .catch() — won't crash if registration fails

Minor Observations

1. PONG Type Check

   • Good defensive practice. Curious if there are other message types on that port, or if this is purely defensive against noise?
   • Code is correct either way.

2. IndexedDB Warmup

   • Concurrent with zombie check is efficient, but getWalletState() might fail independently
   • The Promise.all correctly propagates either error to the catch block
   • 👍

3. No Test Plan Yet

   • PR body lists the test plan checkboxes but they're unchecked
   • Please verify before merge:
     • Reproduce stuck loading (stop SW via DevTools)
     • Verify app recovers after auto-reload (not stuck forever)
     • Verify normal startup unaffected when SW is healthy
     • Check cold start performance improvement

Protocol & Security

• No protocol impact — this is wallet frontend UX only, not Arkade protocol
• No Ark SDK changes — compatible with current MessageBus behavior
• Cross-repo check: No dependencies on unreleased SDK changes ✓

Recommendation

Merge after:

1. Completing the test plan checklist
2. One last check on the latest commit for message race conditions (looks solid, but verify with DevTools)

Great work addressing a real pain point. The defense-in-depth approach (pre-registration + zombie detection + timeout tuning + unregistration) is the right call for reliability.

[ghost]

Review: Service Worker Deadlock Recovery

Overall: ✅ Solid fix for the stuck loading state. The logic is sound and the test plan is comprehensive.

Architecture & Approach

1. Parallel pre-registration (index.tsx)

   • Registers SW early in page bootstrap (parallel with ASP fetch, auth check)
   • Saves activation wait from critical path on cold starts
   • Correct use of to silently ignore pre-reg failures

2. Zombie detection (wallet.tsx)

   • PING/PONG via MessageChannel is the right pattern for detecting unresponsive workers
   • 500ms timeout is reasonable (healthy SWs respond in <10ms)
   • Runs in parallel with (good resource utilization)

3. Graceful fallback

   • Reduces retry count from 5 → 2 after zombie detection (more aggressive once we know SW is dead)
   • Separates activation timeout (5s) from message-bus timeout (30s)
   • Unregisters dead SW after max retries exhausted (allows fresh registration on reload)

4. Minimal service worker changes (wallet-service-worker.ts)

   • PING handler registered before other initialization code
   • Handles MessageChannel port correctly
   • No additional complexity in normal flow

Security & Edge Cases

• ✅ Timeout handling prevents indefinite hangs
• ✅ cleanup is best-effort (wrapped in try/catch)
• ✅ Zombie detection doesn't break healthy SW flow (parallel, independent)
• ✅ Message cleanup (closing port) prevents resource leaks

Test Plan

The checklist is solid, but consider:

• ✅ Reproducing stuck loading (DevTools → stop SW)
• ✅ Verifying recovery after reload
• ✅ Checking normal startup unaffected
• 💡 Also test: What if SW dies between zombie check and full initialization? (Unlikely given timing, but good to confirm the final unregister catches it)

Questions/Nits

• Is timeout truly 30s, or should this be tuned based on real-world latency?
• The check: confirm this correctly catches timeouts (not just missing error property)

Recommendation: Approve. This is a targeted, well-reasoned fix for a real user pain point. The code is clean and the degradation path is explicit.