fix(deployment): reduce noisy websocket errors in Sentry#3050
fix(deployment): reduce noisy websocket errors in Sentry#3050
Conversation
…27J) Skip error reporting when the abort signal has fired (user navigated away from the logs tab) and downgrade remaining websocket errors to warning severity. Include close code and reason in the error message for better Sentry triage.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefined WebSocket and deployment log error handling: WebSocket errors now include a reason and standardized message; tests updated to match. Deployment log stream catch-handler now checks abort signals, conditions UI state updates, and reports errors with severity and an optional cause. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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 `@apps/deploy-web/src/components/deployments/DeploymentLogs.tsx`:
- Around line 137-145: The catch block for getLogsStream(...) leaves
isLoadingLogs true on non-abort failures causing the skeleton to hang; in the
catch handler (the block referencing abortController.signal.aborted and
errorHandler.reportError) set isLoadingLogs to false (or call the setter that
updates the loading state) before calling errorHandler.reportError so the UI
clears the loading state on any non-abort error; ensure you reference the same
abortController and do not change abort handling logic for the abort case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab3f6078-41ce-43e8-a0d1-f85a8215e34f
📒 Files selected for processing (3)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsxapps/deploy-web/src/lib/websocket/createWebsocket.spec.tsapps/deploy-web/src/lib/websocket/createWebsocket.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3050 +/- ##
==========================================
+ Coverage 59.71% 60.83% +1.11%
==========================================
Files 1034 1011 -23
Lines 24273 24154 -119
Branches 6003 5990 -13
==========================================
+ Hits 14494 14693 +199
+ Misses 8528 8256 -272
+ Partials 1251 1205 -46
🚀 New features to boost your workflow:
|
…e target Extract error handling from DeploymentLogs useEffect into a testable handleLogsStreamError function and add unit tests covering both the abort-skip and warning-report paths.
…ws error message Inline handleLogsStreamError into the component per review feedback, use a stable "websocket error" message for Sentry grouping, and forward cause as extra context.
There was a problem hiding this comment.
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 `@apps/deploy-web/src/components/deployments/DeploymentLogs.tsx`:
- Around line 138-147: The reportError call currently fires even for aborted
streams and sets severity inversely; change it so you skip reporting entirely
when abortController.signal.aborted is true, and for non-abort failures set
severity to "warning" (not "error"); update the error payload sent to
errorHandler.reportError (the call in this block) to include the error message
(e.g., error instanceof Error ? error.message : String(error)) as a property or
tag so grouping is stable, and keep using the existing tags { category:
"deployments", label: "followLogs" } while leaving setIsLoadingLogs and
setIsConnectionEstablished behavior unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7ac9ae8-d042-4c8d-9a70-bdf7eb902dd9
📒 Files selected for processing (1)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx
| setIsConnectionEstablished(false); | ||
| } | ||
|
|
||
| errorHandler.reportError({ |
There was a problem hiding this comment.
thought: I doubt that this PR actually reduces websocket errors because all AbortErrors are ignored at errorHandler level ->
There was a problem hiding this comment.
browser websocket on(error) event doesn't expose details about the actual issue. That's why we probably need to send additional http request to websocket, in case of error to get the actual error details (for example, 404, provider network issues, provider is down, etc)
There was a problem hiding this comment.
I tested it in console-beta and I was able to reproduce the error case for websocket. it happens websocket is closed before the connection has been established.
I think that we just need to ignore this situation in websocket abstraction
…andling Resolve silently in websocket abstraction when signal is aborted instead of rejecting. Remove explicit cause logging (Sentry walks .cause automatically) and skip reporting aborted stream errors entirely.
3 participants
Why
Sentry issue DEPLOY-WEB-27J is escalating — 595 occurrences across 79 users in ~2 days. The
Error: websocket errorfires from the deployment logs WebSocket stream (followLogs) on/deployments/[dseq].Most of these errors are caused by users navigating away from the LOGS tab, which aborts the WebSocket connection and triggers the error handler unnecessarily. The remaining genuine errors have a generic message that makes Sentry triage difficult.
Fixes CON-204
What
useEffectcleanup runs (user navigates away), the abort controller fires, closing the WS. Previously this still reported to Sentry; now we checkabortController.signal.abortedbefore reporting.warning— genuine WebSocket connection failures (provider down, network issue) are transient and expected in a streaming context;warningis more appropriate thanerror."websocket error", the message now reads"websocket error: 1006 unknown reason", making Sentry grouping and triage much easier.Summary by CodeRabbit