-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: include doctor command
#3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce a new CLI health check ("doctor") command, restructure command logic into modular functions, and add comprehensive unit tests for CLI commands and utility functions. New utility, API client, and type definition modules are created. Documentation is updated to explain configuration via environment variables. Additional dependencies are included for HTTP requests and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Utils
participant APIClient
participant API
User->>CLI: Run "doctor" command
CLI->>APIClient: getAPIDetails()
APIClient->>Utils: getConfig()
Utils-->>APIClient: Config object
APIClient->>API: GET /api/v1/__health
API-->>APIClient: APIHealthResponse
APIClient-->>CLI: APIHealthResponse
CLI->>Utils: isApiAvailable(), isApiCompatible()
Utils-->>CLI: Boolean results
CLI->>Utils: handleCommandResult(CommandResult)
Utils-->>User: Output messages / errors
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/api-client.ts (1)
12-19: Enhance error handling and type safety.The function handles the happy path well, but could benefit from more specific error handling and safer type assertions.
export const getAPIDetails = async (): Promise<APIHealthResponse> => { const client = apiClient() - const response = await client.get('__health', { responseType: 'json' }) - if (response.statusCode !== 200) { - throw new Error('Failed to get the data from the API') - } - return response.body as APIHealthResponse + try { + const response = await client.get('__health', { responseType: 'json' }) + if (response.statusCode !== 200) { + throw new Error(`API health check failed with status ${response.statusCode}`) + } + + const body = response.body as APIHealthResponse + if (!body || typeof body !== 'object' || !body.status) { + throw new Error('Invalid response format from API health endpoint') + } + + return body + } catch (error) { + if (error instanceof Error) { + throw new Error(`Failed to get API health details: ${error.message}`) + } + throw new Error('Failed to get API health details: Unknown error') + } }src/cli-commands.ts (1)
29-32: Consider more specific error handling.The catch block uses a generic "API is not available" message for all errors. Consider distinguishing between different error types (network errors, timeout, invalid response format, etc.) to provide more helpful diagnostic information to users.
} catch (error) { - messages.push('❌ Seems like the API is not available') + if (error.code === 'ECONNREFUSED' || error.code === 'ENOTFOUND') { + messages.push('❌ Cannot connect to the API - check if the service is running') + } else if (error.message?.includes('timeout')) { + messages.push('❌ API request timed out - the service may be overloaded') + } else { + messages.push(`❌ API health check failed: ${error.message || 'Unknown error'}`) + } success = false }src/utils.ts (1)
10-12: Consider more robust file path resolution.The relative path
../package.jsonassumes a specific directory structure. Consider using a more robust approach to locate the package.json file.export const getPackageJson = () => { - return JSON.parse(readFileSync(join(__dirname, '../package.json'), 'utf8')) + let currentDir = __dirname + while (currentDir !== dirname(currentDir)) { + const packagePath = join(currentDir, 'package.json') + try { + return JSON.parse(readFileSync(packagePath, 'utf8')) + } catch (error) { + currentDir = dirname(currentDir) + } + } + throw new Error('package.json not found') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
README.md(1 hunks)package.json(2 hunks)src/__tests__/cli-commands.test.ts(1 hunks)src/__tests__/utils.test.ts(1 hunks)src/api-client.ts(1 hunks)src/cli-commands.ts(1 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/api-client.ts (2)
src/utils.ts (1)
getConfig(14-27)src/types.ts (1)
APIHealthResponse(8-13)
src/__tests__/utils.test.ts (2)
src/types.ts (1)
APIHealthResponse(8-13)src/utils.ts (3)
getConfig(14-27)isApiCompatible(29-32)isApiAvailable(34-36)
src/cli-commands.ts (3)
src/utils.ts (3)
getPackageJson(10-12)isApiAvailable(34-36)isApiCompatible(29-32)src/types.ts (1)
CommandResult(22-25)src/api-client.ts (1)
getAPIDetails(12-19)
src/utils.ts (1)
src/types.ts (3)
Config(18-20)APIHealthResponse(8-13)CommandResult(22-25)
🪛 Biome (1.9.4)
src/__tests__/utils.test.ts
[error] 10-10: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 23-23: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test on windows-latest with Node 18.x
- GitHub Check: Test on windows-latest with Node 24.x
- GitHub Check: Test on windows-latest with Node 22.x
- GitHub Check: Test on windows-latest with Node 20.x
🔇 Additional comments (15)
README.md (1)
53-74: Excellent documentation of the new configuration system.The Configuration section clearly explains the environment variable usage with practical examples. The emphasis on requiring a protocol is important for user guidance and aligns well with the validation logic in the code.
package.json (2)
31-31: Verify the TypeScript definitions version compatibility.The
@types/validatorversion should be compatible with thevalidatorpackage version to ensure proper type safety.#!/bin/bash # Check if @types/validator@13.15.2 is compatible with validator@13.15.15 npm view @types/validator@13.15.2 | grep -E "(version|description)" npm view validator@13.15.15 | grep -E "(version|description)"
41-44: Verify security advisories for new dependencies.Ensure the specific versions of the new dependencies are secure and free from known vulnerabilities.
#!/bin/bash # Check for security advisories for the new dependencies npm audit --json | jq '.vulnerabilities | keys[]' 2>/dev/null || echo "No vulnerabilities found" # Check specific versions for security advisories for package in "got@14.4.7" "validator@13.15.15" "nock@14.0.5"; do echo "Checking $package..." npm view "$package" | grep -E "(deprecated|security|vulnerabilities)" || echo "No security issues found for $package" donesrc/api-client.ts (1)
5-10: Clean API client implementation.The client factory pattern is well-implemented with proper configuration integration and URL construction.
src/index.ts (3)
5-6: Good separation of concerns with modular imports.The refactoring to extract command logic into separate modules improves maintainability and testability.
14-14: Clean refactoring of version command.The version command now uses the centralized
getVersionfunction and result handling, which improves consistency.
16-23: Well-implemented doctor command with good UX.The doctor command provides immediate feedback to users with the "Checking API availability..." message and properly handles the async operation.
src/__tests__/utils.test.ts (2)
27-49: Excellent test coverage for getConfig function.The tests comprehensively cover default behavior, environment variable reading, and error handling for invalid URLs. The test cases are well-structured and thorough.
51-72: Comprehensive test coverage for API compatibility and availability functions.The tests properly verify both the positive and negative cases for API compatibility and availability checks using well-structured mock data.
src/types.ts (1)
1-26: LGTM! Well-structured type definitions.The TypeScript interfaces are clean, properly exported, and provide good type safety for the CLI application. The
APIHealthResponse,Config, andCommandResultinterfaces are appropriately designed with sensible field types.src/__tests__/cli-commands.test.ts (1)
1-92: Excellent test coverage for CLI commands.The test suite comprehensively covers both
getVersionandrunDoctorfunctions with proper HTTP mocking usingnock. All major scenarios are tested including success cases, connection errors, internal API errors, and version incompatibility. The test structure follows Jest best practices.src/cli-commands.ts (1)
7-12: LGTM! Clean version command implementation.The
getVersionfunction correctly reads the package.json and formats the version information appropriately.src/utils.ts (3)
14-27: LGTM! Proper URL validation and configuration handling.The
getConfigfunction correctly validates the environment variable URL with appropriate options (require_tld: falsefor localhost,require_protocol: truefor security) and provides a sensible default.
38-45: LGTM! Proper command result handling.The
handleCommandResultfunction correctly handles success/failure scenarios by using appropriate output streams (stdout for success, stderr for errors) and proper process exit codes.
29-32: Address the hardcoded version check technical debt.The TODO comment indicates this hardcoded version comparison should be replaced with semantic versioning. This creates a maintenance burden where the CLI must be updated for every API version change.
Consider implementing proper semantic version comparison to make the CLI more resilient to API updates. Would you like me to help implement semantic versioning support or create an issue to track this technical debt?
What are the best practices for semantic version comparison in TypeScript/Node.js applications?
Related #1
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Refactor