Skip to content

fix: don't show 'connected to' on connection error#381

Draft
Pyiner wants to merge 1 commit into
getAlby:masterfrom
Pyiner:fix/connection-error-display
Draft

fix: don't show 'connected to' on connection error#381
Pyiner wants to merge 1 commit into
getAlby:masterfrom
Pyiner:fix/connection-error-display

Conversation

@Pyiner
Copy link
Copy Markdown

@Pyiner Pyiner commented Mar 28, 2026

Summary

Fixes #174 — When a wallet connection fails (e.g., invalid LNbits credentials), the UI incorrectly shows "Connected through [name]" and the Disconnect button.

Root Cause

In store.ts, the getInfo() call's error is silently swallowed (lines 86-90). For connectors like LNbits, getInfo() makes a real API call to /api/v1/wallet which fails with bad credentials. Since the error is caught and ignored, execution continues to set({ connected: true }), causing the UI to display connection success.

The bc-balance component correctly shows the warning icon (because getBalance() also fails), but the rest of the UI still shows "connected."

Fix

Re-throw the getInfo() error so it propagates to the outer catch block (line 114), which already:

  1. Sets the error state for display
  2. Sets connecting: false
  3. Calls disconnect() to clear the connection state

This is the minimal correct fix — the error handling path already exists and works correctly; the inner catch was just preventing errors from reaching it.

Testing

  • With invalid LNbits credentials: connection now correctly shows error and does not display "Connected through" text
  • With valid credentials: connection works normally (getInfo succeeds, no error thrown)
  • Page reload reconnection: works normally since saved credentials are still valid

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling during connection failures to properly display error states and disconnect, instead of leaving the app in an uncertain state.

…ilure

When a wallet connection fails (e.g., invalid credentials), getInfo() throws
an error that was previously swallowed. This caused the UI to incorrectly show
'Connected through [name]' even though the connection failed.

The outer catch block already handles disconnection and error display properly.
By re-throwing the getInfo() error, we ensure it reaches that handler and the
UI correctly reflects the failed connection state.

Fixes getAlby#174
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e50e275e-c6ab-415d-a1b3-05d2cdfdb9c5

📥 Commits

Reviewing files that changed from the base of the PR and between e8ceebe and 195e372.

📒 Files selected for processing (1)
  • src/state/store.ts

📝 Walkthrough

Walkthrough

A change to error handling in the store's provider connection flow. When provider.getInfo() fails, the code now rethrows the error instead of silently continuing, allowing the outer catch block to properly set error state, disable the connecting flag, and disconnect.

Changes

Cohort / File(s) Summary
Error Handling in Provider Connection
src/state/store.ts
Modified provider.getInfo() error handling to log the error object and rethrow, enabling proper error state propagation through the outer connect catch block instead of allowing execution to continue with potentially undefined info.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 When errors arise, don't hide the truth away,
Now throwing properly shows the state of the day,
No more false "connected" when things break and fail,
The UI now whispers the honest, true tale! 🔗✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: don't show 'connected to' on connection error' directly matches the main objective of preventing the UI from displaying a successful connection state when an error occurs.
Linked Issues check ✅ Passed The changes fix issue #174 by re-throwing getInfo() errors so they propagate to the outer catch handler, ensuring error state is properly set and the UI does not show 'connected to' or disconnect button on connection failures.
Out of Scope Changes check ✅ Passed The code changes are minimal (+2/-1 lines) and directly address the root cause in store.ts. All modifications are scoped to fixing the getInfo() error handling to resolve issue #174.
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

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

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

@rolznz
Copy link
Copy Markdown
Member

rolznz commented May 6, 2026

Thanks! this works well. However for NWC, I think we should also try to get the info as part of the connection. I tried to connect to a deleted NWC URL and it still shows connected successfully.

Example URL:

nostr+walletconnect://df7aef466965bd9ccbe4ca9a21ea8186c45d3e89bc68973bb75f2d08e422a4f9?relay=wss://relay.getalby.com/v1&secret=fb16e1121445f0c8214a8a129aca5450a67cb1ebbe4c3da744a4020bdc998242&lud16=lncurl_stricken_spider@getalby.com

@rolznz rolznz marked this pull request as draft May 15, 2026 11:24
@rolznz
Copy link
Copy Markdown
Member

rolznz commented May 15, 2026

@Pyiner are you able to continue working on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't show "connected to" on connection error

2 participants