Skip to content

fix: resolve 5 sentry alerts in sample apps#363

Merged
ignaciosantise merged 3 commits intodevelopfrom
fix-sentry-alerts
Mar 30, 2026
Merged

fix: resolve 5 sentry alerts in sample apps#363
ignaciosantise merged 3 commits intodevelopfrom
fix-sentry-alerts

Conversation

@ignaciosantise
Copy link
Copy Markdown
Collaborator

Summary

Fixes 5 distinct Sentry alerts affecting the wallet and dapp sample apps:

  1. StateError on closed WebSocket sink — Race condition when socket closes during disconnect
  2. TypeError in activity_page — Null check operators in error handlers
  3. ReownSignError not handled — Unsupported chains error propagating to Sentry
  4. HandshakeException during connection — WebSocket TLS handshake failures
  5. App Hanging 2000ms+ — Heavy sequential initialization blocking main thread

Changes

  • websocket_handler.dart: Store stream subscriptions and cancel before closing sink; wrap all sink operations in try-catch
  • activity_page.dart: Use null-safe access in error handlers; only send analytics if session/appKit available
  • appkit_modal_impl.dart: Handle ReownSignError in request flow and broadcast to UI
  • walletkit example main.dart: Increase GetIt timeout from 1s to 5s; add yield points between init steps

Testing

  • All 140 reown_core tests pass
  • All 135 reown_sign tests pass (9 pre-existing network flaky tests)
  • All 111 reown_appkit tests pass (39 pre-existing plugin missing issues)
  • flutter analyze passes on all modified files

Due Diligence

  • Breaking change
  • Requires a documentation update

- Fix StateError on closed WebSocket sink by storing and canceling subscriptions
- Add null-safe access in activity_page error handlers to prevent TypeError
- Handle ReownSignError in modal request flow instead of rethrowing
- Wrap WebSocket handshake and stream errors in try-catch blocks
- Increase app initialization timeout and add yield points to prevent hanging

All changes include test coverage validation (140/140 core tests pass).
Copilot AI review requested due to automatic review settings March 18, 2026 15:12
@claude
Copy link
Copy Markdown

claude bot commented Mar 18, 2026

Claude finished @ignaciosantise's task in 2m 23s —— View job


PR Review

  • Read changed files
  • Analyze code changes
  • Post review
Found 4 issue(s)

Issue 1: ReownSignError still rethrown after UI broadcast — Sentry alert may persist

ID: appkit-modal-reownsignerror-rethrow-a7f2
File: packages/reown_appkit/lib/modal/appkit_modal_impl.dart:1662-1665
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: ReownSignError is caught, broadcast to UI via onModalError.broadcast(ModalError(e.message)), then rethrow executes unconditionally for ALL branches
  • Risk: The rethrow at line 1665 still propagates ReownSignError up the call stack. If the caller doesn't catch it, it will still reach runZonedGuarded / Sentry — meaning Sentry alert WalletKit 1.0.1 #3 may not be fully suppressed
  • Impact: Incomplete fix for the stated Sentry alert. Compare CoinbaseServiceException which replaces the exception (throw ReownAppKitModalException(...)) so callers see a different type; ReownSignError doesn't get the same treatment
  • Trigger: Any requestApproval call with an unsupported chain

Recommendation: Suppress the rethrow for ReownSignError (it's already broadcast to UI):

} else if (e is ReownSignError) {
  onModalError.broadcast(ModalError(e.message));
  return; // don't rethrow — already handled via UI broadcast
}
rethrow;

Issue 2: Pervasive silent catch (_) {} blocks drop messages and hide errors

ID: websocket-handler-silent-catch-b3c1
File: packages/reown_core/lib/relay_client/websocket/websocket_handler.dart:59-86
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: Every callback in the stream subscriptions (onError, onDone, the data handler for sink.add) wraps operations in catch (_) {} with no logging
  • Risk: If _socket?.sink.add(data) throws (e.g. a message fails to send), the message is silently dropped — the caller gets no error, the user gets no feedback, and there's no trace in logs
  • Impact: Future debugging is extremely difficult; real errors are invisible. The fix prevents crashes but at the cost of observability
  • Trigger: Any WebSocket send failure after TLS/network disruption

