Skip to content

Conversation

@Drincann
Copy link
Owner

@Drincann Drincann commented Sep 5, 2025

PR Type

Enhancement


Description

  • Improved CLI command output formatting

    • Pretty-print JSON structures
    • Detailed unknown command messages
  • Enhanced P2P server capabilities

    • Added send method to ClientInterface
    • Introduced onConnect handler

Diagram Walkthrough

flowchart LR
  A["WebSocketServer"]
  B["setupSocket"]
  C["buildPeerContext"]
  D["onConnect Handler"]
  A -- "connection" --> B
  B -- "valid client" --> C
  C -- "invoke connect" --> D
Loading

File Walkthrough

Relevant files
Enhancement
cli.mts
Improve CLI command outputs and error handling                     

src/cli.mts

  • Remove await on node.start
  • Use pretty-print JSON for block/mine output
  • Add block not found and unknown command errors
  • Rename block output label (new block)
+14/-6   
p2p.mts
Add onConnect and send in P2P Server                                         

src/lib/p2p.mts

  • Extend ClientInterface with send method
  • Introduce onConnect handler and connectHandler
  • Refactor buildPeerContext for unified context
  • Invoke onConnect on new peer connections
+33/-13 

@github-actions
Copy link

github-actions bot commented Sep 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Race Condition

Removing await from node.start may cause the REPL loop to begin before the node is fully initialized, leading to unexpected errors.

node.start(parseInt(process.argv[2] || '3001'))
Unhandled Promise

The onConnect handler is invoked without awaiting or catching its returned promise, which could lead to unhandled promise rejections.

this.wss.on('connection', (ws, req) => {
  const client = this.setupSocket(ws, req.socket.remoteAddress + ':' + req.socket.remotePort)
  if (client) {
    this.connectHandler(this.buildPeerContext(client))
  }
})

@github-actions
Copy link

github-actions bot commented Sep 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Await and handle node startup errors

Ensure that the node has fully started before accepting commands and handle any
startup errors to prevent race conditions. Wrap the call in a try/catch with a
top‐level await so failures are logged and the process exits gracefully.

src/cli.mts [5]

-node.start(parseInt(process.argv[2] || '3001'))
+try {
+  await node.start(parseInt(process.argv[2] || '3001'))
+} catch (err) {
+  console.error('Node startup failed:', err)
+  process.exit(1)
+}
Suggestion importance[1-10]: 8

__

Why: The suggestion restores the removed await to ensure startup completion and adds error handling to catch failures, preventing race conditions and unhandled startup errors.

Medium
Catch onConnect handler errors

Prevent unhandled promise rejections from the connect handler by catching any errors
it throws. Chain a .catch() onto the connectHandler invocation to log failures
without crashing the server.

src/lib/p2p.mts [25-30]

 this.wss.on('connection', (ws, req) => {
   const client = this.setupSocket(ws, req.socket.remoteAddress + ':' + req.socket.remotePort)
   if (client) {
-    this.connectHandler(this.buildPeerContext(client))
+    const ctx = this.buildPeerContext(client)
+    this.connectHandler(ctx).catch(error =>
+      console.error('onConnect handler error:', error)
+    )
   }
 })
Suggestion importance[1-10]: 6

__

Why: Wrapping connectHandler with .catch prevents unhandled promise rejections on new connections, improving server stability.

Low

@Drincann Drincann merged commit 9d21eca into main Sep 5, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants