fix(shell-ui): distinguish network from auth failures and surface both in a recovery banner#1553
fix(shell-ui): distinguish network from auth failures and surface both in a recovery banner#1553kgarg2468 wants to merge 3 commits into
Conversation
Typed ConnectionFailure errors at throw sites, NETWORK_ERROR connection
state, no more silent stored-token reconnect failures, and a dismissible
recovery banner with retry / re-auth actions.
Fixes rocketride-ai/rocketride-saas#305
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughConnection recovery now classifies failures with typed errors, applies bounded attach and token-login timeouts, latches unresolved failures in connection status, persists tokens in ChangesConnection recovery and error reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShellLayout
participant ConnectionErrorBanner
participant ConnectionManager
participant RemoteManager
participant ConnectionStatus
ShellLayout->>ConnectionErrorBanner: render latched failure
ConnectionErrorBanner->>ConnectionManager: retry or start OAuth
ConnectionManager->>RemoteManager: attach or login with stored token
RemoteManager-->>ConnectionManager: success or typed failure
ConnectionManager->>ConnectionStatus: update or latch failure
ConnectionStatus-->>ShellLayout: updated status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/shell-ui/src/auth/ApiKeyAuthProvider.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: expand is not a function apps/shell-ui/src/auth/CloudAuthProvider.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: expand is not a function apps/shell-ui/src/connection/connection.tsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: expand is not a function
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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsx`:
- Line 83: Update the retry/sign-in button handler in ConnectionErrorBanner so
errors from onAction() are not silently discarded: catch unexpected exceptions
and log them with the component’s established logging mechanism, while
preserving the existing latched-status behavior for expected connection
failures.
- Around line 76-78: The default Retry handler in ConnectionErrorBanner’s
onAction must recover attach-timeout NETWORK_ERRORs when no token is stored.
Update the fallback around ConnectionManager.getInstance().reconnect() to invoke
bootstrap/attach again (or reload) when reconnect cannot proceed without a
token, while preserving the existing onRetry override and normal reconnect
behavior.
In `@apps/shell-ui/src/connection/connection.ts`:
- Around line 438-443: Extract the duplicated timeout race logic in the
connection class into a shared withTimeout helper that clears its timer in
finally after either promise settles. Replace both the 10-second _attachPromise
race and the corresponding 8-second timeout call site with this helper,
preserving their existing durations and ConnectionFailure messages.
- Around line 719-755: The comment does not request a code change: retain the
existing server-error classification in getConnectionFailureState, even though
current call sites only produce auth and network failures, so future server
failures map to ConnectionState.FAILED.
- Around line 433-452: Update the attach handling in _bootstrap so failures from
_attachPromise are not swallowed before the Promise.race, ensuring both
immediate attach errors and the timeout enter handleStoredTokenFailure as
network failures. Preserve the existing ConnectionFailure conversion and
return-null recovery flow.
- Around line 556-558: Replace the direct lastFailure assignment in the
authenticated connection flow with an updateConnectionStatus call that clears
lastFailure and emits shell:statusChange. Preserve the existing behavior of
resolving the latched authentication failure while ensuring useConnectionStatus
and ConnectionErrorBanner receive the update immediately.
In `@apps/shell-ui/src/connection/remote-manager.ts`:
- Around line 66-91: Extract a shared withTimeout utility for the repeated
Promise.race timeout pattern, accepting the operation promise, timeout duration,
and timeout-error factory. Update the login flow around client.login in the
connection manager and the corresponding flow in connection.ts to use this
helper, centralizing the timeout constant and ensuring timers are cleared when
the operation settles.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3961805d-ebc0-46d8-8615-191409583771
📒 Files selected for processing (6)
apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsxapps/shell-ui/src/components/layout/ShellLayout.tsxapps/shell-ui/src/connection/connection.tsapps/shell-ui/src/connection/errors.tsapps/shell-ui/src/connection/remote-manager.tspackages/shared-ui/src/types/connection.ts
|
@coderabbitai Addressed in 2945b57 — raw attach rejections now propagate to bootstrap (banner in ~0.5s instead of waiting out the 10s race), authenticated recovery clears the latch via updateConnectionStatus (statusChange emitted), shared timer-clearing withTimeout helper replaces the duplicated races, Retry falls back to a reload when no transport ever attached, and recovery-action errors are logged. Re-verified E2E locally: server-down → banner 0.5s + token preserved; Retry → reload; server-restored → session recovers, banner clears. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -1 2945b57a 2>/dev/null || echo "commit not found in shallow clone"
git show --stat 2945b57a 2>/dev/null | head -50Length of output: 877 🏁 Script executed: #!/bin/bash
git show 2945b57a -- apps/shell-ui/src/connection/connection.ts apps/shell-ui/src/connection/errors.ts apps/shell-ui/src/connection/remote-manager.ts apps/shell-ui/src/components/layout/ConnectionErrorBanner.tsxLength of output: 8215 🏁 Script executed: #!/bin/bash
grep -n "_attachPromise" apps/shell-ui/src/connection/connection.tsLength of output: 398
Nice work — this addresses the concerns raised earlier. Thanks for the detailed re-verification notes too. |
…restarts (#1555) * fix(shell-ui): persist session in localStorage across tabs and restarts The rr_ session token is a 90-day sliding server-side session; keeping it in sessionStorage was the only reason every tab/restart forced re-auth. Move to localStorage with one-time migration and cross-tab storage sync. Fixes rocketride-ai/rocketride-saas#303 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(shell-ui): handle replaced sessions * fix(shell-ui): reload tabs on token replacement --------- Co-authored-by: Krish Garg <krishgarg@Krishs-MacBook-Pro.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 participants
Network failures render as auth failures; stored-token reconnects fail silently; a dead server means an infinite splash
Three connected defects in the shell-ui connection layer (reported in rocketride-ai/rocketride-saas#305):
finishConnectclassified AUTH vs other by string-matching the error message ('Authentication failed'/'unknown user'/'invalid credentials'), so transport timeouts could misreport as auth failures — and the SDK's typedAuthenticationException("Invalid or revoked API key") matched neither pattern and surfaced as a network problem.catch { clearToken(); return null; }): the user lands on the logged-out page with no explanation, and a transient network blip destroys a valid token.Fix:
ConnectionFailure(kind: 'auth' | 'network' | 'server') thrown at the sites that know the cause (remote-managertimeout → network, missinguserId→ auth); SDKAuthenticationExceptionmaps to auth. The string match survives only as a documented legacy fallback for untyped errors.ConnectionState.NETWORK_ERROR; network failures preserve the stored token (retryable), auth failures clear it.ConnectionStatus.lastFailure: the SDK's persist-mode retry loop reportsCONNECTING(up to 120 attempts) and a post-failure anonymous connect reportsCONNECTED— either erased a purely state-driven failure before any UI could render it. Network latches auto-clear on reconnection; session-expiry persists until acted on.ConnectionErrorBanner(first consumer of the previously consumer-lessuseConnectionStatus()hook), mounted inShellLayout: "Can't reach the server — check your connection and retry." + Retry, vs "Your session has expired — please sign in again." + Sign in.Testing
--saas), Playwright-driven:rr_token → "session has expired" banner + token cleared + Sign in → Zitadeltsc --noEmitclean for shell-ui and shared-uiLinked Issue
Fixes rocketride-ai/rocketride-saas#305
🤖 Generated with Claude Code
Summary by CodeRabbit