fix: enhance plugin management with interactive removal prompts #1549
Conversation
``` fix: enhance plugin management with interactive removal prompts - Add RemovePluginQuestionSet for interactive plugin removal - Update plugin commands to use PluginManagementService - Improve plugin installation error handling and warnings - Clean up test fixtures and update plugin command tests - Reset dev config to clean state (v4.11.0, no plugins)
|
Caution Review failedThe pull request is closed. WalkthroughThis set of changes refactors the plugin CLI commands to use dedicated service classes and interactive prompts, introduces a new question set for plugin removal, updates related tests to mock services instead of GraphQL, and removes or updates several test fixture files. Additionally, some configuration and module provider adjustments are made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant InquirerService
participant PluginManagementService
participant PluginService
User->>CLI: Run remove-plugin command
CLI->>PluginService: listPlugins()
PluginService-->>CLI: Return installed plugins
CLI->>InquirerService: Prompt user to select plugins to remove
InquirerService-->>CLI: Return selected plugins
CLI->>PluginManagementService: removePlugin(selected)
PluginManagementService-->>CLI: Confirm removal
CLI->>User: Log removal and exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @elibosley's task —— View job Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: Todo List:
✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/unraid-api/cli/plugins/plugin.command.ts (1)
94-106: Consider using specific exit codes for different error scenarios.The error handling differentiates between
NoPluginsFoundError(exit 0) and other errors (exit 1), which is good. However, using exit code 0 for "no plugins found" might be misleading as 0 typically indicates success.Consider using a non-zero exit code for the no plugins scenario:
if (error instanceof NoPluginsFoundError) { this.logService.error(error.message); - process.exit(0); + process.exit(2); // Or another specific code for "no items found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
api/dev/configs/api.json(1 hunks)api/src/unraid-api/cli/__test__/plugin.command.test.ts(6 hunks)api/src/unraid-api/cli/cli.module.ts(2 hunks)api/src/unraid-api/cli/plugins/plugin.command.ts(5 hunks)api/src/unraid-api/cli/plugins/remove-plugin.questions.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/rc.nginx.last-download-time(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts(1 hunks)
💤 Files with no reviewable changes (5)
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/rc.nginx.last-download-time
🧰 Additional context used
📓 Path-based instructions (4)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/src/unraid-api/cli/cli.module.tsapi/dev/configs/api.jsonapi/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.tsapi/src/unraid-api/cli/__test__/plugin.command.test.tsapi/src/unraid-api/cli/plugins/remove-plugin.questions.tsapi/src/unraid-api/cli/plugins/plugin.command.ts
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/cli/cli.module.tsapi/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.tsapi/src/unraid-api/cli/__test__/plugin.command.test.tsapi/src/unraid-api/cli/plugins/remove-plugin.questions.tsapi/src/unraid-api/cli/plugins/plugin.command.ts
api/**/*.{test,spec}.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,ts,tsx}: Use Vitest as the test suite; do not use Jest
Prefer not to mock simple dependencies in tests
Files:
api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.tsapi/src/unraid-api/cli/__test__/plugin.command.test.ts
api/**/*.{test,spec}.{ts,js}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Test suite is VITEST, do not use jest in the API package
Files:
api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.tsapi/src/unraid-api/cli/__test__/plugin.command.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: elibosley
PR: unraid/api#1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Learnt from: mdatelle
PR: unraid/api#1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
api/dev/configs/api.json (4)
Learnt from: mdatelle
PR: #1219
File: api/src/unraid-api/main.ts:57-63
Timestamp: 2025-03-07T17:36:52.790Z
Learning: The CORS configuration in the unraid API is intentionally set to allow all origins (origin: true). This was a deliberate architectural decision when removing the previous custom CORS implementation and implementing helmet security headers instead.
Learnt from: mdatelle
PR: #1219
File: api/src/unraid-api/main.ts:32-55
Timestamp: 2025-03-07T17:35:50.406Z
Learning: Helmet security configuration in the Unraid API is intentionally relaxed (with disabled CSP, CORS policies, and HSTS) to maintain compatibility with existing Unraid plugins. Stricter security settings might break current plugin functionality.
Learnt from: pujitm
PR: #1211
File: api/src/graphql/schema/types/connect/connect.graphql:142-146
Timestamp: 2025-03-14T16:10:38.562Z
Learning: The updateApiSettings mutation in api/src/unraid-api/graph/connect/connect.resolver.ts is protected with the @UsePermissions decorator that requires UPDATE permission on the CONFIG resource.
Learnt from: pujitm
PR: #1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts (15)
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: In the Unraid API, FileModification implementations (like LogRotateModification) don't need to handle errors internally as error handling is managed at the UnraidFileModifierService level.
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations, including detailed error logging with stack traces and modification IDs. Individual FileModification implementations should focus on their core functionality without duplicating error handling.
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: In the Unraid API, FileModification implementations (apply/rollback methods) don't need to implement their own error handling as it's handled by the UnraidFileModifierService caller.
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Override specific action implementations when needed in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Test async operations completely in store tests
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying apiKey.roles in removeRoleFromApiKey and addRoleToApiKey within api/src/unraid-api/auth/auth.service.ts, concurrency issues are not a concern because the keys are stored in the file system.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Verify state changes by updating the store in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Mock external dependencies appropriately in store tests
Learnt from: elibosley
PR: #1111
File: api/src/unraid-api/unraid-file-modifier/file-modification.ts:182-187
Timestamp: 2025-02-04T18:45:23.106Z
Learning: In the FileModification class's patch handling:
results === falseindicates patch application failure- Empty string (
results === '') is a valid patch result indicating the file should be deleted - These are distinct conditions and should be handled differently
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/*.{test,spec}.{ts,js} : Always await async operations before making assertions in web/test
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/components/**/*.ts : Use flushPromises() for more complex promise chains in async tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Test action side effects if not stubbed in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Always await async operations before making assertions
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Test action side effects and state changes in store tests
api/src/unraid-api/cli/__test__/plugin.command.test.ts (20)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Mock external dependencies and services in tests
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/*.{test,spec}.{ts,js} : Mock external dependencies and services in web/test
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Clear mocks between tests to ensure isolation
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Mock external dependencies appropriately in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Prefer not to mock simple dependencies in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Use factory functions for module mocks to avoid hoisting issues
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Don't mix mock declarations and module mocks incorrectly
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/store.{test,spec}.{ts,js} : Use factory functions for module mocks to avoid hoisting issues in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Verify state changes by updating the store in store tests
Learnt from: pujitm
PR: #1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Use Vitest as the test suite; do not use Jest
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/components/**/*.ts : Use mount from Vue Test Utils for component testing
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Use vi.mock() for module-level mocks
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Remember that vi.mock() calls are hoisted
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Reset mocks between tests with vi.clearAllMocks()
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Specify return values for component methods with vi.spyOn()
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/mocks/**/*.ts : Frequently used mocks are stored under web/test/mocks
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Verify proper error handling in store tests
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/store.{test,spec}.{ts,js} : Clear mocks between tests to ensure isolation in store tests
api/src/unraid-api/cli/plugins/plugin.command.ts (5)
Learnt from: elibosley
PR: #1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Learnt from: pujitm
PR: #1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
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.
Learnt from: elibosley
PR: #942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In api/src/unraid-api/auth/api-key.service.ts, when handling read errors in the findById method, throw a GraphQLError instead of an InternalServerErrorException.
Learnt from: pujitm
PR: #1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a set() method for runtime configuration mutation, unlike the standard @nestjs/config package which only provides getter methods.
🔇 Additional comments (5)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts (1)
63-63: Excellent simplification of test file retrieval logic.Replacing the complex
downloadOrRetrieveOriginalFilefunction with a directreadFilecall eliminates external dependencies, caching complexity, and potential network-related test flakiness. This change makes the tests more predictable, faster, and easier to maintain.api/src/unraid-api/cli/cli.module.ts (1)
24-24: LGTM! Clean integration of the new question set.The
RemovePluginQuestionSetis properly imported and registered as a provider, following the existing pattern for other question sets in the module.Also applies to: 76-76
api/dev/configs/api.json (1)
2-6: Configuration reset looks good.The dev configuration has been properly reset to a clean state with version 4.11.0, no extra origins, sandbox disabled, and no plugins installed. This provides a clean baseline for development.
api/src/unraid-api/cli/plugins/remove-plugin.questions.ts (1)
25-37: Confirmed staticlistPlugins—no changes neededVerified that
PluginService.listPlugins()is declared as a static method inapi/src/unraid-api/plugin/plugin.service.ts. Calling it directly is correct; no constructor injection is required.api/src/unraid-api/cli/__test__/plugin.command.test.ts (1)
3-50: Excellent test refactoring to match the new service-based architecture.The tests have been properly updated to:
- Mock services instead of GraphQL client
- Cover interactive prompt scenarios with InquirerService
- Test multi-plugin removal and edge cases
- Use vitest patterns correctly
The test coverage is comprehensive and well-structured.
Also applies to: 124-167, 184-216
| // this can happen if configPlugins is a super set of installedPlugins | ||
| if (installedPlugins.length !== configPlugins.length) { | ||
| const configSet = new Set(configPlugins.map((plugin) => parsePackageArg(plugin).name)); | ||
| const installedSet = new Set(installedPlugins.map(([name]) => name)); | ||
| const notInstalled = Array.from(configSet.difference(installedSet)); | ||
| this.logService.warn(`${notInstalled.length} plugins are not installed:`); | ||
| this.logService.table('warn', notInstalled); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Node.js version compatibility for Set.difference().
The code uses Set.difference() which was introduced in Node.js 22.0.0. Ensure the project's minimum Node.js version supports this method.
If the minimum Node.js version is below 22.0.0, consider using a polyfill or alternative implementation:
-const notInstalled = Array.from(configSet.difference(installedSet));
+const notInstalled = Array.from(configSet).filter(x => !installedSet.has(x));🏁 Script executed:
#!/bin/bash
# Check package.json for Node.js version requirements
cat package.json | jq -r '.engines.node // empty'
# Check if there's a .nvmrc file
if [ -f .nvmrc ]; then
echo "Node version in .nvmrc:"
cat .nvmrc
fi
# Look for any Node version specifications in the project
rg -g "*.json" -g ".nvmrc" "node.*version|engines.*node" -A 2 -B 2Length of output: 215
Ensure Node.js ≥ 22 for Set.difference() or provide a fallback
It looks like your .nvmrc pins Node.js to v22.17.1 (which does include Set.difference()), but there’s no engines.node field in package.json to guarantee that runtime environments use ≥ 22.0.0. Please update accordingly or fall back to a filter-based approach:
• In package.json, declare the minimum Node.js version:
{
// …
"engines": {
"node": ">=22.0.0"
}
}• In api/src/unraid-api/cli/plugins/plugin.command.ts (around line 152), if you need to support Node.js < 22, replace:
- const notInstalled = Array.from(configSet.difference(installedSet));
+ const notInstalled = Array.from(configSet).filter(x => !installedSet.has(x));This ensures Set.difference() is only used when supported, or you have a safe fallback.
🤖 Prompt for AI Agents
In api/src/unraid-api/cli/plugins/plugin.command.ts around lines 150 to 157, the
code uses Set.difference(), which requires Node.js 22 or higher. To fix this,
either add an "engines" field in package.json specifying "node": ">=22.0.0" to
enforce the minimum Node.js version, or replace the Set.difference() call with a
fallback implementation using Array.filter() to compute the difference between
configSet and installedSet for compatibility with older Node.js versions.
|
Claude finished @elibosley's task —— View job Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: Todo List:
✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
|
Claude finished @elibosley's task —— View job I'll analyze this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Todo List:
Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: ✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/config/api-config.module.ts (1)
28-28: Remove debug console.log statement.This appears to be debugging code that should be removed before production.
- console.log('testing 123');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/package.json(1 hunks)api/src/cli.ts(1 hunks)api/src/unraid-api/config/api-config.module.ts(1 hunks)api/src/unraid-api/graph/graph.module.ts(0 hunks)api/vite.config.ts(1 hunks)packages/unraid-shared/src/util/config-file-handler.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/src/unraid-api/graph/graph.module.ts
🧰 Additional context used
📓 Path-based instructions (2)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/vite.config.tsapi/package.jsonapi/src/cli.tsapi/src/unraid-api/config/api-config.module.ts
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/config/api-config.module.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: elibosley
PR: unraid/api#1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Learnt from: mdatelle
PR: unraid/api#1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
api/vite.config.ts (6)
Learnt from: mdatelle
PR: #1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (npm run type-check) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Use Vitest as the test suite; do not use Jest
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Avoid relying on Nuxt's auto-imports in test environment
Learnt from: elibosley
PR: #1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
api/package.json (12)
Learnt from: mdatelle
PR: #1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (npm run type-check) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/* : Always run scripts from api/package.json unless specifically requested otherwise
Learnt from: elibosley
PR: #1120
File: plugin/package.json:0-0
Timestamp: 2025-02-05T14:43:48.568Z
Learning: In Node.js projects, npm scripts should be organized with clear namespacing (e.g., build:, docker:, env:*) and include proper environment validation and error handling. Each script should follow the single responsibility principle.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Use pnpm ONLY for package management in the API package
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Run tests with: pnpm --filter ./api test
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/* : Tests are located under web/test, run with pnpm test
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/* : Use pnpm as the only package manager
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Use Vitest as the test suite; do not use Jest
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
api/src/cli.ts (15)
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Learnt from: elibosley
PR: #1408
File: web/components/ApiKey/PermissionCounter.vue:6-6
Timestamp: 2025-05-23T21:59:29.632Z
Learning: This codebase uses ESM (ECMAScript Modules) and requires .js extensions in import statements, even when importing from TypeScript files, as the imports refer to the compiled JavaScript output.
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying apiKey.roles in removeRoleFromApiKey and addRoleToApiKey within api/src/unraid-api/auth/auth.service.ts, concurrency issues are not a concern because the keys are stored in the file system.
Learnt from: pujitm
PR: #1352
File: api/src/unraid-api/cli/dependency.service.ts:114-117
Timestamp: 2025-04-21T18:25:16.491Z
Learning: Methods in DependencyService like npmInstall and rebuildVendorArchive intentionally propagate errors from execa calls upward rather than handling them internally, allowing callers to implement appropriate error handling. This follows the pattern documented in the rebuildVendorArchive method's docstring.
Learnt from: elibosley
PR: #1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Learnt from: elibosley
PR: #942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In api/src/unraid-api/auth/api-key.service.ts, when handling read errors in the findById method, throw a GraphQLError instead of an InternalServerErrorException.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: elibosley
PR: #1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Learnt from: pujitm
PR: #1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a set() method for runtime configuration mutation, unlike the standard @nestjs/config package which only provides getter methods.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/* : Always run scripts from api/package.json unless specifically requested otherwise
Learnt from: pujitm
PR: #1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Learnt from: elibosley
PR: #1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Learnt from: elibosley
PR: #1120
File: plugin/package.json:0-0
Timestamp: 2025-02-05T14:43:48.568Z
Learning: In Node.js projects, npm scripts should be organized with clear namespacing (e.g., build:, docker:, env:*) and include proper environment validation and error handling. Each script should follow the single responsibility principle.
Learnt from: elibosley
PR: #1120
File: plugin/scripts/build-plugin-and-txz.ts:132-147
Timestamp: 2025-02-06T15:32:09.488Z
Learning: For validation steps in build scripts where the goal is to fail fast on invalid state, simple error handling (console.error + process.exit) is appropriate since we want to abort the build process immediately.
api/src/unraid-api/config/api-config.module.ts (7)
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: In the Unraid API, FileModification implementations (like LogRotateModification) don't need to handle errors internally as error handling is managed at the UnraidFileModifierService level.
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying apiKey.roles in removeRoleFromApiKey and addRoleToApiKey within api/src/unraid-api/auth/auth.service.ts, concurrency issues are not a concern because the keys are stored in the file system.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations, including detailed error logging with stack traces and modification IDs. Individual FileModification implementations should focus on their core functionality without duplicating error handling.
Learnt from: pujitm
PR: #1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a set() method for runtime configuration mutation, unlike the standard @nestjs/config package which only provides getter methods.
🧬 Code Graph Analysis (1)
api/src/cli.ts (1)
api/src/core/log.ts (1)
logger(25-55)
🔇 Additional comments (7)
api/vite.config.ts (1)
113-115: LGTM! Improved portability for command testing.The conditional shebang logic enhances cross-platform compatibility. Using
#!/usr/bin/env nodewhenCOMMAND_TESTERis set makes the CLI more portable by leveraging the system PATH instead of a fixed node binary location.api/package.json (1)
20-20: LGTM! Well-coordinated environment setup.The
COMMAND_TESTER=trueprefix properly coordinates with the vite config changes to enable portable shebang generation during command testing. The environment variable is appropriately scoped to this script execution only.packages/unraid-shared/src/util/config-file-handler.ts (1)
32-32: LGTM! Well-implemented dependency injection pattern.The optional logger injection maintains backward compatibility while enabling better testability and consistency with the broader dependency injection refactoring across the codebase. The fallback logger with a descriptive name based on the config file name is a good practice for debugging.
Also applies to: 37-38
api/src/unraid-api/config/api-config.module.ts (1)
24-25: All loadApiConfig call sites handle errors properly
- In api/src/unraid-api/plugin/plugin.service.ts, the call to
loadApiConfig()is wrapped in atry–catchand falls back to an empty plugins list on failure.- In api/src/unraid-api/config/api-config.test.ts, file‐I/O utilities are mocked to prevent unexpected exceptions during testing.
No further changes are required.
api/src/cli.ts (3)
8-8: LGTM! Dynamic import improves bundle loading.Converting to dynamic import reduces initial bundle size and is a good practice for CLI tools where not all dependencies may be needed on every execution.
17-17: Verify the logger simplification is intentional.The change from a structured pino logger to simple console logging removes features like log levels, structured data, and redaction of sensitive information. This simplification may be appropriate for CLI context, but ensure this aligns with logging requirements.
The relevant code snippet shows the previous logger had structured logging with redaction capabilities. Was this simplification intentional for the CLI context?
Also applies to: 24-24, 33-33
20-20: LGTM! Dynamic import aligns with modular CLI structure.Converting CliModule to dynamic import supports the enhanced modular CLI command structure and reduces initial loading time.
|
Claude finished @elibosley's task —— View job Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: Todo List:
✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
| export const logDestination = pino.destination(); | ||
| const nullDestination = pino.destination({ | ||
| write() { | ||
| // Suppress all logs |
| } | ||
| } | ||
|
|
||
| always(...messages: unknown[]): void { |
| }; | ||
|
|
||
| const getLogger = () => { | ||
| if (LOG_LEVEL === 'TRACE' && !SUPPRESS_LOGS) { |
| interop: 'auto', | ||
| banner: (chunk) => { | ||
| if (chunk.fileName === 'main.js' || chunk.fileName === 'cli.js') { | ||
| if (process.env.COMMAND_TESTER) { |
There was a problem hiding this comment.
I made this change @pujitm - should we just change the shebang to always use /usr/bin/env? Would be 100% reliable unless you blow up your path on Unraid...
There was a problem hiding this comment.
let's test that first -- just want to try some edge cases like rebooting, non-interactive ssh before including it in a release. maybe we can push it to the next release since it isn't critical and the current one is?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/.env.development (1)
18-18: Consider reordering environment variables for consistency.The static analysis tool suggests that
PATHS_LOGS_FILEshould be ordered beforePATHS_LOG_BASEto maintain alphabetical consistency.PATHS_RCLONE_SOCKET=./dev/rclone-socket PATHS_LOG_BASE=./dev/log # Where we store logs +PATHS_LOGS_FILE=./dev/log/graphql-api.log PATHS_CONNECT_STATUS_FILE_PATH=./dev/connectStatus.json # Connect plugin status file -PATHS_LOGS_FILE=./dev/log/graphql-api.log
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/.env.development(1 hunks)api/dev/configs/api.json(1 hunks)api/package.json(1 hunks)api/src/cli.ts(2 hunks)api/src/core/log.ts(2 hunks)api/src/environment.ts(2 hunks)api/src/unraid-api/cli/cli.module.ts(4 hunks)api/src/unraid-api/cli/log.service.ts(2 hunks)api/src/unraid-api/cli/pm2.service.ts(2 hunks)api/src/unraid-api/cli/report.command.ts(2 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(2 hunks)api/src/unraid-api/config/api-config.module.ts(2 hunks)api/src/unraid-api/plugin/plugin.module.ts(3 hunks)api/src/unraid-api/plugin/plugin.service.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- api/src/unraid-api/cli/pm2.service.ts
- api/src/unraid-api/plugin/plugin.service.ts
- api/src/unraid-api/cli/sso/validate-token.command.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- api/src/unraid-api/config/api-config.module.ts
- api/dev/configs/api.json
- api/src/unraid-api/cli/cli.module.ts
- api/src/cli.ts
- api/package.json
🧰 Additional context used
📓 Path-based instructions (2)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/src/unraid-api/cli/log.service.tsapi/src/unraid-api/cli/report.command.tsapi/src/environment.tsapi/src/core/log.tsapi/src/unraid-api/plugin/plugin.module.ts
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/cli/log.service.tsapi/src/unraid-api/cli/report.command.tsapi/src/unraid-api/plugin/plugin.module.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: elibosley
PR: unraid/api#1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Learnt from: mdatelle
PR: unraid/api#1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
api/src/unraid-api/cli/log.service.ts (4)
Learnt from: elibosley
PR: #1222
File: api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts:37-65
Timestamp: 2025-03-17T13:41:52.496Z
Learning: The LogsService in Unraid API uses a reference counting mechanism to track log file subscriptions, ensuring file watchers remain active until all subscribers have unsubscribed.
Learnt from: elibosley
PR: #1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Learnt from: pujitm
PR: #1116
File: api/src/unraid-api/cli/log.service.ts:52-56
Timestamp: 2025-02-04T20:06:52.290Z
Learning: In Node.js logging, console.trace() is specifically for outputting stack traces to stderr and should not be used for general trace-level logging. For trace-level logging of general messages, console.log() is the appropriate choice.
Learnt from: pujitm
PR: #1116
File: api/src/unraid-api/cli/log.service.ts:52-56
Timestamp: 2025-02-04T20:06:52.290Z
Learning: In Node.js:
- console.trace() is a debugging tool that outputs a stack trace to stderr, showing the execution path with line numbers and function names
- For trace-level application logging (most verbose level), use console.log() as it's meant for general message output, not stack traces
- The logging hierarchy from most to least verbose is: trace > debug > info > warn > error
api/src/unraid-api/cli/report.command.ts (2)
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/* : Always run scripts from api/package.json unless specifically requested otherwise
api/src/environment.ts (11)
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Learnt from: pujitm
PR: #1214
File: web/.gitignore:1-2
Timestamp: 2025-03-03T16:04:20.705Z
Learning: In the Unraid web application, environment variables with the VITE_ prefix (like VITE_CALLBACK_KEY) are intentionally exposed client-side and are not considered sensitive secrets, so it's acceptable to track them in Git.
Learnt from: pujitm
PR: #1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: elibosley
PR: #1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: pujitm
PR: #1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: elibosley
PR: #1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:11-16
Timestamp: 2025-05-08T19:31:52.417Z
Learning: The dynamix.my.servers namespace is still valid and should not be changed to dynamix.unraid.net in file paths, as both namespaces coexist in the codebase.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: pujitm
PR: #1367
File: api/src/unraid-api/app/pubsub.module.ts:24-28
Timestamp: 2025-04-23T20:22:36.001Z
Learning: In the Unraid API project, string literals are intentionally used for dependency injection tokens instead of Symbols to enable "collision" behavior across modules that don't directly import from each other. This is a temporary approach during refactoring until a shared library for constants is implemented.
api/src/core/log.ts (2)
Learnt from: elibosley
PR: #1101
File: api/src/unraid-api/unraid-file-modifier/modifications/test/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Learnt from: pujitm
PR: #1116
File: api/src/unraid-api/cli/log.service.ts:52-56
Timestamp: 2025-02-04T20:06:52.290Z
Learning: In Node.js logging, console.trace() is specifically for outputting stack traces to stderr and should not be used for general trace-level logging. For trace-level logging of general messages, console.log() is the appropriate choice.
api/src/unraid-api/plugin/plugin.module.ts (12)
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Use factory functions for module mocks to avoid hoisting issues
Learnt from: elibosley
PR: #1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Learnt from: pujitm
PR: #1075
File: unraid-ui/src/register.ts:15-34
Timestamp: 2025-01-30T19:38:02.478Z
Learning: In the web components registration process for unraid-ui, use a soft-fail approach (logging warnings/errors) instead of throwing errors, to ensure other components can still register successfully even if one component fails.
Learnt from: elibosley
PR: #1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Learnt from: pujitm
PR: #1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a set() method for runtime configuration mutation, unlike the standard @nestjs/config package which only provides getter methods.
Learnt from: elibosley
PR: #1408
File: web/components/ApiKey/PermissionCounter.vue:6-6
Timestamp: 2025-05-23T21:59:29.632Z
Learning: This codebase uses ESM (ECMAScript Modules) and requires .js extensions in import statements, even when importing from TypeScript files, as the imports refer to the compiled JavaScript output.
Learnt from: elibosley
PR: #1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Prefer not to mock simple dependencies in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Mock external dependencies and services in tests
Learnt from: pujitm
PR: #1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom ConfigService implementation that includes a set() method for runtime configuration mutation, unlike the standard @nestjs/config package which only provides getter methods.
api/.env.development (6)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Run tests with: pnpm --filter ./api test
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/* : Always run scripts from api/package.json unless specifically requested otherwise
Learnt from: pujitm
PR: #1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
🧬 Code Graph Analysis (4)
api/src/unraid-api/cli/log.service.ts (2)
api/src/environment.ts (1)
SUPPRESS_LOGS(95-95)api/src/core/log.ts (1)
LogLevel(8-8)
api/src/unraid-api/cli/report.command.ts (1)
api/src/unraid-api/cli/log.service.ts (1)
error(52-56)
api/src/core/log.ts (1)
api/src/environment.ts (4)
LOG_LEVEL(90-94)PATHS_LOGS_FILE(107-107)SUPPRESS_LOGS(95-95)LOG_TYPE(89-89)
api/src/unraid-api/plugin/plugin.module.ts (2)
api/src/unraid-api/plugin/plugin-management.service.ts (1)
plugins(15-17)api/src/unraid-api/plugin/plugin.resolver.ts (1)
plugins(30-38)
🪛 GitHub Check: Test API
api/src/core/log.ts
[failure] 20-20: src/unraid-api/organizer/organizer.validation.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/organizer/organizer-view.validation.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/cron/write-flash-file.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/config/api-config.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/auth/cookie.service.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/auth/auth.service.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/auth/api-key.service.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/test/mothership/index.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/test/utils.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/test/json-forms.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
🪛 GitHub Actions: CI - Main (API)
api/src/core/log.ts
[error] 20-20: Error: EACCES: permission denied, open '/var/log/graphql-api.log'. Permission denied error when trying to open log file.
🪛 dotenv-linter (3.3.0)
api/.env.development
[warning] 18-18: [UnorderedKey] The PATHS_LOGS_FILE key should go before the PATHS_LOG_BASE key
🔇 Additional comments (11)
api/src/environment.ts (1)
95-95: LGTM! Well-designed logging configuration.The new environment variables are well-implemented:
SUPPRESS_LOGSprovides clean boolean conversionPATHS_LOGS_DIRmaintains backward compatibility withLOGS_DIRPATHS_LOGS_FILEhas a sensible default pathAlso applies to: 105-107
api/src/unraid-api/cli/report.command.ts (1)
43-53: LGTM! Appropriate use of always() for CLI output.The change from
logger.warn/logger.infotologger.alwaysensures that critical CLI messages are visible to users regardless of the global log suppression setting. This is the correct approach for interactive CLI commands.Also applies to: 59-59, 62-71
api/src/unraid-api/cli/log.service.ts (1)
9-9: LGTM! Well-implemented log suppression feature.The implementation correctly handles log suppression:
suppressLogsflag properly controls all logging behaviorclear()method respects suppression settingsshouldLog()efficiently short-circuits when suppressedalways()method appropriately bypasses all filtering for critical messagesAlso applies to: 12-15, 18-20, 58-61
api/src/core/log.ts (2)
12-16: LGTM! Well-architected log suppression system.The implementation provides a clean separation between suppressed and normal logging:
nullDestinationeffectively discards logs when needed- Conditional stream selection properly handles both pretty and raw output modes
- The suppression logic is consistently applied across all output streams
Also applies to: 18-19, 25-35
85-85: LGTM! Plugin-specific logging separation.The dedicated
pluginLoggerwith file output provides good separation of plugin logs from core system logs, enabling better debugging and monitoring of plugin operations.api/src/unraid-api/plugin/plugin.module.ts (6)
13-13: LGTM: Static property for storing plugin names.The addition of the static
apiListproperty provides a clean way to store plugin names between the registration phase and the bootstrap phase.
15-21: Good refactoring: Dynamic Logger import prevents circular dependencies.The move from static Logger instances to dynamic imports during
onApplicationBootstrap()is a solid architectural improvement. This approach:
- Avoids potential circular dependency issues during module initialization
- Defers logging until the application is fully bootstrapped
- Follows NestJS lifecycle patterns correctly
29-29: LGTM: Plugin names stored for deferred logging.The assignment of plugin names to the static property enables the bootstrap logging while maintaining the existing module registration logic.
41-41: LGTM: Consistent static property implementation.The
cliListproperty mirrors the implementation inPluginModule, maintaining consistency across both module classes.
43-49: LGTM: Consistent lifecycle implementation.The
onApplicationBootstrap()method implementation is consistent with thePluginModuleapproach, using the same dynamic import pattern and logging format.
57-57: LGTM: Consistent plugin name assignment.The assignment mirrors the implementation in
PluginModule, maintaining consistency across both module classes.
| const localFileDestination = pino.destination({ | ||
| dest: PATHS_LOGS_FILE, | ||
| sync: true, | ||
| }); |
There was a problem hiding this comment.
Fix permission issues with log file creation in tests.
The localFileDestination attempts to write to /var/log/graphql-api.log which causes permission errors in test environments. Tests should not write to system directories.
Consider making the file destination conditional or using a test-specific configuration:
-const localFileDestination = pino.destination({
- dest: PATHS_LOGS_FILE,
- sync: true,
-});
+const localFileDestination = NODE_ENV === 'test'
+ ? nullDestination
+ : pino.destination({
+ dest: PATHS_LOGS_FILE,
+ sync: true,
+ });Alternatively, ensure test environments set PATHS_LOGS_FILE to a writable location like ./test-logs/graphql-api.log.
📝 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 localFileDestination = pino.destination({ | |
| dest: PATHS_LOGS_FILE, | |
| sync: true, | |
| }); | |
| -const localFileDestination = pino.destination({ | |
| - dest: PATHS_LOGS_FILE, | |
| - sync: true, | |
| -}); | |
| +const localFileDestination = NODE_ENV === 'test' | |
| + ? nullDestination | |
| + : pino.destination({ | |
| + dest: PATHS_LOGS_FILE, | |
| + sync: true, | |
| + }); |
🧰 Tools
🪛 GitHub Check: Test API
[failure] 20-20: src/unraid-api/organizer/organizer.validation.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/organizer/organizer-view.validation.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/cron/write-flash-file.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/config/api-config.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/auth/cookie.service.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/auth/auth.service.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/unraid-api/auth/api-key.service.spec.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/test/mothership/index.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/test/utils.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
[failure] 20-20: src/test/json-forms.test.ts
Error: EACCES: permission denied, open '/var/log/graphql-api.log'
❯ openFile ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:81:21
❯ new SonicBoom ../node_modules/.pnpm/sonic-boom@4.2.0/node_modules/sonic-boom/index.js:157:5
❯ buildSafeSonicBoom ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/lib/tools.js:235:18
❯ Function.destination ../node_modules/.pnpm/pino@9.7.0/node_modules/pino/pino.js:217:12
❯ src/core/log.ts:20:35
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { errno: -13, code: 'EACCES', syscall: 'open', path: '/var/log/graphql-api.log' }
🪛 GitHub Actions: CI - Main (API)
[error] 20-20: Error: EACCES: permission denied, open '/var/log/graphql-api.log'. Permission denied error when trying to open log file.
🤖 Prompt for AI Agents
In api/src/core/log.ts around lines 20 to 23, the log file destination is set to
a system directory (/var/log/graphql-api.log) causing permission errors during
tests. Modify the code to conditionally set the log file path based on the
environment, using a writable location like ./test-logs/graphql-api.log for test
environments. This can be done by checking if the environment is test and then
assigning PATHS_LOGS_FILE accordingly before creating the pino destination.
|
Claude finished @elibosley's task —— View job Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: Todo List:
✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
|
Claude finished @elibosley's task —— View job Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: Todo List:
✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/.env.test (1)
16-16: AlphabetizePATHS_LOGS_FILEto silence dotenv-linter warning
dotenv-linterflags this line because it breaks the existing alphabetical ordering of thePATHS_*keys. Moving it abovePATHS_MACHINE_IDkeeps the file tidy and avoids CI noise.-PATHS_MACHINE_ID=./dev/data/machine-id -PATHS_PARITY_CHECKS=./dev/states/parity-checks.log -PATHS_CONFIG_MODULES=./dev/configs -PATHS_ACTIVATION_BASE=./dev/activation -PATHS_PASSWD=./dev/passwd -PATHS_LOGS_FILE=./dev/log/graphql-api.log +PATHS_LOGS_FILE=./dev/log/graphql-api.log +PATHS_MACHINE_ID=./dev/data/machine-id +PATHS_PARITY_CHECKS=./dev/states/parity-checks.log +PATHS_CONFIG_MODULES=./dev/configs +PATHS_ACTIVATION_BASE=./dev/activation +PATHS_PASSWD=./dev/passwdEnsure the tests (and any shell scripts) create
./dev/log/before log writing to prevent ENOENT errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/.env.test(1 hunks)api/dev/log/.gitkeep(1 hunks)api/src/unraid-api/cli/__test__/plugin.command.test.ts(4 hunks)api/src/unraid-api/cli/__test__/report.command.test.ts(4 hunks)api/src/unraid-api/cli/log.service.ts(2 hunks)api/src/unraid-api/rest/__test__/rest-module.integration.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/dev/log/.gitkeep
- api/src/unraid-api/rest/test/rest-module.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/cli/test/plugin.command.test.ts
- api/src/unraid-api/cli/log.service.ts
🧰 Additional context used
📓 Path-based instructions (4)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/src/unraid-api/cli/__test__/report.command.test.ts
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/cli/__test__/report.command.test.ts
api/**/*.{test,spec}.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,ts,tsx}: Use Vitest as the test suite; do not use Jest
Prefer not to mock simple dependencies in tests
Files:
api/src/unraid-api/cli/__test__/report.command.test.ts
api/**/*.{test,spec}.{ts,js}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Test suite is VITEST, do not use jest in the API package
Files:
api/src/unraid-api/cli/__test__/report.command.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: elibosley
PR: unraid/api#1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Learnt from: mdatelle
PR: unraid/api#1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
api/src/unraid-api/cli/__test__/report.command.test.ts (22)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Verify state changes by updating the store in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/components/**/*.ts : Test component behavior and output, not implementation details
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Mock external dependencies appropriately in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Mock external dependencies and services in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Remember that vi.mock() calls are hoisted
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Don't mix mock declarations and module mocks incorrectly
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Verify state changes after actions in store tests
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/*.{test,spec}.{ts,js} : Test component behavior and output, not implementation details in web/test
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/*.{test,spec}.{ts,js} : Mock external dependencies and services in web/test
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Clear mocks between tests to ensure isolation
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Use vi.mock() for module-level mocks
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Reset mocks between tests with vi.clearAllMocks()
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Use Vitest as the test suite; do not use Jest
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Specify return values for component methods with vi.spyOn()
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/mocks/**/*.ts : Frequently used mocks are stored under web/test/mocks
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Prefer not to mock simple dependencies in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/{components,store,mocks}/**/*.ts : Always await async operations before making assertions
Learnt from: elibosley
PR: #1101
File: api/src/unraid-api/unraid-file-modifier/modifications/test/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/test/store/**/*.ts : Verify proper error handling in store tests
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/*.{test,spec}.{ts,js} : Always await async operations before making assertions in web/test
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Prefer to not mock simple dependencies in API tests
api/.env.test (7)
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Run tests with: pnpm --filter ./api test
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/test/**/* : Tests are located under web/test, run with pnpm test
Learnt from: pujitm
PR: #1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/**/* : Always run scripts from api/package.json unless specifically requested otherwise
🧬 Code Graph Analysis (1)
api/src/unraid-api/cli/__test__/report.command.test.ts (1)
api/src/unraid-api/cli/log.service.ts (1)
ILogService(6-17)
🪛 dotenv-linter (3.3.0)
api/.env.test
[warning] 16-16: [UnorderedKey] The PATHS_LOGS_FILE key should go before the PATHS_MACHINE_ID key
🔇 Additional comments (5)
api/src/unraid-api/cli/__test__/report.command.test.ts (5)
5-5: LGTM: Proper interface import for type safety.Good addition of the
ILogServiceinterface import to ensure proper typing of the mock service.
11-22: LGTM: Complete mock implementation aligned with ILogService interface.The mock service now properly implements all methods from the
ILogServiceinterface, ensuring type safety and completeness. This aligns well with the logging service refactoring mentioned in the PR objectives.
110-110: LGTM: Test assertion updated to match logging service changes.The assertion correctly reflects the change from using
info()toalways()for logging output, which is consistent with the broader logging service refactoring across CLI commands.
122-125: LGTM: Consistent assertion update for error scenarios.The test properly verifies that the warning message is logged using the
always()method instead of the previous approach, maintaining consistency with the logging service changes.
137-139: LGTM: Error handling test updated correctly.The assertion properly checks that error messages are logged using the
always()method, ensuring the test remains valid after the logging service refactoring.
- Set sandbox mode to false in api.json for production readiness. - Introduce methods for handling bundled and unbundled plugins in PluginManagementService. - Enhance plugin installation and uninstallation processes to differentiate between bundled and unbundled plugins.
|
Claude finished @elibosley's task —— View job Code Review AnalysisI've analyzed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage. Here's my comprehensive review: Todo List:
✅ StrengthsArchitecture & Design:
Error Handling:
Test Quality:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.12.0](v4.11.0...v4.12.0) (2025-07-30) ### Features * add ups monitoring to graphql api ([#1526](#1526)) ([6ea94f0](6ea94f0)) ### Bug Fixes * enhance plugin management with interactive removal prompts ([#1549](#1549)) ([23ef760](23ef760)) * remove connect api plugin upon removal of Connect Unraid plugin ([#1548](#1548)) ([782d5eb](782d5eb)) * SSO not being detected ([#1546](#1546)) ([6b3b951](6b3b951)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores