feat use zod for config parsing #1072
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive refactoring of the Unraid API's command-line interface (CLI), transitioning from a custom implementation to the NestJS Commander library. The changes involve removing multiple existing command files and replacing them with a new, more structured approach to CLI commands. Key additions include new commands for key management, reporting, environment switching, and other API-related operations. The project also updates configuration handling, introduces new logging services, and modifies the overall CLI architecture to provide more robust and flexible command-line interactions. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant APIKeyService
participant ReportGenerator
User->>CLI: Run command (e.g., key create)
CLI->>APIKeyService: Validate and create key
APIKeyService-->>CLI: Return key details
CLI->>User: Display key information
User->>CLI: Run report command
CLI->>ReportGenerator: Generate system report
ReportGenerator-->>CLI: Collect system data
CLI->>User: Display report details
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
elibosley
changed the base branch from
main
to
feat/add-cli-command-for-api-keys
elibosley
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 13
🧹 Nitpick comments (24)
api/src/core/utils/files/config-file-normalizer.ts (1)
43-43: Avoid usingas anyto maintain type safetyCasting the result to
anyinreturn schema.parse(mergedConfig) as anyundermines TypeScript's type safety. Consider refining the types or leveraging generic type parameters to ensure strong typing without resorting toany.You might adjust the return statement to maintain type safety:
-return schema.parse(mergedConfig) as any; // Narrowing ensures correct typing +return schema.parse(mergedConfig) as T extends 'memory' ? MyServersConfigMemory : MyServersConfig;api/src/unraid-api/cli/validate-token.command.ts (1)
76-77: Nitpick: Improve error message clarityConsider rephrasing the error message for better clarity and readability:
this.logger.error( - createJsonErrorString('Username on token does not match logged in user name') + createJsonErrorString('Username in token does not match logged-in user name') );api/src/unraid-api/cli/switch-env.command.ts (2)
83-86: Ensure consistent usage of the logger instance.In lines 83 and 86, you are using
cliLoggerinstead ofthis.logger, whereas the rest of the class usesthis.loggerfor logging. For consistency and to adhere to best practices, consider usingthis.loggerthroughout the class.Apply this diff to make the change:
- cliLogger.debug('Copying %s to %s', source, destination); + this.logger.debug('Copying %s to %s', source, destination); ... - cliLogger.info('Now using %s', newEnv); + this.logger.info('Now using %s', newEnv);
77-85: Add error handling for filesystem operations.The
writeFileandcopyFileoperations don't have error handling. If these operations fail, it could lead to unexpected behavior without clear feedback. Consider wrapping them in try-catch blocks to handle potential errors appropriately.Example implementation:
try { await writeFile(envFlashFilePath, newEnvLine); } catch (error) { this.logger.error('Failed to write to %s: %s', envFlashFilePath, error.message); throw error; } try { await copyFile(source, destination); } catch (error) { this.logger.error('Failed to copy file from %s to %s: %s', source, destination, error.message); throw error; }api/src/unraid-api/cli/key.command.ts (3)
69-69: Replace console.log with the logger for consistent logging.Using
console.login production code is discouraged. Replace it withthis.logger.logto maintain consistent logging practices.Apply this diff:
- console.log(options, passedParams); + this.logger.debug('Options: %o, Params: %o', options, passedParams);
84-88: Adjust logic considering unimplemented permissions feature.Since permissions are not yet implemented, checking for
options.permissionsmay cause confusion. Adjust the error message and logic to reflect the current state.Apply this diff to focus on roles:
- if (options.roles?.length === 0 && options.permissions?.length === 0) { + if (!options.roles || options.roles.length === 0) { this.logger.error( - 'Please add at least one role or permission with --roles or --permissions' + 'Please specify at least one role with --roles' ); return; }
64-66: Handle unimplemented method without throwing an error.The
parsePermissionsmethod throws an error indicating it's a stub. Throwing errors in option parsers can lead to unexpected behavior in the CLI. Instead, consider logging a warning or info message and returning a default value.Apply this diff to prevent the method from throwing an error:
- throw new Error('Stub Method Until Permissions PR is merged'); + this.logger.warn('Permissions feature is not yet implemented.'); + return [];api/src/unraid-api/cli/report.command.ts (3)
237-237: Replacevoidwithundefinedin the return type for clarity.Using
voidin a union type can be confusing and may not behave as expected. Replacevoidwithundefinedto make the return type explicit.Apply this diff:
- async report(options?: ReportOptions): Promise<string | ReportObject | void> { + async report(options?: ReportOptions): Promise<string | ReportObject | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 237-237: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
364-370: Ensure consistent error handling without usingconsole.log.Using
console.logfor error handling is inconsistent with the rest of the code, which uses the logger. Replaceconsole.logwiththis.logger.errororthis.logger.debugas appropriate.Apply this diff:
- console.log({ error }); + this.logger.debug('Error generating report:', error); if (error instanceof Error) { - this.logger.debug(error); + this.logger.debug(error.stack); this.logger.error(`\nFailed generating report with "${error.message}"\n`); return; }
238-248: Simplify TTY check and adjust logic for raw output.The logic for determining
isInteractivecan be simplified. Ensure that therawOutputflag correctly influences the output format.Consider revising this block:
const isInteractive = process.stdout.isTTY && !rawOutput;Ensure that all cases where
rawOutputaffects the output are consistently handled throughout the method.api/src/unraid-api/cli/version.command.ts (2)
6-7: Add command description for better CLI help output.The command decorator is missing a description that would help users understand its purpose.
-@Command({ name: 'version' }) +@Command({ name: 'version', description: 'Display the current API version' })
11-13: Add error handling for undefined API_VERSION.The implementation should handle cases where API_VERSION might be undefined.
async run(): Promise<void> { + if (!API_VERSION) { + this.logger.error('API version is not defined'); + return; + } this.logger.info(`Unraid API v${API_VERSION}`); }api/src/unraid-api/cli/report.spec.ts (1)
7-10: Consider using beforeEach for better test isolation.Using
beforeAllfor store configuration might cause test interdependence. Consider usingbeforeEachfor better isolation.-beforeAll(async () => { +beforeEach(async () => { // Load cfg into store await store.dispatch(loadConfigFile()); });api/src/unraid-api/cli/stop.command.ts (1)
7-9: Add command description for better CLI help output.The command decorator is missing a description that would help users understand its purpose.
@Command({ name: 'stop', + description: 'Stop the unraid-api service', })api/src/unraid-api/cli/log.service.ts (2)
27-29: Consider adding type safety to debug parametersThe
debugmethod acceptsanytype parameters which reduces type safety. Consider using a more specific type or generic constraint.- debug(message: any, ...optionalParams: any[]): void { + debug<T>(message: T, ...optionalParams: unknown[]): void { this.logger.debug(message, ...optionalParams); }
5-29: Consider enhancing the logging implementationThe current implementation is a simple wrapper around console methods. Consider these improvements:
- Add timestamp prefixes to log messages
- Add color support for different log levels using a library like
chalk- Add log level configuration
- Consider using Winston or Pino instead of console for production logging
Would you like me to provide an enhanced implementation with these features?
api/src/unraid-api/cli/restart.command.ts (1)
1-16: Clean up imports and add missing dependenciesRemove empty lines between imports and add missing dependencies.
import { execSync } from 'child_process'; import { join } from 'path'; - - - import { Command, CommandRunner } from 'nest-commander'; - - - import { ECOSYSTEM_PATH, PM2_PATH } from '@app/consts'; +import { LogService } from '@app/unraid-api/cli/log.service'; +import { execa } from 'execa';api/src/unraid-api/cli/logs.command.ts (1)
23-35: Add process termination handlingThe command should handle process termination gracefully.
async run(_: string[], options?: LogsOptions): Promise<void> { const lines = options?.lines ?? 100; const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); + + // Handle process termination + const cleanup = () => { + subprocess.kill(); + }; + process.on('SIGINT', cleanup); + process.on('SIGTERM', cleanup); + subprocess.stdout?.on('data', (data) => { this.logger.log(data.toString()); }); subprocess.stderr?.on('data', (data) => { this.logger.error(data.toString()); }); - await subprocess; + try { + await subprocess; + } finally { + process.off('SIGINT', cleanup); + process.off('SIGTERM', cleanup); + } }api/src/unraid-api/cli/start.command.ts (1)
39-47: Simplify type casting in parseLogLevel method.The current implementation has unnecessary type casting.
Apply this diff to simplify the type casting:
@Option({ flags: `--log-level <${levels.join('|')}>`, description: 'log level to use', }) parseLogLevel(val: string): typeof levels { - return (levels.includes(val as (typeof levels)[number]) - ? (val as (typeof levels)[number]) - : 'info') as unknown as typeof levels; + return levels.includes(val as (typeof levels)[number]) + ? val as (typeof levels)[number] + : 'info'; }api/src/types/my-servers-config.ts (1)
15-29: Enhance validation in RemoteConfigSchema.The current schema could benefit from stronger validation rules for specific fields:
wanportshould be validated as a valid port numberupnpEnabledshould be a boolean instead of stringApply this diff to enhance validation:
const RemoteConfigSchema = z.object({ wanaccess: z.string(), - wanport: z.string(), + wanport: z.string().regex(/^\d+$/).transform(Number) + .refine(port => port >= 1 && port <= 65535, { + message: 'Port must be between 1 and 65535' + }), upnpEnabled: z.string(), apikey: z.string(), localApiKey: z.string(), - email: z.string(), + email: z.string().email(), username: z.string(), avatar: z.string(), regWizTime: z.string(), accesstoken: z.string(), idtoken: z.string(), refreshtoken: z.string(), dynamicRemoteAccessType: z.nativeEnum(DynamicRemoteAccessType), });api/src/unraid-api/rest/rest.service.ts (1)
18-27: Improve error handling in saveApiReport.The current implementation catches all errors and logs them generically. It would be better to handle specific error types differently.
Apply this diff to improve error handling:
async saveApiReport(pathToReport: string) { try { const reportCommand = new ReportCommand(new LogService()); const apiReport = await reportCommand.report({ json: true, verbose: 2, raw: false }); this.logger.debug('Report object %o', apiReport); await writeFile(pathToReport, JSON.stringify(apiReport, null, 2), 'utf-8'); } catch (error) { - this.logger.warn('Could not generate report for zip with error %o', error); + if (error instanceof Error) { + this.logger.warn('Could not generate report: %s', error.message); + throw error; + } + this.logger.warn('Could not generate report due to unknown error: %o', error); + throw new Error('Failed to generate report'); } }api/README.md (3)
46-66: Add language specifiers to code blocks.Add 'bash' as the language specifier to the code blocks for better syntax highlighting and markdown compliance.
-``` +```bash unraid-api key get --name "my-app-key"Apply similar changes to all code blocks in this section. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 46-46: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 52-52: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 58-58: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 64-64: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `70-71`: **Fix grammar in the mothership connection description.** Add the missing article "the" before "mothership". ```diff -To view the current status of the unraid-api and its connection to mothership, run: +To view the current status of the unraid-api and its connection to the mothership, run:🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: You might be missing the article “the” here.
Context: ...of the unraid-api and its connection to mothership, run:unraid-api reportTo vi...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
89-90: Fix punctuation in the Secrets section.Add missing commas for better readability.
-If you found this file you're likely a developer. If you'd like to know more about the API and when it's available please join [our discord](https://discord.unraid.net/). +If you found this file, you're likely a developer. If you'd like to know more about the API and when it's available, please join [our discord](https://discord.unraid.net/).🧰 Tools
🪛 LanguageTool
[uncategorized] ~90-~90: Possible missing comma found.
Context: ... -vv ``` ## Secrets If you found this file you're likely a developer. If you'd lik...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~90-~90: It seems that a comma is missing.
Context: ...re about the API and when it's available please join [our discord](https://discord.unra...(IF_PLEASE_COMMA)
🛑 Comments failed to post (13)
api/src/cli.ts (1)
12-19:
⚠️ Potential issueTrim trailing newline from
shellToUseto prevent potential errorsThe output of
execSync('which bash')may contain a trailing newline character. If not trimmed, this can lead to issues whenexecutablePathis used, as the path might not be recognized due to the newline. It's advisable to trim the output before using it.Apply this diff to fix the issue:
const shellToUse = execSync('which bash'); +const shellPath = shellToUse.toString('utf-8').trim(); await CommandFactory.run(CliModule, { cliName: 'unraid-api', logger: false, completion: { fig: true, cmd: 'unraid-api', - nativeShell: { executablePath: shellToUse.toString('utf-8') }, + nativeShell: { executablePath: shellPath }, }, });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const shellToUse = execSync('which bash'); const shellPath = shellToUse.toString('utf-8').trim(); await CommandFactory.run(CliModule, { cliName: 'unraid-api', logger: false, completion: { fig: true, cmd: 'unraid-api', nativeShell: { executablePath: shellPath },api/src/unraid-api/cli/validate-token.command.ts (2)
65-65:
⚠️ Potential issueEnsure token payload contains
usernamefieldWhen extracting the
usernamefromtokenPayload, it's possible that neitherusernamenorcognito:usernameproperties exist. Add a check to handle this scenario and provide an appropriate error message if theusernamecannot be retrieved.Apply this diff to add the necessary check:
const username = tokenPayload!.username ?? tokenPayload!['cognito:username']; +if (!username) { + this.logger.error(createJsonErrorString('Username not found in token payload')); + process.exit(1); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const username = tokenPayload!.username ?? tokenPayload!['cognito:username']; if (!username) { this.logger.error(createJsonErrorString('Username not found in token payload')); process.exit(1); }
71-79:
⚠️ Potential issueHandle potential exceptions when decoding existing user token
The
decodeJwtfunction may encounter issues ifconfigFile.remote?.accesstokenis undefined or invalid, leading to unexpected behavior. Ensure thataccesstokenexists before decoding and verify thatexistingUserPayload.usernameis present before proceeding with the comparison.Apply this diff to add necessary checks:
const existingUserPayload = configFile.remote?.accesstoken + ? decodeJwt(configFile.remote.accesstoken) + : null; +if (!existingUserPayload || !existingUserPayload.username) { + this.logger.error(createJsonErrorString('Failed to decode existing user token or username not found')); + process.exit(1); +} if (username === existingUserPayload.username) { this.logger.info(JSON.stringify({ error: null, valid: true })); } else { this.logger.error( - createJsonErrorString('Username on token does not match logged in user name') + createJsonErrorString('Username in token does not match logged-in user name') ); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const existingUserPayload = configFile.remote?.accesstoken ? decodeJwt(configFile.remote.accesstoken) : null; if (!existingUserPayload || !existingUserPayload.username) { this.logger.error(createJsonErrorString('Failed to decode existing user token or username not found')); process.exit(1); } if (username === existingUserPayload.username) { this.logger.info(JSON.stringify({ error: null, valid: true })); } else { this.logger.error( createJsonErrorString('Username in token does not match logged-in user name') ); } }api/src/unraid-api/cli/switch-env.command.ts (1)
46-47:
⚠️ Potential issueHandle undefined values when parsing the environment file.
In the
getEnvironmentFromFilemethod,currentEnvInFilemight beundefinedif the regex does not match. PassingundefinedtoparseStringToEnvcan cause unexpected results. Consider providing a default value or adjusting the method to handleundefined.Apply this diff to ensure
currentEnvInFileis a string:const [, , currentEnvInFile] = matchArray && matchArray.length === 3 ? matchArray : []; - return this.parseStringToEnv(currentEnvInFile); + return this.parseStringToEnv(currentEnvInFile ?? '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const [, , currentEnvInFile] = matchArray && matchArray.length === 3 ? matchArray : []; return this.parseStringToEnv(currentEnvInFile ?? '');api/src/unraid-api/cli/key.command.ts (2)
71-71: 🛠️ Refactor suggestion
Inject
ApiKeyServicevia dependency injection instead of direct instantiation.Directly instantiating services bypasses the benefits of NestJS's dependency injection system, such as lifecycle management and testability. Inject
ApiKeyServicein the constructor instead.Modify the constructor to include
ApiKeyService:constructor( private readonly apiKeyService: ApiKeyService ) { super(); }Update line 71 to use the injected service:
- const apiKeyService = new ApiKeyService();Don't forget to register
ApiKeyServiceas a provider in your module.
99-100: 🛠️ Refactor suggestion
Avoid using
process.exit(1)in the code.Calling
process.exit(1)can terminate the Node.js process abruptly, which might prevent proper cleanup. Consider throwing an error or allowing the process to complete gracefully.Apply this diff:
- process.exit(1); + throw new Error('No Key Found');Ensure that any errors thrown are appropriately caught and handled at a higher level if necessary.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.throw new Error('No Key Found'); }api/src/unraid-api/cli/status.command.ts (1)
9-12:
⚠️ Potential issueReplace blocking execSync with async execution and add error handling.
The current implementation has several issues:
execSyncblocks the event loop and can freeze the application- Forced
process.exit(0)bypasses proper cleanup- Missing error handling for PM2 command failures
Consider this safer implementation:
- async run(): Promise<void> { - execSync(`${PM2_PATH} status unraid-api`, { stdio: 'inherit' }); - process.exit(0); - } + async run(): Promise<void> { + try { + const { execa } = await import('execa'); + const { stdout, stderr } = await execa(PM2_PATH, ['status', 'unraid-api']); + if (stderr) { + console.error(stderr); + return; + } + console.log(stdout); + } catch (error) { + console.error('Failed to check service status:', error.message); + throw error; + } + }Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/cli/report.spec.ts (1)
12-18: 🛠️ Refactor suggestion
Add more test cases for edge scenarios.
The current tests cover basic scenarios but miss important edge cases.
Consider adding these test cases:
test('anonymise origins handles empty array', () => { expect(anonymiseOrigins([])).toEqual([]); }); test('anonymise origins handles invalid URLs', () => { expect(anonymiseOrigins(['invalid-url'])).toEqual(['invalid-url']); }); test('anonymise origins handles multiple URLs', () => { expect(anonymiseOrigins([ 'https://domain1.tld:8443', 'https://domain2.tld:9443' ])).toEqual([ 'https://domain1.tld:WANPORT', 'https://domain2.tld:WANPORT' ]); });api/src/unraid-api/cli/stop.command.ts (1)
14-22: 🛠️ Refactor suggestion
Improve error handling and add timeout protection.
The current implementation has inconsistent error handling and no timeout protection.
Consider this improved implementation:
- async run() { - const { stderr, stdout } = await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]); - if (stdout) { - this.logger.info(stdout); - } else if (stderr) { - this.logger.warn(stderr); - process.exit(1); - } - } + async run() { + try { + const { stderr, stdout } = await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH], { + timeout: 30000, // 30 seconds timeout + }); + + if (stderr) { + this.logger.warn(stderr); + } + + if (stdout) { + this.logger.info(stdout); + } + + // Verify the service is actually stopped + const { stdout: statusOut } = await execa(PM2_PATH, ['status', 'unraid-api']); + if (statusOut.includes('online')) { + throw new Error('Service failed to stop'); + } + } catch (error) { + this.logger.error(`Failed to stop service: ${error.message}`); + throw error; + } + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async run() { try { const { stderr, stdout } = await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH], { timeout: 30000, // 30 seconds timeout }); if (stderr) { this.logger.warn(stderr); } if (stdout) { this.logger.info(stdout); } // Verify the service is actually stopped const { stdout: statusOut } = await execa(PM2_PATH, ['status', 'unraid-api']); if (statusOut.includes('online')) { throw new Error('Service failed to stop'); } } catch (error) { this.logger.error(`Failed to stop service: ${error.message}`); throw error; } }api/src/unraid-api/cli/restart.command.ts (1)
21-36:
⚠️ Potential issueReplace execSync with async execution and add error handling
Using
execSyncblocks the Node.js event loop. Consider these critical improvements:
- Use async execution
- Add proper error handling
- Use the injected LogService instead of console.log
+ constructor(private readonly logger: LogService) { + super(); + } + async run(_): Promise<void> { - console.log( - 'Dirname is ', - import.meta.dirname, - ' command is ', - `${PM2_PATH} restart ${ECOSYSTEM_PATH} --update-env` - ); - execSync( - `${PM2_PATH} restart ${ECOSYSTEM_PATH} --update-env`, - { - env: process.env, - stdio: 'pipe', - cwd: process.cwd(), - } - ); + try { + this.logger.info(`Executing: ${PM2_PATH} restart ${ECOSYSTEM_PATH} --update-env`); + const subprocess = execa( + PM2_PATH, + ['restart', ECOSYSTEM_PATH, '--update-env'], + { + env: process.env, + cwd: process.cwd(), + } + ); + + subprocess.stdout?.pipe(process.stdout); + subprocess.stderr?.pipe(process.stderr); + + await subprocess; + } catch (error) { + this.logger.error(`Failed to restart: ${error.message}`); + throw error; + } }Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/cli/logs.command.ts (1)
17-21: 🛠️ Refactor suggestion
Improve input validation in parseLines method
The current implementation has several issues:
- Uses unsafe
isNaNinstead ofNumber.isNaN- Doesn't validate for negative numbers
- Doesn't validate maximum line limit
@Option({ flags: '-l, --lines', description: 'Number of lines to tail'}) parseLines(input: string): number { - return isNaN(parseInt(input)) ? 100 : parseInt(input) + const lines = Number(input); + if (Number.isNaN(lines) || lines < 1) { + return 100; + } + return Math.min(lines, 10000); // Add reasonable maximum limit }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.@Option({ flags: '-l, --lines', description: 'Number of lines to tail'}) parseLines(input: string): number { const lines = Number(input); if (Number.isNaN(lines) || lines < 1) { return 100; } return Math.min(lines, 10000); // Add reasonable maximum limit }🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
api/src/unraid-api/cli/start.command.ts (1)
21-37:
⚠️ Potential issueImprove error handling in the run method.
The current implementation has a few issues:
- The process exits immediately after stdout, potentially missing stderr.
- The error handling could be more robust.
Apply this diff to improve error handling:
async run(_, options: StartCommandOptions): Promise<void> { this.logger.info('Starting the Unraid API'); const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : ''; - const { stderr, stdout } = await execa( - `${envLog} ${PM2_PATH}`.trim(), - ['start', ECOSYSTEM_PATH, '--update-env'], - { stdio: 'inherit' } - ); - if (stdout) { - this.logger.log(stdout); - process.exit(0); - } - if (stderr) { - this.logger.error(stderr); - process.exit(1); - } + try { + const { stderr, stdout } = await execa( + `${envLog} ${PM2_PATH}`.trim(), + ['start', ECOSYSTEM_PATH, '--update-env'], + { stdio: 'inherit' } + ); + if (stderr) { + this.logger.error(stderr); + process.exit(1); + } + if (stdout) { + this.logger.log(stdout); + } + process.exit(0); + } catch (error) { + this.logger.error('Failed to start Unraid API:', error); + process.exit(1); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async run(_, options: StartCommandOptions): Promise<void> { this.logger.info('Starting the Unraid API'); const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : ''; try { const { stderr, stdout } = await execa( `${envLog} ${PM2_PATH}`.trim(), ['start', ECOSYSTEM_PATH, '--update-env'], { stdio: 'inherit' } ); if (stderr) { this.logger.error(stderr); process.exit(1); } if (stdout) { this.logger.log(stdout); } process.exit(0); } catch (error) { this.logger.error('Failed to start Unraid API:', error); process.exit(1); } }api/src/types/my-servers-config.ts (1)
58-60: 🛠️ Refactor suggestion
Remove redundant connectionStatus field.
The
MyServersConfigMemoryWithMandatoryHiddenFieldsSchemaextendsMyServersConfigMemorySchemabut redundantly includes the sameconnectionStatusfield that's already present in the parent schema.Apply this diff to remove the redundancy:
export const MyServersConfigMemoryWithMandatoryHiddenFieldsSchema = MyServersConfigMemorySchema.extend({ - connectionStatus: ConnectionStatusSchema, });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const MyServersConfigMemoryWithMandatoryHiddenFieldsSchema = MyServersConfigMemorySchema.extend({ });
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
merged commit e6e3c53
into
feat/add-cli-command-for-api-keys
Summary by CodeRabbit
Here are the release notes for the pull request:
New Features
Improvements
Changes
Documentation