Skip to content

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Sep 22, 2025

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

Copy link
Contributor

@Copilot 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

This PR improves the resilience and maintainability of the HTTP/2 client implementation by adding comprehensive JSDoc type annotations and making defensive programming improvements.

  • Adds JSDoc type definitions and function documentation for better type safety
  • Improves error handling in GOAWAY frame processing with null checks
  • Updates variable declarations and comparisons for better code clarity

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// Specifically, we do not verify the "valid" stream id.

const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, util.getSocketInfo(this[kSocket]))
const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] && util.getSocketInfo(this[kSocket]))
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

The null check this[kSocket] && will pass false to util.getSocketInfo() when socket is null/undefined, which may cause unexpected behavior. Consider using a ternary operator: this[kSocket] ? util.getSocketInfo(this[kSocket]) : null

Suggested change
const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] && util.getSocketInfo(this[kSocket]))
const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] ? util.getSocketInfo(this[kSocket]) : null)

Copilot uses AI. Check for mistakes.


/** @type {import('node:http2').ClientHttp2Stream} */
let stream = null
let stream
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

[nitpick] Variable stream is declared without initialization. Consider initializing it explicitly as let stream = undefined for clarity, or use the original null initialization if that was intentional.

Suggested change
let stream
let stream = undefined

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant