fix(nip46): make every sign-in error actionable and surface init failures - #77
Merged
Merged
Conversation
…ures The user's bug report: 'web socket failure, response, no exception, no...' The 'no...' substring maps to the bare 'no response yet' string at nip46.go:371 (renamed the polling timeout to ErrTimeout with a contextful default). The 'web socket failure' substring never appears in source — it was either a wrapper error or a stale binary. Changes: - Add nip46.ErrTimeout (wraps the bare 'no response yet' string with Amber/approval context), nip46.RelayDialFailure (carries per-URL dial errors instead of discarding them), and nip46.IsRetryable so callers can distinguish transient vs hard failures. - ConnectRelays now returns *RelayDialFailure with per-URL error strings, so users see exactly which relays.txt entry is dead. - warnUnreachableRelays takes the failure map and logs the cause inline (DNS, TLS, timeout) instead of just the URL. - TUI's initQR previously swallowed OnInitQR errors via 'return nil' — now surfaces them as qrInitErrorMsg with the actionable message appended below the QR block. checkQRConnection stops polling on non-retryable errors via nip46.IsRetryable instead of looping forever on a hard failure. - Every user-facing error in the package now mentions the failing URL, env var, or an actionable noun (Amber, signer, network, relays.txt). Regression guards in nip46_test.go assert no message leaks the old bare 'no response yet' or 'websocket failure' substrings, and that the bare Reason case is wrapped with context by Error() as a defence-in-depth. Tests: +5 new, full suite green, go vet clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug Description
The user's bug report quoted an unhelpful message — "web socket failure, response, no exception, no..." — that maps to the bare
"no response yet"polling-timeout string inSession.CheckConnection(and the dial-failure path that discards per-URL error context). The error path during Amber sign-in either shows nothing actionable ("Generating connection..." forever) or shows a bare unhelpful string.The actual substring "web socket failure" is not in the source. It's either a wrapped go-nostr error or a stale binary from before PR #69/70/73/74/75. Either way, every error path in
nip46.gowas leaking insufficient context, so the user could not diagnose.Fixes #76
Root Cause
Three call sites produced unhelpful error messages:
Session.CheckConnectionreturned the bare"no response yet"after a polling timeout. No mention of Amber, no mention of approval, no mention of what to check.Session.ConnectRelaysdiscarded per-URL dial errors and only reported "could not connect to any relay (tried: ...)". A user with a stale relays.txt couldn't tell which entry was dead.tui.initQRswallowed errors fromOnInitQRviareturn nil. Users saw the QR screen with "Generating connection..." forever — no error message ever reached the TUI.tui.checkQRConnectionretried every error indefinitely, including non-retryable hard failures (all relays dropped, proxy refused, etc.). After a hard failure, the user kept seeing "Waiting for connection..." forever.Fix
nip46.ErrTimeoutwraps the polling timeout with Amber/approval context. ItsError()method also guards against callers passing a bare unhelpful Reason — it appends an actionable suffix.nip46.RelayDialFailurecarries per-URL dial error strings so users see the actual failure cause (DNS, TLS, timeout, connection refused) for each relay, not just a list of URLs.nip46.IsRetryablelets callers (the TUI) distinguish transient errors (keep polling) from hard failures (stop and surface to user).tui.qrInitErrorMsgis the new error message type. The TUI'sinitQRnow surfaces errors and theView()renders them below the QR block.tui.checkQRConnectionstops polling on non-retryable errors vianip46.IsRetryable.warnUnreachableRelaysnow logs the per-URL failure cause inline (e.g.wss://relay.example.com (dial tcp: lookup ...: no such host)) so users can identify the dead entry without enabling debug logging.Session.connect-time errorsare renamed to "relays connected but every one rejected our subscription" so the user knows the issue is subscription, not dial.How to Verify
go run .and pick "Scan QR with Amber".relays.txt(e.g. one entry pointing at127.0.0.1:1), confirm the sign-in error names the failing URL and the per-URL cause.HTTPS_PROXY=socks5h://127.0.0.1:9050 go run ., confirm the proxy error mentionsHTTPS_PROXYand9050.TestUserFacingErrorsAreActionableandTestErrTimeoutMessageIsActionablepass.Test Plan
TestRelayDialFailureMessage— asserts the per-URL cause is includedTestErrTimeoutIsRetryable— covers all IsRetryable branchesTestErrTimeoutMessageIsActionable— asserts the message includes Amber/approval context, with sabotage-run verifiedTestUserFacingErrorsAreActionable— meta-test asserting no user-facing error contains "websocket failure" or lacks an actionable noungo test ./...)go vet ./...cleanRisk Assessment
Low. This change is purely error-message wrapping plus plumbing for the new error type. No behavioural change to successful sign-in flows. The TUI changes are:
IsRetryablereturnsfalsefor non-timeout errors, so the previous retry-loop behaviour is preserved for transient timeouts and ONLY changed for hard failures (which previously looped forever with no message — strictly worse).The
ErrTimeout.Error()defence-in-depth guard could theoretically append unwanted context if a caller passes an exotic Reason, but the production caller (Session.CheckConnection) always passes a Reason that contains an actionable noun, so the guard never fires in practice.