Skip to content

Convert bootstrap socket helper to TypeScript#28377

Merged
EvanHahn merged 1 commit into
mainfrom
convert-bootstrap-socket-to-ts
Jun 5, 2026
Merged

Convert bootstrap socket helper to TypeScript#28377
EvanHahn merged 1 commit into
mainfrom
convert-bootstrap-socket-to-ts

Conversation

@EvanHahn
Copy link
Copy Markdown
Contributor

@EvanHahn EvanHahn commented Jun 4, 2026

no ref

This change should have no user impact.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 53545b31-4390-4ad5-bbb0-33961bf2f412

📥 Commits

Reviewing files that changed from the base of the PR and between 527f7e4 and 1206096.

📒 Files selected for processing (4)
  • ghost/core/core/server/lib/bootstrap-socket.ts
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.js
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.ts
  • ghost/core/test/unit/server/notify.test.js
💤 Files with no reviewable changes (1)
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
  • ghost/core/test/unit/server/notify.test.js
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.ts
  • ghost/core/core/server/lib/bootstrap-socket.ts

Walkthrough

This PR converts the bootstrap-socket module from CommonJS/JavaScript to TypeScript/ESM, improving type safety and error handling. The main implementation adds typed contracts for SocketAddress and ConnectOptions, introduces type-guard helpers (hasSocketAddress, getErrorCode), and refactors socket connection logic to use immutable options objects instead of mutation. Unit tests are migrated from JavaScript to TypeScript with a new test case, and the dependent notify.test.js is updated to use rewire-based stubbing to work with the new module format.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Convert bootstrap socket helper to TypeScript' directly and clearly summarizes the main change: converting a JavaScript bootstrap socket module to TypeScript.
Description check ✅ Passed The description, though minimal, is related to the changeset. It correctly states that the change has no user impact, which is appropriate for a TypeScript conversion.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch convert-bootstrap-socket-to-ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@ghost/core/core/server/lib/bootstrap-socket.ts`:
- Line 27: The guard in bootstrap-socket.ts currently treats falsy JsonValue
messages as invalid by using "!message"; update the condition to only reject an
absent message by replacing the "!message" check with an explicit undefined
check (e.g., "message === undefined") so that valid falsy JsonValue values like
0, false, and '' are accepted; keep the other checks
(hasSocketAddress(socketAddress), logging, logging.info, logging.warn) unchanged
and only alter the message portion of the if condition.
- Around line 20-23: hasSocketAddress currently treats any truthy numeric port
as valid so negative or >65535 values slip through; update the predicate
(hasSocketAddress) to validate that socketAddress.port is an integer within the
TCP port range 1..65535 (e.g., Number.isInteger(socketAddress.port) &&
socketAddress.port >= 1 && socketAddress.port <= 65535) while keeping the
existing checks for host string; this ensures callers like client.connect only
receive valid ports.
🪄 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: CHILL

Plan: Pro

Run ID: bfa4a6d3-2b20-4e85-8e5d-a7e91b8c240c

📥 Commits

Reviewing files that changed from the base of the PR and between 24244bb and 527f7e4.

📒 Files selected for processing (4)
  • ghost/core/core/server/lib/bootstrap-socket.ts
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.js
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.ts
  • ghost/core/test/unit/server/notify.test.js
💤 Files with no reviewable changes (1)
  • ghost/core/test/unit/server/lib/bootstrap-socket.test.js

Comment thread ghost/core/core/server/lib/bootstrap-socket.ts
Comment thread ghost/core/core/server/lib/bootstrap-socket.ts Outdated
no ref

This change should have no user impact.
@EvanHahn EvanHahn force-pushed the convert-bootstrap-socket-to-ts branch from 527f7e4 to 1206096 Compare June 4, 2026 22:07
@EvanHahn EvanHahn merged commit b1b035c into main Jun 5, 2026
50 checks passed
@EvanHahn EvanHahn deleted the convert-bootstrap-socket-to-ts branch June 5, 2026 15:50
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