Skip to content

http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect #30785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from

http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect

e1a5778
Select commit
Loading
Failed to load commit list.
Closed

http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect #30785

http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect
e1a5778
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed May 22, 2026 in 13m 58s

Code review found 1 potential issue

Found 2 candidates, confirmed 1. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 1
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit test/js/web/websocket/websocket-custom-headers.test.ts:407-411 New WebSocket tests don't track ws in clients[] for afterEach cleanup

Annotations

Check warning on line 411 in test/js/web/websocket/websocket-custom-headers.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

New WebSocket tests don't track ws in clients[] for afterEach cleanup 

The new `openAndEchoHeaders` helper creates a `WebSocket` without pushing it to the module-level `clients[]` array, unlike every other test in this file. If `Promise.all([opened, headersSeen])` rejects via `ws.onerror`, `ws.close()` is skipped and `afterEach` won't terminate the socket — add `clients.push(ws);` right after construction to match the file's established cleanup pattern (test/CLAUDE.md: "Track resources (servers, clients) in arrays for cleanup in afterEach()").
View more details on Claude