-
Couldn't load subscription status.
- Fork 11
fix: invalid state for unraid plugin #1492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce a new service ( Changes
Sequence Diagram(s)sequenceDiagram
participant NodeJS_Service as ConnectStatusWriterService
participant ConfigService
participant FileSystem
participant PHP_Script
NodeJS_Service->>ConfigService: Subscribe to config changes
ConfigService-->>NodeJS_Service: Notify on connect.mothership change
NodeJS_Service->>FileSystem: Write connectStatus.json with status info
PHP_Script->>FileSystem: Read connectStatus.json
PHP_Script->>PHP_Script: Parse JSON and extract connection status/origins
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts (2)
27-29: Apply optional chaining to simplify the condition.The static analysis tool correctly identified that this can be simplified.
- const minigraphChanged = Array.isArray(changes) && changes.some((change: any) => - change.path && change.path.startsWith('connect.mothership') - ); + const minigraphChanged = Array.isArray(changes) && changes.some((change: any) => + change.path?.startsWith('connect.mothership') + );
58-58: Use ES6 shorthand property syntax.Since the property name matches the variable name, you can use shorthand syntax.
- allowedOrigins: allowedOrigins, + allowedOrigins,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/UpdateFlashBackup.php(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: TypeScript imports use .js extensions for ESM compatibility
**/*.{ts,tsx}: TypeScript imports use .js extensions for ESM compatibility
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/unraid-api-plugin-connect/src/index.tspackages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.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: pujitm
PR: unraid/api#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: pujitm
PR: unraid/api#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.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
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: pujitm
PR: unraid/api#1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.
packages/unraid-api-plugin-connect/src/index.ts (4)
Learnt from: pujitm
PR: unraid/api#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: pujitm
PR: unraid/api#1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
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: pujitm
PR: unraid/api#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.
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php (2)
Learnt from: elibosley
PR: unraid/api#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: elibosley
PR: unraid/api#1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:19-24
Timestamp: 2025-05-08T19:28:54.365Z
Learning: The directory `/usr/local/emhttp/plugins/dynamix.my.servers` is a valid directory that exists as part of the Unraid API plugin installation and should be included in verification checks.
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/UpdateFlashBackup.php (2)
Learnt from: elibosley
PR: unraid/api#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: pujitm
PR: unraid/api#1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts (4)
Learnt from: pujitm
PR: unraid/api#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: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
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: elibosley
PR: unraid/api#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: unraid/api#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.
🧬 Code Graph Analysis (2)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php (1)
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts (1)
statusFilePath(13-16)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/UpdateFlashBackup.php (1)
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts (1)
statusFilePath(13-16)
🪛 Biome (1.9.4)
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts
[error] 28-28: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts (1)
1-5: TypeScript imports properly use .js extensions.Good adherence to the coding guidelines for ESM compatibility.
packages/unraid-api-plugin-connect/src/index.ts (1)
6-6: Clean module registration.The service is properly imported and registered as a provider, ensuring it will be instantiated and initialized with the module.
Also applies to: 14-14
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/UpdateFlashBackup.php (1)
26-35: Correct transition from INI to JSON status file.The implementation properly reads from the new JSON status file written by the
MinigraphStatusWriterService, extracts theminigraphfield, and maintains the same boolean logic for connection status. Error suppression with@json_decodeis appropriate for handling missing or invalid files.plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php (2)
171-172: Update comment to reflect new configuration location.The comment correctly describes the new location where the API writes the memory configuration.
214-225: Consistent JSON status file integration.The implementation correctly reads from the same JSON status file (
/var/local/emhttp/minigraph-status.json) as the other PHP components, maintaining consistency across the codebase. The extraction of bothminigraphstatus andallowedOriginsaligns with the data structure written by theMinigraphStatusWriterService.
| // Try to get allowed origins from the store | ||
| let allowedOrigins = ''; | ||
| try { | ||
| // We can't import from @app here, so we'll skip allowed origins for now | ||
| // This can be added later if needed | ||
| allowedOrigins = ''; | ||
| } catch (error) { | ||
| this.logger.debug('Could not get allowed origins:', error); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove redundant try-catch block.
This try-catch block doesn't serve any purpose since both the try and catch blocks set allowedOrigins to the same empty string value.
- // Try to get allowed origins from the store
- let allowedOrigins = '';
- try {
- // We can't import from @app here, so we'll skip allowed origins for now
- // This can be added later if needed
- allowedOrigins = '';
- } catch (error) {
- this.logger.debug('Could not get allowed origins:', error);
- }
+ // We can't import from @app here, so we'll skip allowed origins for now
+ // This can be added later if needed
+ const allowedOrigins = '';📝 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.
| // Try to get allowed origins from the store | |
| let allowedOrigins = ''; | |
| try { | |
| // We can't import from @app here, so we'll skip allowed origins for now | |
| // This can be added later if needed | |
| allowedOrigins = ''; | |
| } catch (error) { | |
| this.logger.debug('Could not get allowed origins:', error); | |
| } | |
| // We can't import from @app here, so we'll skip allowed origins for now | |
| // This can be added later if needed | |
| const allowedOrigins = ''; |
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/config/minigraph-status-writer.service.ts
between lines 44 and 52, remove the redundant try-catch block that sets
allowedOrigins to an empty string in both the try and catch sections. Instead,
directly assign allowedOrigins to an empty string without the try-catch,
simplifying the code and eliminating unnecessary error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts (2)
27-27: Simplify boolean check using optional chaining.The static analysis tool correctly identifies that this can be simplified using optional chaining.
- const connectionChanged = change.path && change.path.startsWith('connect.mothership'); + const connectionChanged = change.path?.startsWith('connect.mothership');
42-50: Remove redundant assignment in stubbed code.The
allowedOriginsvariable is set to empty string twice, which is redundant.// Try to get allowed origins from the store let allowedOrigins = ''; try { // We can't import from @app here, so we'll skip allowed origins for now // This can be added later if needed - allowedOrigins = ''; } catch (error) { this.logger.debug('Could not get allowed origins:', error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.claude/settings.local.json(1 hunks)packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/UpdateFlashBackup.php(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.local.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/unraid-api-plugin-connect/src/index.ts
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/UpdateFlashBackup.php
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: TypeScript imports use .js extensions for ESM compatibility
**/*.{ts,tsx}: TypeScript imports use .js extensions for ESM compatibility
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts
🧠 Learnings (2)
📓 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: pujitm
PR: unraid/api#1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Learnt from: elibosley
PR: unraid/api#969
File: web/justfile:7-9
Timestamp: 2024-11-27T15:30:02.252Z
Learning: In the Unraid Connect project, the different implementations of the `setup` commands in `web/justfile` and `api/justfile` are intentional and correct behavior.
Learnt from: pujitm
PR: unraid/api#1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:0-0
Timestamp: 2025-03-14T19:22:11.839Z
Learning: In the Unraid API project, input validation should happen at the store layer, not in service methods like ConnectSettingsService, to maintain proper separation of concerns.
Learnt from: pujitm
PR: unraid/api#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: mdatelle
PR: unraid/api#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: elibosley
PR: unraid/api#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: 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.
Learnt from: elibosley
PR: unraid/api#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: unraid/api#1425
File: api/dev/configs/connect.json:5-13
Timestamp: 2025-06-20T20:50:14.050Z
Learning: Connect configuration files like api/dev/configs/connect.json use their own naming conventions (e.g., apikey, accesstoken, idtoken, refreshtoken) as implementation details, and these should not be changed to follow general camelCase patterns.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:24-27
Timestamp: 2025-01-31T22:01:22.708Z
Learning: The Unraid UI uses a modern notification system with a custom `unraid-toaster` component replacing the legacy jGrowl notifications. The system is backed by a comprehensive GraphQL API with real-time subscription support for notification updates.
packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts (4)
Learnt from: pujitm
PR: unraid/api#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: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
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: pujitm
PR: unraid/api#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.
Learnt from: pujitm
PR: unraid/api#1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
🪛 Biome (1.9.4)
packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts
[error] 27-27: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Web App
- GitHub Check: Build Unraid UI Library (Webcomponent Version)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/unraid-api-plugin-connect/src/config/connect-status-writer.service.ts (5)
1-6: Imports follow ESM compatibility guidelines.The TypeScript imports correctly use .js extensions for ESM compatibility as required by the coding guidelines.
7-11: Well-structured service with proper dependency injection.The service correctly implements the OnModuleInit interface and properly injects the ConfigService with appropriate typing.
13-16: Hard-coded path is appropriate for this use case.The fixed path
/var/local/emhttp/connectStatus.jsonis well-documented and suitable for PHP consumption as indicated by the comment.
18-36: Robust lifecycle implementation with proper error handling.The onModuleInit method correctly writes initial status and sets up subscription for config changes with appropriate error handling.
38-68: Comprehensive status writing with excellent error handling.The writeStatus method properly retrieves connection metadata, constructs status data, and handles file writing with appropriate error logging.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.9.2](v4.9.1...v4.9.2) (2025-07-09) ### Bug Fixes * invalid configs no longer crash API ([#1491](#1491)) ([6bf3f77](6bf3f77)) * invalid state for unraid plugin ([#1492](#1492)) ([39b8f45](39b8f45)) * release note escaping ([5b6bcb6](5b6bcb6)) --- 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>
Summary by CodeRabbit
New Features
Chores