Feat/mothership v2 #1551
Feat/mothership v2 #1551
Conversation
WalkthroughThe changes transition the Mothership client architecture from a GraphQL-based service to a WebSocket-based Changes
Sequence DiagramsequenceDiagram
actor User
participant Controller as MothershipController
participant Service as UnraidServerClientService
participant WSClient as UnraidServerClient
participant Executor as LocalGraphQLExecutor
participant API as Local API
User->>Controller: initOrRestart()
activate Controller
Controller->>Service: reconnect()
activate Service
Service->>WSClient: new UnraidServerClient(url, apiKey, executor)
Service->>WSClient: connect()
activate WSClient
WSClient->>WSClient: WebSocket connect
Note over WSClient: Ping/Pong keep-alive
WSClient-->>Service: onConnected
deactivate WSClient
Service-->>Controller:
deactivate Service
deactivate Controller
User->>WSClient: GraphQL query via WebSocket
activate WSClient
WSClient->>Executor: execute(remoteQuery)
activate Executor
Note over Executor: Transform remote → local<br/>(remove "remote" prefix)
Executor->>API: local query (e.g., docker, vms, system)
activate API
API-->>Executor: query result
deactivate API
Executor-->>WSClient: GraphQLResponse
deactivate Executor
WSClient->>WSClient: Send response via WebSocket
deactivate WSClient
User->>Controller: stop()
activate Controller
Controller->>Service: clientService.getClient()?.disconnect()
activate Service
Service->>WSClient: disconnect()
activate WSClient
WSClient->>WSClient: Close WebSocket
deactivate WSClient
deactivate Service
deactivate Controller
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
| // Stop all subscriptions via the UnraidServerClient | ||
| const client = this.mothershipClient.getClient(); | ||
| if (client) { | ||
| for (const [sha256, subscription] of this.activeSubscriptions.entries()) { |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 25 days ago
To fix the problem, we should only destructure the values from the map entries where the key is not used. In JavaScript/TypeScript, iterating over map entries returns [key, value] pairs, but if the key is unused, we can either use a throwaway _ or just destructure only the value. Alternatively, use .values() which directly yields the values without requiring destructuring. In this case, replacing
for (const [sha256, subscription] of this.activeSubscriptions.entries())with
for (const subscription of this.activeSubscriptions.values())will eliminate the unused variable and clarify intent. Only one change is required, on line 62.
| @@ -59,7 +59,7 @@ | ||
| // Stop all subscriptions via the UnraidServerClient | ||
| const client = this.mothershipClient.getClient(); | ||
| if (client) { | ||
| for (const [sha256, subscription] of this.activeSubscriptions.entries()) { | ||
| for (const subscription of this.activeSubscriptions.values()) { | ||
| if (subscription.operationId) { | ||
| this.logger.debug(`Should stop subscription with operationId: ${subscription.operationId}`); | ||
| } |
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.
packages/unraid-api-plugin-connect-2/src/network/url-resolver.service.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/connection-status/cloud.service.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/unraid-connect/connect-settings.service.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/unraid-connect/connect-settings.resolver.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/remote-access/static-remote-access.service.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/__test__/mothership.events.test.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/__test__/graphql.client.test.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/__test__/config.validation.test.ts
Fixed
Show fixed
Hide fixed
packages/unraid-api-plugin-connect-2/src/__test__/config.persistence.test.ts
Fixed
Show fixed
Hide fixed
- 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.
…pendencies - Renamed the unraid-api-plugin-connect-2 package back to unraid-api-plugin-connect for consistency. - Updated pnpm-lock.yaml to reflect the new package structure and dependencies. - Modified environment variables to standardize Mothership API integration. - Removed deprecated GraphQL code generation and related types, streamlining the API. - Enhanced connection status handling and introduced new services for WebSocket communication with Mothership.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts (1)
43-50: Remove dead code: try-catch block is redundant.Both the try block (line 47) and catch block (line 49) set
allowedOrigins = ''. The try-catch serves no purpose.// Try to get allowed origins from the store let allowedOrigins = ''; - try { - // We can't import from @app here, so we'll skip allowed origins for now - // This can be added later if needed - allowedOrigins = ''; - } catch (error) { - this.logger.debug('Could not get allowed origins:', error); - } + // TODO: Get allowed origins from store when @app imports are availablepackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (1)
4-15: Remove unused imports after refactoring.The following imports are not used in this file and should be removed:
Subscription(line 4)isDefined(line 3)EVENTS_SUBSCRIPTION,RemoteGraphQL_Fragment(line 7)ClientType,RemoteGraphQlEventFragmentFragment,RemoteGraphQlEventType(lines 8-12)useFragment(line 13)SEND_REMOTE_QUERY_RESPONSE(line 14)parseGraphQLQuery(line 15)
♻️ Duplicate comments (9)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts (1)
16-19: Fix type safety for PATHS_CONNECT_STATUS to resolve pipeline failures.Pipeline errors confirm
configService.get('PATHS_CONNECT_STATUS')returns an object, causingTypeErrorwhen passed todirname(). Add explicit type parameter and runtime validation.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'; + const defaultPath = '/var/local/emhttp/connectStatus.json'; + const rawPath = this.configService.get<string>('PATHS_CONNECT_STATUS'); + if (rawPath === undefined || rawPath === null) { + return defaultPath; + } + if (typeof rawPath !== 'string') { + this.logger.warn(`Invalid PATHS_CONNECT_STATUS config type: ${typeof rawPath}, using default`); + return defaultPath; + } + return rawPath; }packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (1)
48-53: Execute method ignoresoperationTypeparameter - mutations won't work correctly.The method always calls
client.query()regardless of whether the operation is a mutation. This will cause mutations to fail or behave incorrectly.// 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, + }); + }packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (2)
25-33: Remove duplicate GraphQLExecutor interface.This interface is already defined in
local-graphql-executor.service.ts. Import it from there instead.+import { LocalGraphQLExecutor, 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> -}Note: The
GraphQLExecutorinterface in the other file would need to be exported first.
156-158: Redundant operationId fallback.Line 157 already checks that
errorRequest.operationIdexists, making the fallback on line 158 unreachable.if (errorRequest.operationId && (errorRequest.event === 'remote_query' || errorRequest.event === 'route_message')) { - const operationId = errorRequest.operationId || `error-${Date.now()}` + const operationId = errorRequest.operationId this.sendResponse({packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts (5)
117-119: Incomplete implementation:stopMothershipSubscription.This method is a no-op placeholder. Based on the controller usage (
this.subscriptionHandler.stopMothershipSubscription()), this needs to be implemented to properly clean up mothership subscriptions.
134-157:executeQueryreturns mock data.The TODO comment indicates this is incomplete. The method returns hardcoded success data instead of executing actual queries via the
UnraidServerClient.
44-50: Address the architectural limitation noted in the comment.The comment on lines 47-48 indicates that stopping subscriptions via the client is not possible because
stopSubscriptionis private. This was flagged in a past review. Consider exposing a public method onUnraidServerClientto properly stop subscriptions.
121-132:subscribeToMothershipEventsis incomplete.This method only logs client availability status without setting up actual event subscriptions. The
UnraidServerClientalready handles WebSocket communication, but this handler needs to register for specific events if required.
60-67: Unused loop variablesha256.Static analysis flagged that
sha256is unused in the loop at line 62. Consider using underscore prefix or removing if not needed.- 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}`); } }Alternatively, if you need to log the sha256 for debugging:
for (const [sha256, subscription] of this.activeSubscriptions.entries()) { if (subscription.operationId) { - this.logger.debug(`Should stop subscription with operationId: ${subscription.operationId}`); + this.logger.debug(`Stopping subscription ${sha256} with operationId: ${subscription.operationId}`); } }
🧹 Nitpick comments (8)
packages/unraid-api-plugin-connect/codegen.ts (1)
31-33: Consider removing the entire codegen config file.If GraphQL code generation is permanently disabled for this package, the codegen.ts file and related devDependencies (@graphql-codegen/cli, etc.) could be removed entirely to avoid confusion and reduce maintenance burden.
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts (1)
39-41: Simplify redundant null check.
getClient()is called twice, and the optional chaining is unnecessary after the truthy check.- 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)
58-68: Avoid usinganyfor error typing.The
catchblock useserror: anywhich violates coding guidelines.- } catch (error: any) { - this.logger.error(`GraphQL execution error: ${error?.message}`); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : 'Unknown error'; + this.logger.error(`GraphQL execution error: ${message}`); return { errors: [ { - message: error?.message || 'Unknown error', + message, extensions: { code: 'EXECUTION_ERROR' }, }, ], }; }
131-156: Consider making field name mappings configurable or centralized.The hardcoded field name mappings (
remoteGetDockerInfo→docker, etc.) may need maintenance as new remote fields are added. Consider extracting these to a configuration object for easier updates.const REMOTE_FIELD_MAPPINGS: Record<string, string> = { remoteGetDockerInfo: 'docker', remoteGetVms: 'vms', remoteGetSystemInfo: 'system', // Add new mappings here }; private transformRemoteFieldName(fieldName: string): string { if (REMOTE_FIELD_MAPPINGS[fieldName]) { return REMOTE_FIELD_MAPPINGS[fieldName]; } // ... rest of generic transformation logic }packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (4)
36-49: Consider using a logger instead of console for consistent logging.
UnraidServerClientusesconsole.log/error/warnthroughout whileUnraidServerClientServiceuses NestJS Logger. For consistency and better log management, consider passing a logger instance toUnraidServerClientor using a static logger.export class UnraidServerClient { + private readonly logger: Logger private ws: WebSocket | null = null private reconnectAttempts = 0 private readonly initialReconnectDelay = 1000 // 1 second private readonly maxReconnectDelay = 30 * 60 * 1000 // 30 minutes private pingInterval: NodeJS.Timeout | null = null private reconnectTimeout: NodeJS.Timeout | null = null private shouldReconnect = true constructor( private mothershipUrl: string, private apiKey: string, private executor: GraphQLExecutor, + logger?: Logger, - ) {} + ) { + this.logger = logger ?? new Logger(UnraidServerClient.name) + }
109-117: Avoid usinganytype for message parsing.The
messagevariable usesanytype which violates coding guidelines. Consider defining proper types for the expected message formats.+interface IncomingMessage { + type?: string + event?: 'remote_query' | 'subscription_start' | 'subscription_stop' | 'route_message' + operationId?: string + messageId?: string + payload?: { + query?: string + variables?: Record<string, unknown> + operationName?: string + } + ping?: boolean + pong?: boolean +} // Try to parse as JSON for structured messages -let message: any +let message: IncomingMessage try { message = JSON.parse(data) } catch (parseError) {
314-400: Consider extracting shared logic to reduce duplication.
handleRouteMessageandhandleRequestMessageare nearly identical. Consider extracting the common logic into a shared helper method.+private async executeAndRespond( + message: { operationId?: string; payload?: { query?: string; variables?: Record<string, unknown>; operationName?: string } }, + messageType: string +): Promise<void> { + if (!message.payload || !message.payload.query) { + console.warn(`Invalid ${messageType} - missing payload or query:`, message) + return + } + + const operationId = message.operationId || `auto-${Date.now()}` + + try { + const result = await this.executor.execute({ + query: message.payload.query, + variables: message.payload.variables, + operationName: message.payload.operationName, + operationType: 'query', + }) + + this.sendResponse({ + operationId, + messageId: `msg_response_${Date.now()}`, + event: 'query_response', + type: result.errors ? 'error' : 'data', + payload: result, + }) + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error' + this.sendResponse({ + operationId, + messageId: `msg_error_${Date.now()}`, + event: 'query_response', + type: 'error', + payload: { + errors: [{ message: errorMessage, extensions: { code: 'EXECUTION_ERROR' } }], + }, + }) + } +}
454-461: Remove unnecessary variable assignment.The
executorvariable on line 455 is redundant sincethis.localExecutorcan be used directly.- // Use the injected LocalGraphQLExecutor - const executor = this.localExecutor; - this.client = new UnraidServerClient( mothershipUrl, identityState.state.apiKey, - executor + this.localExecutor );
📜 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 (12)
api/dev/states/connectStatus.json(1 hunks)api/scripts/build.ts(1 hunks)packages/unraid-api-plugin-connect/codegen.ts(1 hunks)packages/unraid-api-plugin-connect/package.json(2 hunks)packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts(3 hunks)packages/unraid-api-plugin-connect/src/graphql/remote-response.ts(1 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 skipped from review as they are similar to previous changes (1)
- api/scripts/build.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/src/connection-status/connect-status-writer.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.tspackages/unraid-api-plugin-connect/src/connection-status/cloud.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect/src/graphql/remote-response.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/src/connection-status/connect-status-writer.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.tspackages/unraid-api-plugin-connect/src/connection-status/cloud.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect/src/graphql/remote-response.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/src/connection-status/connect-status-writer.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.tspackages/unraid-api-plugin-connect/package.jsonpackages/unraid-api-plugin-connect/src/connection-status/cloud.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tsapi/dev/states/connectStatus.jsonpackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.tspackages/unraid-api-plugin-connect/src/graphql/remote-response.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/dev/states/connectStatus.json
🧠 Learnings (20)
📓 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: 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.
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.
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.
Learnt from: elibosley
Repo: unraid/api PR: 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.
📚 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:
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
📚 Learning: 2025-04-21T18:44:39.643Z
Learnt from: elibosley
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
📚 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/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect/src/graphql/remote-response.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/src/mothership-proxy/mothership.module.tspackages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/package.jsonpackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect/src/graphql/remote-response.ts
📚 Learning: 2025-11-24T17:52:00.572Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-11-24T17:52:00.572Z
Learning: Always run `pnpm codegen` for GraphQL code generation in the web directory
Applied to files:
packages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/package.jsonpackages/unraid-api-plugin-connect/src/graphql/remote-response.ts
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/package.json
📚 Learning: 2024-11-05T14:48:56.949Z
Learnt from: elibosley
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/codegen.ts
📚 Learning: 2025-01-15T21:34:00.006Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/codegen.ts
📚 Learning: 2025-11-24T17:51:37.915Z
Learnt from: CR
Repo: unraid/api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:51:37.915Z
Learning: Applies to web/**/* : Always run `pnpm codegen` for GraphQL code generation in the web directory
Applied to files:
packages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/package.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/codegen.tspackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.tspackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.tspackages/unraid-api-plugin-connect/src/graphql/remote-response.ts
📚 Learning: 2025-11-24T17:52:00.572Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-11-24T17:52:00.572Z
Learning: Applies to web/**/*.{query.ts,mutation.ts} : All GraphQL under `web/` must follow the `.query.ts` and `.mutation.ts` naming convention
Applied to files:
packages/unraid-api-plugin-connect/codegen.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.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/src/mothership-proxy/mothership.controller.tspackages/unraid-api-plugin-connect/src/connection-status/cloud.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.tspackages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.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:
packages/unraid-api-plugin-connect/package.json
📚 Learning: 2025-02-05T14:43:48.568Z
Learnt from: elibosley
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/package.json
📚 Learning: 2024-12-11T17:20:09.604Z
Learnt from: pujitm
Repo: unraid/api 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.
Applied to files:
packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts
📚 Learning: 2025-11-24T17:52:00.572Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-11-24T17:52:00.572Z
Learning: Applies to web/**/*.mutation.ts : GraphQL mutations must be placed in `.mutation.ts` files
Applied to files:
packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts
📚 Learning: 2025-11-24T17:52:00.572Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-11-24T17:52:00.572Z
Learning: Applies to web/**/*.query.ts : GraphQL queries must be placed in `.query.ts` files
Applied to files:
packages/unraid-api-plugin-connect/src/graphql/remote-response.ts
🧬 Code graph analysis (2)
packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (2)
packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (1)
Injectable(23-162)packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts (1)
Injectable(15-61)
packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (4)
packages/unraid-api-plugin-connect/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)web/src/helpers/create-apollo-client.ts (1)
client(123-126)api/src/unraid-api/cli/generated/gql.ts (1)
gql(96-98)
🪛 GitHub Actions: CI - Main (API)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
[error] 1-200: ConnectStatusWriterService tests are failing: expected bootstrap to write initial status, but the write action was not invoked (spy not called).
[error] 60-90: TypeError: The 'path' argument must be of type string. Received an instance of Object while writing status; path handling logic is receiving an object instead of a string.
[error] 1-100: Repeated runtime errors: Error writing status to '[object Object]' and invalid path processing (unhandled object being passed to dirname).
🪛 GitHub Check: CodeQL
packages/unraid-api-plugin-connect/src/mothership-proxy/mothership-subscription.handler.ts
[notice] 62-62: Unused variable, import, function or class
Unused variable sha256.
🔇 Additional comments (8)
packages/unraid-api-plugin-connect/src/graphql/remote-response.ts (1)
2-2: LGTM! Path simplification improves clarity.The import path has been simplified from
../graphql/generated/client/gql.jsto./generated/client/gql.js. Both resolve to the same location, but the new path is clearer since the file is already in thegraphqldirectory.api/dev/states/connectStatus.json (1)
1-7: LGTM!Development state file with sensible defaults for initial connection status testing.
packages/unraid-api-plugin-connect/package.json (2)
16-16: LGTM!Codegen script simplified to remove the staging GraphQL link, aligning with the WebSocket-based architecture migration.
60-60: Remove unusedpifydependency.The
pifypackage (^6.1.0) is listed as a devDependency but is not imported or used anywhere in the package. Remove it to keep dependencies lean.⛔ Skipped due to learnings
Learnt from: CR Repo: unraid/api PR: 0 File: .cursor/rules/web-graphql.mdc:0-0 Timestamp: 2025-11-24T17:52:00.572Z Learning: Always run `pnpm codegen` for GraphQL code generation in the web directorypackages/unraid-api-plugin-connect/src/mothership-proxy/mothership.module.ts (1)
14-28: LGTM!Module correctly wires the new
LocalGraphQLExecutorandUnraidServerClientServiceproviders, replacing the previous GraphQL client approach. The static analysis warnings about unused imports appear to be false positives as both services are registered in the providers array.packages/unraid-api-plugin-connect/src/mothership-proxy/mothership.controller.ts (1)
49-60: LGTM!The
initOrRestartflow correctly validates identity state before reconnecting, ensuring API key availability before establishing the mothership connection.packages/unraid-api-plugin-connect/src/mothership-proxy/local-graphql-executor.service.ts (1)
158-161: Subscription handling is a stub - verify this is intentional.The
stopSubscriptionmethod only logs and doesn't actually clean up subscriptions. Ensure this is acceptable for the current implementation scope or add a TODO.packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts (1)
86-89: Error handler only rejects during initial connection.The
onerrorhandler callsreject(error), but onceonopenresolves the promise, subsequent errors will only log. Consider whether this is the intended behavior or if a separate error event mechanism is needed for post-connection errors.
|
|
||
| private async hardCheckDns() { | ||
| const mothershipGqlUri = this.configService.getOrThrow<string>('MOTHERSHIP_GRAPHQL_LINK'); | ||
| const mothershipGqlUri = this.configService.getOrThrow<string>('MOTHERSHIP_BASE_URL'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all references to MOTHERSHIP_BASE_URL and MOTHERSHIP_GRAPHQL_LINK
echo "=== MOTHERSHIP_BASE_URL references ==="
rg -n "MOTHERSHIP_BASE_URL" --type ts --type tsx --type js --type jsx
echo -e "\n=== MOTHERSHIP_GRAPHQL_LINK references ==="
rg -n "MOTHERSHIP_GRAPHQL_LINK" --type ts --type tsx --type js --type jsx
echo -e "\n=== Environment configuration files ==="
fd -e env -e env.example --type fRepository: unraid/api
Length of output: 250
🏁 Script executed:
# Retry search without problematic file types
echo "=== MOTHERSHIP_BASE_URL references ==="
rg -n "MOTHERSHIP_BASE_URL"
echo -e "\n=== MOTHERSHIP_GRAPHQL_LINK references ==="
rg -n "MOTHERSHIP_GRAPHQL_LINK"
echo -e "\n=== Environment files ==="
find . -name ".env*" -type f | grep -v node_modules | head -20Repository: unraid/api
Length of output: 1773
🏁 Script executed:
# Read the cloud.service.ts file to understand the context
cat -n packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts | sed -n '90,110p'
echo -e "\n=== Context around line 207 ==="
cat -n packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts | sed -n '200,220p'
echo -e "\n=== Check environment.ts for MOTHERSHIP_BASE_URL ==="
rg -n "MOTHERSHIP_BASE_URL" api/src/environment.tsRepository: unraid/api
Length of output: 2233
🏁 Script executed:
# Check environment.ts for configuration definitions
cat -n api/src/environment.ts | sed -n '90,105p'
echo -e "\n=== Full search for MOTHERSHIP_BASE_URL in entire repo ==="
find . -type f \( -name "*.ts" -o -name "*.js" -o -name ".env*" \) ! -path "*/node_modules/*" -exec grep -l "MOTHERSHIP_BASE_URL" {} \;Repository: unraid/api
Length of output: 1184
🏁 Script executed:
# Check .env.development for MOTHERSHIP_BASE_URL
cat -n api/.env.development | grep -A2 -B2 "MOTHERSHIP"
echo -e "\n=== Full content of .env.development ==="
cat api/.env.developmentRepository: unraid/api
Length of output: 2102
🏁 Script executed:
# Check other .env files for MOTHERSHIP_BASE_URL
echo "=== .env.staging ==="
grep "MOTHERSHIP" api/.env.staging
echo -e "\n=== .env.production ==="
grep "MOTHERSHIP" api/.env.production
echo -e "\n=== .env.test ==="
grep "MOTHERSHIP" api/.env.test
echo -e "\n=== plugin/.env.example ==="
grep "MOTHERSHIP" plugin/.env.example
echo -e "\n=== Check if MOTHERSHIP_BASE_URL should be in environment.ts ==="
rg -n "export const MOTHERSHIP" api/src/environment.tsRepository: unraid/api
Length of output: 417
🏁 Script executed:
# Verify configService type and how it accesses config
rg -n "configService.getOrThrow" packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts | head -5
echo -e "\n=== Check ConfigService implementation ==="
fd -n "config.service" packages/unraid-api-plugin-connect/src --type f
echo -e "\n=== Check if there's a NestJS ConfigModule setup ==="
rg -n "ConfigModule" packages/unraid-api-plugin-connect/src --type ts | head -10Repository: unraid/api
Length of output: 1415
Critical: Incomplete refactoring with missing environment configuration for MOTHERSHIP_BASE_URL.
Line 207 uses MOTHERSHIP_BASE_URL, but this config key is only defined in .env.development and missing from .env.staging, .env.production, and .env.test. Additionally, it is not exported from environment.ts like MOTHERSHIP_GRAPHQL_LINK is, causing configService.getOrThrow() to fail at runtime in non-development environments.
Furthermore, hardCheckCloud (line 96) still uses MOTHERSHIP_GRAPHQL_LINK, creating an inconsistency where DNS resolution (hardCheckDns, line 207) validates a different configuration than what canReachMothership and checkMothershipAuthentication use (line 98–106). The variable name mothershipGqlUri on line 207 is misleading when assigned a base URL instead of a GraphQL URL.
Required fixes:
- Add
MOTHERSHIP_BASE_URLto all.env.*files (staging, production, test) with appropriate values - Export
MOTHERSHIP_BASE_URLfromapi/src/environment.tswith defaults, matching the pattern used forMOTHERSHIP_GRAPHQL_LINK - Decide on the intended behavior:
- If both URLs should be identical, update line 96 to use
MOTHERSHIP_BASE_URLand rename variables consistently - If URLs intentionally differ, document why DNS checks validate a different hostname than connection attempts
- If both URLs should be identical, update line 96 to use
- Rename
mothershipGqlUritomothershipBaseUrlon line 207 for clarity
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/connection-status/cloud.service.ts
around line 207 (and related usages at lines ~96 and 98–106), the code reads
MOTHERSHIP_BASE_URL via configService.getOrThrow while that key is missing from
non-development .env files and not exported from api/src/environment.ts, and
there is an inconsistency between hardCheckCloud (using MOTHERSHIP_GRAPHQL_LINK)
and DNS checks (using MOTHERSHIP_BASE_URL) plus a misleading variable name
mothershipGqlUri; fix by (1) adding MOTHERSHIP_BASE_URL to all
.env.staging/.env.production/.env.test with appropriate values, (2) export
MOTHERSHIP_BASE_URL from api/src/environment.ts with a sensible default
following the existing MOTHERSHIP_GRAPHQL_LINK pattern, (3) decide whether the
base URL and GraphQL link should be the same—if yes, change hardCheckCloud to
use MOTHERSHIP_BASE_URL and align variable names; if they must differ, add
comments documenting why DNS checks validate a different host than connection
attempts, and (4) rename mothershipGqlUri to mothershipBaseUrl at line 207 and
update any usages to reflect the consistent naming.
| 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.
🛠️ Refactor suggestion | 🟠 Major
Replace any types with proper typing.
The interface uses any in multiple places, violating the coding guideline: "Never use the any type." Define proper types for the GraphQL execution contract.
+interface GraphQLResult<T = Record<string, unknown>> {
+ data?: T;
+ errors?: Array<{
+ message: string;
+ extensions?: Record<string, unknown>;
+ }>;
+}
+
interface GraphQLExecutor {
execute(params: {
query: string
- variables?: Record<string, any>
+ variables?: Record<string, unknown>
operationName?: string
operationType?: 'query' | 'mutation' | 'subscription'
- }): Promise<any>
+ }): Promise<GraphQLResult>
stopSubscription?(operationId: string): Promise<void>
}As per coding guidelines, avoid any in favor of proper typing.
📝 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.
| interface GraphQLExecutor { | |
| execute(params: { | |
| query: string | |
| variables?: Record<string, any> | |
| operationName?: string | |
| operationType?: 'query' | 'mutation' | 'subscription' | |
| }): Promise<any> | |
| stopSubscription?(operationId: string): Promise<void> | |
| } | |
| interface GraphQLResult<T = Record<string, unknown>> { | |
| data?: T; | |
| errors?: Array<{ | |
| message: string; | |
| extensions?: Record<string, unknown>; | |
| }>; | |
| } | |
| interface GraphQLExecutor { | |
| execute(params: { | |
| query: string | |
| variables?: Record<string, unknown> | |
| operationName?: string | |
| operationType?: 'query' | 'mutation' | 'subscription' | |
| }): Promise<GraphQLResult> | |
| stopSubscription?(operationId: string): Promise<void> | |
| } |
| private async handleNewFormatGraphQLRequest(message: any) { | ||
| if (!message.payload || !message.payload.query) { | ||
| console.warn('Invalid GraphQL request - missing payload or query:', message) | ||
| return | ||
| } | ||
|
|
||
| const operationId = message.operationId || `auto-${Date.now()}` | ||
| const messageId = message.messageId || `msg_${operationId}_${Date.now()}` | ||
|
|
||
| // Handle subscription stop | ||
| if (message.event === 'subscription_stop') { | ||
| if (this.executor.stopSubscription) { | ||
| await this.executor.stopSubscription(operationId) | ||
| } | ||
| this.sendResponse({ | ||
| operationId, | ||
| messageId, | ||
| event: 'query_response', | ||
| type: 'complete', | ||
| payload: { data: null }, | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
subscription_stop may not require a query payload.
The validation at lines 251-254 requires payload.query for all events, but subscription_stop typically only needs an operationId to stop a subscription. This could block valid stop requests.
private async handleNewFormatGraphQLRequest(message: any) {
+ // Handle subscription stop first - it only needs operationId, not query
+ if (message.event === 'subscription_stop') {
+ const operationId = message.operationId || `auto-${Date.now()}`
+ if (this.executor.stopSubscription) {
+ await this.executor.stopSubscription(operationId)
+ }
+ this.sendResponse({
+ operationId,
+ messageId: message.messageId || `msg_${operationId}_${Date.now()}`,
+ event: 'query_response',
+ type: 'complete',
+ payload: { data: null },
+ })
+ return
+ }
+
if (!message.payload || !message.payload.query) {
console.warn('Invalid GraphQL request - missing payload or query:', message)
return
}
-
- const operationId = message.operationId || `auto-${Date.now()}`
- const messageId = message.messageId || `msg_${operationId}_${Date.now()}`
-
- // Handle subscription stop
- if (message.event === 'subscription_stop') {
- if (this.executor.stopSubscription) {
- await this.executor.stopSubscription(operationId)
- }
- this.sendResponse({
- operationId,
- messageId,
- event: 'query_response',
- type: 'complete',
- payload: { data: null },
- })
- return
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/mothership-proxy/unraid-server-client.service.ts
around lines 250 to 272, the current early validation requires
message.payload.query for all events which incorrectly rejects valid
subscription_stop requests; modify the logic to check for the event first (or
allow a bypass) so that if message.event === 'subscription_stop' you accept the
request based on a present operationId (or the computed operationId fallback)
without requiring payload.query, then proceed to handle stopSubscription and
send the complete response as implemented.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Updates
✏️ Tip: You can customize this high-level summary in your review settings.