Recommendation: Log the error before swallowing, at minimum:

(data) {
  try {
    _socket?.sink.add(data);
  } catch (e) {
    // log instead of silently dropping
    debugPrint('[WebSocketHandler] sink.add failed: $e');
  }
},

Issue 3: Force-unwrap ! in initState not fixed — same crash path remains

ID: activity-page-initstate-forcenull-d4e9
File: packages/reown_appkit/lib/modal/pages/activity_page.dart:71-73
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: initState at lines 71-73 still uses selectedChain!.chainId, session!.getAddress(namespace)!, and (line 57) appKit!.core with force-unwrap operators. The PR fixes the error-handler path but leaves the initial load path unchanged
  • Risk: If selectedChain, session, or getAddress(namespace) returns null when ActivityPage opens, the widget crashes with an unhandled Null check operator used on a null value — a new Sentry alert on the same page
  • Impact: Partial fix; the error handler won't fire if the crash happens in initState before _fetchActivities
  • Trigger: Race condition where session/chain is cleared while the page is opening, or on first load before session fully initializes

Recommendation: Guard initState the same way the catch block was fixed:

final chain = widget.appKitModal.selectedChain;
final session = widget.appKitModal.session;
if (chain == null || session == null) return;
_currentChain = chain.chainId;
final namespace = NamespaceUtils.getNamespaceFromChain(_currentChain);
_currentAddress = session.getAddress(namespace) ?? '';

Issue 4: Dead code — _channel == null check is unreachable

ID: websocket-handler-dead-null-check-f1a3
File: packages/reown_core/lib/relay_client/websocket/websocket_handler.dart:91-98
Severity: LOW
Category: code_quality

Context:

  • Pattern: if (_channel == null) at line 91 checks nullability immediately after _channel is assigned at line 89 with no await or async gap between them
  • Risk: This branch can never be entered; the dead code adds confusion about what invariants actually hold
  • Impact: Misleading dead code that suggests _channel could be null in this position (it can't be)

Recommendation: Remove lines 91-98 entirely.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Addresses multiple Sentry alerts in the walletkit/appkit sample apps and underlying relay/websocket plumbing by hardening error handling and reducing UI-thread blocking during initialization.

Changes:

  • Add stream subscription tracking + safer close/write handling in WebSocketHandler to avoid “write to closed sink” races and wrap connect readiness failures.
  • Make activity page analytics/error handling null-safe to avoid crashy ! usage in error paths.
  • Adjust modal request error handling (Coinbase + ReownSignError) and sample walletkit initialization timing/yield points to reduce UI hangs.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
packages/reown_walletkit/example/lib/main.dart Extends GetIt allReady timeout and yields between init steps to reduce UI stalls in the example app.
packages/reown_core/lib/relay_client/websocket/websocket_handler.dart Tracks input/output subscriptions, wraps sink operations, and improves readiness error handling for websocket connect/disconnect races.
packages/reown_appkit/lib/modal/pages/activity_page.dart Removes unsafe null assertions in error/analytics paths by gating analytics on session/appKit presence.
packages/reown_appkit/lib/modal/appkit_modal_impl.dart Adds ReownSignError handling/broadcasting during request flow and refactors Coinbase exception branch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +1658 to 1665
} else if (e is CoinbaseServiceException) {
// If the error is due to no session on Coinbase Wallet we disconnnect the session on Modal.
// This is the only way to detect a missing session since Coinbase Wallet is not sending any event.
throw ReownAppKitModalException('Coinbase Wallet Error');
} else if (e is ReownSignError) {
onModalError.broadcast(ModalError(e.message));
}
rethrow;
ignaciosantise and others added 2 commits March 26, 2026 15:06
- Stop rethrowing ReownSignError after UI broadcast (comment #1)
- Add debugPrint logging in all silent catch blocks (comment #2)
- Clean up socket/controllers on failed handshake via close() (comment #3)
- Move chainId validation inside try/catch so it's handled (comment #4)
- Fix "disconnnect" typo in comment (comment #6)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ignaciosantise ignaciosantise merged commit d19cc62 into develop Mar 30, 2026
4 checks passed
@ignaciosantise ignaciosantise deleted the fix-sentry-alerts branch March 30, 2026 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants