refactor(webkit): extract protocol transport (#706 2/5)#734
Conversation
Behavior-preserving; same wire protocol; same error semantics. Extracts WebSocket / RDP message-passing layer from client.ts into src/webkit/protocol-transport.ts: - ProtocolTransport interface (adapter) used by WebKitClient to avoid circular dependencies and enable test doubles - WebSocketProtocolTransport concrete class owns the WebSocket, outer/inner pending-request maps, message-id generation, and Target.dispatchMessageFromTarget response routing - WebKitClient delegates send/connect/disconnect/isConnected to the transport; all public method signatures unchanged (facade preserved) - New tests/unit/protocol-transport.test.ts: happy path, out-of-order routing, TimeoutError, ConnectionError, ProtocolError propagation (outer + inner), domain event emission, disconnect rejection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the WebKit client by extracting the WebSocket and Remote Debugging Protocol (RDP) message-passing logic into a new ProtocolTransport layer. This change improves modularity and includes a comprehensive suite of unit tests for the new transport implementation. Feedback focuses on improving the abstraction between the client and transport layers: specifically, avoiding brittle monkey-patching for event forwarding, removing type casts that violate the ProtocolTransport interface, and ensuring the transport layer automatically clears pending requests upon connection loss to prevent hanging promises.
| const originalEmit = this.transport.emit.bind(this.transport); | ||
| (this.transport as any).emit = (event: string, ...args: any[]): boolean => { | ||
| const result = originalEmit(event, ...args); | ||
| if (event !== 'transport:close' && event !== 'transport:error' && event !== 'newListener' && event !== 'removeListener') { | ||
| (EventEmitter.prototype.emit as any).call(this, event, ...args); | ||
| } | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
Monkey-patching this.transport.emit is a brittle way to implement event forwarding as it relies on internal EventEmitter behavior and can lead to maintenance challenges.
Additionally, the call to EventEmitter.prototype.emit at line 84 is unnecessarily verbose. Since WebKitClient inherits from EventEmitter and does not override the emit method, you can simplify this to this.emit(event, ...args).
Consider if the ProtocolTransport interface could be updated to provide a more formal mechanism for subscribing to protocol events (e.g., a dedicated onMessage or onEvent callback).
| const originalEmit = this.transport.emit.bind(this.transport); | |
| (this.transport as any).emit = (event: string, ...args: any[]): boolean => { | |
| const result = originalEmit(event, ...args); | |
| if (event !== 'transport:close' && event !== 'transport:error' && event !== 'newListener' && event !== 'removeListener') { | |
| (EventEmitter.prototype.emit as any).call(this, event, ...args); | |
| } | |
| return result; | |
| }; | |
| const originalEmit = this.transport.emit.bind(this.transport); | |
| (this.transport as any).emit = (event: string, ...args: any[]): boolean => { | |
| const result = originalEmit(event, ...args); | |
| if (event !== 'transport:close' && event !== 'transport:error' && event !== 'newListener' && event !== 'removeListener') { | |
| this.emit(event, ...args); | |
| } | |
| return result; | |
| }; |
| // Clear stale pending requests before reconnect | ||
| (this.transport as WebSocketProtocolTransport).clearPendingRequests(); |
There was a problem hiding this comment.
This type cast to WebSocketProtocolTransport violates the abstraction of the ProtocolTransport interface.
It is recommended to move the responsibility of clearing pending requests into the transport layer itself (e.g., within its close event handler). This ensures the transport maintains its own internal state consistently and allows the client to remain decoupled from the concrete implementation details.
| ws.on('close', () => { | ||
| this._connected = false; | ||
| this.emit('transport:close'); | ||
| }); |
There was a problem hiding this comment.
The transport should automatically clear all pending requests when the connection is closed. This ensures that any awaiting promises are rejected immediately with a ConnectionError upon connection loss, rather than hanging until their individual timeouts expire.
| ws.on('close', () => { | |
| this._connected = false; | |
| this.emit('transport:close'); | |
| }); | |
| ws.on('close', () => { | |
| this._connected = false; | |
| this.clearPendingRequests(); | |
| this.emit('transport:close'); | |
| }); |
- Replace the brittle transport.emit monkey-patch with a formal
onProtocolEvent(handler) subscription on the ProtocolTransport
interface. The transport now relays raw RDP events to subscribers via
a dedicated channel, so transport-lifecycle events ('transport:close',
'transport:error') and EventEmitter housekeeping events
('newListener', 'removeListener') stay scoped to the transport.
- Auto-clear pending requests inside the WebSocket close handler so any
awaiting promises reject immediately with ConnectionError instead of
hanging until their per-request timers expire.
- Drop the (transport as WebSocketProtocolTransport) cast in
handleDisconnect. WebKitClient now reuses the public
transport.disconnect() path, which already clears pending state, so
the client no longer needs to know the concrete transport class.
- Cover the new behaviors with two unit tests (ws close auto-clears
pending; onProtocolEvent relays raw RDP events without leaking
lifecycle events).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bbf26ec3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Surface listener errors via the standard 'error' channel so a | ||
| // misbehaving subscriber cannot break the message-routing loop. | ||
| this.emit('error', err as Error); | ||
| } |
There was a problem hiding this comment.
If an onProtocolEvent subscriber throws (for example, a WebKitClient event listener throwing during re-emit), this catch path calls this.emit('error', ...). In Node, emitting 'error' without a registered 'error' listener throws and can terminate the process, so the transport can still crash the app despite the guard comment. Emitting 'transport:error' (which the client already handles) or swallowing/logging here would preserve the intended isolation.
Useful? React with 👍 / 👎.
…error Codex flagged that emitting Node's standard 'error' event from the onProtocolEvent guard could terminate the process if no 'error' listener is registered. Switch to the existing transport:error lifecycle channel that WebKitClient already handles, and add a regression test that confirms a throwing subscriber surfaces on transport:error without ever hitting the default error event.
|
/gemini review |
|
@codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91a1b11f7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| }); | ||
| await this.transport.connect(wsUrl); | ||
| this.connected = true; |
There was a problem hiding this comment.
connectToTarget() marks this.connected only after await this.transport.connect(...), but the close handler in bindTransportEvents() only calls handleDisconnect() when this.connected is already true. If the socket opens and then closes immediately (flaky network/proxy restart) before line 1110 runs, the emitted transport:close is ignored, so reconnect logic is skipped and the client can stay in a stale state until some later heartbeat failure. This is a regression from the previous flow where connected was set in the WebSocket open callback before close handling could run.
Useful? React with 👍 / 👎.
1 participant
Stacked on PR #732 (base = `refactor/706a-errors`). Will retarget to develop after #732 merges.
Implements #706 step 2 of 5. Behavior-preserving. Follow-ups: target-session, browser-commands, events+facade.
Does not Close — multi-PR refactor.
What changed
Validation
Size