feat(cmd): add status command with ckb-tui#323
Conversation
b694b50 to
c6e7be1
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new status command that integrates the ckb-tui tool to provide a terminal user interface for monitoring CKB network status.
Key Changes:
- Adds
CKBTuiclass with automatic binary download, installation, and execution capabilities - Implements a
statuscommand that checks RPC port availability and launches ckb-tui - Updates configuration settings to support tool versioning and directory management
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/ckb-tui.ts | New class implementing ckb-tui binary management with platform-specific download and installation |
| src/cmd/status.ts | New status command that validates RPC connectivity before launching ckb-tui interface |
| src/cli.ts | Registers the status command with network option support |
| src/cfg/setting.ts | Adds configuration for tools root folder and ckb-tui version settings |
| README.md | Documents the new status command usage and TUI monitoring capability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If the binary is not found after extraction (neither in the bin directory nor in subdirectories), the code silently continues without throwing an error or logging a warning. This could lead to confusing behavior later when trying to execute the binary. Consider adding an error check after the loop at line 84 to verify that the binary was successfully extracted and moved.
| // Check that the binary was successfully extracted and moved | |
| if (!fs.existsSync(this.binaryPath!)) { | |
| logger.error(`ckb-tui binary was not found after extraction. Expected at: ${this.binaryPath}`); | |
| throw new Error('Failed to extract and locate ckb-tui binary.'); | |
| } |
| client.once('error', () => { | ||
| resolve(false); | ||
| }); | ||
| client.connect(port, '127.0.0.1'); | ||
| client.once('connect', () => { | ||
| client.end(); | ||
| resolve(true); | ||
| }); |
There was a problem hiding this comment.
The socket connection is never cleaned up if neither the 'error' nor 'connect' event fires in a timely manner. This can lead to resource leaks. Consider adding a timeout to ensure the socket is closed and the promise resolves even if the connection hangs indefinitely.
| client.once('error', () => { | |
| resolve(false); | |
| }); | |
| client.connect(port, '127.0.0.1'); | |
| client.once('connect', () => { | |
| client.end(); | |
| resolve(true); | |
| }); | |
| let settled = false; | |
| const TIMEOUT_MS = 5000; | |
| const timeout = setTimeout(() => { | |
| if (!settled) { | |
| settled = true; | |
| client.destroy(); | |
| resolve(false); | |
| } | |
| }, TIMEOUT_MS); | |
| client.once('error', () => { | |
| if (!settled) { | |
| settled = true; | |
| clearTimeout(timeout); | |
| resolve(false); | |
| } | |
| }); | |
| client.once('connect', () => { | |
| if (!settled) { | |
| settled = true; | |
| clearTimeout(timeout); | |
| client.end(); | |
| resolve(true); | |
| } | |
| }); | |
| client.connect(port, '127.0.0.1'); |
src/tools/ckb-tui.ts
Outdated
| static runWithArgs(args: string[]) { | ||
| this.run(args); | ||
| } |
There was a problem hiding this comment.
The runWithArgs method is redundant and simply calls the run method. This doesn't add any value and can be removed. All callers should directly use the run method instead.
| static runWithArgs(args: string[]) { | |
| this.run(args); | |
| } |
src/cmd/status.ts
Outdated
| } | ||
|
|
||
| async function isRPCPortListening(port: number): Promise<boolean> { | ||
| const net = require('net'); |
There was a problem hiding this comment.
Using require inline within a function is not consistent with the ES6 import style used elsewhere in the file. Consider importing the net module at the top of the file with import * as net from 'net'; for consistency.
src/tools/ckb-tui.ts
Outdated
| // Clean up archive | ||
| fs.unlinkSync(archivePath); | ||
|
|
||
| logger.info('ckb-tui installed successfully.'); | ||
| } catch (error) { | ||
| logger.error('Failed to download/install ckb-tui:', (error as Error).message); | ||
| throw error; |
There was a problem hiding this comment.
If an error occurs during extraction or binary setup (lines 59-85), the downloaded archive file won't be cleaned up. Consider wrapping the cleanup in a finally block or moving it earlier to ensure cleanup happens even when errors occur.
| // Clean up archive | |
| fs.unlinkSync(archivePath); | |
| logger.info('ckb-tui installed successfully.'); | |
| } catch (error) { | |
| logger.error('Failed to download/install ckb-tui:', (error as Error).message); | |
| throw error; | |
| logger.info('ckb-tui installed successfully.'); | |
| } catch (error) { | |
| logger.error('Failed to download/install ckb-tui:', (error as Error).message); | |
| throw error; | |
| } finally { | |
| // Clean up archive even if error occurs | |
| if (fs.existsSync(archivePath)) { | |
| try { | |
| fs.unlinkSync(archivePath); | |
| } catch (cleanupError) { | |
| logger.warn('Failed to clean up archive file:', (cleanupError as Error).message); | |
| } | |
| } |
| if (platform === 'darwin') { | ||
| if (arch === 'arm64') { | ||
| assetName = `ckb-tui-with-node-macos-aarch64.tar.gz`; | ||
| } else { | ||
| throw new Error(`Unsupported architecture for macOS: ${arch}`); | ||
| } |
There was a problem hiding this comment.
macOS x64 architecture is not supported, but it's a valid and commonly used platform. Consider adding support for macOS x64 systems by including the appropriate asset name (similar to how Linux x64 is supported).
src/tools/ckb-tui.ts
Outdated
|
|
||
| logger.info('ckb-tui installed successfully.'); | ||
| } catch (error) { | ||
| logger.error('Failed to download/install ckb-tui:', (error as Error).message); |
There was a problem hiding this comment.
The error message logs (error as Error).message but doesn't provide context about what might have gone wrong. Consider including more specific guidance, such as checking network connectivity, verifying the version exists in the releases, or checking file system permissions.
| logger.error('Failed to download/install ckb-tui:', (error as Error).message); | |
| logger.error( | |
| 'Failed to download/install ckb-tui:', | |
| (error as Error).message, | |
| '\nPlease check your network connectivity, verify that the specified version exists in the releases, and ensure you have sufficient file system permissions.' | |
| ); |
src/cli.ts
Outdated
| .description('Show ckb-tui status interface') | ||
| .option('--network <network>', 'Specify the network to deploy to', 'devnet') | ||
| .action(async (option) => { | ||
| status({ network: option.network }); |
There was a problem hiding this comment.
The status function is called without await, but it's declared as async in the status.ts file. This means errors thrown in the function won't be caught by this action handler. Add await to properly handle the async operation.
| status({ network: option.network }); | |
| return status({ network: option.network }); |
| program | ||
| .command('status') | ||
| .description('Show ckb-tui status interface') | ||
| .option('--network <network>', 'Specify the network to deploy to', 'devnet') |
There was a problem hiding this comment.
The network option accepts any string value without validation. If a user provides an invalid network name (e.g., "production" or "dev"), it would be passed through without error checking. Consider validating that the network value is one of the valid Network enum values ('devnet', 'testnet', 'mainnet') and providing a helpful error message if it's not.
src/tools/ckb-tui.ts
Outdated
| const command = `"${binaryPath}" ${args.join(' ')}`; | ||
| return spawnSync(command, { stdio: 'inherit', shell: true }); |
There was a problem hiding this comment.
The run method constructs a shell command by joining args without sanitization, which could allow command injection if any argument contains shell metacharacters (e.g., ;, |, &, etc.). Consider using spawnSync with the arguments array directly instead of constructing a shell command string: spawnSync(binaryPath, args, { stdio: 'inherit' }).
| const command = `"${binaryPath}" ${args.join(' ')}`; | |
| return spawnSync(command, { stdio: 'inherit', shell: true }); | |
| return spawnSync(binaryPath, args, { stdio: 'inherit' }); |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…error handling (#342) * Initial plan * Address PR review comments - fix security, correctness, and style issues Co-authored-by: RetricSu <23436060+RetricSu@users.noreply.github.com> * Remove TODO comment and use Network enum for validation Co-authored-by: RetricSu <23436060+RetricSu@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: RetricSu <23436060+RetricSu@users.noreply.github.com>
This PR adds a new cmd
status. Once you start the CKB Node withoffckb nodeon your local computer, you can useoffckb status --network devnet/testnet/mainnetto start a CKB-TUI interface to monitor the CKB network from your node.