fix: report issues + pm2 issues #1081
Conversation
WalkthroughThe pull request introduces several modifications to the Unraid API project, focusing on changes in configuration management, dependency handling, and code structure. Key updates include enhancing logging and process management in the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/src/unraid-api/cli/start.command.ts (1)
19-25: Consider using a consistent async/await error-handling style.In
cleanupPM2State, you're mixingawaitwith.then(...).catch(...)patterns. For clarity and consistency, you might refactor to a singletry/catchapproach for each execa call. This improves code readability and maintains uniform error management.async cleanupPM2State() { - await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]) - .then(({ stdout }) => this.logger.debug(stdout)) - .catch(({ stderr }) => this.logger.error('PM2 Stop Error: ' + stderr)); + try { + const { stdout } = await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]); + this.logger.debug(stdout); + } catch (error: any) { + this.logger.error('PM2 Stop Error: ' + error.stderr); + } - await execa(PM2_PATH, ['delete', ECOSYSTEM_PATH]) - .then(({ stdout }) => this.logger.debug(stdout)) - .catch(({ stderr }) => this.logger.error('PM2 Delete API Error: ' + stderr)); + try { + const { stdout } = await execa(PM2_PATH, ['delete', ECOSYSTEM_PATH]); + this.logger.debug(stdout); + } catch (error: any) { + this.logger.error('PM2 Delete API Error: ' + error.stderr); + } // Update PM2 ... }api/src/unraid-api/cli/report.command.ts (2)
34-48: Consider handling partial or malformed config data.
getBothMyServersConfigsWithoutErrorreturnsnullif neither config is found. If one of the INI files is malformed, the parse call might throw. Currently, you catch only the file read error, not parse errors. You might wrap the parse in atry/catchto avoid unexpected runtime exceptions.try { return MyServersConfigMemorySchema.parse(ini.parse(diskConfig)); } catch (parseError) { this.logger.debug('Failed to parse diskConfig: ' + parseError); return null; }
51-51: Refine the return type to avoid confusion with 'void'.TypeScript lint tools warn that
voidin a union can be confusing. Consider usingPromise<string | undefined>instead.- async report(): Promise<string | void> { + async report(): Promise<string | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
api/src/core/utils/pm2/unraid-api-running.ts (1)
4-4: Use a structured logger for consistency.The direct
console.log(err)calls differ from the logger usage in other modules. Consider using a central logging mechanism to standardize log formatting and levels across the codebase.- console.error(err); + logger.error(err); - console.log(false); + logger.debug('PM2 process not found. Returning false.'); - console.log(isOnline); + logger.debug(`Unraid API Online: ${isOnline}`);Also applies to: 10-10, 21-21
api/ecosystem.config.json (1)
21-21: Document the kill_timeout parameter.The addition of
kill_timeout: 3000is good for process management, but consider adding a comment explaining why 3 seconds was chosen as the timeout value."ignore_watch": [ "node_modules", "src", ".env.*", "myservers.cfg" ], "out_file": "/var/log/unraid-api/unraid-api.log", "error_file": "/var/log/unraid-api/unraid-api.error.log", + // Allow 3 seconds for graceful shutdown before force killing the process "kill_timeout": 3000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
api/ecosystem.config.json(1 hunks)api/package.json(0 hunks)api/src/core/utils/pm2/unraid-api-running.ts(2 hunks)api/src/index.ts(0 hunks)api/src/unraid-api/cli/report.command.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/rest/rest.service.ts(1 hunks)
💤 Files with no reviewable changes (2)
- api/src/index.ts
- api/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/cli/report.command.ts
[error] 51-51: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ 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 (8)
api/src/unraid-api/cli/start.command.ts (1)
35-35: Great step ordering.Cleaning up the PM2 state before starting the Unraid API helps ensure a fresh environment and can prevent stale process conflicts.
api/src/unraid-api/cli/report.command.ts (4)
1-2: Good use of modern file system promises.Using
readFilefrom'fs/promises'is a clean approach and keeps the codebase aligned with modern async patterns.
5-7: Imports are consistent with the new configuration approach.Importing
MyServersConfigMemoryand its schema helps maintain a strongly typed interface for configuration data. This guards against schema drift.
53-75: Lazy importing the PM2 helper is a good approach.Dynamically importing
isUnraidApiRunningkeeps dependencies lightweight for users of this CLI command. Validate PM2 usage within this code block is thoroughly tested to confirm it handles all PM2 states (e.g., if PM2 is not installed).
79-80: Method signature simplifies the usage of CLI commands.Removing parameters from
run()and delegating the logic toreport()creates a more streamlined flow. This is a concise improvement.api/src/core/utils/pm2/unraid-api-running.ts (1)
2-2: Dynamic import of PM2 streamlines dependencies.This approach loads PM2 functions only when needed. However, ensure that environment constraints allow dynamic imports at runtime, especially in containerized or server environments.
api/src/unraid-api/rest/rest.service.ts (1)
22-22: Verify the impact of removing report parameters.The removal of parameters (
{ json: true, verbose: 2, raw: false }) fromreportCommand.report()could affect the report format. Please ensure this change:
- Aligns with the new implementation in
ReportCommand- Maintains backward compatibility if other parts of the system expect specific report formats
Let's verify the usage of this method:
✅ Verification successful
The parameter removal is correct and safe
The
ReportCommand.report()method always returns JSON-formatted output regardless of the parameters, making the previous parameters ({ json: true, verbose: 2, raw: false }) redundant. This change aligns with the actual implementation and removes unused parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other callers of saveApiReport to ensure they can handle the new format rg "saveApiReport" -A 5 # Search for consumers of the report.json file rg "report\.json" -A 5Length of output: 1545
Script:
#!/bin/bash # Check ReportCommand implementation ast-grep --pattern 'class ReportCommand { $$$ report($$$) { $$$ } $$$ }' # Look for related files fd "report.command" --type f # Check git history for context git log --grep="report" -n 5 --onelineLength of output: 156
Script:
#!/bin/bash # View ReportCommand implementation cat api/src/unraid-api/cli/report.command.ts # Check recent changes to this file git log -p api/src/unraid-api/cli/report.command.tsLength of output: 2773
api/ecosystem.config.json (1)
19-20: LGTM! Improved log separation.Splitting logs into separate files for standard output and errors will improve debugging capabilities.
Let's verify the log directory exists and has proper permissions:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/index.ts (1)
99-99: Replace console.log with logger.Use the already imported logger instead of console.log for consistent logging throughout the application.
- console.log('exithook', signal); + logger.info('Initiating exit hook', { signal });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/index.ts(3 hunks)api/src/unraid-api/cli/report.spec.ts(0 hunks)
💤 Files with no reviewable changes (1)
- api/src/unraid-api/cli/report.spec.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 (3)
api/src/index.ts (3)
12-12: LGTM! Import changes align with async exit handling.The change from
exitHookto{ asyncExitHook, gracefulExit }provides better control over the application shutdown process.
123-123: LGTM! Consistent error handling exit.The change to
gracefulExit(1)maintains the error status code while ensuring graceful shutdown, consistent with the new exit handling approach.
97-109: Verify wait time alignment with PM2 kill_timeout.The async exit hook implementation looks good and addresses the previous review comment. However, we should verify that the wait time (9999ms) aligns with the PM2 kill_timeout configuration to ensure proper shutdown coordination.
✅ Verification successful
✅ PM2 kill_timeout and asyncExitHook wait time are properly aligned
The asyncExitHook wait time (9999ms) is correctly set to be slightly under PM2's kill_timeout (10000ms), ensuring proper shutdown coordination.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check PM2 ecosystem config for kill_timeout setting # Search for kill_timeout in ecosystem config rg -A 5 "kill_timeout" ecosystem.config.jsonLength of output: 106
Script:
#!/bin/bash # Search for potential PM2 config files and kill_timeout settings # Look for ecosystem config files echo "=== Searching for PM2 config files ===" fd -e js -e json -e yaml -e yml ecosystem # Search for PM2 related settings in package.json echo -e "\n=== Checking package.json for PM2 config ===" fd package.json --exec cat {} | jq -r 'select(.pm2 != null) | .pm2' # Search for kill_timeout pattern in all files echo -e "\n=== Searching for kill_timeout pattern ===" rg -i "kill_timeout|pm2.*timeout|graceful.*timeout" --type ts --type js --type jsonLength of output: 580
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
force-pushed
the
feat/report-refactor
branch
from
45f02f6 to
fda7c7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
api/src/unraid-api/cli/report.command.ts (1)
Line range hint
18-33: Remove or implement unused command options.The
parseRawandparseJsonoptions are defined but not utilized in the current implementation. They always returntrueregardless of the input, and thereportmethod doesn't use these values.Either:
- Remove these unused options, or
- Implement the JSON/raw output formatting in the
reportmethod based on these options.- @Option({ - flags: '-r, --raw', - description: 'whether to enable raw command output', - defaultValue: false, - }) - parseRaw(): boolean { - return true; - } - - @Option({ - flags: '-j, --json', - description: 'Display JSON output for this command', - defaultValue: false, - }) - parseJson(): boolean { - return true; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🧹 Nitpick comments (1)
api/src/unraid-api/cli/report.command.ts (1)
79-81: Consider handling the return value from report.The
runmethod ignores the return value fromreport, which could be useful for testing or scripting purposes.Consider this improvement:
-async run(): Promise<void> { - await this.report(); +async run(): Promise<string | undefined> { + return this.report(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
api/ecosystem.config.json(1 hunks)api/package.json(0 hunks)api/src/core/utils/pm2/unraid-api-running.ts(2 hunks)api/src/index.ts(3 hunks)api/src/unraid-api/cli/report.command.ts(2 hunks)api/src/unraid-api/cli/report.spec.ts(0 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/rest/rest.service.ts(1 hunks)
💤 Files with no reviewable changes (2)
- api/package.json
- api/src/unraid-api/cli/report.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- api/src/unraid-api/rest/rest.service.ts
- api/ecosystem.config.json
- api/src/index.ts
- api/src/core/utils/pm2/unraid-api-running.ts
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/unraid-api/cli/report.command.ts
[error] 51-51: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ 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
| this.logger.info('Starting the Unraid API'); | ||
| await this.configurePm2(); | ||
|
|
||
| await this.cleanupPM2State(); | ||
| const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve startup robustness and graceful shutdown handling.
The current implementation has several potential issues:
- No validation of PM2_PATH and ECOSYSTEM_PATH
- Direct process.exit calls could prevent cleanup
- No graceful shutdown handling
Consider this improved implementation:
async run(_: string[], options: StartCommandOptions): Promise<void> {
this.logger.info('Starting the Unraid API');
+ // Validate paths
+ if (!PM2_PATH || !ECOSYSTEM_PATH) {
+ throw new Error('PM2_PATH and ECOSYSTEM_PATH must be defined');
+ }
+
+ // Register cleanup handler
+ const cleanup = async (signal?: string) => {
+ if (signal) {
+ this.logger.info(`Received ${signal}, cleaning up...`);
+ }
+ try {
+ await this.cleanupPM2State();
+ process.exit(0);
+ } catch (error) {
+ this.logger.error('Cleanup failed:', error);
+ process.exit(1);
+ }
+ };
+
+ process.on('SIGTERM', () => cleanup('SIGTERM'));
+ process.on('SIGINT', () => cleanup('SIGINT'));
+
await this.cleanupPM2State();
const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : '';
- const { stderr, stdout } = await execa(`${envLog} ${PM2_PATH}`.trim(), [
- 'start',
- ECOSYSTEM_PATH,
- '--update-env',
- ]);
- if (stdout) {
- this.logger.log(stdout);
- }
- if (stderr) {
- this.logger.error(stderr);
- process.exit(1);
+ try {
+ const { stderr, stdout } = await execa(`${envLog} ${PM2_PATH}`.trim(), [
+ 'start',
+ ECOSYSTEM_PATH,
+ '--update-env',
+ ]);
+ if (stdout) {
+ this.logger.log(stdout);
+ }
+ if (stderr) {
+ this.logger.error(stderr);
+ await cleanup();
+ }
+ } catch (error) {
+ this.logger.error('Failed to start PM2:', error);
+ await cleanup();
}
- process.exit(0);
}Committable suggestion skipped: line range outside the PR's diff.
| async cleanupPM2State() { | ||
| await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Set Error: ' + stderr)); | ||
| await execa(PM2_PATH, ['set', 'pm2-logrotate:compress', 'true']) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Stop Error: ' + stderr)); | ||
| await execa(PM2_PATH, ['delete', ECOSYSTEM_PATH]) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Compress Error: ' + stderr)); | ||
| await execa(PM2_PATH, ['set', 'pm2-logrotate:max_size', '1M']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Max Size Error: ' + stderr)); | ||
|
|
||
| // PM2 Save Settings | ||
| await execa(PM2_PATH, ['set', 'pm2:autodump', 'true']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Autodump Error: ' + stderr)); | ||
| .catch(({ stderr }) => this.logger.error('PM2 Delete API Error: ' + stderr)); | ||
|
|
||
| // Update PM2 | ||
| await execa(PM2_PATH, ['update']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Update Error: ' + stderr)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and process management in cleanupPM2State.
The current implementation has several areas for improvement:
- Error handling continues execution even if critical operations fail
- No timeout handling for PM2 operations
- Potential race conditions between PM2 operations
Consider this improved implementation:
async cleanupPM2State() {
+ const PM2_TIMEOUT = 10000; // 10 seconds
+ try {
await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH], {
+ timeout: PM2_TIMEOUT,
+ stripFinalNewline: true,
})
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Stop Error: ' + stderr));
+ .then(({ stdout }) => this.logger.debug('PM2 Stop Success:', stdout));
+ } catch (error) {
+ const err = error as { stderr?: string, message: string };
+ this.logger.error('PM2 Stop Error:', err.stderr || err.message);
+ throw new Error('Failed to stop PM2 process');
+ }
+
+ try {
await execa(PM2_PATH, ['delete', ECOSYSTEM_PATH])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Delete API Error: ' + stderr));
+ .then(({ stdout }) => this.logger.debug('PM2 Delete Success:', stdout));
+ } catch (error) {
+ const err = error as { stderr?: string, message: string };
+ this.logger.error('PM2 Delete Error:', err.stderr || err.message);
+ throw new Error('Failed to delete PM2 process');
+ }
- // Update PM2
- await execa(PM2_PATH, ['update'])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Update Error: ' + stderr));
+ // Only update PM2 if necessary (could be moved to a separate method)
+ if (process.env.UPDATE_PM2 === 'true') {
+ try {
+ await execa(PM2_PATH, ['update'], { timeout: PM2_TIMEOUT })
+ .then(({ stdout }) => this.logger.debug('PM2 Update Success:', stdout));
+ } catch (error) {
+ const err = error as { stderr?: string, message: string };
+ this.logger.warn('PM2 Update Warning:', err.stderr || err.message);
+ // Don't throw on update failure as it's not critical
+ }
+ }
}📝 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 cleanupPM2State() { | |
| await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH]) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Set Error: ' + stderr)); | |
| await execa(PM2_PATH, ['set', 'pm2-logrotate:compress', 'true']) | |
| .catch(({ stderr }) => this.logger.error('PM2 Stop Error: ' + stderr)); | |
| await execa(PM2_PATH, ['delete', ECOSYSTEM_PATH]) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Compress Error: ' + stderr)); | |
| await execa(PM2_PATH, ['set', 'pm2-logrotate:max_size', '1M']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Max Size Error: ' + stderr)); | |
| // PM2 Save Settings | |
| await execa(PM2_PATH, ['set', 'pm2:autodump', 'true']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Autodump Error: ' + stderr)); | |
| .catch(({ stderr }) => this.logger.error('PM2 Delete API Error: ' + stderr)); | |
| // Update PM2 | |
| await execa(PM2_PATH, ['update']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Update Error: ' + stderr)); | |
| async cleanupPM2State() { | |
| const PM2_TIMEOUT = 10000; // 10 seconds | |
| try { | |
| await execa(PM2_PATH, ['stop', ECOSYSTEM_PATH], { | |
| timeout: PM2_TIMEOUT, | |
| stripFinalNewline: true, | |
| }) | |
| .then(({ stdout }) => this.logger.debug('PM2 Stop Success:', stdout)); | |
| } catch (error) { | |
| const err = error as { stderr?: string, message: string }; | |
| this.logger.error('PM2 Stop Error:', err.stderr || err.message); | |
| throw new Error('Failed to stop PM2 process'); | |
| } | |
| try { | |
| await execa(PM2_PATH, ['delete', ECOSYSTEM_PATH]) | |
| .then(({ stdout }) => this.logger.debug('PM2 Delete Success:', stdout)); | |
| } catch (error) { | |
| const err = error as { stderr?: string, message: string }; | |
| this.logger.error('PM2 Delete Error:', err.stderr || err.message); | |
| throw new Error('Failed to delete PM2 process'); | |
| } | |
| // Only update PM2 if necessary (could be moved to a separate method) | |
| if (process.env.UPDATE_PM2 === 'true') { | |
| try { | |
| await execa(PM2_PATH, ['update'], { timeout: PM2_TIMEOUT }) | |
| .then(({ stdout }) => this.logger.debug('PM2 Update Success:', stdout)); | |
| } catch (error) { | |
| const err = error as { stderr?: string, message: string }; | |
| this.logger.warn('PM2 Update Warning:', err.stderr || err.message); | |
| // Don't throw on update failure as it's not critical | |
| } | |
| } | |
| } |
| async getBothMyServersConfigsWithoutError(): Promise<MyServersConfigMemory | null> { | ||
| const ini = await import('ini'); | ||
| const diskConfig = await readFile(getters.paths()['myservers-config'], 'utf-8').catch( | ||
| (_) => null | ||
| ); | ||
| const memoryConfig = await readFile(getters.paths()['myservers-config-states'], 'utf-8').catch( | ||
| (_) => null | ||
| ); | ||
|
|
||
| if (memoryConfig) { | ||
| return MyServersConfigMemorySchema.parse(ini.parse(memoryConfig)); | ||
| } else if (diskConfig) { | ||
| return MyServersConfigMemorySchema.parse(ini.parse(diskConfig)); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and imports in config retrieval.
Several improvements can be made to this method:
- The dynamic import of 'ini' could be moved to top-level imports
- Error handling is too permissive, silently returning null
- Missing type assertion for diskConfig parse case
- No explicit handling of parse errors
Consider this improved implementation:
+import ini from 'ini';
+
async getBothMyServersConfigsWithoutError(): Promise<MyServersConfigMemory | null> {
- const ini = await import('ini');
const diskConfig = await readFile(getters.paths()['myservers-config'], 'utf-8').catch(
- (_) => null
+ (error) => {
+ this.logger.debug(`Failed to read disk config: ${error.message}`);
+ return null;
+ }
);
const memoryConfig = await readFile(getters.paths()['myservers-config-states'], 'utf-8').catch(
- (_) => null
+ (error) => {
+ this.logger.debug(`Failed to read memory config: ${error.message}`);
+ return null;
+ }
);
if (memoryConfig) {
- return MyServersConfigMemorySchema.parse(ini.parse(memoryConfig));
+ try {
+ return MyServersConfigMemorySchema.parse(ini.parse(memoryConfig));
+ } catch (error) {
+ this.logger.debug(`Failed to parse memory config: ${error.message}`);
+ return null;
+ }
} else if (diskConfig) {
- return MyServersConfigMemorySchema.parse(ini.parse(diskConfig));
+ try {
+ return MyServersConfigMemorySchema.parse(ini.parse(diskConfig));
+ } catch (error) {
+ this.logger.debug(`Failed to parse disk config: ${error.message}`);
+ return null;
+ }
}
return null;
}Committable suggestion skipped: line range outside the PR's diff.
| async report(): Promise<string | void> { | ||
| try { | ||
| // Show loading message | ||
| if (isInteractive) { | ||
| this.logger.info('Generating report please wait…'); | ||
| } | ||
|
|
||
| const jsonReport = options?.json ?? false; | ||
|
|
||
| // Find all processes called "unraid-api" which aren't this process | ||
| const unraidApiRunning = await isUnraidApiRunning(); | ||
|
|
||
| // Load my servers config file into store | ||
| await store.dispatch(loadConfigFile()); | ||
| await store.dispatch(loadStateFiles()); | ||
|
|
||
| const { config, emhttp } = store.getState(); | ||
|
|
||
| const client = getApiApolloClient({ localApiKey: config.remote.localApiKey || '' }); | ||
| // Fetch the cloud endpoint | ||
| const cloud = await getCloudData(client) | ||
| .then((data) => { | ||
| this.logger.debug('Cloud Data', data); | ||
| return data; | ||
| }) | ||
| .catch((error) => { | ||
| this.logger.debug( | ||
| 'Failed fetching cloud from local graphql with "%s"', | ||
| error instanceof Error ? error.message : 'Unknown Error' | ||
| ); | ||
| return null; | ||
| }); | ||
|
|
||
| // Query local graphql using upc's API key | ||
| // Get the servers array | ||
| const servers = await getServersData({ client, verbosity: options.verbose }); | ||
|
|
||
| // Check if the API key is valid | ||
| const isApiKeyValid = cloud?.apiKey.valid ?? false; | ||
|
|
||
| const reportObject: ReportObject = { | ||
| os: { | ||
| serverName: emhttp.var.name, | ||
| version: emhttp.var.version, | ||
| }, | ||
| api: { | ||
| version: API_VERSION, | ||
| status: unraidApiRunning ? 'running' : 'stopped', | ||
| environment: process.env.ENVIRONMENT ?? 'THIS_WILL_BE_REPLACED_WHEN_BUILT', | ||
| nodeVersion: process.version, | ||
| }, | ||
| apiKey: isApiKeyValid ? 'valid' : (cloud?.apiKey.error ?? 'invalid'), | ||
| ...(servers ? { servers } : {}), | ||
| myServers: { | ||
| status: config?.remote?.username ? 'authenticated' : 'signed out', | ||
| ...(config?.remote?.username | ||
| ? { | ||
| myServersUsername: config?.remote?.username?.includes('@') | ||
| ? 'REDACTED' | ||
| : config?.remote.username, | ||
| } | ||
| : {}), | ||
| }, | ||
| minigraph: { | ||
| status: cloud?.minigraphql.status ?? MinigraphStatus.PRE_INIT, | ||
| timeout: cloud?.minigraphql.timeout ?? null, | ||
| error: | ||
| (cloud?.minigraphql.error ?? !cloud?.minigraphql.status) | ||
| ? 'API Disconnected' | ||
| : null, | ||
| }, | ||
| cloud: { | ||
| status: cloud?.cloud.status ?? 'error', | ||
| ...(cloud?.cloud.error ? { error: cloud.cloud.error } : {}), | ||
| ...(cloud?.cloud.status === 'ok' ? { ip: cloud.cloud.ip ?? 'NO_IP' } : {}), | ||
| ...(getAllowedOrigins(cloud, options.verbose) | ||
| ? { allowedOrigins: getAllowedOrigins(cloud, options.verbose) } | ||
| : {}), | ||
| }, | ||
| }; | ||
|
|
||
| if (jsonReport) { | ||
| this.logger.clear(); | ||
| this.logger.info(JSON.stringify(reportObject) + '\n'); | ||
| return reportObject; | ||
| } else { | ||
| // Generate the actual report | ||
| const report = ` | ||
| <-----UNRAID-API-REPORT-----> | ||
| SERVER_NAME: ${reportObject.os.serverName} | ||
| ENVIRONMENT: ${reportObject.api.environment} | ||
| UNRAID_VERSION: ${reportObject.os.version} | ||
| UNRAID_API_VERSION: ${reportObject.api.version} | ||
| UNRAID_API_STATUS: ${reportObject.api.status} | ||
| API_KEY: ${reportObject.apiKey} | ||
| MY_SERVERS: ${reportObject.myServers.status}${ | ||
| reportObject.myServers.myServersUsername | ||
| ? `\nMY_SERVERS_USERNAME: ${reportObject.myServers.myServersUsername}` | ||
| : '' | ||
| } | ||
| CLOUD: ${getReadableCloudDetails(reportObject, options.verbose)} | ||
| MINI-GRAPH: ${getReadableMinigraphDetails(reportObject)}${getReadableServerDetails( | ||
| reportObject, | ||
| options.verbose | ||
| )}${getReadableAllowedOrigins(reportObject)} | ||
| </----UNRAID-API-REPORT-----> | ||
| `; | ||
| this.logger.clear(); | ||
|
|
||
| this.logger.info(report); | ||
| return report; | ||
| } | ||
| } catch (error: unknown) { | ||
| if (error instanceof Error) { | ||
| this.logger.debug(error); | ||
| this.logger.error(`\nFailed generating report with "${error.message}"\n`); | ||
| return; | ||
| } else { | ||
| this.logger.error('Failed generating report'); | ||
| return; | ||
| } | ||
| const { isUnraidApiRunning } = await import('@app/core/utils/pm2/unraid-api-running'); | ||
|
|
||
| const apiRunning = await isUnraidApiRunning().catch((err) => { | ||
| this.logger.debug('failed to get PM2 state with error: ' + err); | ||
| return false; | ||
| }); | ||
|
|
||
| const config = | ||
| (await this.getBothMyServersConfigsWithoutError()) as MyServersConfigMemory & { | ||
| connectionStatus: { running: 'yes' | 'no' }; | ||
| }; | ||
| config.connectionStatus.running = apiRunning ? 'yes' : 'no'; | ||
| this.logger.clear(); | ||
| this.logger.info(JSON.stringify(config, null, 2)); | ||
| } catch (error) { | ||
| this.logger.debug('Error Generating Config: ' + error); | ||
| this.logger.warn( | ||
| JSON.stringify( | ||
| { error: 'Please ensure the API is configured before attempting to run a report' }, | ||
| null, | ||
| 2 | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in report method.
Several improvements can be made:
- Return type includes void in union (flagged by static analysis)
- Unsafe type assertion for config
- Dynamic import could be moved to top-level
- Generic error message
Consider this improved implementation:
-import { readFile } from 'fs/promises';
+import { readFile } from 'fs/promises';
+import { isUnraidApiRunning } from '@app/core/utils/pm2/unraid-api-running';
-async report(): Promise<string | void> {
+async report(): Promise<string | undefined> {
try {
- const { isUnraidApiRunning } = await import('@app/core/utils/pm2/unraid-api-running');
-
const apiRunning = await isUnraidApiRunning().catch((err) => {
this.logger.debug('failed to get PM2 state with error: ' + err);
return false;
});
- const config =
- (await this.getBothMyServersConfigsWithoutError()) as MyServersConfigMemory & {
- connectionStatus: { running: 'yes' | 'no' };
- };
+ const config = await this.getBothMyServersConfigsWithoutError();
+ if (!config) {
+ throw new Error('Failed to retrieve configuration');
+ }
+
+ const configWithStatus = {
+ ...config,
+ connectionStatus: {
+ running: apiRunning ? 'yes' : 'no' as const
+ }
+ };
- config.connectionStatus.running = apiRunning ? 'yes' : 'no';
this.logger.clear();
- this.logger.info(JSON.stringify(config, null, 2));
+ const output = JSON.stringify(configWithStatus, null, 2);
+ this.logger.info(output);
+ return output;
} catch (error) {
- this.logger.debug('Error Generating Config: ' + error);
+ this.logger.debug(`Failed to generate report: ${error instanceof Error ? error.message : String(error)}`);
this.logger.warn(
JSON.stringify(
{ error: 'Please ensure the API is configured before attempting to run a report' },
null,
2
)
);
+ return undefined;
}
}📝 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 report(): Promise<string | void> { | |
| try { | |
| // Show loading message | |
| if (isInteractive) { | |
| this.logger.info('Generating report please wait…'); | |
| } | |
| const jsonReport = options?.json ?? false; | |
| // Find all processes called "unraid-api" which aren't this process | |
| const unraidApiRunning = await isUnraidApiRunning(); | |
| // Load my servers config file into store | |
| await store.dispatch(loadConfigFile()); | |
| await store.dispatch(loadStateFiles()); | |
| const { config, emhttp } = store.getState(); | |
| const client = getApiApolloClient({ localApiKey: config.remote.localApiKey || '' }); | |
| // Fetch the cloud endpoint | |
| const cloud = await getCloudData(client) | |
| .then((data) => { | |
| this.logger.debug('Cloud Data', data); | |
| return data; | |
| }) | |
| .catch((error) => { | |
| this.logger.debug( | |
| 'Failed fetching cloud from local graphql with "%s"', | |
| error instanceof Error ? error.message : 'Unknown Error' | |
| ); | |
| return null; | |
| }); | |
| // Query local graphql using upc's API key | |
| // Get the servers array | |
| const servers = await getServersData({ client, verbosity: options.verbose }); | |
| // Check if the API key is valid | |
| const isApiKeyValid = cloud?.apiKey.valid ?? false; | |
| const reportObject: ReportObject = { | |
| os: { | |
| serverName: emhttp.var.name, | |
| version: emhttp.var.version, | |
| }, | |
| api: { | |
| version: API_VERSION, | |
| status: unraidApiRunning ? 'running' : 'stopped', | |
| environment: process.env.ENVIRONMENT ?? 'THIS_WILL_BE_REPLACED_WHEN_BUILT', | |
| nodeVersion: process.version, | |
| }, | |
| apiKey: isApiKeyValid ? 'valid' : (cloud?.apiKey.error ?? 'invalid'), | |
| ...(servers ? { servers } : {}), | |
| myServers: { | |
| status: config?.remote?.username ? 'authenticated' : 'signed out', | |
| ...(config?.remote?.username | |
| ? { | |
| myServersUsername: config?.remote?.username?.includes('@') | |
| ? 'REDACTED' | |
| : config?.remote.username, | |
| } | |
| : {}), | |
| }, | |
| minigraph: { | |
| status: cloud?.minigraphql.status ?? MinigraphStatus.PRE_INIT, | |
| timeout: cloud?.minigraphql.timeout ?? null, | |
| error: | |
| (cloud?.minigraphql.error ?? !cloud?.minigraphql.status) | |
| ? 'API Disconnected' | |
| : null, | |
| }, | |
| cloud: { | |
| status: cloud?.cloud.status ?? 'error', | |
| ...(cloud?.cloud.error ? { error: cloud.cloud.error } : {}), | |
| ...(cloud?.cloud.status === 'ok' ? { ip: cloud.cloud.ip ?? 'NO_IP' } : {}), | |
| ...(getAllowedOrigins(cloud, options.verbose) | |
| ? { allowedOrigins: getAllowedOrigins(cloud, options.verbose) } | |
| : {}), | |
| }, | |
| }; | |
| if (jsonReport) { | |
| this.logger.clear(); | |
| this.logger.info(JSON.stringify(reportObject) + '\n'); | |
| return reportObject; | |
| } else { | |
| // Generate the actual report | |
| const report = ` | |
| <-----UNRAID-API-REPORT-----> | |
| SERVER_NAME: ${reportObject.os.serverName} | |
| ENVIRONMENT: ${reportObject.api.environment} | |
| UNRAID_VERSION: ${reportObject.os.version} | |
| UNRAID_API_VERSION: ${reportObject.api.version} | |
| UNRAID_API_STATUS: ${reportObject.api.status} | |
| API_KEY: ${reportObject.apiKey} | |
| MY_SERVERS: ${reportObject.myServers.status}${ | |
| reportObject.myServers.myServersUsername | |
| ? `\nMY_SERVERS_USERNAME: ${reportObject.myServers.myServersUsername}` | |
| : '' | |
| } | |
| CLOUD: ${getReadableCloudDetails(reportObject, options.verbose)} | |
| MINI-GRAPH: ${getReadableMinigraphDetails(reportObject)}${getReadableServerDetails( | |
| reportObject, | |
| options.verbose | |
| )}${getReadableAllowedOrigins(reportObject)} | |
| </----UNRAID-API-REPORT-----> | |
| `; | |
| this.logger.clear(); | |
| this.logger.info(report); | |
| return report; | |
| } | |
| } catch (error: unknown) { | |
| if (error instanceof Error) { | |
| this.logger.debug(error); | |
| this.logger.error(`\nFailed generating report with "${error.message}"\n`); | |
| return; | |
| } else { | |
| this.logger.error('Failed generating report'); | |
| return; | |
| } | |
| const { isUnraidApiRunning } = await import('@app/core/utils/pm2/unraid-api-running'); | |
| const apiRunning = await isUnraidApiRunning().catch((err) => { | |
| this.logger.debug('failed to get PM2 state with error: ' + err); | |
| return false; | |
| }); | |
| const config = | |
| (await this.getBothMyServersConfigsWithoutError()) as MyServersConfigMemory & { | |
| connectionStatus: { running: 'yes' | 'no' }; | |
| }; | |
| config.connectionStatus.running = apiRunning ? 'yes' : 'no'; | |
| this.logger.clear(); | |
| this.logger.info(JSON.stringify(config, null, 2)); | |
| } catch (error) { | |
| this.logger.debug('Error Generating Config: ' + error); | |
| this.logger.warn( | |
| JSON.stringify( | |
| { error: 'Please ensure the API is configured before attempting to run a report' }, | |
| null, | |
| 2 | |
| ) | |
| ); | |
| async report(): Promise<string | undefined> { | |
| try { | |
| const apiRunning = await isUnraidApiRunning().catch((err) => { | |
| this.logger.debug('failed to get PM2 state with error: ' + err); | |
| return false; | |
| }); | |
| const config = await this.getBothMyServersConfigsWithoutError(); | |
| if (!config) { | |
| throw new Error('Failed to retrieve configuration'); | |
| } | |
| const configWithStatus = { | |
| ...config, | |
| connectionStatus: { | |
| running: apiRunning ? 'yes' : 'no' as const | |
| } | |
| }; | |
| this.logger.clear(); | |
| const output = JSON.stringify(configWithStatus, null, 2); | |
| this.logger.info(output); | |
| return output; | |
| } catch (error) { | |
| this.logger.debug(`Failed to generate report: ${error instanceof Error ? error.message : String(error)}`); | |
| this.logger.warn( | |
| JSON.stringify( | |
| { error: 'Please ensure the API is configured before attempting to run a report' }, | |
| null, | |
| 2 | |
| ) | |
| ); | |
| return undefined; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
Configuration Updates
ip-regexdependencyCode Refactoring
Cleanup
anonymiseOriginsfunction