Feat/mothership v2 #1551
Feat/mothership v2 #1551
Conversation
WalkthroughThis pull request adds mothership proxy infrastructure to the Unraid API, introducing GraphQL forwarding services, subscription handling, and WebSocket-based server communication. Development configurations are updated with new environment variables, state files, and a package path alias change from Changes
Sequence DiagramsequenceDiagram
participant Mothership as Mothership Server
participant Client as UnraidServerClient
participant Executor as LocalGraphQLExecutor
participant Handler as MothershipSubscriptionHandler
participant Local as Local API
Mothership->>Client: WebSocket Connect
Client->>Mothership: Send API Key (Headers)
activate Client
Client->>Client: Periodic Ping/Keep-Alive
Mothership->>Client: Send GraphQL Query (remote_query)
activate Client
Client->>Client: Parse Message
Client->>Executor: execute({ query, operationName, variables })
activate Executor
Executor->>Executor: Transform Remote Query to Local<br/>(AST manipulation)
Executor->>Local: Execute Transformed Query
activate Local
Local-->>Executor: Query Result
deactivate Local
Executor-->>Client: Return Data or Error
deactivate Executor
Client->>Mothership: Send GraphQLResponse<br/>(operationId, messageId, data/error)
deactivate Client
Mothership->>Client: Subscription Start (subscription_start)
activate Client
Client->>Handler: addSubscription(sha256, operationId)
activate Handler
Handler->>Handler: Track in activeSubscriptions
Handler-->>Client: Subscription Registered
deactivate Handler
Client->>Mothership: Periodically Ping Subscriptions
activate Handler
Handler->>Handler: pingSubscription(sha256)
Handler->>Handler: Update lastPing
deactivate Handler
Mothership->>Client: Subscription Stop (subscription_stop)
activate Client
Client->>Handler: removeSubscription(sha256)
activate Handler
Handler->>Handler: Remove from activeSubscriptions
deactivate Handler
deactivate Client
rect rgba(200, 220, 255, 0.3)
note over Client: Reconnection with Exponential Backoff<br/>triggered on disconnect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
elibosley
force-pushed
the
feat/mothership-v2
branch
from
9ab5977 to
5caeb09
Compare
|
Claude finished @elibosley's task —— View job PR Review: Mothership v2I've completed a comprehensive review of this PR. Here's my detailed feedback: Review Tasks
🔍 Code Quality & ArchitectureStrengths:
Areas for Improvement: 1. Error Handling & Logging 🚨
2. Hardcoded Values & Magic Numbers
3. Code Duplication
🔒 Security Concerns1. API Key Exposure
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
api/.env.development (1)
20-20: LGTM: New environment variable supports centralized configuration.The addition of
PATHS_CONNECT_STATUSaligns with the shift toward centralized configuration management viaConfigService.Consider adding quotes around the value to match the project's .env formatting conventions:
-PATHS_CONNECT_STATUS=./dev/states/connectStatus.json # Connect status file for development +PATHS_CONNECT_STATUS="./dev/states/connectStatus.json" # Connect status file for developmentpackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts (1)
39-41: Simplify the disconnect logic.The null check is redundant since the optional chaining operator already handles null/undefined cases safely.
- if (this.clientService.getClient()) { - this.clientService.getClient()?.disconnect(); - } + this.clientService.getClient()?.disconnect();packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (2)
125-150: Consider making field name mappings configurable.The hardcoded field name mappings in
transformRemoteFieldNamecould become a maintenance burden as the API evolves. Consider extracting these mappings to a configuration object.+private readonly fieldNameMappings: Record<string, string> = { + 'remoteGetDockerInfo': 'docker', + 'remoteGetVms': 'vms', + 'remoteGetSystemInfo': 'system', +}; + private transformRemoteFieldName(fieldName: string): string { - // Handle common patterns - if (fieldName === 'remoteGetDockerInfo') { - return 'docker'; - } - if (fieldName === 'remoteGetVms') { - return 'vms'; - } - if (fieldName === 'remoteGetSystemInfo') { - return 'system'; - } + // Check configured mappings first + if (this.fieldNameMappings[fieldName]) { + return this.fieldNameMappings[fieldName]; + } // Generic transformation: remove "remoteGet" and convert to camelCase if (fieldName.startsWith('remoteGet')) {
152-155: Implement the stopSubscription method.The
stopSubscriptionmethod is currently just a stub. This could lead to memory leaks if subscriptions are not properly cleaned up.Do you want me to help implement the subscription cleanup logic or open an issue to track this task?
packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (1)
56-56: Validate WebSocket URL construction.The WebSocket URL is constructed by appending to the base URL. Ensure the base URL doesn't already end with a slash to avoid double slashes.
- const wsUrl = `${this.mothershipUrl}/ws/server` + const wsUrl = `${this.mothershipUrl.replace(/\/$/, '')}/ws/server`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/.env.development(1 hunks)api/dev/configs/api.json(1 hunks)api/dev/configs/connect.json(1 hunks)api/dev/keys/7789353b-40f4-4f3b-a230-b1f22909abff.json(1 hunks)api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json(0 hunks)api/generated-schema.graphql(5 hunks)api/src/environment.ts(1 hunks)packages/unraid-api-plugin-connect/codegen.ts(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts(2 hunks)packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts(3 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts(4 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/dev/keys/b5b4aa3d-8e40-4c92-bc40-d50182071886.json
🧰 Additional context used
📓 Path-based instructions (1)
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/dev/configs/api.jsonapi/dev/keys/7789353b-40f4-4f3b-a230-b1f22909abff.jsonapi/dev/configs/connect.jsonapi/src/environment.tsapi/generated-schema.graphql
🧠 Learnings (15)
📓 Common learnings
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: pujitm
PR: unraid/api#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: 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: 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.
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: mdatelle
PR: unraid/api#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: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-02-03T17:21:26.738Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.
api/dev/keys/7789353b-40f4-4f3b-a230-b1f22909abff.json (3)
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:41:22.303Z
Learning: In api/src/unraid-api/auth/auth.service.ts, the addRoleToApiKey function operates on API keys stored as JSON files in a directory, not a database, so concurrency is not a concern for modifying apiKey.roles.
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: elibosley
PR: #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.
api/dev/configs/connect.json (2)
Learnt from: elibosley
PR: #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: pujitm
PR: #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/connection-status/cloud.service.ts (6)
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: #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: #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.
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: 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: 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.
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts (9)
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: mdatelle
PR: #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: 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: 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: 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: 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.
packages/unraid-api-plugin-connect/package.json (14)
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: 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: pujitm
PR: #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.
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: 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: 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: #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: #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: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention
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: #1120
File: plugin/Dockerfile:1-3
Timestamp: 2025-02-05T14:25:37.316Z
Learning: The Dockerfile in the plugin directory is specifically for building artifacts during CI and is not used for production deployments. It creates a build environment with Node.js and necessary dependencies to generate plugin files.
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts (12)
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: .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: 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: 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: #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: #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: #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: #1367
File: packages/unraid-api-plugin-connect/src/event-handler/mothership.handler.ts:50-57
Timestamp: 2025-06-05T14:27:57.441Z
Learning: For NestJS applications with async event handlers using @onevent decorator, implement generic error handling using a custom decorator or base class rather than individual try-catch blocks in each handler method to avoid application crashes from unhandled promise rejections.
Learnt from: pujitm
PR: #1367
File: packages/unraid-api-plugin-connect/src/event-handler/mothership.handler.ts:50-57
Timestamp: 2025-06-05T14:27:57.441Z
Learning: NestJS async event handlers using @onevent decorator can crash the entire application if exceptions are not caught. Industry best practices recommend generic error handling approaches like custom decorators, Sentry's automatic wrapping, or global event exception services rather than individual try-catch blocks in each handler.
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: #1352
File: packages/unraid-api-plugin-connect/src/config.persistence.ts:0-0
Timestamp: 2025-04-21T18:44:39.643Z
Learning: Use Promise-based file writing instead of callback-based approaches for better error handling and to allow proper propagation of success/failure states to callers.
api/.env.development (5)
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: 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.
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: #1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in web/_data/serverState.ts are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
api/src/environment.ts (11)
Learnt from: elibosley
PR: #1381
File: plugin/builder/cli/common-environment.ts:18-22
Timestamp: 2025-05-08T19:28:37.034Z
Learning: In plugin/builder/cli/common-environment.ts, the getDefaultBaseUrl() function intentionally returns an invalid URL when running in CI environments to force explicit specification of the base URL parameter rather than relying on defaults.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.mutation.ts : GraphQL mutations must be placed in .mutation.ts files
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention (.query.ts for queries, .mutation.ts for mutations)
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.mutation.ts : GraphQL mutations must be placed in .mutation.ts files under web/
Learnt from: elibosley
PR: #1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in web/_data/serverState.ts are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
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: #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/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 scripts from api/package.json unless requested
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.
packages/unraid-api-plugin-connect/codegen.ts (13)
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: #942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:41-47
Timestamp: 2024-11-05T14:48:56.949Z
Learning: Input types for GraphQL mutations can be pulled from the GraphQL codegen types, as they are generated into the API files. Therefore, explicit typing in the resolver files may be unnecessary.
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: #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.
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: 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: 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.
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention (.query.ts for queries, .mutation.ts for mutations)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.query.ts : GraphQL queries must be placed in .query.ts files
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.query.ts : GraphQL queries must be placed in .query.ts files under web/
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: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.mutation.ts : GraphQL mutations must be placed in .mutation.ts files under web/
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts (5)
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: mdatelle
PR: #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: 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: 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.
packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (7)
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: #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/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention (.query.ts for queries, .mutation.ts for mutations)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.query.ts : GraphQL queries must be placed in .query.ts files
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.query.ts : GraphQL queries must be placed in .query.ts files under web/
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (9)
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: pujitm
PR: #1075
File: web/components/Notifications/Sidebar.vue:52-57
Timestamp: 2025-01-30T19:56:42.055Z
Learning: Notification links in the Unraid API are validated at creation time on the server side, making additional client-side validation unnecessary when receiving notifications through GraphQL subscriptions.
Learnt from: elibosley
PR: #1526
File: api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts:82-85
Timestamp: 2025-07-22T13:29:32.175Z
Learning: In GraphQL subscriptions using the graphql-subscriptions PubSub, the correct method is asyncIterableIterator(), not asyncIterator(). The asyncIterableIterator method returns an AsyncIterableIterator that conforms to both AsyncIterable and AsyncIterator interfaces, which is required for GraphQL subscription resolvers. Using asyncIterator() would cause TypeScript type errors as it only returns an AsyncIterator that doesn't satisfy the AsyncIterable interface requirement.
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: 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: pujitm
PR: #978
File: web/helpers/apollo-cache/index.ts:133-157
Timestamp: 2024-12-11T17:20:09.604Z
Learning: In web/helpers/apollo-cache/index.ts, when updating the cache in the cache.updateQuery function, it's sufficient to check if data is present, as type checking inside data is handled at the GraphQL layer.
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: pujitm
PR: #1148
File: web/composables/gql/graphql.ts:1812-1821
Timestamp: 2025-02-19T17:30:55.418Z
Learning: When using Apollo Vue's subscribeToMore, throttling can be implemented by wrapping the update function with lodash's throttle or using RxJS's throttleTime operator to prevent flooding the client with updates.
Learnt from: elibosley
PR: #1526
File: api/src/unraid-api/graph/resolvers/ups/ups.resolver.ts:82-85
Timestamp: 2025-07-22T13:29:32.175Z
Learning: In GraphQL subscriptions using the graphql-subscriptions PubSub, the correct method to create an async iterator is asyncIterableIterator(), not asyncIterator(). This is the modern/current method used in the graphql-subscriptions library.
packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (7)
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: 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: 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/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: 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: #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: 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.
api/generated-schema.graphql (1)
Learnt from: elibosley
PR: #942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:41-47
Timestamp: 2024-11-05T14:48:56.949Z
Learning: Input types for GraphQL mutations can be pulled from the GraphQL codegen types, as they are generated into the API files. Therefore, explicit typing in the resolver files may be unnecessary.
🪛 Gitleaks (8.27.2)
api/dev/keys/7789353b-40f4-4f3b-a230-b1f22909abff.json
5-5: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: CI - Main (API)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
[error] 64-64: TypeError: The "path" argument must be of type string. Received an instance of Object. This error occurs multiple times during writeStatus calls.
🪛 dotenv-linter (3.3.0)
api/.env.development
[warning] 20-20: [UnorderedKey] The PATHS_CONNECT_STATUS key should go before the PATHS_CONNECT_STATUS_FILE_PATH key
[warning] 20-20: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 21-21: [QuoteCharacter] The value has quote characters (', ")
[warning] 21-21: [UnorderedKey] The ENVIRONMENT key should go before the PATHS_ACTIVATION_BASE key
[warning] 22-22: [QuoteCharacter] The value has quote characters (', ")
[warning] 22-22: [UnorderedKey] The NODE_ENV key should go before the PATHS_ACTIVATION_BASE key
[warning] 23-23: [QuoteCharacter] The value has quote characters (', ")
[warning] 24-24: [UnorderedKey] The PLAYGROUND key should go before the PORT key
[warning] 25-25: [UnorderedKey] The INTROSPECTION key should go before the NODE_ENV key
[warning] 26-26: [QuoteCharacter] The value has quote characters (', ")
[warning] 26-26: [UnorderedKey] The MOTHERSHIP_BASE_URL key should go before the NODE_ENV key
🔇 Additional comments (20)
api/src/environment.ts (1)
96-100: LGTM! Clean environment variable refactor.The renaming from
MOTHERSHIP_GRAPHQL_LINKtoMOTHERSHIP_BASE_URLand removal of the/wssuffix aligns well with the architectural shift from GraphQL-over-HTTP to WebSocket-based communication. The conditional logic and fallback behavior are preserved correctly.api/dev/configs/api.json (1)
2-2: Appropriate version bump for the architectural changes.The minor version increment from "4.11.0" to "4.12.0" correctly reflects the introduction of new WebSocket-based mothership communication features while maintaining backward compatibility.
packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts (2)
96-96: LGTM! Consistent environment variable update.The change from
MOTHERSHIP_GRAPHQL_LINKtoMOTHERSHIP_BASE_URLaligns with the environment configuration updates and maintains the existing connectivity check functionality.
207-207: LGTM! Consistent environment variable update.The change from
MOTHERSHIP_GRAPHQL_LINKtoMOTHERSHIP_BASE_URLaligns with the environment configuration updates and maintains the existing DNS check functionality.api/dev/keys/7789353b-40f4-4f3b-a230-b1f22909abff.json (1)
1-11: Development API key correctly scoped to development environmentNo references to this key or its secret were found in production code. Keeping the JSON file under
api/dev/keys/is appropriate for development/testing purposes.packages/unraid-api-plugin-connect/package.json (1)
16-16: LGTM! Script cleanup aligns with architectural changes.Removing the inline
MOTHERSHIP_GRAPHQL_LINKenvironment variable from the codegen script is consistent with the transition to WebSocket-based communication and the renaming of environment variables throughout the codebase.packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts (2)
11-11: LGTM: Clean architectural transition to WebSocket-based services.The addition of
LocalGraphQLExecutorandUnraidServerClientServiceimports aligns well with the broader shift from HTTP GraphQL to WebSocket-based mothership communication.Also applies to: 15-15
23-25: LGTM: Provider registration looks correct.The replacement of
MothershipGraphqlClientServicewithUnraidServerClientServiceandLocalGraphQLExecutorin the providers array is properly structured and consistent with the architectural changes.api/.env.development (1)
26-26: LGTM: URL change supports WebSocket architecture transition.The change from
MOTHERSHIP_GRAPHQL_LINKtoMOTHERSHIP_BASE_URLwith the new localhost URL correctly supports the shift to WebSocket-based communication.api/dev/configs/connect.json (1)
11-18: LGTM: Configuration additions support enhanced connect functionality.The new fields (
version,extraOrigins,sandbox, and token fields) provide necessary configuration placeholders for the updated connect architecture. The JSON structure remains valid and maintains backward compatibility.packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts (2)
4-5: LGTM: Proper imports for directory handling.The addition of
mkdiranddirnameimports supports the enhanced directory creation functionality.
63-66: LGTM: Enhanced robustness with directory creation.The addition of directory creation before writing the status file improves robustness by ensuring the target directory exists.
packages/unraid-api-plugin-connect/codegen.ts (1)
29-31: LGTM: GraphQL codegen removal aligns with WebSocket architecture.Removing the mothership GraphQL type generation is the correct approach since the new
UnraidServerClientServicehandles GraphQL operations dynamically over WebSocket connections rather than using pre-generated static types.packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts (2)
5-19: LGTM! Clean transition to WebSocket-based architecture.The migration from
MothershipGraphqlClientServicetoUnraidServerClientServiceis well-implemented, with appropriate updates to comments and dependency injection.
51-57: Good defensive programming with proper validation.The addition of the
apiKeycheck alongsideisLoadedensures the mothership connection is only established when proper credentials are available.packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (1)
24-24: LocalGraphQLExecutor correctly injects InternalClientService
- The constructor’s dependency on InternalClientService is intentional and matches its use elsewhere (e.g., in MothershipSubscriptionHandler). No change to use UnraidServerClientService is needed.
- Remaining suggestions for this new executor:
- Implement stopSubscription (around line 154) or document its intended behavior.
- Externalize the hard-coded field-name mappings (currently in lines 68–109 and 125–150) into a configuration or constants file for easier maintenance.
[fix_required]packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (2)
402-418: LGTM! Proper cleanup in disconnect method.The disconnect method correctly:
- Disables reconnection
- Clears ping interval
- Clears reconnection timeout
- Closes the WebSocket connection
This prevents memory leaks and ensures clean shutdown.
210-248: Excellent reconnection implementation with exponential backoff.The reconnection logic is well-implemented with:
- Exponential backoff starting at 1 second
- Maximum delay cap of 30 minutes
- Clear logging of reconnection attempts
- Proper cleanup of existing timeouts
api/generated-schema.graphql (2)
1-3: Auto-generated file - changes look appropriate.This schema file is automatically generated and reflects the architectural changes in the PR. The removal of remote access/cloud types and addition of comprehensive UPS management features align with the WebSocket-based mothership communication refactor.
1398-1526: Well-documented UPS types with clear field descriptions.The new UPS-related types are thoroughly documented with units, examples, and clear explanations. This will greatly help API consumers understand the data structure.
| // Use environment variable if set, otherwise default to /var/local/emhttp/connectStatus.json | ||
| return this.configService.get('PATHS_CONNECT_STATUS') || '/var/local/emhttp/connectStatus.json'; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix critical path handling issue causing pipeline failures.
The pipeline failures indicate that configService.get('PATHS_CONNECT_STATUS') is returning an object instead of a string, causing a TypeError when passed to dirname().
The issue is likely that the configuration value is not being properly typed or retrieved. Apply this fix:
get statusFilePath() {
- // Use environment variable if set, otherwise default to /var/local/emhttp/connectStatus.json
- return this.configService.get('PATHS_CONNECT_STATUS') || '/var/local/emhttp/connectStatus.json';
+ // Use environment variable if set, otherwise default to /var/local/emhttp/connectStatus.json
+ return this.configService.get<string>('PATHS_CONNECT_STATUS') || '/var/local/emhttp/connectStatus.json';
}Run this script to verify the configuration service implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check how PATHS_CONNECT_STATUS is configured in the environment and config service
# Find configuration files that might define PATHS_CONNECT_STATUS
rg -n "PATHS_CONNECT_STATUS" --type json --type ts
# Look for ConfigService usage patterns
ast-grep --pattern 'configService.get($$$)'Length of output: 428
Enforce string type and validate PATHS_CONNECT_STATUS to prevent TypeError
Ensure the config value is both statically typed and runtime-validated in connect-status-writer.service.ts so that a non-string never reaches dirname().
• File: packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
Method: statusFilePath
get statusFilePath(): string {
- // Use environment variable if set, otherwise default to /var/local/emhttp/connectStatus.json
- return this.configService.get('PATHS_CONNECT_STATUS') || '/var/local/emhttp/connectStatus.json';
+ // Use environment variable if set, otherwise default to /var/local/emhttp/connectStatus.json
+ const rawPath = this.configService.get<string>('PATHS_CONNECT_STATUS');
+ if (typeof rawPath !== 'string') {
+ throw new TypeError(
+ `Invalid config "PATHS_CONNECT_STATUS": expected string, received ${typeof rawPath}`
+ );
+ }
+ return rawPath || '/var/local/emhttp/connectStatus.json';
}Also verify in your configuration files (JSON, .env or TS) that PATHS_CONNECT_STATUS is defined as a string.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
around lines 17 to 19, the config value for PATHS_CONNECT_STATUS should be
explicitly checked to ensure it is a string before returning it. Update the
statusFilePath method to validate the type of the config value at runtime,
returning the default path only if the config value is either undefined or not a
string. Also, verify that PATHS_CONNECT_STATUS is defined as a string in all
configuration files such as JSON, .env, or TypeScript files.
| const client = this.mothershipClient.getClient(); | ||
| if (client && subscription.operationId) { | ||
| // Note: We can't directly call stopSubscription on the client since it's private | ||
| // This would need to be exposed or handled differently in a real implementation | ||
| this.logger.debug(`Should stop subscription with operationId: ${subscription.operationId}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the architectural limitation with stopSubscription.
The comment indicates that stopSubscription cannot be called on the client because it's private. This is an architectural issue that should be resolved by exposing the necessary methods on the UnraidServerClient.
Consider exposing a public method on UnraidServerClient to stop subscriptions:
// In UnraidServerClient class
public async stopSubscription(operationId: string): Promise<void> {
if (this.executor.stopSubscription) {
await this.executor.stopSubscription(operationId);
}
// Send subscription_stop message to mothership
this.sendResponse({
operationId,
event: 'query_response',
type: 'complete',
payload: { data: null },
});
}🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
around lines 32 to 37, the code cannot call stopSubscription on the client
because it is private. To fix this, add a public async method
stopSubscription(operationId: string) to the UnraidServerClient class that
internally calls the private executor.stopSubscription if available and sends
the appropriate subscription_stop message to mothership. Then update this
handler to call the new public stopSubscription method on the client instead of
accessing the private method directly.
| stopMothershipSubscription() { | ||
| this.logger.verbose('Stopping mothership subscription (not implemented yet)'); | ||
| } | ||
|
|
||
| async handleRemoteGraphQLEvent(event: RemoteGraphQlEventFragmentFragment) { | ||
| const { body, type, sha256 } = event.remoteGraphQLEventData; | ||
| switch (type) { | ||
| case RemoteGraphQlEventType.REMOTE_QUERY_EVENT: | ||
| return this.safeExecuteQuery(sha256, body); | ||
| case RemoteGraphQlEventType.REMOTE_SUBSCRIPTION_EVENT: | ||
| return this.addSubscription(event.remoteGraphQLEventData); | ||
| case RemoteGraphQlEventType.REMOTE_SUBSCRIPTION_EVENT_PING: | ||
| return this.pingSubscription(sha256); | ||
| default: | ||
| return; | ||
| async subscribeToMothershipEvents() { | ||
| this.logger.log('Subscribing to mothership events via UnraidServerClient'); | ||
|
|
||
| // For now, just log that we're connected | ||
| // The UnraidServerClient handles the WebSocket connection automatically | ||
| const client = this.mothershipClient.getClient(); | ||
| if (client) { | ||
| this.logger.log('UnraidServerClient is connected and handling mothership communication'); | ||
| } else { | ||
| this.logger.warn('UnraidServerClient is not available'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement proper mothership event subscription.
The current implementation only logs connection status. This should set up actual event subscriptions through the WebSocket client.
The method needs to:
- Subscribe to mothership events via the WebSocket client
- Handle incoming events and route them appropriately
- Manage subscription lifecycle
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
around lines 104 to 119, the subscribeToMothershipEvents method currently only
logs connection status without setting up real event subscriptions. Update this
method to use the WebSocket client from mothershipClient to subscribe to actual
mothership events, add event listeners to handle incoming events and route them
properly, and implement logic to manage the subscription lifecycle including
cleanup or reconnection as needed.
| async executeQuery(sha256: string, body: string) { | ||
| this.logger.debug(`Request to execute query ${sha256}: ${body} (simplified implementation)`); | ||
|
|
||
| try { | ||
| // For now, just return a success response | ||
| // TODO: Implement actual query execution via the UnraidServerClient | ||
| return { | ||
| data: { | ||
| message: 'Query executed successfully (simplified)', | ||
| sha256, | ||
| } | ||
| }, | ||
| }); | ||
| }; | ||
| } catch (error: any) { | ||
| this.logger.error(`Error executing query ${sha256}:`, error); | ||
| return { | ||
| errors: [ | ||
| { | ||
| message: `Query execution failed: ${error?.message || 'Unknown error'}`, | ||
| extensions: { code: 'EXECUTION_ERROR' }, | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Complete the executeQuery implementation.
The method currently returns mock data with a TODO comment. This needs to be implemented to execute actual queries via the UnraidServerClient.
This method should execute queries through the WebSocket client. Do you want me to help implement the proper query execution logic?
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
between lines 121 and 144, the executeQuery method currently returns mock data
and has a TODO to implement actual query execution. Replace the mock response by
invoking the appropriate method on the UnraidServerClient WebSocket client to
execute the query using the provided sha256 and body parameters. Handle the
response and errors properly, returning the actual query result or formatted
error information as needed.
| interface GraphQLExecutor { | ||
| execute(params: { | ||
| query: string | ||
| variables?: Record<string, any> | ||
| operationName?: string | ||
| operationType?: 'query' | 'mutation' | 'subscription' | ||
| }): Promise<any> | ||
| stopSubscription?(operationId: string): Promise<void> | ||
| } |
There was a problem hiding this comment.
Remove duplicate GraphQLExecutor interface.
This interface is already defined in local-graphql-executor.service.ts. Import it instead of duplicating the definition.
+import { GraphQLExecutor } from './local-graphql-executor.service.js';
+
-interface GraphQLExecutor {
- execute(params: {
- query: string
- variables?: Record<string, any>
- operationName?: string
- operationType?: 'query' | 'mutation' | 'subscription'
- }): Promise<any>
- stopSubscription?(operationId: string): Promise<void>
-}🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts
around lines 25 to 33, the GraphQLExecutor interface is duplicated but already
exists in local-graphql-executor.service.ts. Remove this local interface
definition and import the GraphQLExecutor interface from
local-graphql-executor.service.ts to avoid duplication and maintain consistency.
|
Claude finished @elibosley's task —— View job PR Review: Mothership v2I've completed a comprehensive review of this PR. Here's my detailed feedback: Review Tasks
🔍 Code Quality & ArchitectureStrengths:
Areas for Improvement: 1. Error Handling & Logging 🚨
2. Hardcoded Values & Magic Numbers
3. Code Duplication
🔒 Security Concerns1. API Key Exposure
|
…ient - Updated package.json scripts to remove MOTHERSHIP_GRAPHQL_LINK environment variable. - Changed MOTHERSHIP_GRAPHQL_LINK to MOTHERSHIP_BASE_URL in environment.ts. - Removed GraphQL code generation for Mothership types in codegen.ts. - Updated connection status services to use MOTHERSHIP_BASE_URL. - Refactored MothershipSubscriptionHandler to utilize UnraidServerClient instead of GraphQL client. - Implemented UnraidServerClient for WebSocket communication with Mothership. - Enhanced MothershipController to manage UnraidServerClient lifecycle. - Added reconnection logic and ping/pong handling in UnraidServerClient. - Simplified GraphQL execution logic in UnraidServerClient.
- Introduced new types and enums for managing remote access configurations, including AccessUrl, RemoteAccess, and Connect settings. - Added mutations for updating API settings and managing remote access. - Updated the API configuration to include the new connect plugin. - Enhanced the pnpm lock file with the addition of the pify package. - Implemented logic to skip file modifications in development mode.
elibosley
force-pushed
the
feat/mothership-v2
branch
from
4dd3718 to
fcf74d3
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts
Fixed
Show fixed
Hide fixed
…port - Added a new package for the Unraid API plugin, featuring a modular structure for connection management and remote access. - Implemented GraphQL resolvers and services for cloud connection status, dynamic remote access, and network management. - Updated code generation configuration to support new GraphQL types and queries. - Refactored existing services to utilize the new GraphQL client for improved performance and maintainability. - Included comprehensive tests for new functionalities to ensure reliability and stability.
| portSsl, | ||
| }: UrlForFieldInputInsecure | UrlForFieldInputSecure): URL { | ||
| let portToUse = ''; | ||
| let httpMode = 'https://'; |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To fix the useless assignment, simply remove the initialization of httpMode on line 135. Instead, declare httpMode as a local let variable without a value (i.e., let httpMode;). This makes it clear that its value will be set later based on conditional logic. The assignment of 'http://' or 'https://' remains as part of the flow in the branches, so this edit preserves all existing functionality.
Edits are restricted to lines 135 of the method inside the shown code in packages/unraid-api-plugin-connect-2/src/network/url-resolver.service.ts.
No new methods or imports are required.
| @@ -132,7 +132,7 @@ | ||
| portSsl, | ||
| }: UrlForFieldInputInsecure | UrlForFieldInputSecure): URL { | ||
| let portToUse = ''; | ||
| let httpMode = 'https://'; | ||
| let httpMode; | ||
|
|
||
| if (!url || url === '') { | ||
| throw new Error('No URL Provided'); |
| } | ||
|
|
||
| private async fastCheckCloud(): Promise<CloudResponse> { | ||
| let ip = 'FAST_CHECK_NO_IP_FOUND'; |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To fix the problem, simply remove the initial assignment to ip on line 170 and declare the variable as a let without initialization (let ip;). This ensures ip is only set by the results of the try and catch blocks and eliminates the useless assignment, making the code cleaner without altering its behaviour. This change is local to the declaration of ip in the method fastCheckCloud() within packages/unraid-api-plugin-connect-2/src/connection-status/cloud.service.ts. No further changes or imports are required.
| @@ -167,7 +167,7 @@ | ||
| } | ||
|
|
||
| private async fastCheckCloud(): Promise<CloudResponse> { | ||
| let ip = 'FAST_CHECK_NO_IP_FOUND'; | ||
| let ip; | ||
| try { | ||
| ip = await this.checkDns(); | ||
| } catch (error) { |
| import { UserSettingsService } from '@unraid/shared/services/user-settings.js'; | ||
| import { execa } from 'execa'; | ||
| import { GraphQLError } from 'graphql/error/GraphQLError.js'; | ||
| import { decodeJwt } from 'jose'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To resolve this issue, the unused import of decodeJwt from the 'jose' package should be removed. This can be done by deleting line 14 from the file packages/unraid-api-plugin-connect-2/src/unraid-connect/connect-settings.service.ts, which reads import { decodeJwt } from 'jose';. No other code or imports need to be changed, as no usage of this import is shown elsewhere in the provided code.
| @@ -11,7 +11,6 @@ | ||
| import { UserSettingsService } from '@unraid/shared/services/user-settings.js'; | ||
| import { execa } from 'execa'; | ||
| import { GraphQLError } from 'graphql/error/GraphQLError.js'; | ||
| import { decodeJwt } from 'jose'; | ||
|
|
||
| import type { | ||
| ConnectSettingsInput, |
| import { | ||
| AllowedOriginInput, | ||
| ConnectSettings, | ||
| ConnectSettingsInput, | ||
| ConnectSettingsValues, | ||
| ConnectSignInInput, | ||
| EnableDynamicRemoteAccessInput, | ||
| RemoteAccess, | ||
| SetupRemoteAccessInput, | ||
| } from './connect.model.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To fix the issue, we should remove the unused import AllowedOriginInput from the destructured import statement at line 15. Only changes to the import statement are required; there is no need to modify any other part of the code. The rest of the imports from './connect.model.js' remain untouched, as they might be used elsewhere in the file.
This change is scoped to the file packages/unraid-api-plugin-connect-2/src/unraid-connect/connect-settings.resolver.ts, specifically the import region at lines 14–23.
| @@ -12,7 +12,6 @@ | ||
| import { EVENTS } from '../helper/nest-tokens.js'; | ||
| import { ConnectSettingsService } from './connect-settings.service.js'; | ||
| import { | ||
| AllowedOriginInput, | ||
| ConnectSettings, | ||
| ConnectSettingsInput, | ||
| ConnectSettingsValues, |
| import { ConfigService } from '@nestjs/config'; | ||
| import { EventEmitter2 } from '@nestjs/event-emitter'; | ||
|
|
||
| import { ConfigType, DynamicRemoteAccessType, MyServersConfig } from '../config/connect.config.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To fix this problem, simply remove the unused imports DynamicRemoteAccessType and MyServersConfig from the import statement on line 5. This will improve code clarity and maintainability without changing existing functionality. No additional methods, imports, or definitions are needed. Only the relevant names should be deleted from the curly-brace list of imports.
| @@ -2,7 +2,7 @@ | ||
| import { ConfigService } from '@nestjs/config'; | ||
| import { EventEmitter2 } from '@nestjs/event-emitter'; | ||
|
|
||
| import { ConfigType, DynamicRemoteAccessType, MyServersConfig } from '../config/connect.config.js'; | ||
| import { ConfigType } from '../config/connect.config.js'; | ||
| import { EVENTS } from '../helper/nest-tokens.js'; | ||
| import { AccessUrl, UrlResolverService } from '../network/url-resolver.service.js'; | ||
|
|
| import { | ||
| IsArray, | ||
| IsBoolean, | ||
| IsEmail, | ||
| IsEnum, | ||
| IsNumber, | ||
| IsOptional, | ||
| IsString, | ||
| Matches, | ||
| ValidateIf, | ||
| } from 'class-validator'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To resolve the issue, remove the unused imports IsArray and Matches from the import statement for class-validator in packages/unraid-api-plugin-connect-2/src/config/connect.config.ts. This will improve code clarity, maintainability, and possibly build performance (especially for tools that perform tree shaking). The only change needed is to update the import block on lines 7–17, removing these two names. No other regions of code need to be edited, and no further actions or definitions are required.
| @@ -5,14 +5,12 @@ | ||
| import { URL_TYPE } from '@unraid/shared/network.model.js'; | ||
| import { plainToInstance } from 'class-transformer'; | ||
| import { | ||
| IsArray, | ||
| IsBoolean, | ||
| IsEmail, | ||
| IsEnum, | ||
| IsNumber, | ||
| IsOptional, | ||
| IsString, | ||
| Matches, | ||
| ValidateIf, | ||
| } from 'class-validator'; | ||
|
|
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { MinigraphStatus } from '../config/connect.config.js'; | ||
| import { EVENTS, GRAPHQL_PUBSUB_CHANNEL } from '../helper/nest-tokens.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To fix this problem, remove the unused EVENTS import from the import statement on line 7 in packages/unraid-api-plugin-connect-2/src/__test__/mothership.events.test.ts. You should retain any other actually-used imports in the same statement—in this case, GRAPHQL_PUBSUB_CHANNEL. This change does not affect any existing functionality as the identifier was never referenced in the current file.
| @@ -4,7 +4,7 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { MinigraphStatus } from '../config/connect.config.js'; | ||
| import { EVENTS, GRAPHQL_PUBSUB_CHANNEL } from '../helper/nest-tokens.js'; | ||
| import { GRAPHQL_PUBSUB_CHANNEL } from '../helper/nest-tokens.js'; | ||
| import { MothershipConnectionService } from '../mothership-proxy/connection.service.js'; | ||
| import { MothershipController } from '../mothership-proxy/mothership.controller.js'; | ||
| import { MothershipHandler } from '../mothership-proxy/mothership.events.js'; |
|
|
||
| // Since we're not mocking Apollo Client, this will create a real client | ||
| // We just want to verify the state check works | ||
| const client = service.getClient(); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
The best way to fix the identified issue is simply to remove the unused declaration of client on line 130. This means deleting the entire line:
const client = service.getClient();This line is not referenced later in the test, and its removal will not alter the logic or clarity of the test case. No other changes are needed elsewhere in the file, as getClient() is still invoked for side-effect and verified in the very next lines. Keeping the invocation only in the assertion (as is already the case immediately below with expect(() => service.getClient()).not.toThrow();) is sufficient.
| @@ -127,7 +127,6 @@ | ||
|
|
||
| // Since we're not mocking Apollo Client, this will create a real client | ||
| // We just want to verify the state check works | ||
| const client = service.getClient(); | ||
|
|
||
| // The client should either be null (if not created yet) or an Apollo client instance | ||
| // The key is that it doesn't throw an error when state is valid |
| @@ -0,0 +1,304 @@ | |||
| import { ConfigService } from '@nestjs/config'; | |||
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To remediate this issue, remove the unused import of ConfigService from @nestjs/config at the top of the file (packages/unraid-api-plugin-connect-2/src/__test__/config.validation.test.ts). This is done by deleting line 1, as ConfigService is not referenced anywhere in the file. No other code changes are necessary.
| @@ -1,5 +1,5 @@ | ||
| import { ConfigService } from '@nestjs/config'; | ||
|
|
||
|
|
||
| import { faker } from '@faker-js/faker'; | ||
| import * as fc from 'fast-check'; | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; |
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { ConnectConfigPersister } from '../config/config.persistence.js'; | ||
| import { ConfigType, DynamicRemoteAccessType } from '../config/connect.config.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
To resolve this issue, the unused import DynamicRemoteAccessType should be removed from the file packages/unraid-api-plugin-connect-2/src/__test__/config.persistence.test.ts. Specifically, the import statement on line 8 should be edited to only import ConfigType from ../config/connect.config.js. This change will improve code quality and readability, with no effect on existing functionality.
| @@ -5,7 +5,7 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { ConnectConfigPersister } from '../config/config.persistence.js'; | ||
| import { ConfigType, DynamicRemoteAccessType } from '../config/connect.config.js'; | ||
| import { ConfigType } from '../config/connect.config.js'; | ||
|
|
||
| describe('ConnectConfigPersister', () => { | ||
| let service: ConnectConfigPersister; |
- Removed the deprecated internal API key JSON file. - Renamed environment variable from MOTHERSHIP_BASE_URL to MOTHERSHIP_GRAPHQL_LINK for clarity. - Updated CloudService and UnraidServerClientService to utilize the new MOTHERSHIP_GRAPHQL_LINK for API calls, ensuring consistent access to the Mothership API.
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/generated-schema.graphql (1)
1983-2111: Duplicate UPS type definitions in schema.The UPS types (
UPSBattery,UPSPower,UPSDevice,UPSConfiguration) are defined twice in the schema. Lines 2113–2241 are exact duplicates of lines 1983–2111. This will cause GraphQL schema validation to fail with "Type already defined" errors.Since this file is auto-generated (per the header on line 2), verify the upstream schema source or code generation pipeline to eliminate duplicate type definitions. Remove one set of duplicate type definitions.
Also applies to: 2113-2241
🧹 Nitpick comments (1)
api/.env.development (1)
22-22: Add quotes around the PATHS_CONNECT_STATUS value for consistency.Line 22 defines
PATHS_CONNECT_STATUSwithout surrounding quotes, while line 28 (MOTHERSHIP_BASE_URL) does use quotes. To maintain consistency and follow dotenv best practices, wrap the value in quotes.-PATHS_CONNECT_STATUS=./dev/states/connectStatus.json # Connect status file for development +PATHS_CONNECT_STATUS="./dev/states/connectStatus.json" # Connect status file for development
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/.env.development(1 hunks)api/dev/states/connectStatus.json(1 hunks)api/generated-schema.graphql(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/dev/states/connectStatus.jsonapi/generated-schema.graphql
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
api/dev/states/connectStatus.jsonapi/generated-schema.graphql
🧠 Learnings (1)
📚 Learning: 2025-06-20T20:50:14.050Z
Learnt from: elibosley
Repo: unraid/api PR: 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.
Applied to files:
api/.env.development
🪛 dotenv-linter (4.0.0)
api/.env.development
[warning] 22-22: [UnorderedKey] The PATHS_CONNECT_STATUS key should go before the PATHS_CONNECT_STATUS_FILE_PATH key
(UnorderedKey)
[warning] 22-22: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 23-23: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 23-23: [UnorderedKey] The ENVIRONMENT key should go before the PATHS_ACTIVATION_BASE key
(UnorderedKey)
[warning] 24-24: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 24-24: [UnorderedKey] The NODE_ENV key should go before the PATHS_ACTIVATION_BASE key
(UnorderedKey)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [UnorderedKey] The PLAYGROUND key should go before the PORT key
(UnorderedKey)
[warning] 27-27: [UnorderedKey] The INTROSPECTION key should go before the NODE_ENV key
(UnorderedKey)
[warning] 28-28: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 28-28: [UnorderedKey] The MOTHERSHIP_BASE_URL key should go before the NODE_ENV key
(UnorderedKey)
🔇 Additional comments (2)
api/dev/states/connectStatus.json (1)
1-7: LGTM!Valid development state file with appropriate structure for the connect status initialization.
api/generated-schema.graphql (1)
2529-2531: Duplicate UPS query fields in Query type.The UPS-related query fields (
upsDevices,upsDeviceById,upsConfiguration) appear twice in the Query type definition—at lines 2529–2531 and again at lines 2548–2550. GraphQL schema validation will fail with "Field already defined" errors.Remove the duplicate query field definitions at lines 2548–2550. Only one set of UPS query definitions should remain in the Query type.
Also applies to: 2548-2550
⛔ Skipped due to learnings
Learnt from: pujitm Repo: unraid/api 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.
…raphQL integration - Renamed the unraid-api-plugin-connect package to unraid-api-plugin-connect-2 for improved clarity. - Updated dependencies in pnpm-lock.yaml to reflect the new package structure. - Modified environment variables for Mothership API integration to use a new GraphQL link. - Refactored services to utilize the updated package and improved internal client handling. - Removed deprecated UPS-related types from the GraphQL schema to streamline the API.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts (1)
25-33: Remove duplicate GraphQLExecutor interface.This interface is already defined in
local-graphql-executor.service.ts. Import it instead.+import { GraphQLExecutor } from './local-graphql-executor.service.js'; + -interface GraphQLExecutor { - execute(params: { - query: string - variables?: Record<string, any> - operationName?: string - operationType?: 'query' | 'mutation' | 'subscription' - }): Promise<any> - stopSubscription?(operationId: string): Promise<void> -}packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.ts (2)
60-66: Unused loop variablesha256.The
sha256variable is destructured but never used in the loop body. Use an underscore prefix to indicate intentional non-use.- for (const [sha256, subscription] of this.activeSubscriptions.entries()) { + for (const [_sha256, subscription] of this.activeSubscriptions.entries()) { if (subscription.operationId) { this.logger.debug(`Should stop subscription with operationId: ${subscription.operationId}`); } }
3-17: Remove unused imports.Static analysis correctly identified multiple unused imports. Remove them to clean up the file.
import { Inject, Injectable, Logger } from '@nestjs/common'; -import { isDefined } from 'class-validator'; import { type Subscription } from 'zen-observable-ts'; import { CANONICAL_INTERNAL_CLIENT_TOKEN, type CanonicalInternalClientService } from '@unraid/shared'; -import { EVENTS_SUBSCRIPTION, RemoteGraphQL_Fragment } from '../graphql/event.js'; -import { - ClientType, - RemoteGraphQlEventFragmentFragment, - RemoteGraphQlEventType, -} from '../graphql/generated/client/graphql.js'; -import { useFragment } from '../graphql/generated/client/index.js'; -import { SEND_REMOTE_QUERY_RESPONSE } from '../graphql/remote-response.js'; -import { parseGraphQLQuery } from '../helper/parse-graphql.js'; import { MothershipConnectionService } from './connection.service.js'; import { UnraidServerClientService } from './unraid-server-client.service.js';Note: Also remove the
Subscriptiontype import if it's not used elsewhere in the file.
🧹 Nitpick comments (9)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.ts (3)
10-18: Consider exporting the interface for reuse.The
GraphQLExecutorinterface is duplicated inunraid-server-client.service.ts. Export it from this file and import it where needed to avoid duplication.Additionally, prefer a more specific type over
anyfor thevariablesparameter and return type per coding guidelines.-interface GraphQLExecutor { +export interface GraphQLExecutor { execute(params: { query: string - variables?: Record<string, any> + variables?: Record<string, unknown> operationName?: string operationType?: 'query' | 'mutation' | 'subscription' - }): Promise<any> + }): Promise<{ data?: unknown; errors?: Array<{ message: string; extensions?: Record<string, unknown> }> }> stopSubscription?(operationId: string): Promise<void> }
58-68: Preferunknownoveranyfor the error catch clause.As per coding guidelines, avoid using the
anytype.- } catch (error: any) { - this.logger.error(`GraphQL execution error: ${error?.message}`); + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + this.logger.error(`GraphQL execution error: ${errorMessage}`); return { errors: [ { - message: error?.message || 'Unknown error', + message: errorMessage, extensions: { code: 'EXECUTION_ERROR' }, }, ], }; }
158-161: Stub implementation for subscription cleanup.The
stopSubscriptionmethod is a stub. Ensure this is tracked for completion before the feature is finalized.Would you like me to open an issue to track implementing proper subscription cleanup logic?
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts (3)
16-23: Use stricter types for GraphQLResponse interface.Replace
anywith more specific types per coding guidelines.interface GraphQLResponse { operationId: string messageId?: string event: 'query_response' type: 'data' | 'error' | 'complete' - payload: any + payload: { data?: unknown; errors?: Array<{ message: string; extensions?: Record<string, unknown> }> } requestHash?: string }
63-89: Replace console.log/error with Logger for consistency.The
UnraidServerClientServiceuses NestJSLogger, butUnraidServerClientusesconsole.log. For consistency and proper log level control, inject or instantiate a Logger.export class UnraidServerClient { + private readonly logger = new Logger(UnraidServerClient.name) private ws: WebSocket | null = null // ... this.ws.onopen = () => { - console.log('Connected to mothership') + this.logger.log('Connected to mothership') this.reconnectAttempts = 0 this.setupPingInterval() resolve() } // ... apply similar changes to other console.log/error callsYou'll need to add the Logger import from
@nestjs/common.
109-117: Consider stricter typing for parsed message.The
messagevariable is typed asany. Define an interface for expected message formats.+interface MothershipMessage { + type?: string + event?: string + ping?: boolean + pong?: boolean + operationId?: string + messageId?: string + payload?: { query?: string; variables?: Record<string, unknown>; operationName?: string } +} // Try to parse as JSON for structured messages - let message: any + let message: MothershipMessage try { message = JSON.parse(data)packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.ts (1)
117-132: Multiple stub implementations need completion.Both
stopMothershipSubscriptionandsubscribeToMothershipEventsare stubs. These should be tracked for completion before release.Would you like me to open an issue to track completing these subscription management methods?
api/vite.config.ts (1)
24-27: Workspace path change is consistent; value currently informational onlyUpdating the
workspaceDependenciesentry to pointunraid-api-plugin-connectatpackages/unraid-api-plugin-connect-2keeps this config in sync with the build script and docs. Since this file only usesObject.keys(workspaceDependencies), the behavior of optimize/external/ssr is unchanged; the path value is effectively documentation here. If this remains the case long term, you could optionally simplify this to a list of package names instead of a name→path map, but it’s not required for this PR.api/scripts/build.ts (1)
10-10: Alias-based import may depend on zx/TS path-alias setupImporting
getDeploymentVersionvia'@app/../scripts/get-deployment-version.js'will only work if your zx TypeScript runtime honors the@apptsconfig path alias and correctly normalizes the../scriptssegment. This is a bit opaque compared to a direct path to thescriptsdirectory.It’s worth double-checking that the script still runs correctly in all environments (local, CI) with this alias-based path; if not, consider reverting to a simpler, explicit relative or dedicated alias for the scripts directory to avoid surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
api/.env.development(1 hunks)api/dev/configs/api.json(1 hunks)api/dev/configs/connect.json(1 hunks)api/dev/states/connectStatus.json(1 hunks)api/docs/developer/api-plugins.md(2 hunks)api/scripts/build.ts(2 hunks)api/vite.config.ts(1 hunks)packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.ts(1 hunks)packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.ts(1 hunks)packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/dev/configs/api.json
🚧 Files skipped from review as they are similar to previous changes (1)
- api/dev/states/connectStatus.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.tsapi/scripts/build.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tsapi/vite.config.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.tsapi/scripts/build.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tsapi/vite.config.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.tsapi/docs/developer/api-plugins.mdapi/scripts/build.tsapi/dev/configs/connect.jsonpackages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tsapi/vite.config.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/docs/developer/api-plugins.mdapi/scripts/build.tsapi/dev/configs/connect.jsonapi/vite.config.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/scripts/build.tsapi/vite.config.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: elibosley
Repo: unraid/api PR: 1639
File: web/components/standalone-mount.ts:38-54
Timestamp: 2025-09-02T19:35:14.427Z
Learning: In the unraid/api project, exposing Apollo client and GraphQL utilities (apolloClient, gql, graphqlParse) on the global window object is intentional to allow developers to interact with GraphQL from the browser console without needing to be inside Vue components. Developer experience is prioritized over minimizing global exposure in this context.
Learnt from: mdatelle
Repo: unraid/api PR: 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.
📚 Learning: 2025-09-02T19:35:14.427Z
Learnt from: elibosley
Repo: unraid/api PR: 1639
File: web/components/standalone-mount.ts:38-54
Timestamp: 2025-09-02T19:35:14.427Z
Learning: In the unraid/api project, exposing Apollo client and GraphQL utilities (apolloClient, gql, graphqlParse) on the global window object is intentional to allow developers to interact with GraphQL from the browser console without needing to be inside Vue components. Developer experience is prioritized over minimizing global exposure in this context.
Applied to files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
Repo: unraid/api 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.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-11-24T17:51:46.348Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-11-24T17:51:46.348Z
Learning: Always run scripts from api/package.json unless explicitly requested otherwise
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
Repo: unraid/api 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.
Applied to files:
api/scripts/build.ts
📚 Learning: 2025-06-20T20:50:14.050Z
Learnt from: elibosley
Repo: unraid/api PR: 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.
Applied to files:
api/dev/configs/connect.json
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
Repo: unraid/api 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`.
Applied to files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
📚 Learning: 2024-12-10T15:57:42.593Z
Learnt from: pujitm
Repo: unraid/api PR: 975
File: web/helpers/apollo-cache/index.ts:79-82
Timestamp: 2024-12-10T15:57:42.593Z
Learning: The `isDefined` function is autoloaded and placed on `globalThis`; it does not need to be imported or defined explicitly in files.
Applied to files:
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.ts
📚 Learning: 2025-03-03T16:04:20.705Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
api/.env.development
🧬 Code graph analysis (3)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.ts (1)
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts (1)
Module(15-30)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.ts (3)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts (1)
Injectable(421-480)packages/unraid-shared/src/tokens.ts (1)
CANONICAL_INTERNAL_CLIENT_TOKEN(7-7)api/src/unraid-api/cli/generated/gql.ts (1)
gql(96-98)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts (3)
packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.ts (1)
Injectable(23-162)packages/unraid-api-plugin-connect-2/src/mothership-proxy/connection.service.ts (1)
Injectable(51-241)packages/unraid-api-plugin-connect-2/src/mothership-proxy/graphql.client.ts (1)
Injectable(29-344)
🪛 dotenv-linter (4.0.0)
api/.env.development
[warning] 22-22: [UnorderedKey] The PATHS_CONNECT_STATUS key should go before the PATHS_CONNECT_STATUS_FILE_PATH key
(UnorderedKey)
[warning] 22-22: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 23-23: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 23-23: [UnorderedKey] The ENVIRONMENT key should go before the PATHS_ACTIVATION_BASE key
(UnorderedKey)
[warning] 24-24: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 24-24: [UnorderedKey] The NODE_ENV key should go before the PATHS_ACTIVATION_BASE key
(UnorderedKey)
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 26-26: [UnorderedKey] The PLAYGROUND key should go before the PORT key
(UnorderedKey)
[warning] 27-27: [UnorderedKey] The INTROSPECTION key should go before the NODE_ENV key
(UnorderedKey)
[warning] 28-28: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 28-28: [UnorderedKey] The MOTHERSHIP_GRAPHQL_LINK key should go before the NODE_ENV key
(UnorderedKey)
[warning] 29-29: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 29-29: [UnorderedKey] The MOTHERSHIP_BASE_URL key should go before the MOTHERSHIP_GRAPHQL_LINK key
(UnorderedKey)
🪛 GitHub Check: CodeQL
packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.ts
[notice] 62-62: Unused variable, import, function or class
Unused variable sha256.
[notice] 15-15: Unused variable, import, function or class
Unused import parseGraphQLQuery.
[notice] 14-14: Unused variable, import, function or class
Unused import SEND_REMOTE_QUERY_RESPONSE.
[notice] 13-13: Unused variable, import, function or class
Unused import useFragment.
[notice] 8-12: Unused variable, import, function or class
Unused imports ClientType, RemoteGraphQlEventFragmentFragment, RemoteGraphQlEventType.
[notice] 7-7: Unused variable, import, function or class
Unused imports EVENTS_SUBSCRIPTION, RemoteGraphQL_Fragment.
[notice] 3-3: Unused variable, import, function or class
Unused import isDefined.
🔇 Additional comments (9)
api/dev/configs/connect.json (1)
5-5: LGTM!The placeholder value aligns with the development environment setup and matches the existing
localApiKeyformat. Based on learnings, this configuration file follows its own naming conventions and no changes are needed.api/.env.development (2)
28-29: LGTM!The mothership endpoints are correctly updated to the preview environment (
wss://preview.mothership2.unraid.netfor GraphQL subscriptions andhttps://preview.mothership2.unraid.netfor the base URL), aligning with the mothership v2 infrastructure changes.
33-37: LGTM!The new development environment variables are appropriate: polling enables file watching in containers, logging configuration provides detailed tracing, and the flags control experimental features and plugin checks during development.
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts (2)
210-248: Exponential backoff implementation looks correct.The reconnection logic with exponential backoff capped at 30 minutes is well implemented. The
shouldReconnectflag properly prevents reconnection after intentional disconnects.
444-468: Remove this review comment—the concern is already addressed by an event-driven reinitialiation mechanism.When identity state changes,
connectionServiceemits anIDENTITY_CHANGEDevent (connection.service.ts:100), which is captured bymothership.events.ts(@OnEvent(EVENTS.IDENTITY_CHANGED)). This triggersmothershipController.initOrRestart(), which callsclientService.reconnect(), andreconnect()invokesinitializeClient()(unraid-server-client.service.ts:478). The system properly reinitializes the client whenever identity becomes available.packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership.module.ts (1)
14-30: Module structure looks correct.The imports
LocalGraphQLExecutorandUnraidServerClientServiceflagged by static analysis are correctly used in theprovidersarray. The module properly wires up all the mothership proxy components.packages/unraid-api-plugin-connect-2/src/mothership-proxy/mothership-subscription.handler.ts (1)
134-157: executeQuery returns mock data.The method returns hardcoded mock data instead of executing actual queries. The TODO comment indicates this needs implementation.
Ensure this stub is completed before merging or mark it clearly as not production-ready.
api/docs/developer/api-plugins.md (1)
20-36: Doc updates correctly reflect new plugin-connect-2 workspace pathThe examples now mapping
unraid-api-plugin-connecttopackages/unraid-api-plugin-connect-2in both the build and Vite config snippets look consistent with the corresponding code changes, while keeping the package name unchanged in the peerDependencies section. No issues from a docs/code-alignment standpoint.api/scripts/build.ts (1)
22-25: Vendoring map update for plugin-connect-2 is consistentPointing the
WORKSPACE_PACKAGES_TO_VENDORentry forunraid-api-plugin-connectatpackages/unraid-api-plugin-connect-2looks consistent with the Vite config and docs. Assuming the package in that folder is still namedunraid-api-plugin-connect, this keeps the vendoring behavior aligned with the existing peerDependency name.
| PATHS_CONNECT_STATUS_FILE_PATH=./dev/connectStatus.json # Connect plugin status file | ||
| PATHS_OIDC_JSON=./dev/configs/oidc.local.json | ||
| PATHS_LOCAL_SESSION_FILE=./dev/local-session | ||
| PATHS_CONNECT_STATUS=./dev/states/connectStatus.json # Connect status file for development |
There was a problem hiding this comment.
Add quotes around the value on line 22.
The .env file format requires quoted values for consistent parsing by dotenv libraries.
Apply this fix:
-PATHS_CONNECT_STATUS=./dev/states/connectStatus.json # Connect status file for development
+PATHS_CONNECT_STATUS="./dev/states/connectStatus.json" # Connect status file for development📝 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.
| PATHS_CONNECT_STATUS=./dev/states/connectStatus.json # Connect status file for development | |
| PATHS_CONNECT_STATUS="./dev/states/connectStatus.json" # Connect status file for development |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 22-22: [UnorderedKey] The PATHS_CONNECT_STATUS key should go before the PATHS_CONNECT_STATUS_FILE_PATH key
(UnorderedKey)
[warning] 22-22: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 Prompt for AI Agents
In api/.env.development around line 22, the PATHS_CONNECT_STATUS value is
unquoted; update the line to wrap the value in quotes so dotenv parsers handle
it consistently (e.g., change
PATHS_CONNECT_STATUS=./dev/states/connectStatus.json to
PATHS_CONNECT_STATUS="./dev/states/connectStatus.json").
| // Execute the transformed query against local API | ||
| const client = await this.internalClient.getClient(); | ||
| const result = await client.query({ | ||
| query: gql`${localQuery}`, | ||
| variables, | ||
| }); |
There was a problem hiding this comment.
Execute method ignores operationType parameter.
The method accepts operationType but always calls client.query(). Mutations should use client.mutate() to ensure proper behavior.
// Execute the transformed query against local API
const client = await this.internalClient.getClient();
- const result = await client.query({
- query: gql`${localQuery}`,
- variables,
- });
+ let result;
+ if (operationType === 'mutation') {
+ result = await client.mutate({
+ mutation: gql`${localQuery}`,
+ variables,
+ });
+ } else {
+ result = await client.query({
+ query: gql`${localQuery}`,
+ variables,
+ });
+ }📝 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.
| // Execute the transformed query against local API | |
| const client = await this.internalClient.getClient(); | |
| const result = await client.query({ | |
| query: gql`${localQuery}`, | |
| variables, | |
| }); | |
| // Execute the transformed query against local API | |
| const client = await this.internalClient.getClient(); | |
| let result; | |
| if (operationType === 'mutation') { | |
| result = await client.mutate({ | |
| mutation: gql`${localQuery}`, | |
| variables, | |
| }); | |
| } else { | |
| result = await client.query({ | |
| query: gql`${localQuery}`, | |
| variables, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect-2/src/mothership-proxy/local-graphql-executor.service.ts
around lines 48 to 53, the method ignores the operationType parameter and always
calls client.query(); change the logic to branch on operationType: if it's
'query' call client.query({ query: gql`${localQuery}`, variables }), if it's
'mutation' call client.mutate({ mutation: gql`${localQuery}`, variables }); keep
using gql for the document, pass the same variables, and return the
mutation/query result as before (no other API changes).
| try { | ||
| const errorRequest = JSON.parse(data) | ||
| // Only send error response for GraphQL requests that have operationId | ||
| if (errorRequest.operationId && (errorRequest.event === 'remote_query' || errorRequest.event === 'route_message')) { | ||
| const operationId = errorRequest.operationId || `error-${Date.now()}` | ||
| this.sendResponse({ | ||
| operationId, | ||
| event: 'query_response', | ||
| type: 'error', | ||
| payload: { | ||
| errors: [ | ||
| { | ||
| message: error?.message || 'Unknown error', | ||
| extensions: { code: 'EXECUTION_ERROR' }, | ||
| }, | ||
| ], | ||
| }, | ||
| }) | ||
| } | ||
| } catch (e) { | ||
| console.error('Failed to send error response:', e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant operationId assignment.
Line 158 assigns operationId but line 156 already checks for its existence, making the fallback at line 158 unreachable in the truthy branch.
// Only send error response for GraphQL requests that have operationId
if (errorRequest.operationId && (errorRequest.event === 'remote_query' || errorRequest.event === 'route_message')) {
- const operationId = errorRequest.operationId || `error-${Date.now()}`
+ const operationId = errorRequest.operationId
this.sendResponse({📝 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 { | |
| const errorRequest = JSON.parse(data) | |
| // Only send error response for GraphQL requests that have operationId | |
| if (errorRequest.operationId && (errorRequest.event === 'remote_query' || errorRequest.event === 'route_message')) { | |
| const operationId = errorRequest.operationId || `error-${Date.now()}` | |
| this.sendResponse({ | |
| operationId, | |
| event: 'query_response', | |
| type: 'error', | |
| payload: { | |
| errors: [ | |
| { | |
| message: error?.message || 'Unknown error', | |
| extensions: { code: 'EXECUTION_ERROR' }, | |
| }, | |
| ], | |
| }, | |
| }) | |
| } | |
| } catch (e) { | |
| console.error('Failed to send error response:', e) | |
| } | |
| } | |
| try { | |
| const errorRequest = JSON.parse(data) | |
| // Only send error response for GraphQL requests that have operationId | |
| if (errorRequest.operationId && (errorRequest.event === 'remote_query' || errorRequest.event === 'route_message')) { | |
| const operationId = errorRequest.operationId | |
| this.sendResponse({ | |
| operationId, | |
| event: 'query_response', | |
| type: 'error', | |
| payload: { | |
| errors: [ | |
| { | |
| message: error?.message || 'Unknown error', | |
| extensions: { code: 'EXECUTION_ERROR' }, | |
| }, | |
| ], | |
| }, | |
| }) | |
| } | |
| } catch (e) { | |
| console.error('Failed to send error response:', e) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect-2/src/mothership-proxy/unraid-server-client.service.ts
around lines 154-176, the code checks that errorRequest.operationId exists
before entering the branch, so the fallback (`|| \`error-\${Date.now()}\``) when
assigning operationId is redundant and unreachable; remove the fallback and just
use the asserted operationId (e.g., const operationId =
errorRequest.operationId) or, if you intend to handle missing operationId,
change the if-condition to not require operationId and keep the fallback—pick
one and update the condition and assignment to be consistent.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.