Feat/hypervisor async instantiation #1076
Conversation
WalkthroughThis pull request introduces several architectural and code organization improvements across the Unraid API codebase. The changes primarily focus on enhancing modularity, improving import strategies, and refactoring hypervisor management. Key modifications include implementing a singleton Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![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.
🛑 Comments failed to post (36)
api/src/unraid-api/cli/status.command.ts (1)
9-12:
⚠️ Potential issueAdd error handling and remove forced exit.
The current implementation has several issues:
- No error handling for PM2 command failures
- Forcing
process.exit(0)bypasses proper cleanup- Missing execution status logging
Consider this implementation:
- async run(): Promise<void> { - execSync(`${PM2_PATH} status unraid-api`, { stdio: 'inherit' }); - process.exit(0); + async run(): Promise<void> { + try { + return new Promise((resolve, reject) => { + exec(`${PM2_PATH} status unraid-api`, { stdio: 'inherit' }, (error, stdout, stderr) => { + if (error) { + console.error(`Failed to check status: ${error.message}`); + reject(error); + return; + } + if (stderr) { + console.error(`Status check stderr: ${stderr}`); + } + resolve(); + }); + }); + } catch (error) { + console.error('Failed to execute status command:', error); + 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(): Promise<void> { try { return new Promise((resolve, reject) => { exec(`${PM2_PATH} status unraid-api`, { stdio: 'inherit' }, (error, stdout, stderr) => { if (error) { console.error(`Failed to check status: ${error.message}`); reject(error); return; } if (stderr) { console.error(`Status check stderr: ${stderr}`); } resolve(); }); }); } catch (error) { console.error('Failed to execute status command:', error); throw error; } }api/src/unraid-api/cli/start.command.ts (2)
21-25:
⚠️ Potential issueSecure the command construction against injection.
The current command construction using template literals could be vulnerable to command injection. Consider using execa's built-in environment option instead.
Replace the command construction with:
- const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : ''; - const { stderr, stdout } = await execa(`${envLog} ${PM2_PATH}`.trim(), [ + const env = options['log-level'] ? { LOG_LEVEL: options['log-level'] } : {}; + const { stderr, stdout } = await execa(PM2_PATH, [ 'start', ECOSYSTEM_PATH, '--update-env', - ]); + ], { env });📝 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 env = options['log-level'] ? { LOG_LEVEL: options['log-level'] } : {}; const { stderr, stdout } = await execa(PM2_PATH, [ 'start', ECOSYSTEM_PATH, '--update-env', ], { env });
18-34: 🛠️ Refactor suggestion
Add proper error handling and graceful shutdown.
The current implementation has several areas for improvement:
- Direct process.exit calls can prevent cleanup
- No try-catch for execa execution
- No validation of PM2_PATH and ECOSYSTEM_PATH
Consider this improved implementation:
async run(_: string[], options: StartCommandOptions): Promise<void> { + try { + if (!PM2_PATH || !ECOSYSTEM_PATH) { + throw new Error('PM2_PATH or ECOSYSTEM_PATH not configured'); + } 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(), [ + const env = options['log-level'] ? { LOG_LEVEL: options['log-level'] } : {}; + const { stderr, stdout } = await execa(PM2_PATH, [ 'start', ECOSYSTEM_PATH, '--update-env', - ]); + ], { env }); if (stdout) { this.logger.log(stdout); } if (stderr) { this.logger.error(stderr); - process.exit(1); + throw new Error(stderr); } - process.exit(0); + } catch (error) { + this.logger.error(`Failed to start Unraid API: ${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(_: string[], options: StartCommandOptions): Promise<void> { try { if (!PM2_PATH || !ECOSYSTEM_PATH) { throw new Error('PM2_PATH or ECOSYSTEM_PATH not configured'); } this.logger.info('Starting the Unraid API'); const env = options['log-level'] ? { LOG_LEVEL: options['log-level'] } : {}; const { stderr, stdout } = await execa(PM2_PATH, [ 'start', ECOSYSTEM_PATH, '--update-env', ], { env }); if (stdout) { this.logger.log(stdout); } if (stderr) { this.logger.error(stderr); throw new Error(stderr); } } catch (error) { this.logger.error(`Failed to start Unraid API: ${error.message}`); throw error; } }api/src/unraid-api/cli/stop.command.ts (1)
14-23: 🛠️ Refactor suggestion
⚠️ Potential issueSeveral critical issues need to be addressed in the run method.
- The
else ifcondition means stderr is ignored when stdout exists, which could miss important error messages.- Direct
process.exit()calls make the code harder to test and can prevent proper cleanup.- Missing error handling for the execa call.
- Missing return type annotation.
Here's a suggested refactor:
- async run() { + async run(): Promise<void> { + try { const { stderr, stdout } = await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]); - if (stdout) { - this.logger.info(stdout); - } else if (stderr) { + if (stdout) this.logger.info(stdout); + if (stderr) { this.logger.warn(stderr); - process.exit(1); + throw new Error(stderr); } - process.exit(0); + } catch (error) { + this.logger.error(`Failed to stop PM2 process: ${error.message}`); + throw error; + } }Also, consider adding these improvements:
- Add input validation for PM2_PATH and ECOSYSTEM_PATH.
- Add debug logging before executing the command.
- Consider implementing a graceful shutdown mechanism instead of throwing errors.
📝 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(): Promise<void> { try { const { stderr, stdout } = await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]); if (stdout) this.logger.info(stdout); if (stderr) { this.logger.warn(stderr); throw new Error(stderr); } } catch (error) { this.logger.error(`Failed to stop PM2 process: ${error.message}`); throw error; } }api/src/cli.ts (1)
12-14:
⚠️ Potential issuePotential bug in
execausage.
res.toString()may not yield the expected path becauseexecareturns an object with astdoutproperty; considerres.stdout.trim()instead.- .then((res) => res.toString().trim()) + .then((res) => res.stdout.trim())📝 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 = await execa('which unraid-api') .then((res) => res.stdout.trim()) .catch((_) => '/usr/local/bin/unraid-api');api/src/core/sso/sso-setup.ts (1)
65-67: 🛠️ Refactor suggestion
Final write.
Overwrites are performed with no rollback if a subsequent step fails. Consider adding error handling for partial updates or letting callers handle it.api/src/unraid-api/cli/apikey/api-key.command.ts (1)
90-119:
⚠️ Potential issueReview secret/API key logging for security.
Currently, the API key is logged in plain text (lines 94 and 115). This can lead to compromised credentials if logs are stored in unsecure locations. Consider masking or omitting the key from logs.- this.logger.log(key.key); + this.logger.log('API key created successfully. (Key omitted from logs)');📝 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(_: string[], options: KeyOptions = { create: false, name: '' }): Promise<void> { try { const key = this.apiKeyService.findByField('name', options.name); if (key) { this.logger.log('API key created successfully. (Key omitted from logs)'); } else if (options.create) { options = await this.inquirerService.prompt(AddApiKeyQuestionSet.name, options); this.logger.log('Creating API Key...' + JSON.stringify(options)); if (!options.roles && !options.permissions) { this.logger.error('Please add at least one role or permission to the key.'); return; } if (options.roles?.length === 0 && options.permissions?.length === 0) { this.logger.error('Please add at least one role or permission to the key.'); return; } const key = await this.apiKeyService.create({ name: options.name, description: options.description || `CLI generated key: ${options.name}`, roles: options.roles, permissions: options.permissions, overwrite: true, }); this.logger.log('API key created successfully. (Key omitted from logs)'); } else { this.logger.log('No Key Found'); process.exit(1); }api/src/unraid-api/cli/report.spec.ts (1)
7-10: 🛠️ Refactor suggestion
Add error handling for store configuration loading.
The
beforeAllhook should handle potential configuration loading failures to prevent silent test setup issues.beforeAll(async () => { - // Load cfg into store - await store.dispatch(loadConfigFile()); + try { + await store.dispatch(loadConfigFile()); + } catch (error) { + console.error('Failed to load configuration:', error); + 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.beforeAll(async () => { try { await store.dispatch(loadConfigFile()); } catch (error) { console.error('Failed to load configuration:', error); throw error; } });api/src/core/sso/sso-remove.ts (1)
5-20:
⚠️ Potential issueCritical improvements needed for file operations and error handling.
Several issues need to be addressed:
- Synchronous file operations could block the event loop
- Missing error handling for file operations
- Hardcoded paths should be configuration-driven
Suggested improvements:
+import { promises as fs } from 'node:fs'; + +const PATHS = { + LOGIN: '/usr/local/emhttp/plugins/dynamix/include/.login.php', + LOGIN_BACKUP: '/usr/local/emhttp/plugins/dynamix/include/.login.php.bak' +} as const; + -export const removeSso = () => { +export const removeSso = async () => { try { - const path = '/usr/local/emhttp/plugins/dynamix/include/.login.php'; - const backupPath = path + '.bak'; + const backupExists = await fs.access(PATHS.LOGIN_BACKUP) + .then(() => true) + .catch(() => false); - if (existsSync(backupPath)) { + if (backupExists) { // Remove the SSO login inject file if it exists - if (existsSync(path)) { - unlinkSync(path); + try { + await fs.unlink(PATHS.LOGIN); + } catch (error) { + if (error.code !== 'ENOENT') { + throw error; + } } - renameSync(backupPath, path); + await fs.rename(PATHS.LOGIN_BACKUP, PATHS.LOGIN); cliLogger.debug('SSO login file restored.'); } else { cliLogger.debug('No SSO login file backup found.'); } + } catch (error) { + cliLogger.error('Failed to remove SSO:', error); + 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.import { promises as fs } from 'node:fs'; const PATHS = { LOGIN: '/usr/local/emhttp/plugins/dynamix/include/.login.php', LOGIN_BACKUP: '/usr/local/emhttp/plugins/dynamix/include/.login.php.bak' } as const; export const removeSso = async () => { try { const backupExists = await fs.access(PATHS.LOGIN_BACKUP) .then(() => true) .catch(() => false); if (backupExists) { // Remove the SSO login inject file if it exists try { await fs.unlink(PATHS.LOGIN); } catch (error) { if (error.code !== 'ENOENT') { throw error; } } await fs.rename(PATHS.LOGIN_BACKUP, PATHS.LOGIN); cliLogger.debug('SSO login file restored.'); } else { cliLogger.debug('No SSO login file backup found.'); } } catch (error) { cliLogger.error('Failed to remove SSO:', error); throw error; } };api/src/unraid-api/cli/config.command.ts (1)
19-24:
⚠️ Potential issueAdd error handling and avoid direct process.exit().
The current implementation has several potential issues:
- No error handling for file read operations
- Direct process.exit() usage can prevent cleanup
- Missing validation of configuration content
Consider this safer implementation:
async run(): Promise<void> { this.logger.log('\nDisk Configuration:'); - const diskConfig = await readFile(getters.paths()['myservers-config'], 'utf8'); - this.logger.log(diskConfig); - process.exit(0); + try { + const paths = getters.paths(); + if (!paths['myservers-config']) { + throw new Error('myservers-config path not found'); + } + const diskConfig = await readFile(paths['myservers-config'], 'utf8'); + // Optional: Add JSON validation if it's a JSON file + this.logger.log(diskConfig); + return; + } catch (error) { + this.logger.error(`Failed to read configuration: ${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(): Promise<void> { this.logger.log('\nDisk Configuration:'); try { const paths = getters.paths(); if (!paths['myservers-config']) { throw new Error('myservers-config path not found'); } const diskConfig = await readFile(paths['myservers-config'], 'utf8'); // Optional: Add JSON validation if it's a JSON file this.logger.log(diskConfig); return; } catch (error) { this.logger.error(`Failed to read configuration: ${error.message}`); throw error; } }plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/sso-login.php (2)
11-12:
⚠️ Potential issueAdd validation for $docroot and error handling.
Missing validation for $docroot and error handling for required files could lead to security issues.
+if (!isset($docroot) || !is_string($docroot)) { + error_log('$docroot is not set or invalid'); + exit(1); +} + require_once("$docroot/plugins/dynamix.my.servers/include/state.php"); require_once("$docroot/plugins/dynamix.my.servers/include/web-components-extractor.php");📝 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.if (!isset($docroot) || !is_string($docroot)) { error_log('$docroot is not set or invalid'); exit(1); } require_once("$docroot/plugins/dynamix.my.servers/include/state.php"); require_once("$docroot/plugins/dynamix.my.servers/include/web-components-extractor.php");
20-22:
⚠️ Potential issuePrevent potential XSS vulnerability.
The direct echo of $serverState->ssoEnabled could be exploited if ServerState class is compromised.
<unraid-i18n-host> - <unraid-sso-button ssoenabled="<?= $serverState->ssoEnabled ? "true" : "false" ?>"></unraid-sso-button> + <unraid-sso-button ssoenabled="<?= htmlspecialchars($serverState->ssoEnabled ? "true" : "false", ENT_QUOTES, 'UTF-8') ?>"></unraid-sso-button> </unraid-i18n-host>📝 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.<unraid-i18n-host> <unraid-sso-button ssoenabled="<?= htmlspecialchars($serverState->ssoEnabled ? "true" : "false", ENT_QUOTES, 'UTF-8') ?>"></unraid-sso-button> </unraid-i18n-host>api/src/unraid-api/cli/sso/list-sso-user.command.ts (1)
22-27:
⚠️ Potential issueAdd error handling and null checks.
The current implementation lacks error handling and proper null checks which could lead to runtime errors.
- async run(_input: string[]): Promise<void> { + async run(_input: string[] = []): Promise<void> { try { await store.dispatch(loadConfigFile()); + const state = store.getState(); + const ssoSubIds = state.config?.remote?.ssoSubIds; + + if (!ssoSubIds) { + this.logger.info('No SSO users found'); + return; + } + this.logger.info( - store.getState().config.remote.ssoSubIds.split(',').filter(Boolean).join('\n') + ssoSubIds.split(',').filter(Boolean).join('\n') || 'No active SSO users' ); + } catch (error) { + this.logger.error(`Failed to list SSO users: ${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(_input: string[] = []): Promise<void> { try { await store.dispatch(loadConfigFile()); const state = store.getState(); const ssoSubIds = state.config?.remote?.ssoSubIds; if (!ssoSubIds) { this.logger.info('No SSO users found'); return; } this.logger.info( ssoSubIds.split(',').filter(Boolean).join('\n') || 'No active SSO users' ); } catch (error) { this.logger.error(`Failed to list SSO users: ${error.message}`); throw error; } }api/src/unraid-api/cli/sso/remove-sso-user.questions.ts (1)
22-26: 🛠️ Refactor suggestion
Consider more graceful error handling.
Instead of directly calling
process.exit(0), consider:
- Throwing a custom error that can be caught by the command handler
- Allowing the command runner to handle the exit gracefully
- Providing more detailed error information
- this.logger.error('No SSO Users Found'); - process.exit(0); + throw new Error('No SSO users found. Please add SSO users first.');📝 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 users = store.getState().config.remote.ssoSubIds.split(',').filter((user) => user !== ''); if (users.length === 0) { throw new Error('No SSO users found. Please add SSO users first.'); }api/src/unraid-api/cli/restart.command.ts (1)
13-36: 🛠️ Refactor suggestion
Improve error handling and process management.
Several suggestions to enhance robustness:
- Process exit calls are scattered throughout the code
- No validation of PM2_PATH and ECOSYSTEM_PATH
- Error handling could be more specific
async run(_): Promise<void> { + let exitCode = 0; + if (!PM2_PATH || !ECOSYSTEM_PATH) { + this.logger.error('PM2_PATH or ECOSYSTEM_PATH not configured'); + process.exit(1); + } try { const { stderr, stdout } = await execa(PM2_PATH, [ 'restart', ECOSYSTEM_PATH, '--update-env', ]); if (stderr) { this.logger.error(stderr); - process.exit(1); + exitCode = 1; } if (stdout) { this.logger.info(stdout); } } catch (error) { + exitCode = 1; if (error instanceof Error) { + if (error.code === 'ENOENT') { + this.logger.error(`PM2 not found at ${PM2_PATH}`); + } else { this.logger.error(error.message); + } } else { this.logger.error('Unknown error occurred'); } - process.exit(1); } - process.exit(0); + process.exit(exitCode); }📝 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(_): Promise<void> { let exitCode = 0; if (!PM2_PATH || !ECOSYSTEM_PATH) { this.logger.error('PM2_PATH or ECOSYSTEM_PATH not configured'); process.exit(1); } try { const { stderr, stdout } = await execa(PM2_PATH, [ 'restart', ECOSYSTEM_PATH, '--update-env', ]); if (stderr) { this.logger.error(stderr); exitCode = 1; } if (stdout) { this.logger.info(stdout); } } catch (error) { exitCode = 1; if (error instanceof Error) { if (error.code === 'ENOENT') { this.logger.error(`PM2 not found at ${PM2_PATH}`); } else { this.logger.error(error.message); } } else { this.logger.error('Unknown error occurred'); } } process.exit(exitCode); }api/src/unraid-api/cli/logs.command.ts (2)
23-35:
⚠️ Potential issueAdd proper subprocess and stream management.
The current implementation lacks cleanup handling and could lead to memory leaks.
async run(_: string[], options?: LogsOptions): Promise<void> { const lines = options?.lines ?? 100; + let subprocess: ExecaChildProcess; + + const cleanup = () => { + if (subprocess) { + subprocess.kill(); + } + }; + + // Handle interrupts + process.on('SIGINT', cleanup); + process.on('SIGTERM', cleanup); + try { - const subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); + subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); + // Add timeout + const timeout = setTimeout(() => { + this.logger.error('Log retrieval timed out'); + cleanup(); + }, 30000); + subprocess.stdout?.on('data', (data) => { this.logger.log(data.toString()); }); subprocess.stderr?.on('data', (data) => { this.logger.error(data.toString()); }); await subprocess; + clearTimeout(timeout); + } finally { + // Remove interrupt handlers + process.off('SIGINT', cleanup); + process.off('SIGTERM', cleanup); } }📝 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(_: string[], options?: LogsOptions): Promise<void> { const lines = options?.lines ?? 100; let subprocess: ExecaChildProcess; const cleanup = () => { if (subprocess) { subprocess.kill(); } }; // Handle interrupts process.on('SIGINT', cleanup); process.on('SIGTERM', cleanup); try { subprocess = execa(PM2_PATH, ['logs', ECOSYSTEM_PATH, '--lines', lines.toString()]); // Add timeout const timeout = setTimeout(() => { this.logger.error('Log retrieval timed out'); cleanup(); }, 30000); subprocess.stdout?.on('data', (data) => { this.logger.log(data.toString()); }); subprocess.stderr?.on('data', (data) => { this.logger.error(data.toString()); }); await subprocess; clearTimeout(timeout); } finally { // Remove interrupt handlers process.off('SIGINT', cleanup); process.off('SIGTERM', cleanup); } }
18-21: 🛠️ Refactor suggestion
Add input validation for lines parameter.
The current implementation silently defaults to 100 lines when parsing fails. Consider adding explicit validation.
@Option({ flags: '-l, --lines <lines>', description: 'Number of lines to tail', defaultValue: 100 }) parseLines(input: string): number { const parsedValue = parseInt(input); - return Number.isNaN(parsedValue) ? 100 : parsedValue; + if (Number.isNaN(parsedValue) || parsedValue <= 0) { + this.logger.warn(`Invalid line count "${input}", using default of 100`); + return 100; + } + return parsedValue; }Committable suggestion skipped: line range outside the PR's diff.
api/src/unraid-api/auth/casbin/casbin.service.ts (1)
23-25: 🛠️ Refactor suggestion
Improve LOG_LEVEL handling.
The current implementation has potential issues with case sensitivity and type safety.
+ // Ensure case-insensitive comparison and type safety + const logLevel = String(LOG_LEVEL).toUpperCase(); - if (LOG_LEVEL === 'TRACE') { + if (logLevel === 'TRACE') { enforcer.enableLog(true); }📝 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 logLevel = String(LOG_LEVEL).toUpperCase(); if (logLevel === 'TRACE') { enforcer.enableLog(true); }api/dev/states/myservers.cfg (1)
16-16:
⚠️ Potential issueEnhance security by removing hardcoded tokens.
The configuration file contains sensitive authentication tokens. Consider:
- Moving these tokens to environment variables or a secure vault
- Using placeholder values in the configuration file
- Adding the file to .gitignore if not already
Also applies to: 19-19
api/src/core/utils/files/config-file-normalizer.ts (1)
35-41: 🛠️ Refactor suggestion
Reduce type assertions.
Multiple type assertions make the code fragile and harder to maintain. Consider restructuring to avoid them.
- if (mode === 'memory') { - (mergedConfig as MyServersConfigMemory).remote.allowedOrigins = getAllowedOrigins().join(', '); - (mergedConfig as MyServersConfigMemory).connectionStatus = { - ...(defaultConfig as MyServersConfigMemory).connectionStatus, - ...(config as MyServersConfigMemory).connectionStatus, - }; - } + if (mode === 'memory') { + const memoryConfig = mergedConfig as MyServersConfigMemory; + const defaultMemoryConfig = defaultConfig as MyServersConfigMemory; + const inputMemoryConfig = config as MyServersConfigMemory; + + memoryConfig.remote.allowedOrigins = getAllowedOrigins().join(', '); + memoryConfig.connectionStatus = { + ...defaultMemoryConfig.connectionStatus, + ...inputMemoryConfig.connectionStatus, + }; + }📝 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.if (mode === 'memory') { const memoryConfig = mergedConfig as MyServersConfigMemory; const defaultMemoryConfig = defaultConfig as MyServersConfigMemory; const inputMemoryConfig = config as MyServersConfigMemory; memoryConfig.remote.allowedOrigins = getAllowedOrigins().join(', '); memoryConfig.connectionStatus = { ...defaultMemoryConfig.connectionStatus, ...inputMemoryConfig.connectionStatus, }; }api/src/core/sso/auth-request-setup.ts (2)
9-10: 🛠️ Refactor suggestion
Consider environment-specific path configuration.
Hardcoded paths could cause issues across different environments. Consider making these paths configurable through environment variables or configuration files.
-const AUTH_REQUEST_FILE = '/usr/local/emhttp/auth-request.php'; -const WEB_COMPS_DIR = '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/'; +const AUTH_REQUEST_FILE = process.env.AUTH_REQUEST_PATH || '/usr/local/emhttp/auth-request.php'; +const WEB_COMPS_DIR = process.env.WEB_COMPS_PATH || '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/';📝 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 AUTH_REQUEST_FILE = process.env.AUTH_REQUEST_PATH || '/usr/local/emhttp/auth-request.php'; const WEB_COMPS_DIR = process.env.WEB_COMPS_PATH || '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/';
17-46:
⚠️ Potential issueAdd proper error handling for file operations.
The function lacks error handling for file operations which could lead to silent failures or inconsistent states.
export const setupAuthRequest = async () => { + try { const JS_FILES = await getJsFiles(WEB_COMPS_DIR); logger.debug(`Found ${JS_FILES.length} .js files in ${WEB_COMPS_DIR}`); const FILES_TO_ADD = ['/webGui/images/partner-logo.svg', ...JS_FILES]; if (existsSync(AUTH_REQUEST_FILE)) { const fileContent = await readFile(AUTH_REQUEST_FILE, 'utf8'); if (fileContent.includes('$arrWhitelist')) { const backupFile = `${AUTH_REQUEST_FILE}.bak`; await writeFile(backupFile, fileContent); logger.debug(`Backup of ${AUTH_REQUEST_FILE} created at ${backupFile}`); const filesToAddString = FILES_TO_ADD.map((file) => ` '${file}',`).join('\n'); const updatedContent = fileContent.replace( /(\$arrWhitelist\s*=\s*\[)/, `$1\n${filesToAddString}` ); await writeFile(AUTH_REQUEST_FILE, updatedContent); logger.debug(`Default values and .js files from ${WEB_COMPS_DIR} added to $arrWhitelist.`); } else { - logger.debug(`$arrWhitelist array not found in the file.`); + throw new Error('$arrWhitelist array not found in the file.'); } } else { - logger.debug(`File ${AUTH_REQUEST_FILE} not found.`); + throw new Error(`File ${AUTH_REQUEST_FILE} not found.`); } + } catch (error) { + logger.error('Failed to setup auth request:', error); + 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.export const setupAuthRequest = async () => { try { const JS_FILES = await getJsFiles(WEB_COMPS_DIR); logger.debug(`Found ${JS_FILES.length} .js files in ${WEB_COMPS_DIR}`); const FILES_TO_ADD = ['/webGui/images/partner-logo.svg', ...JS_FILES]; if (existsSync(AUTH_REQUEST_FILE)) { const fileContent = await readFile(AUTH_REQUEST_FILE, 'utf8'); if (fileContent.includes('$arrWhitelist')) { const backupFile = `${AUTH_REQUEST_FILE}.bak`; await writeFile(backupFile, fileContent); logger.debug(`Backup of ${AUTH_REQUEST_FILE} created at ${backupFile}`); const filesToAddString = FILES_TO_ADD.map((file) => ` '${file}',`).join('\n'); const updatedContent = fileContent.replace( /(\$arrWhitelist\s*=\s*\[)/, `$1\n${filesToAddString}` ); await writeFile(AUTH_REQUEST_FILE, updatedContent); logger.debug(`Default values and .js files from ${WEB_COMPS_DIR} added to $arrWhitelist.`); } else { throw new Error('$arrWhitelist array not found in the file.'); } } else { throw new Error(`File ${AUTH_REQUEST_FILE} not found.`); } } catch (error) { logger.error('Failed to setup auth request:', error); throw error; } };api/src/store/watch/config-watch.ts (1)
29-35:
⚠️ Potential issuePotential race condition and memory leak in file recreation.
The current implementation has several issues:
- Race condition: Multiple watchers might try to recreate the file simultaneously
- Memory leak: Recursive setup calls could stack up watchers
- No debouncing of change events
.on('unlink', async () => { + // Close watcher before file operations to prevent race conditions + await watcher.close(); const config = safelySerializeObjectToIni(getWriteableConfig(initialState, 'flash')); await writeFileSync(myServersConfigPath, config, 'utf-8'); - watcher.close(); - setupConfigPathWatch(); + // Use setTimeout to ensure file operations are complete + setTimeout(() => setupConfigPathWatch(), 100); store.dispatch(logoutUser({ reason: 'Config File was Deleted' })); });📝 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..on('unlink', async () => { // Close watcher before file operations to prevent race conditions await watcher.close(); const config = safelySerializeObjectToIni(getWriteableConfig(initialState, 'flash')); await writeFileSync(myServersConfigPath, config, 'utf-8'); // Use setTimeout to ensure file operations are complete setTimeout(() => setupConfigPathWatch(), 100); store.dispatch(logoutUser({ reason: 'Config File was Deleted' })); });api/src/unraid-api/cli/sso/add-sso-user.questions.ts (1)
16-28: 🛠️ Refactor suggestion
Avoid using process.exit() in validation.
Using
process.exit()in validation logic makes the code harder to test and maintain. Consider throwing an error instead.validate(input) { if (!input) { return 'Please provide a response'; } if (!['y', 'n'].includes(input.toLowerCase())) { return 'Please provide a valid response'; } if (input.toLowerCase() === 'n') { - process.exit(1); + throw new Error('User declined to proceed'); } return true; },📝 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.validate(input) { if (!input) { return 'Please provide a response'; } if (!['y', 'n'].includes(input.toLowerCase())) { return 'Please provide a valid response'; } if (input.toLowerCase() === 'n') { throw new Error('User declined to proceed'); } return true; }, })api/src/core/modules/vms/get-domains.ts (1)
58-60:
⚠️ Potential issueSanitize error messages in GraphQL errors.
The current error handling could potentially expose sensitive information through error messages.
throw new GraphQLError( - `Failed to fetch domains with error: ${error instanceof Error ? error.message : 'Unknown Error'}` + 'Failed to fetch domains. Please check the hypervisor status.' );📝 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 GraphQLError( 'Failed to fetch domains. Please check the hypervisor status.' );api/src/unraid-api/cli/sso/remove-sso-user.command.ts (2)
53-53:
⚠️ Potential issueFix template string syntax
The error message uses incorrect template string syntax.
Apply this diff to fix the template string:
-throw new Error('Username must be in the format of a UUID (e.g., ${v4()}}\n'); +throw new Error(`Username must be in the format of a UUID (e.g., ${v4()})\n`);📝 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(`Username must be in the format of a UUID (e.g., ${v4()})\n`);
28-38:
⚠️ Potential issueAdd error handling for store operations
The
runmethod lacks try-catch blocks for store operations and file writing, which could fail silently.Apply this diff to add proper error handling:
public async run(_input: string[], options: RemoveSSOUserCommandOptions): Promise<void> { + try { await store.dispatch(loadConfigFile()); options = await this.inquirerService.prompt(RemoveSSOUserQuestionSet.name, options); store.dispatch(removeSsoUser(options.username === 'all' ? null : options.username)); if (options.username === 'all') { this.logger.info('All users removed from SSO'); } else { this.logger.info('User removed: ' + options.username); } writeConfigSync('flash'); + } catch (error) { + this.logger.error(`Failed to remove SSO user: ${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.public async run(_input: string[], options: RemoveSSOUserCommandOptions): Promise<void> { try { await store.dispatch(loadConfigFile()); options = await this.inquirerService.prompt(RemoveSSOUserQuestionSet.name, options); store.dispatch(removeSsoUser(options.username === 'all' ? null : options.username)); if (options.username === 'all') { this.logger.info('All users removed from SSO'); } else { this.logger.info('User removed: ' + options.username); } writeConfigSync('flash'); } catch (error) { this.logger.error(`Failed to remove SSO user: ${error.message}`); throw error; } }api/src/types/my-servers-config.ts (2)
25-41: 🛠️ Refactor suggestion
Enhance SSO sub IDs validation
The current validation allows empty strings and has a complex transform/refine chain. Consider simplifying and strengthening the validation.
Apply this diff to improve validation:
ssoSubIds: z - .string() - .transform((val) => { - // If valid, return as is - if (val === '' || val.split(',').every((id) => id.trim().match(/^[a-zA-Z0-9-]+$/))) { - return val; - } - // Otherwise, replace with an empty string - return ''; - }) - .refine( - (val) => val === '' || val.split(',').every((id) => id.trim().match(/^[a-zA-Z0-9-]+$/)), - { - message: - 'ssoSubIds must be empty or a comma-separated list of alphanumeric strings with dashes', - } - ), + .string() + .refine( + (val) => { + if (val === '') return true; + return val.split(',') + .map(id => id.trim()) + .every(id => /^[a-zA-Z0-9-]{1,36}$/.test(id)); + }, + { + message: 'ssoSubIds must be empty or a comma-separated list of valid UUIDs' + } + ),📝 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.ssoSubIds: z .string() .refine( (val) => { if (val === '') return true; return val.split(',') .map(id => id.trim()) .every(id => /^[a-zA-Z0-9-]{1,36}$/.test(id)); }, { message: 'ssoSubIds must be empty or a comma-separated list of valid UUIDs' } ),
11-42: 🛠️ Refactor suggestion
Consider adding sensitive data protection
The
RemoteConfigSchemacontains sensitive information like tokens and API keys. Consider adding a transform to mask these values in logs.
[security]Apply this diff to add sensitive data protection:
const RemoteConfigSchema = z.object({ wanaccess: z.string(), wanport: z.string(), upnpEnabled: z.string(), - apikey: z.string(), - localApiKey: z.string(), + apikey: z.string().transform(val => val ? '[REDACTED]' : val), + localApiKey: z.string().transform(val => val ? '[REDACTED]' : val), email: z.string(), username: z.string(), avatar: z.string(), regWizTime: z.string(), - accesstoken: z.string(), - idtoken: z.string(), - refreshtoken: z.string(), + accesstoken: z.string().transform(val => val ? '[REDACTED]' : val), + idtoken: z.string().transform(val => val ? '[REDACTED]' : val), + refreshtoken: z.string().transform(val => val ? '[REDACTED]' : val),📝 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 RemoteConfigSchema = z.object({ wanaccess: z.string(), wanport: z.string(), upnpEnabled: z.string(), apikey: z.string().transform(val => val ? '[REDACTED]' : val), localApiKey: z.string().transform(val => val ? '[REDACTED]' : val), email: z.string(), username: z.string(), avatar: z.string(), regWizTime: z.string(), accesstoken: z.string().transform(val => val ? '[REDACTED]' : val), idtoken: z.string().transform(val => val ? '[REDACTED]' : val), refreshtoken: z.string().transform(val => val ? '[REDACTED]' : val), dynamicRemoteAccessType: z.nativeEnum(DynamicRemoteAccessType), ssoSubIds: z .string() .transform((val) => { // If valid, return as is if (val === '' || val.split(',').every((id) => id.trim().match(/^[a-zA-Z0-9-]+$/))) { return val; } // Otherwise, replace with an empty string return ''; }) .refine( (val) => val === '' || val.split(',').every((id) => id.trim().match(/^[a-zA-Z0-9-]+$/)), { message: 'ssoSubIds must be empty or a comma-separated list of alphanumeric strings with dashes', } ), });api/src/unraid-api/cli/apikey/add-api-key.questions.ts (3)
43-45:
⚠️ Potential issueAdd error handling for role conversion
The
parseRolesmethod should handle potential conversion errors.Apply this diff to add error handling:
parseRoles(val: string[]): Role[] { - return this.apiKeyService.convertRolesStringArrayToRoles(val); + try { + if (!val || val.length === 0) { + throw new Error('At least one role must be selected'); + } + return this.apiKeyService.convertRolesStringArrayToRoles(val); + } catch (error) { + this.logger.error(`Failed to parse roles: ${error.message}`); + throw error; + } }Committable suggestion skipped: line range outside the PR's diff.
34-36: 🛠️ Refactor suggestion
Add input validation for description
The
parseDescriptionmethod should validate the description length and content.Apply this diff to add validation:
parseDescription(val: string) { + if (!val || val.trim().length === 0) { + throw new Error('Description cannot be empty'); + } + if (val.length > 255) { + throw new Error('Description cannot exceed 255 characters'); + } return val; }📝 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.parseDescription(val: string) { if (!val || val.trim().length === 0) { throw new Error('Description cannot be empty'); } if (val.length > 255) { throw new Error('Description cannot exceed 255 characters'); } return val; }
26-28:
⚠️ Potential issueAdd input validation for API key name
The
parseNamemethod lacks input validation. API key names should follow specific rules.Apply this diff to add validation:
parseName(val: string) { + if (!val || val.trim().length === 0) { + throw new Error('API key name cannot be empty'); + } + if (!/^[a-zA-Z0-9-_]+$/.test(val)) { + throw new Error('API key name can only contain alphanumeric characters, hyphens, and underscores'); + } return val; }📝 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.parseName(val: string) { if (!val || val.trim().length === 0) { throw new Error('API key name cannot be empty'); } if (!/^[a-zA-Z0-9-_]+$/.test(val)) { throw new Error('API key name can only contain alphanumeric characters, hyphens, and underscores'); } return val; }api/src/unraid-api/cli/sso/add-sso-user.command.ts (1)
33-54: 🛠️ Refactor suggestion
Add transaction rollback on error.
The run method updates the configuration but doesn't roll back changes if a subsequent operation fails (e.g., if the restart fails).
Consider implementing rollback:
async run(_input: string[], options: AddSSOUserCommandOptions): Promise<void> { try { options = await this.inquirerService.prompt(AddSSOUserQuestionSet.name, options); if (options.disclaimer === 'y' && options.username) { await store.dispatch(loadConfigFile()); const shouldRestart = store.getState().config.remote.ssoSubIds.length === 0; + const previousConfig = { ...store.getState().config }; store.dispatch(addSsoUser(options.username)); writeConfigSync('flash'); this.logger.info(`User added ${options.username}`); if (shouldRestart) { this.logger.info('Restarting the Unraid API to enable to SSO button'); - await this.restartCommand.run([]); + try { + await this.restartCommand.run([]); + } catch (error) { + // Rollback changes + store.dispatch({ type: 'config/setState', payload: previousConfig }); + writeConfigSync('flash'); + 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(_input: string[], options: AddSSOUserCommandOptions): Promise<void> { try { options = await this.inquirerService.prompt(AddSSOUserQuestionSet.name, options); if (options.disclaimer === 'y' && options.username) { await store.dispatch(loadConfigFile()); const shouldRestart = store.getState().config.remote.ssoSubIds.length === 0; const previousConfig = { ...store.getState().config }; store.dispatch(addSsoUser(options.username)); writeConfigSync('flash'); this.logger.info(`User added ${options.username}`); if (shouldRestart) { this.logger.info('Restarting the Unraid API to enable to SSO button'); try { await this.restartCommand.run([]); } catch (error) { // Rollback changes store.dispatch({ type: 'config/setState', payload: previousConfig }); writeConfigSync('flash'); throw error; } } } } catch (e: unknown) { if (e instanceof Error) { this.logger.error('Error adding user: ' + e.message); } else { this.logger.error('Error adding user'); } } }web/pages/login.vue (2)
10-14:
⚠️ Potential issueFix template root element requirement.
Vue.js requires exactly one root element in the template. Wrap the
headandbodyelements in a single root element.<template> + <div> <head> <title><?=$var['NAME']?>/Login</title> </head> <body> <!-- existing content --> </body> + </div> </template>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 14-14:
The template root requires exactly one element
4-6: 🛠️ Refactor suggestion
Consider enhancing form submission handling.
The
handleSubmitfunction only logs a message. Consider implementing proper form validation and error handling.const handleSubmit = () => { - console.log('Form submitted'); + try { + // Add form validation + const formData = new FormData(event.target as HTMLFormElement); + const username = formData.get('username') as string; + const password = formData.get('password') as string; + + if (!username || !password) { + throw new Error('Username and password are required'); + } + + // Add authentication logic here + + } catch (error) { + console.error('Form submission failed:', error); + // Add error handling UI feedback + } };Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/main.yml (1)
241-241: 💡 Codebase verification
Restore the main branch requirement for releases
The removal of the main branch requirement from the release-pull-request job is concerning. The original condition was specifically designed to ensure releases only happen for PRs targeting the main branch. Recent PR history shows a mix of branches, and removing this protection could lead to unintended releases from feature branch PRs.
- Add back the condition:
github.event.pull_request.base.ref == 'main'🔗 Analysis chain
Verify the intentional removal of main branch requirement.
The condition for the release-pull-request job has been simplified to run for any PR, not just those targeting the main branch. Please confirm if this is intentional to support releases for feature branches.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any recent PRs merged to non-main branches that required releases # List recent PRs merged to non-main branches gh pr list --search "is:merged base:!main" --limit 5 --json number,title,baseRefName # Check the workflow run history for these PRs gh run list --workflow main.yml --limit 10 --json conclusion,headBranchLength of output: 911
Script:
#!/bin/bash # Check recent PRs and their target branches gh pr list --state merged --limit 5 --json number,title,baseRefName # View the workflow file from the default branch git show origin/main:.github/workflows/main.yml | grep -A 5 -B 5 "release-pull-request"Length of output: 1029
elibosley
force-pushed
the
feat/hypervisor-async-instantiation
branch
from
9fcc391 to
700d096
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/core/utils/vms/get-hypervisor.ts (2)
21-30: Improve singleton implementation by using explicit class name.The singleton pattern is well-implemented, but using
thisin static methods can be confusing. Consider using the explicit class name for better clarity.export class UnraidHypervisor { private static instance: UnraidHypervisor | null = null; private hypervisor: HypervisorType | null = null; private constructor() {} public static getInstance(): UnraidHypervisor { - if (this.instance === null) { - this.instance = new UnraidHypervisor(); + if (UnraidHypervisor.instance === null) { + UnraidHypervisor.instance = new UnraidHypervisor(); } - return this.instance; + return UnraidHypervisor.instance; }🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
41-44: Enhance error message with troubleshooting guidance.The error message could be more helpful by including troubleshooting steps.
if (!running) { this.hypervisor = null; - throw new Error('Libvirt is not running'); + throw new Error('Libvirt service is not running. Please ensure the service is started using "systemctl start libvirtd"'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/src/core/modules/vms/get-domains.ts(4 hunks)api/src/core/utils/vms/get-hypervisor.ts(2 hunks)api/src/core/utils/vms/parse-domain.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/unraid-api/cli/start.command.ts
- api/src/core/utils/vms/parse-domain.ts
- api/src/core/modules/vms/get-domains.ts
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/core/utils/vms/get-hypervisor.ts
[error] 27-27: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/core/utils/vms/get-hypervisor.ts (3)
4-4: LGTM! Good use of type-only imports.The type-only import with a clear alias improves code clarity and compilation efficiency.
45-53: LGTM! Well-implemented error handling.The implementation includes:
- Dynamic import for better code splitting
- Proper error handling with detailed logging
- Clean error propagation
37-39: Consider adding connection status verification.The current check only verifies if the hypervisor instance exists and if libvirt is running. Consider adding a connection status check to ensure the connection is still valid.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1)
Line range hint
21-28: Remove redundant @resolver decorator on servers method.The servers method has both @resolver('servers') and @query() decorators. Since this is a query method, only the @query() decorator is needed.
- @Resolver('servers') @Query() @UsePermissions({ action: AuthActionVerb.READ, resource: Resource.SERVERS, possession: AuthPossession.ANY, }) public async servers(): Promise<Server[]> {
🧹 Nitpick comments (8)
api/src/unraid-api/cli/restart.command.ts (2)
15-21: LGTM! Consider debug level for stdout logging.The added logging statements improve visibility of the restart operation. Consider using debug level for stdout logging since it's typically less critical than error information.
this.logger.info('Restarting the Unraid API'); const { stderr, stdout } = await execa(PM2_PATH, [ 'restart', ECOSYSTEM_PATH, '--update-env', ]); this.logger.info('Unraid API restarted'); if (stderr) { this.logger.error(stderr); process.exit(1); } if (stdout) { - this.logger.info(stdout); + this.logger.debug(stdout); }
Line range hint
22-37: Improve error handling structure.The error handling could be more specific and consolidated.
- } catch (error) { - if (error instanceof Error) { - this.logger.error(error.message); - } else { - this.logger.error('Unknown error occurred'); - } - process.exit(1); - } - process.exit(0); + } catch (error) { + const errorMessage = error instanceof Error + ? `PM2 restart failed: ${error.message}` + : 'Unknown PM2 restart error occurred'; + this.logger.error(errorMessage); + process.exit(1); + } + return;api/src/cli.ts (1)
27-33: Simplify logger configuration.The logger configuration could be simplified and the completion settings look good.
- logger: LOG_LEVEL === 'TRACE' && new LogService(), // - enable this to see nest initialization issues + logger: LOG_LEVEL === 'TRACE' ? new LogService() : undefined,api/src/environment.ts (1)
Line range hint
22-26: Improve LOG_LEVEL type safety and readability.The current implementation could be improved for better type safety and readability.
-export const LOG_LEVEL = process.env.LOG_LEVEL?.toUpperCase() as - | 'TRACE' - | 'DEBUG' - | 'INFO' - | 'WARN' - | 'ERROR' - | 'FATAL' ?? process.env.ENVIRONMENT === 'production' ? 'INFO' : 'TRACE'; +type LogLevel = 'TRACE' | 'DEBUG' | 'INFO' | 'WARN' | 'ERROR' | 'FATAL'; +const defaultLogLevel = process.env.ENVIRONMENT === 'production' ? 'INFO' : 'TRACE'; +export const LOG_LEVEL = (process.env.LOG_LEVEL?.toUpperCase() ?? defaultLogLevel) as LogLevel;api/scripts/deploy-dev.sh (2)
48-49: Add error handling for chown command.The chown command should check its exit status.
# Chown the directory -ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api" +if ! ssh root@"${server_name}" "chown -R root:root /usr/local/unraid-api"; then + echo "Failed to change ownership of files" + exit 1 +fi
51-52: Improve restart command robustness.The restart command should check its exit status and could benefit from parameterized environment variables.
# Run unraid-api restart on remote host -ssh root@"${server_name}" "INTROSPECTION=true LOG_LEVEL=trace unraid-api restart" +INTROSPECTION_VALUE=${INTROSPECTION:-true} +LOG_LEVEL_VALUE=${LOG_LEVEL:-trace} +if ! ssh root@"${server_name}" "INTROSPECTION=${INTROSPECTION_VALUE} LOG_LEVEL=${LOG_LEVEL_VALUE} unraid-api restart"; then + echo "Failed to restart unraid-api" + exit 1 +fiapi/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)
Line range hint
61-113: Consider enhancing error handling and type safety.While the implementation works, consider these improvements:
- Use an enum for case states instead of a plain object to improve type safety
- Add error logging for file read failures
- Consider using a more descriptive type for the case model configuration
Example implementation:
enum CaseState { CUSTOM = 'custom', DEFAULT = 'default', CONFIG_READ_ERROR = 'could-not-read-config-file', IMAGE_READ_ERROR = 'could-not-read-image', IMAGE_MISSING = 'image-missing', IMAGE_TOO_BIG = 'image-too-big', IMAGE_CORRUPT = 'image-corrupt' } interface CaseConfig { url: string; icon: string; error: string; base64: string; } // Add logging if (serverCase === 'error_reading_config_file') { console.error(`Failed to read config file: ${configFilePath}`); return { case: states.couldNotReadConfigFile, ...result }; }api/src/graphql/schema/types/vms/domain.graphql (1)
6-9: Consider adding field documentation.While the ID field addition is good, consider adding documentation to explain its purpose and format.
type Vms { + """Unique identifier for the VMs collection""" id: ID! domain: [VmDomain!] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (15)
api/scripts/deploy-dev.sh(2 hunks)api/src/cli.ts(1 hunks)api/src/core/modules/vms/get-domains.ts(4 hunks)api/src/environment.ts(1 hunks)api/src/graphql/schema/types/vms/domain.graphql(1 hunks)api/src/unraid-api/cli/restart.command.ts(1 hunks)api/src/unraid-api/graph/resolvers/display/display.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/me/me.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/online/online.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/servers/server.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- api/src/unraid-api/graph/resolvers/online/online.resolver.ts
- api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts
- api/src/unraid-api/graph/resolvers/me/me.resolver.ts
- api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/core/modules/vms/get-domains.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
api/src/unraid-api/graph/resolvers/vars/vars.resolver.ts (1)
8-8: LGTM! Explicit resolver naming improves clarity.The addition of the explicit name 'Vars' to the resolver decorator enhances schema clarity and follows NestJS best practices.
api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts (1)
8-8: LGTM! Consistent with resolver naming pattern.The explicit naming of the Flash resolver maintains consistency with other resolver changes and improves code clarity.
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2)
22-26: LGTM! Improved field resolution implementation.The changes to the domain method are well-structured:
- Using @ResolveField is more appropriate for GraphQL schema design
- Added type safety with explicit Promise<Array> return type
- Proper error handling with await
24-24: Consider implications of dynamic import.The dynamic import of getDomains could impact initial load time and bundle size. Consider if this needs to be dynamic or if it could be a static import.
✅ Verification successful
Dynamic import is appropriately used here ✅
The dynamic import pattern is well-justified in this case as it helps lazy load the heavy VM-related dependencies (
@vmngr/libvirt) only when needed. This is particularly beneficial since VM management is likely not a primary operation for all users.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if getDomains is used elsewhere in the codebase rg "getDomains" --type tsLength of output: 341
Script:
#!/bin/bash # Check the implementation of getDomains and its dependencies ast-grep --pattern 'export const getDomains = async () => { $$$ }' # Also check for any imports in the file rg "import .+ from" api/src/core/modules/vms/get-domains.tsLength of output: 3874
api/src/unraid-api/graph/resolvers/servers/server.resolver.ts (1)
10-10: LGTM! Explicit resolver naming.The addition of the explicit name 'Server' to the class resolver decorator improves schema clarity.
api/src/unraid-api/graph/resolvers/display/display.resolver.ts (1)
61-61: LGTM! Explicit resolver naming improves code clarity.The change to add an explicit name to the resolver improves code clarity and debugging capabilities.
api/src/graphql/schema/types/vms/domain.graphql (1)
7-7: LGTM! Adding ID field follows GraphQL best practices.The addition of a required ID field to the Vms type is a good practice for unique identification and caching.
| }) | ||
| public async vms() { | ||
| return {}; | ||
| console.log('Resolving Domains'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug console.log statement.
Production code should not contain debug console.log statements. Consider using a proper logging service if logging is needed.
- console.log('Resolving Domains');
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/src/cli.ts (2)
13-22: Enhance error logging in getUnraidApiLocation.Good job addressing the previous review comments! Consider enhancing the error logging to include more details:
- } catch (err) { - logger.debug('Could not find unraid-api in PATH, using default location'); + } catch (error) { + logger.debug('Could not find unraid-api in PATH, using default location', { error });
27-31: Improve LOG_LEVEL check and documentation.Consider these enhancements:
- Make the LOG_LEVEL check more robust
- Improve the comment about nest initialization
- logger: LOG_LEVEL === 'TRACE' && new LogService(), // - enable this to see nest initialization issues + logger: LOG_LEVEL?.toUpperCase() === 'TRACE' ? new LogService() : undefined, // Enable detailed logging of NestJS initialization processapi/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (1)
Line range hint
9-18: Consider adding an explicit return type.While the implementation is correct, adding an explicit return type would improve type safety and documentation.
- public async vms() { + public async vms(): Promise<{ id: string }> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/cli.ts(1 hunks)api/src/core/utils/vms/parse-domain.ts(1 hunks)api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/core/utils/vms/parse-domain.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/src/cli.ts (2)
8-11: LGTM! Import statements are well-organized.The new imports for logging functionality are correctly added and necessary for the changes.
Line range hint
36-41: LGTM! Error handling is well-structured.The error handling implementation is robust:
- Uses structured logging for internal tracking
- Provides user-friendly error output
- Properly exits with error status
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2)
1-5: LGTM! Well-organized imports.The imports are clean, well-structured, and follow TypeScript best practices with proper type imports.
7-8: LGTM! Good use of explicit resolver naming.Using an explicit name in the
@Resolverdecorator improves maintainability and makes the GraphQL schema more predictable.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
api/src/unraid-api/exceptions/graphql-exceptions.filter.ts (2)
8-8: Add type safety for the response object.The generic type parameter
Tis only used once. Consider adding proper type constraints for better type safety.-export class GraphQLExceptionsFilter<T extends GraphQLError> implements ExceptionFilter { +export class GraphQLExceptionsFilter implements ExceptionFilter<GraphQLError> {
1-5: Consider organizing imports by their source.While the imports have been reorganized, they could be better structured by grouping them based on their source (external vs internal) with a blank line between groups.
import type { ArgumentsHost, ExceptionFilter } from '@nestjs/common'; import { Catch } from '@nestjs/common'; - import { type FastifyReply } from 'fastify'; import { GraphQLError } from 'graphql'; +api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (1)
Line range hint
9-19: Add explicit return type to the vms query method.While the implementation is correct, adding an explicit return type would improve type safety and documentation.
- public async vms() { + public async vms(): Promise<{ id: string }> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
api/src/graphql/schema/types/auth/auth.graphql(1 hunks)api/src/unraid-api/auth/api-key.service.ts(2 hunks)api/src/unraid-api/exceptions/graphql-exceptions.filter.ts(1 hunks)api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts (1)
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-12T03:43:52.962Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/vms/vms.resolver.ts (2)
1-7: LGTM! Good use of explicit resolver name.The explicit name in
@Resolver('Vms')improves code clarity and maintainability.
21-27: LGTM! Good error handling implementation.The error handling implementation follows the suggested pattern from previous reviews, properly catching and transforming errors from both the dynamic import and getDomains call.
api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts (1)
59-64: Verify role validation consistency between schema and service.The GraphQL schema makes
rolesoptional (roles: [Role!]), but the service enforces that at least one role must be specified. This inconsistency could lead to confusing error messages for API consumers.Consider either:
- Making
rolesrequired in the GraphQL schema to match the service requirement, or- Updating the service to handle the case when roles are not provided.
input CreateApiKeyInput { name: String! description: String - roles: [Role!] + roles: [Role!]! permissions: [AddPermissionInput!] }api/src/graphql/schema/types/auth/auth.graphql (1)
28-29: Review permission and role requirements.The current schema makes
rolesoptional butpermissionsrequired, which seems inconsistent with the service implementation that requires at least one role.Consider one of these approaches:
- Make both fields required to ensure proper authorization
- Make both fields optional with clear validation in the service
- Document the reasoning behind making permissions required while roles are optional
input CreateApiKeyInput { name: String! description: String - roles: [Role!] - permissions: [AddPermissionInput!] + roles: [Role!]! + permissions: [AddPermissionInput!]! }
| if (response.code) { | ||
| response.code(200).send({ | ||
| data: null, | ||
| errors: [ | ||
| { | ||
| message: exception.message, | ||
| locations: exception.locations, | ||
| path: exception.path, | ||
| }, | ||
| ], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error logging and proper error status codes.
The current implementation has several concerns:
- Always returning 200 status code for errors doesn't follow REST conventions
- The conditional check could silently fail without any logging
- No error tracking/logging mechanism
Consider this implementation:
- if (response.code) {
- response.code(200).send({
+ const statusCode = exception.extensions?.code || 500;
+ console.error('GraphQL Error:', {
+ message: exception.message,
+ path: exception.path,
+ stack: exception.stack,
+ });
+
+ if (response.code) {
+ response.code(statusCode).send({
data: null,
errors: [
{
message: exception.message,
locations: exception.locations,
path: exception.path,
+ extensions: exception.extensions,
},
],
});
+ } else {
+ console.error('Invalid response object');
}📝 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.
| if (response.code) { | |
| response.code(200).send({ | |
| data: null, | |
| errors: [ | |
| { | |
| message: exception.message, | |
| locations: exception.locations, | |
| path: exception.path, | |
| }, | |
| ], | |
| }); | |
| } | |
| const statusCode = exception.extensions?.code || 500; | |
| console.error('GraphQL Error:', { | |
| message: exception.message, | |
| path: exception.path, | |
| stack: exception.stack, | |
| }); | |
| if (response.code) { | |
| response.code(statusCode).send({ | |
| data: null, | |
| errors: [ | |
| { | |
| message: exception.message, | |
| locations: exception.locations, | |
| path: exception.path, | |
| extensions: exception.extensions, | |
| }, | |
| ], | |
| }); | |
| } else { | |
| console.error('Invalid response object'); | |
| } |
| // Consider using a proper logger here | ||
| throw new Error(`Failed to retrieve VM domains: ${error.message}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace the logging comment with proper logger implementation.
The comment suggests using a proper logger. Consider using NestJS's built-in Logger or a custom logging service.
- // Consider using a proper logger here
- throw new Error(`Failed to retrieve VM domains: ${error.message}`);
+ const logger = new Logger(VmsResolver.name);
+ logger.error(`Failed to retrieve VM domains: ${error.message}`, error.stack);
+ throw new Error(`Failed to retrieve VM domains: ${error.message}`);📝 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.
| // Consider using a proper logger here | |
| throw new Error(`Failed to retrieve VM domains: ${error.message}`); | |
| const logger = new Logger(VmsResolver.name); | |
| logger.error(`Failed to retrieve VM domains: ${error.message}`, error.stack); | |
| throw new Error(`Failed to retrieve VM domains: ${error.message}`); |
| description: string | undefined; | ||
| roles?: Role[]; | ||
| permissions?: Permission[]; | ||
| permissions?: Permission[] | AddPermissionInput[]; |
There was a problem hiding this comment.
Add validation for AddPermissionInput type.
The permissions parameter now accepts AddPermissionInput[] but there's no validation or conversion logic to handle this new type. This could lead to runtime errors when saving the API key.
Consider adding validation and conversion logic:
async create({
name,
description,
roles,
permissions,
overwrite = false,
}: {
name: string;
description: string | undefined;
roles?: Role[];
permissions?: Permission[] | AddPermissionInput[];
overwrite?: boolean;
}): Promise<ApiKeyWithSecret> {
+ const normalizedPermissions = permissions?.map(permission => {
+ if ('actions' in permission) {
+ return {
+ resource: permission.resource,
+ actions: permission.actions.map(action =>
+ AuthActionVerb[action.toUpperCase() as keyof typeof AuthActionVerb]
+ ).filter(Boolean)
+ };
+ }
+ return permission;
+ });
// ... rest of the method
- apiKey.permissions = permissions ?? [];
+ apiKey.permissions = normalizedPermissions ?? [];Committable suggestion skipped: line range outside the PR's diff.
| resource: Resource! | ||
| action: String! | ||
| possession: String! | ||
| actions: [String!]! |
There was a problem hiding this comment.
Constrain action values to valid AuthActionVerb.
The actions field accepts any string, which could lead to runtime errors if invalid action values are provided.
Consider using an enum type for actions:
+ enum AuthActionVerb {
+ CREATE
+ READ
+ UPDATE
+ DELETE
+ }
input AddPermissionInput {
resource: Resource!
- actions: [String!]!
+ actions: [AuthActionVerb!]!
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts (1)
89-89: Consider adding more test cases for permissions.While the current test covers the basic case with empty permissions, consider adding test cases for:
- Creating API key with non-empty permissions
- Validation of permission format
- Error cases for invalid permissions
Example test case structure:
it('should create new API key with permissions', async () => { const input = { name: 'New API Key', description: 'New API Key Description', roles: [Role.GUEST], permissions: [{ resource: Resource.VMS, action: AuthActionVerb.READ, possession: AuthPossession.ANY }] }; // ... rest of the test }); it('should reject invalid permissions', async () => { const input = { // ... invalid permissions }; // ... expect error });Also applies to: 98-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/auth/auth.resolver.spec.ts (1)
26-26: LGTM! Appropriate initialization of the new permissions field.The addition of empty permission arrays to the mock objects aligns with the schema changes and maintains type safety.
Also applies to: 36-36
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
idfield to the Virtual Machines (VMs) GraphQL schema.permissionsfield in the API key creation process.Refactor
VmsResolverto return structured responses with anidproperty.createApiKeymutation to accept a single object parameter.Chores
console.logstatements withapiLoggermethods for consistent logging.