chore: extract connect to an API plugin #1367
Conversation
to create a new convention of storing and loading environment-related config from that file.
WalkthroughThis update introduces a major refactor and modularization of the Unraid API and Connect plugin codebases. It migrates remote access, connection, and cloud logic into a new plugin architecture, centralizes configuration and event handling, and implements unified settings management via JSON schema. Numerous shared utilities, types, and services are extracted into a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphQL API
participant PluginModule
participant ConfigService
participant EventBus
User->>GraphQL API: Query/Mutation (e.g., updateSettings)
GraphQL API->>PluginModule: Delegate to plugin/service
PluginModule->>ConfigService: Read/write config
PluginModule->>EventBus: Emit events (e.g., LOGIN, LOGOUT, CONNECTION_STATUS)
EventBus-->>PluginModule: Event-driven updates (e.g., dynamic remote access)
PluginModule-->>GraphQL API: Return result
GraphQL API-->>User: Response
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
api/src/unraid-api/config/store-sync.service.ts (1)
8-23: Consider optimizing store sync performanceThe service correctly implements
OnModuleDestroyfor proper cleanup and synchronizes the Redux store state to NestJS config. However, it currently syncs the entire store state on every change without any error handling.Consider these improvements:
- Add error handling for store subscriptions
- Consider selective synchronization if the store is large
- Consider debouncing frequent updates
constructor(private configService: ConfigService) { - this.unsubscribe = store.subscribe(() => { - this.configService.set('store', store.getState()); - this.logger.verbose('Synced store to NestJS Config'); + this.unsubscribe = store.subscribe(() => { + try { + this.configService.set('store', store.getState()); + this.logger.verbose('Synced store to NestJS Config'); + } catch (error) { + this.logger.error('Failed to sync store to NestJS Config', error); + } }); }api/src/unraid-api/config/legacy-config.module.ts (1)
8-15: Consider adding documentation for the loadersThe module effectively sets up the integration between legacy and NestJS config systems, but could benefit from JSDoc comments explaining the purpose of the loader functions.
@Module({ imports: [ConfigModule.forRoot({ isGlobal: true, + // Load environment variables and legacy Redux store state into NestJS ConfigService load: [loadAppEnvironment, loadLegacyStore], })], providers: [StoreSyncService], exports: [StoreSyncService], }) export class LegacyConfigModule {}api/src/unraid-api/app/pubsub.module.ts (1)
38-43: Consider tightening the EventEmitter memory‑leak guard
verboseMemoryLeak: trueis great, but you might want to capmaxListenersto a sane default (e.g. 20) so leaking handlers fail fast in production instead of only logging warnings.EventEmitterModule.forRoot({ wildcard: true, verboseMemoryLeak: true, + maxListeners: 20, }),packages/unraid-api-plugin-connect/src/pubsub/user.service.ts (1)
33-40: Use an informational log level for normal logout flow
logger.error()implies an unexpected failure, yet logout is a normal, user‑initiated action. Recommend downgrading tologorwarnto keep error channels meaningful.-this.logger.error('Logging out user: %s', reason ?? 'No reason provided'); +this.logger.log('User logout: %s', reason ?? 'No reason provided');packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
114-118: URL protocol replacement is brittle
replace('http', 'ws')mistakenly produceswss://→ws://s://forhttpsURLs. Safer:-url: this.mothershipGraphqlLink.replace('http', 'ws'), +url: this.mothershipGraphqlLink.replace(/^http/, 'ws'),or use
new URL():const url = new URL(this.mothershipGraphqlLink); url.protocol = url.protocol === 'https:' ? 'wss:' : 'ws:';packages/unraid-api-plugin-connect/src/config.persistence.ts (1)
23-26: Avoid magic‑string key"connect.config"That literal appears in three separate places; a single typo would silently break persistence. Consider extracting a constant:
export const CONNECT_CFG = 'connect.config' as const;then reuse
CONNECT_CFGeverywhere.
Besides safety it improves intent‑clarity for future readers.Also applies to: 57-58, 100-104
packages/unraid-api-plugin-connect/src/config.entity.ts (1)
37-40: Minor doc typo
// Remote Access Configurationx→ stray “x”.-// Remote Access Configurationx +// Remote‑access configurationpackages/unraid-api-plugin-connect/src/mothership/connection.service.ts (2)
45-60: ImplementOnModuleInitfor clarity
onModuleInitis present but the class does not implement the marker
interface. Addingimplements OnModuleInit(and the import) makes the
lifecycle expectation explicit and prevents accidental renaming.-import { Injectable, Logger } from '@nestjs/common'; +import { Injectable, Logger, OnModuleInit } from '@nestjs/common'; ... -export class MothershipConnectionService { +export class MothershipConnectionService implements OnModuleInit {
98-102: Narrow headers/params types after validationAfter the
isLoadedguard you know the values are defined, yetstate
keeps the original (possiblyundefined) types. A quick cast (or a
helper) lets TypeScript enforce non‑undefined on the returned object,
eliminating future!assertions.Not urgent, but worth tightening.
📜 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 (24)
.vscode/settings.json(1 hunks)api/.vscode/settings.json(1 hunks)api/package.json(1 hunks)api/src/consts.ts(0 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/app/app.module.ts(1 hunks)api/src/unraid-api/app/pubsub.module.ts(1 hunks)api/src/unraid-api/cli/pm2.service.ts(1 hunks)api/src/unraid-api/cli/restart.command.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/cli/stop.command.ts(1 hunks)api/src/unraid-api/config/config.loader.ts(1 hunks)api/src/unraid-api/config/legacy-config.module.ts(1 hunks)api/src/unraid-api/config/store-sync.service.ts(1 hunks)packages/unraid-api-plugin-connect/.prettierrc.cjs(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/config.entity.ts(1 hunks)packages/unraid-api-plugin-connect/src/config.persistence.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/connection.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/delay-function.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/pubsub/consts.ts(1 hunks)packages/unraid-api-plugin-connect/src/pubsub/user.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/src/consts.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
api/src/unraid-api/config/config.loader.ts (1)
api/src/store/index.ts (1)
store(6-12)
api/src/unraid-api/config/legacy-config.module.ts (2)
api/src/unraid-api/app/app.module.ts (1)
Module(20-68)api/src/unraid-api/config/config.loader.ts (2)
loadAppEnvironment(14-19)loadLegacyStore(29-33)
api/src/unraid-api/config/store-sync.service.ts (1)
api/src/store/index.ts (1)
store(6-12)
packages/unraid-api-plugin-connect/src/index.ts (2)
packages/unraid-api-plugin-generator/src/templates/index.ts (1)
adapter(7-7)packages/unraid-api-plugin-connect/src/config.entity.ts (1)
configFeature(145-150)
packages/unraid-api-plugin-connect/src/mothership/connection.service.ts (2)
packages/unraid-api-plugin-connect/src/pubsub/user.service.ts (1)
Injectable(12-53)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
Injectable(25-300)
packages/unraid-api-plugin-connect/src/config.entity.ts (3)
packages/unraid-api-plugin-connect/src/helpers/my-servers-config.ts (1)
MyServersConfig(29-53)packages/unraid-api-plugin-generator/src/templates/config.entity.ts (1)
configFeature(16-20)packages/unraid-api-plugin-connect/src/config.demo.ts (1)
ConnectConfig(3-6)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (21)
api/package.json (1)
70-70: Addition of event-emitter module looks good.This dependency is correctly added to support the new event-driven architecture being introduced with the PubSubModule.
api/src/environment.ts (1)
99-100:✅ Verification successful
Path constants appropriately moved from consts.ts to environment.ts.
The PM2 path constants are now correctly centralized with other environment-related configuration.
🏁 Script executed:
#!/bin/bash # Verify that references to these constants have been updated throughout the codebase rg -A 1 "import.*PM2_PATH|import.*ECOSYSTEM_PATH" --type tsLength of output: 846
All PM2_PATH and ECOSYSTEM_PATH imports updated
Verified that every reference now points to
@app/environment.js:
- api/src/unraid-api/cli/stop.command.ts
- api/src/unraid-api/cli/start.command.ts
- api/src/unraid-api/cli/restart.command.ts
- api/src/unraid-api/cli/pm2.service.ts
LGTM.
api/src/unraid-api/config/config.loader.ts (1)
14-19: Environment extraction implementation is clean and effective.The function correctly filters out functions and undefined values, providing a clean configuration object.
api/src/unraid-api/cli/start.command.ts (1)
4-4: Import path correctly updated to reflect constant relocation.The import now properly references the new location of ECOSYSTEM_PATH in the environment.ts file.
api/src/unraid-api/cli/restart.command.ts (1)
3-3: Import path updated correctlyThe import path for
ECOSYSTEM_PATHhas been properly updated from@app/consts.jsto@app/environment.js, aligning with the centralization of environment-related constants.api/src/unraid-api/cli/stop.command.ts (1)
3-3: Import path updated correctlyThe import path for
ECOSYSTEM_PATHhas been properly updated from@app/consts.jsto@app/environment.js, consistent with the centralization of environment constants.api/src/unraid-api/cli/pm2.service.ts (1)
9-9: Import path consolidated correctlyThe imports for
PM2_HOMEandPM2_PATHhave been correctly consolidated to a single import from@app/environment.js, consistent with the centralization of environment constants.api/src/unraid-api/app/app.module.ts (1)
11-11:✅ Verification successful
New modules properly integrated
The integration of
LegacyConfigModuleandPubSubModulein the AppModule is correctly implemented. These modules enhance the application with legacy configuration synchronization and event handling capabilities.Let's verify these modules are correctly initialized and don't cause any issues on application startup:
Also applies to: 14-14, 22-23
🏁 Script executed:
#!/bin/bash # Search for any initialization errors related to these new modules in the codebase rg -A 3 -B 3 "PubSubModule|LegacyConfigModule" --type ts rg -A 3 "Error.*PubSub|Error.*LegacyConfig" --type tsLength of output: 2113
LegacyConfigModule and PubSubModule integration verified – no errors found
- Definitions located in
api/src/unraid-api/config/legacy-config.module.tsapi/src/unraid-api/app/pubsub.module.ts- Both modules are correctly imported and listed in the
importsarray of
api/src/unraid-api/app/app.module.ts- No occurrences of initialization errors (
Error.*PubSuborError.*LegacyConfig) were found across the codebaseAll checks pass. Approving the integration of these modules.
api/.vscode/settings.json (1)
6-8: Import style configuration addedThe addition of non-relative import preferences for both TypeScript and JavaScript is a good standardization practice. This will help maintain consistent import paths across the codebase.
.vscode/settings.json (1)
12-14: Import style configuration added at root levelAdding the non-relative import preferences at the root level complements the same settings in the API directory, ensuring consistent import style throughout the entire project.
api/src/unraid-api/config/store-sync.service.ts (1)
1-6: Imports look goodThe imports include the necessary NestJS decorators and the Redux store, with appropriate type imports.
api/src/unraid-api/config/legacy-config.module.ts (2)
1-2: Informative commentThe comment clearly explains the purpose of this module as a bridge between legacy config and NestJS config system.
3-15: Module structure looks goodThe module correctly imports ConfigModule and configures it with appropriate loaders. It properly provides and exports the StoreSyncService for use in other modules.
packages/unraid-api-plugin-connect/package.json (1)
19-25: Dependency updates align with new GraphQL and event handling featuresThe added dependencies support the new GraphQL client service with WebSocket capabilities, the PubSub event bus system, and connection management features mentioned in the PR description. Good choice of specific versions that should work well together.
Also applies to: 32-34, 36-40, 43-48, 52-58
packages/unraid-api-plugin-connect/src/mothership/delay-function.ts (1)
1-22: Well-implemented retry delay function with best practicesThis utility function implements exponential backoff with optional jitter, which are excellent practices for network retry logic. The jitter helps prevent the "thundering herd" problem in distributed systems, and the exponential backoff ensures retries don't overwhelm the system.
The code is well-commented and has sensible defaults. The use of optional parameters with destructuring makes the function flexible and easy to use.
packages/unraid-api-plugin-connect/src/index.ts (1)
1-7: Style changes align with new Prettier configurationThe changes to import style, indentation, and constructor formatting align with the new Prettier configuration while maintaining the same functionality. Good job enforcing consistent code style across the codebase.
Also applies to: 8-8, 10-13, 15-21
packages/unraid-api-plugin-connect/.prettierrc.cjs (1)
1-38: Well-structured Prettier configuration improves code consistencyThe Prettier configuration is comprehensive and logically organized:
- Base formatting rules are clear and consistent
- Import grouping is logical with good separation between Nest.js/Node.js, third-party, and application code
- Type imports are prioritized within each group
- Parser plugins handle TypeScript and decorators correctly
This will significantly improve code readability and maintainability across the project.
packages/unraid-api-plugin-connect/src/pubsub/consts.ts (1)
1-8: Comment acknowledges maintenance concerns with shared constantsThe comment clearly explains the current limitation in the build/release system and indicates this approach is temporary. This transparency is good for future developers.
packages/unraid-api-plugin-connect/src/pubsub/user.service.ts (1)
21-23: Token name mismatch could break DI
GRAPHQL_PUB_SUB_TOKENcomes from./consts.ts, whereas the API side exports (in this PR)GRAPHQL_PUB_SUB. If the literal string values ever diverge, Nest will fail to resolve the dependency at runtime.Either:
- Re‑export the constant from the API package and import it here, or
- Keep a shared
tokens.tspackage to guarantee single‑source‑of‑truth.This avoids fragile “same string, different file” coupling.
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
169-176:appErrorLinkswallows errors silentlyThe custom
ApolloLinkcompletes the observer without callingobserver.error, meaning callers never know a request failed. If intentional, add a comment; otherwise pass the error downstream:- error: (error) => { - this.logger.warn('Apollo error, will not retry: %s', error?.message); - observer.complete(); - }, + error: (error) => { + this.logger.warn('Apollo error, propagating: %s', error?.message); + observer.error?.(error); + },packages/unraid-api-plugin-connect/src/config.entity.ts (1)
115-116:⚠️ Potential issueTypo breaks validation –
MinigraphStatus≠MinigraphStatus
@IsEnum(MinigraphStatus)references the wrong identifier (missing “a”).
At compile‑time this is an undefined symbol → build failure.-@IsEnum(MinigraphStatus) +@IsEnum(MinigraphStatus)Likely an incorrect or invalid review comment.
| if (this.wsClient) { | ||
| this.clearClientEventHandlers(); | ||
| this.wsClient.terminate(); | ||
| await this.wsClient.dispose(); | ||
| this.wsClient = null; | ||
| } |
There was a problem hiding this comment.
Client.terminate() is not part of graphql‑ws API
terminate() exists on the underlying low‑level WebSocket, not on the Client returned by createClient(). Calling it will raise TypeError: client.terminate is not a function.
-if (this.wsClient) {
- this.clearClientEventHandlers();
- this.wsClient.terminate();
- await this.wsClient.dispose();
- this.wsClient = null;
-}
+if (this.wsClient) {
+ this.clearClientEventHandlers();
+ await this.wsClient.dispose(); // disposes and closes the socket internally
+ this.wsClient = null;
+}📝 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.
| if (this.wsClient) { | |
| this.clearClientEventHandlers(); | |
| this.wsClient.terminate(); | |
| await this.wsClient.dispose(); | |
| this.wsClient = null; | |
| } | |
| if (this.wsClient) { | |
| this.clearClientEventHandlers(); | |
| await this.wsClient.dispose(); // disposes and closes the socket internally | |
| this.wsClient = null; | |
| } |
| async onModuleInit() { | ||
| this.logger.debug(`Config path: ${this.configPath}`); | ||
| await this.loadOrMigrateConfig(); | ||
| // Persist changes to the config. | ||
| const HALF_SECOND = 500; | ||
| this.configService.changes$.pipe(debounceTime(HALF_SECOND)).subscribe({ | ||
| next: async ({ newValue, oldValue, path }) => { | ||
| if (path.startsWith('connect.config')) { | ||
| this.logger.debug(`Config changed: ${path} from ${oldValue} to ${newValue}`); | ||
| await this.persist(); | ||
| } | ||
| }, | ||
| error: (err) => { | ||
| this.logger.error('Error receiving config changes:', err); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential subscription leak – keep a reference and dispose on module destroy
onModuleInit creates a subscription to this.configService.changes$ but the ConnectConfigPersister never unsubscribes from it.
If the provider is ever re‑started (hot‑reload, testing, etc.) the old subscription remains alive, producing duplicate work and memory leaks.
+private readonly subscriptions: Subscription[] = [];
...
- this.configService.changes$
+ const sub = this.configService.changes$
.pipe(debounceTime(HALF_SECOND))
.subscribe(/* … */);
+ this.subscriptions.push(sub);and in onModuleDestroy:
+ for (const sub of this.subscriptions) sub.unsubscribe();Committable suggestion skipped: line range outside the PR's diff.
packages/unraid-api-plugin-connect/src/mothership/connection.service.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/pubsub/consts.ts (1)
10-24: Enums still diverge from the API – verify before mergePrevious reviews flagged that API-side
PUBSUB_CHANNELincludes more entries (e.g.DASHBOARD,NOTIFICATION). Syncing both sides avoids silent event loss.If alignment is planned for a later commit, please confirm and add a TODO with a ticket reference.
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
99-104:⚠️ Potential issue
Client.terminate()is not part ofgraphql-wsAPICalling it throws
TypeError: client.terminate is not a function. Rely solely ondispose()(which closes the underlying socket) or calldisposefirst and skipterminate():- this.clearClientEventHandlers(); - this.wsClient.terminate(); - await this.wsClient.dispose(); + this.clearClientEventHandlers(); + await this.wsClient.dispose(); // graceful shutdown
🧹 Nitpick comments (4)
packages/unraid-api-plugin-connect/src/helpers/utils.ts (1)
60-65: Guard against nullish roots & improve type-safety
getNestedValuethrows ifobjisnull | undefined. A quick guard keeps its “safe accessor” promise and avoids theas anyescape-hatch:export function getNestedValue<TObj extends object, TPath extends string>( - obj: TObj, + obj: TObj | null | undefined, path: TPath ): Get<TObj, TPath> { - return path.split('.').reduce((acc, part) => acc?.[part], obj as any) as Get<TObj, TPath>; + if (obj == null) return undefined as Get<TObj, TPath>; + return path + .split('.') + .reduce((acc: any, part) => acc?.[part], obj) as Get<TObj, TPath>; }packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
291-298: Unusedeventsparameter – dead code
clearClientEventHandlersignores itseventsargument and instead drainsdisposalQueue. Drop the parameter to avoid confusion:-private clearClientEventHandlers( - events: Array<'connected' | 'connecting' | 'error' | 'ping'> = [ - 'connected', - 'connecting', - 'error', - 'ping', - ] -): void { +private clearClientEventHandlers(): void {packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts (1)
30-38: Bubble-up errors from client initialisation
createClientInstance()may throw (e.g., unreachable server, malformed headers).
Catching and logging here prevents unhandled rejections and makes retries explicit:-await this.clientService.createClientInstance(); +try { + await this.clientService.createClientInstance(); +} catch (err) { + this.logger.error('Failed to create GraphQL client: %o', err); + // leave status in ERROR_RETRYING so the retry logic can kick in + this.connectionService.setConnectionStatus({ + status: MinigraphStatus.ERROR_RETRYING, + error: (err as Error).message, + }); +}packages/unraid-api-plugin-connect/src/mothership/connection.service.ts (1)
165-171: Normalise header names to lowercase for Node.jsNode automatically lower-cases headers, but providing mixed-case keys can lead to duplicates
(User-Agentvsuser-agent) if another layer sets them too.
Conventionally send them in lower-case:- 'User-Agent': `unraid-api/${state.apiVersion}`, + 'user-agent': `unraid-api/${state.apiVersion}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
api/src/unraid-api/config/factory/api-state.model.ts(1 hunks)api/src/unraid-api/config/factory/api-state.register.ts(1 hunks)api/src/unraid-api/config/factory/api-state.service.ts(1 hunks)api/src/unraid-api/config/factory/config.injection.ts(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/helpers/utils.ts(2 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/connection.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/pubsub/consts.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- api/src/unraid-api/config/factory/config.injection.ts
- api/src/unraid-api/config/factory/api-state.model.ts
- api/src/unraid-api/config/factory/api-state.register.ts
- api/src/unraid-api/config/factory/api-state.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/unraid-api-plugin-connect/src/index.ts
- packages/unraid-api-plugin-connect/package.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts (3)
packages/unraid-api-plugin-connect/src/mothership/connection.service.ts (1)
Injectable(50-210)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
Injectable(27-305)packages/unraid-api-plugin-connect/src/pubsub/consts.ts (1)
GRAPHQL_PUB_SUB_TOKEN(17-17)
packages/unraid-api-plugin-connect/src/mothership/connection.service.ts (3)
packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts (1)
Injectable(11-76)packages/unraid-api-plugin-connect/src/config.persistence.ts (1)
Injectable(17-172)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
Injectable(27-305)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
190-193:⚠️ Potential issue
ConfigServicehas nosetmethod
@nestjs/configexposesget/getOrThrow; mutating config at runtime is unsupported. Persist the timeout elsewhere (e.g. dedicated service or in-memory state).⛔ Skipped due to learnings
Learnt from: pujitm PR: unraid/api#1367 File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52 Timestamp: 2025-04-23T20:19:42.542Z Learning: The project uses a custom ConfigService implementation that includes a `set()` method for runtime configuration mutation, unlike the standard @nestjs/config package which only provides getter methods.Learnt from: pujitm PR: unraid/api#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.
| const apolloClient = new ApolloClient({ | ||
| link: ApolloLink.from([appErrorLink, retryLink, errorLink, wsLink]), | ||
| cache: new InMemoryCache(), | ||
| defaultOptions: { | ||
| watchQuery: { | ||
| fetchPolicy: 'no-cache', | ||
| errorPolicy: 'all', | ||
| }, | ||
| query: { | ||
| fetchPolicy: 'no-cache', | ||
| errorPolicy: 'all', | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| this.initEventHandlers(); | ||
| return apolloClient; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror assignment inside createGraphqlClient (defensive)
Even after fixing createClientInstance, assigning here eliminates future foot-guns if someone bypasses that helper:
- const apolloClient = new ApolloClient({
+ this.apolloClient = new ApolloClient({
…
- });
-
- this.initEventHandlers();
- return apolloClient;
+ });
+
+ this.initEventHandlers();
+ return this.apolloClient;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const apolloClient = new ApolloClient({ | |
| link: ApolloLink.from([appErrorLink, retryLink, errorLink, wsLink]), | |
| cache: new InMemoryCache(), | |
| defaultOptions: { | |
| watchQuery: { | |
| fetchPolicy: 'no-cache', | |
| errorPolicy: 'all', | |
| }, | |
| query: { | |
| fetchPolicy: 'no-cache', | |
| errorPolicy: 'all', | |
| }, | |
| }, | |
| }); | |
| this.initEventHandlers(); | |
| return apolloClient; | |
| this.apolloClient = new ApolloClient({ | |
| link: ApolloLink.from([appErrorLink, retryLink, errorLink, wsLink]), | |
| cache: new InMemoryCache(), | |
| defaultOptions: { | |
| watchQuery: { | |
| fetchPolicy: 'no-cache', | |
| errorPolicy: 'all', | |
| }, | |
| query: { | |
| fetchPolicy: 'no-cache', | |
| errorPolicy: 'all', | |
| }, | |
| }, | |
| }); | |
| this.initEventHandlers(); | |
| return this.apolloClient; |
packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts
Outdated
Show resolved
Hide resolved
| @OnEvent(EVENTS.LOGOUT, { async: true, prependListener: true }) | ||
| async logout({ reason }: { reason?: string }) { |
There was a problem hiding this comment.
prependListener is not a recognised option for @OnEvent in NestJS
@OnEvent accepts options such as async, once, prepend, etc. – but not prependListener.
Using an unknown property is silently ignored at runtime and will trip the type-checker (unless noImplicitAny is off).
-@OnEvent(EVENTS.LOGOUT, { async: true, prependListener: true })
+@OnEvent(EVENTS.LOGOUT, { async: true, prepend: true })📝 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.
| @OnEvent(EVENTS.LOGOUT, { async: true, prependListener: true }) | |
| async logout({ reason }: { reason?: string }) { | |
| @OnEvent(EVENTS.LOGOUT, { async: true, prepend: true }) | |
| async logout({ reason }: { reason?: string }) { |
| resetMetadata() { | ||
| this.setMetadata({ status: MinigraphStatus.PRE_INIT }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
resetMetadata should also clear stale errors
Leaving the error field unchanged keeps the connection in an error state even
after a deliberate reset.
- resetMetadata() {
- this.setMetadata({ status: MinigraphStatus.PRE_INIT });
- }
+ resetMetadata() {
+ this.setMetadata({ status: MinigraphStatus.PRE_INIT, error: null });
+ }📝 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.
| resetMetadata() { | |
| this.setMetadata({ status: MinigraphStatus.PRE_INIT }); | |
| } | |
| resetMetadata() { | |
| this.setMetadata({ status: MinigraphStatus.PRE_INIT, error: null }); | |
| } |
| private updateMetadata(data: Partial<ConnectionMetadata>) { | ||
| this.configService.set('connect.mothership', { | ||
| ...this.configService.get<ConnectionMetadata>('connect.mothership'), | ||
| ...data, | ||
| }); |
There was a problem hiding this comment.
Crash-proof updateMetadata when the config key is unset
Spreading ...undefined throws TypeError: Cannot convert undefined or null to object
on the very first call (before resetMetadata() is executed).
- this.configService.set('connect.mothership', {
- ...this.configService.get<ConnectionMetadata>('connect.mothership'),
- ...data,
- });
+ const existing =
+ this.configService.get<ConnectionMetadata>('connect.mothership') ?? {};
+ this.configService.set('connect.mothership', {
+ ...existing,
+ ...data,
+ });📝 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.
| private updateMetadata(data: Partial<ConnectionMetadata>) { | |
| this.configService.set('connect.mothership', { | |
| ...this.configService.get<ConnectionMetadata>('connect.mothership'), | |
| ...data, | |
| }); | |
| private updateMetadata(data: Partial<ConnectionMetadata>) { | |
| const existing = | |
| this.configService.get<ConnectionMetadata>('connect.mothership') ?? {}; | |
| this.configService.set('connect.mothership', { | |
| ...existing, | |
| ...data, | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (3)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (3)
12-28: 🛠️ Refactor suggestionAuto-creating a root session for localhost is risky
Granting root privileges solely on
REMOTE_ADDR === 127.0.0.1 / ::1ignores common reverse-proxy setups (::ffff:127.0.0.1, Docker networks, etc.) and may be exploitable if traffic is forwarded.Consider:
- Checking
HTTP_X_FORWARDED_FOR/ proxy headers or disabling the shortcut when a proxy is detected.- Allowing an opt-in admin setting rather than unconditional elevation.
61-65:⚠️ Potential issueLogo anchor removed but closing tag remains elsewhere
The
<a>wrapper around the SVG was deleted but its closing tag is now missing, leaving malformed HTML. Either restore the anchor or remove the closing</a>(and adjust styles accordingly).
88-110:⚠️ Potential issueBell-icon logic references removed elements
#bellclass manipulation remains, but the element was removed in the header section, which will cause jQuery to operate on an empty set.Either:
- Re-introduce the bell element, or
- Delete the JS that mutates it.
Misalignment will break alert-count visual feedback.
♻️ Duplicate comments (6)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
781-804: SamenchanPausedtiming caveat as in the fixture fileThe snapshot reproduces the timer-reload script verbatim, so apply the same guard or variable-hoisting fix here to keep the snapshot in sync.
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (2)
124-129:⚠️ Potential issue
terminate()is still not part ofgraphql-wsClient – will throw at runtimeThis exact issue was raised previously and remains unresolved: the
Clientreturned bycreateClient()does not expose aterminate()method. Calling it will crash withTypeError: client.terminate is not a function.- this.clearClientEventHandlers(); - this.wsClient.terminate(); - await this.wsClient.dispose(); + this.clearClientEventHandlers(); + await this.wsClient.dispose(); // closes the socket internally
110-113:⚠️ Potential issue
apolloClientstill never cached – singleton pattern broken
createClientInstance()returns a fresh client but never persists it; subsequent calls getnulland rebuild a second WebSocket. The same feedback was given earlier and is still pending.- async createClientInstance(): Promise<ApolloClient<NormalizedCacheObject>> { - return this.getClient() ?? this.createGraphqlClient(); + async createClientInstance(): Promise<ApolloClient<NormalizedCacheObject>> { + if (!this.apolloClient) { + this.apolloClient = this.createGraphqlClient(); + } + return this.apolloClient; } … - const apolloClient = new ApolloClient({ + this.apolloClient = new ApolloClient({ link: ApolloLink.from([appErrorLink, retryLink, errorLink, wsLink]), cache: new InMemoryCache(), … }); - this.initEventHandlers(); - return apolloClient; + this.initEventHandlers(); + return this.apolloClient;Also applies to: 148-165
packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts (1)
72-73:⚠️ Potential issueInvalid option
prependListener– NestJS ignores it
@OnEventsupportsprepend, notprependListener. Using an unknown property is silently ignored and defeats the ordering you intend. This was pointed out earlier but is still present.-@OnEvent(EVENTS.LOGOUT, { async: true, prependListener: true }) +@OnEvent(EVENTS.LOGOUT, { async: true, prepend: true })packages/unraid-api-plugin-connect/src/mothership/connection.service.ts (2)
68-72:⚠️ Potential issueCrash risk: spreading
undefinedwhen config key is unsetIf
connect.mothershipis missing,this.configService.get()returnsundefined; spreading it throwsTypeError: Cannot convert undefined or null to object.- this.configService.set('connect.mothership', { - ...this.configService.get<ConnectionMetadata>('connect.mothership'), - ...data, - }); + const existing = + this.configService.get<ConnectionMetadata>('connect.mothership') ?? {}; + this.configService.set('connect.mothership', { + ...existing, + ...data, + });
207-209: 🛠️ Refactor suggestion
resetMetadata()leaves stale error messageReset only touches the status; the previous
error(and timestamps) survive the reset, so the UI still shows an error even after a manual clear.- resetMetadata() { - this.setMetadata({ status: MinigraphStatus.PRE_INIT }); + resetMetadata() { + this.setMetadata({ status: MinigraphStatus.PRE_INIT, error: null, lastPing: null, selfDisconnectedSince: null }); }
🧹 Nitpick comments (4)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (1)
1299-1321:pageFocusFunction()may explode silentlyInside
nchanFocusStart()the code calls an optionalpageFocusFunction()inside atry … catch {}block whosecatchis empty.
While this avoids a hard crash, it also hides real problems and makes debugging harder.If the function is truly optional, test for its existence instead of swallowing all errors:
- try { - pageFocusFunction(); - } catch(error) {} + if (typeof pageFocusFunction === 'function') { + pageFocusFunction(); + }The check is cheaper and prevents unrelated runtime exceptions from being ignored.
packages/unraid-api-plugin-connect/src/mothership/mothership-graphql.handler.ts (1)
1-4: Remove unused imports to avoid dead-code noise
OnEventandOnModuleDestroyare imported but not used. Until they are needed, drop them to keep the file lint-clean.-import { Inject, Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; -import { OnEvent } from '@nestjs/event-emitter'; +import { Inject, Injectable, Logger } from '@nestjs/common';packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (2)
316-323: Unused parameter – dead code & misleading API
clearClientEventHandlers(events = …)accepts aneventsarray but never uses it; the function simply drainsthis.disposalQueue. Either remove the parameter or honour it by filtering the disposables.
212-229: Unbounded retry may hammer the server
RetryLinksetsmax: Infinity; for network errors that are not invalid-API-key related the client will retry forever, even after hours of outage. Consider capping the total attempts or time-window to avoid flooding logs and wasting bandwidth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (5)
packages/unraid-api-plugin-connect/src/graphql/generated/client/fragment-masking.tsis excluded by!**/generated/**packages/unraid-api-plugin-connect/src/graphql/generated/client/gql.tsis excluded by!**/generated/**packages/unraid-api-plugin-connect/src/graphql/generated/client/graphql.tsis excluded by!**/generated/**packages/unraid-api-plugin-connect/src/graphql/generated/client/index.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/package.json(1 hunks)api/src/consts.ts(0 hunks)api/src/environment.ts(1 hunks)api/src/unraid-api/app/pubsub.module.ts(1 hunks)api/src/unraid-api/cli/start.command.ts(1 hunks)api/src/unraid-api/config/legacy-config.module.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php(3 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php(3 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(2 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/graphql/event.ts(1 hunks)packages/unraid-api-plugin-connect/src/graphql/remote-response.ts(1 hunks)packages/unraid-api-plugin-connect/src/helpers/parse-graphql.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/connection.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/internal.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership-graphql.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership-subscription.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/src/consts.ts
✅ Files skipped from review due to trivial changes (9)
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
- packages/unraid-api-plugin-connect/src/graphql/remote-response.ts
- api/dev/Unraid.net/myservers.cfg
- api/src/environment.ts
- packages/unraid-api-plugin-connect/codegen.ts
- packages/unraid-api-plugin-connect/src/graphql/event.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- api/package.json
- api/src/unraid-api/cli/start.command.ts
- api/src/unraid-api/config/legacy-config.module.ts
- packages/unraid-api-plugin-connect/package.json
- api/src/unraid-api/app/pubsub.module.ts
- packages/unraid-api-plugin-connect/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts (3)
packages/unraid-api-plugin-connect/src/mothership/connection.service.ts (1)
Injectable(50-222)packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts (1)
Injectable(12-83)packages/unraid-api-plugin-connect/src/mothership/mothership-subscription.handler.ts (1)
Injectable(29-215)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (1)
770-795:❌ Incorrect review comment
Reload-timer helpers work, but guard against an undefined
nchanPaused
setTimerReload()(and the click handler) accessnchanPaused, yet that variable is declared later in a separate<script>block.
If the page author (or a future refactor) ever moves thevar nchanPaused = false;block below the interval’s first tick, the callback will throwReferenceError: nchanPaused is not defined.A tiny defensive check eliminates that risk without altering behaviour:
- if (nchanPaused === false && ! dialogOpen() ) { + if ((window.nchanPaused === false || typeof window.nchanPaused === 'undefined') + && !dialogOpen()) { location.reload(); }(or simply hoist
var nchanPaused = false;above this<script>).Please confirm that
nchanPausedis guaranteed to be defined before the first interval fires (especially whenLoadis set to small values such as1).
🏁 Script executed:
#!/bin/bash # Locate all occurrences of nchanPaused to verify its declaration position rg -n "nchanPaused"Length of output: 2645
nchanPaused is defined before any timer callback—no change needed
The inline<script>forvar nchanPaused = false;runs immediately after the reload-timer script, and even with a 1-minute interval the variable is guaranteed to exist before the first tick. This is a test fixture, not production code, so no defensive check or hoisting is required.Likely an incorrect or invalid review comment.
packages/unraid-api-plugin-connect/src/mothership/mothership-graphql.handler.ts (1)
12-23: Class is empty – clarify intent or mark TODO
MothershipGraphqlHandleris registered as a provider but contains no hooks or behaviour yet.
If this is intentional (PR is WIP), add a short// TODO:comment so future readers know implementation is forthcoming; otherwise, flesh out the methods now to avoid an inert provider in production.api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (1)
38-53:openNotifierstill references jGrowl setup variablesThe jGrowl call block was stripped, yet
$.jGrowlassets may still be loaded, and other code later (bell badge updates) assumes those side-effects. Verify that:
jquery.jgrowl.js& CSS are no longer loaded — otherwise unused bytes are shipped.- All downstream code querying
.jGrowl-notificationis removed to avoid dead selectors.packages/unraid-api-plugin-connect/src/mothership/internal.client.ts (1)
79-87: Polyfillfetchfor Node < 18 or older deployment images.
HttpLinkrelies on a globalfetch. In Node ≤ 16 this is absent, and many Docker base images are still on those versions.
Consider importingcross-fetch(or similar) and passing it explicitly toHttpLinkto guarantee portability.import fetch from 'cross-fetch'; // ... const httpLink = new HttpLink({ uri: httpUri, fetch, // ✅ always defined headers: { /* … */ }, });
| interface ParsedQuery { | ||
| query?: string; | ||
| variables?: Record<string, string>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden the variable type to support all valid GraphQL inputs
variables may legitimately contain numbers, booleans, nested objects, arrays, etc.
Using Record<string, string> rejects those cases at compile time.
- variables?: Record<string, string>;
+ variables?: Record<string, unknown>;This change keeps type–safety while permitting every value shape that GraphQL allows.
📝 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 ParsedQuery { | |
| query?: string; | |
| variables?: Record<string, string>; | |
| } | |
| interface ParsedQuery { | |
| query?: string; | |
| variables?: Record<string, unknown>; | |
| } |
| export const parseGraphQLQuery = (body: string): QueryOptions => { | ||
| try { | ||
| const parsedBody: ParsedQuery = JSON.parse(body); | ||
| if (parsedBody.query && parsedBody.variables && typeof parsedBody.variables === 'object') { | ||
| return { | ||
| query: gql(parsedBody.query), | ||
| variables: parsedBody.variables, | ||
| }; | ||
| } | ||
| throw new Error('Invalid Body'); | ||
| } catch (error) { | ||
| throw new Error('Invalid Body Provided'); | ||
| } |
There was a problem hiding this comment.
Relax validation and surface parsing errors
- A query without variables (
{ query: "..."}) is perfectly legal, but the current guard fails it. - Swallowing the original
JSON.parseerror makes troubleshooting harder.
- if (parsedBody.query && parsedBody.variables && typeof parsedBody.variables === 'object') {
+ if (parsedBody.query && (!parsedBody.variables || typeof parsedBody.variables === 'object')) {
return {
query: gql(parsedBody.query),
- variables: parsedBody.variables,
+ // variables is optional in Apollo `QueryOptions`
+ ...(parsedBody.variables ? { variables: parsedBody.variables } : {}),
};
}
- throw new Error('Invalid Body');
+ throw new Error('Invalid body – missing "query" string');
} catch (error) {
- throw new Error('Invalid Body Provided');
+ /* re-throw with context to aid debugging */
+ const reason = error instanceof Error ? error.message : String(error);
+ throw new Error(`Body could not be parsed: ${reason}`);
}This keeps the contract intact while being more permissive and developer-friendly.
📝 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.
| export const parseGraphQLQuery = (body: string): QueryOptions => { | |
| try { | |
| const parsedBody: ParsedQuery = JSON.parse(body); | |
| if (parsedBody.query && parsedBody.variables && typeof parsedBody.variables === 'object') { | |
| return { | |
| query: gql(parsedBody.query), | |
| variables: parsedBody.variables, | |
| }; | |
| } | |
| throw new Error('Invalid Body'); | |
| } catch (error) { | |
| throw new Error('Invalid Body Provided'); | |
| } | |
| export const parseGraphQLQuery = (body: string): QueryOptions => { | |
| try { | |
| const parsedBody: ParsedQuery = JSON.parse(body); | |
| if (parsedBody.query && (!parsedBody.variables || typeof parsedBody.variables === 'object')) { | |
| return { | |
| query: gql(parsedBody.query), | |
| // variables is optional in Apollo `QueryOptions` | |
| ...(parsedBody.variables ? { variables: parsedBody.variables } : {}), | |
| }; | |
| } | |
| throw new Error('Invalid body – missing "query" string'); | |
| } catch (error) { | |
| /* re-throw with context to aid debugging */ | |
| const reason = error instanceof Error ? error.message : String(error); | |
| throw new Error(`Body could not be parsed: ${reason}`); | |
| } | |
| }; |
| public clearClient() { | ||
| this.client?.stop(); | ||
| this.client = null; | ||
| } |
There was a problem hiding this comment.
ApolloClient.stop() may be undefined – confirm version compatibility.
stop() exists in Apollo Client v2 but was removed in v3+.
If the project has already upgraded to v3 (many other imports suggest this), this call will throw TypeError: client.stop is not a function.
- this.client?.stop();
+ // v3 does not expose `stop`; fall back to terminating WebSocket connections if needed
+ (this.client as any)?.stop?.(); // optional chaining keeps this safeAlternatively, keep a reference to the WsLink client returned from createClient(...) and dispose it directly via dispose().
| private getNonSocketPortOverride() { | ||
| const port = this.configService.get<string | number | undefined>('PORT'); | ||
| if (!port || port.toString().includes('.sock')) { | ||
| return undefined; | ||
| } | ||
| return Number(port); | ||
| } |
There was a problem hiding this comment.
Guard against non-numeric PORT values to avoid NaN.
Number(port) will silently yield NaN for unexpected input (e.g. 'abc'). Down-stream callers treat the result as a valid number, which could generate malformed URLs (http://127.0.0.1:NaN/graphql) and connection failures.
- const port = this.configService.get<string | number | undefined>('PORT');
- if (!port || port.toString().includes('.sock')) {
+ const portValue = this.configService.get<string | number | undefined>('PORT');
+ if (!portValue || portValue.toString().includes('.sock')) {
return undefined;
}
- return Number(port);
+ const numericPort = Number(portValue);
+ return Number.isFinite(numericPort) ? numericPort : undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private getNonSocketPortOverride() { | |
| const port = this.configService.get<string | number | undefined>('PORT'); | |
| if (!port || port.toString().includes('.sock')) { | |
| return undefined; | |
| } | |
| return Number(port); | |
| } | |
| private getNonSocketPortOverride() { | |
| - const port = this.configService.get<string | number | undefined>('PORT'); | |
| - if (!port || port.toString().includes('.sock')) { | |
| - return undefined; | |
| - } | |
| - return Number(port); | |
| + const portValue = this.configService.get<string | number | undefined>('PORT'); | |
| + if (!portValue || portValue.toString().includes('.sock')) { | |
| + return undefined; | |
| + } | |
| + const numericPort = Number(portValue); | |
| + return Number.isFinite(numericPort) ? numericPort : undefined; | |
| } |
packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts
Show resolved
Hide resolved
| async subscribeToMothershipEvents(client = this.mothershipClient.getClient()) { | ||
| if (!client) { | ||
| this.logger.error('Mothership client unavailable. State might not be loaded.'); | ||
| return; | ||
| } | ||
| const subscription = client.subscribe({ | ||
| query: EVENTS_SUBSCRIPTION, | ||
| fetchPolicy: 'no-cache', | ||
| }); | ||
| this.mothershipSubscription = subscription.subscribe({ | ||
| next: (event) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error / complete handlers for mothership event stream.
If the upstream subscription errors out the local handler silently dies, leaving the system in an inconsistent state.
Consider supplying error and complete callbacks to attempt automatic resubscription or update connection status.
| clearStaleSubscriptions({ maxAgeMs }: { maxAgeMs: number }) { | ||
| const totalSubscriptions = this.subscriptions.size; | ||
| let numOfStaleSubscriptions = 0; | ||
| const now = Date.now(); | ||
| this.subscriptions | ||
| .entries() | ||
| .filter(([, { lastPing }]) => { | ||
| return now - lastPing > maxAgeMs; | ||
| }) | ||
| .forEach(([sha256]) => { | ||
| this.removeSubscription(sha256); | ||
| numOfStaleSubscriptions++; | ||
| }); | ||
| this.logger.debug( | ||
| `Cleared ${numOfStaleSubscriptions}/${totalSubscriptions} subscriptions (older than ${maxAgeMs}ms)` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Map.entries() returns an iterator – current chain will crash.
Iterators don’t expose filter/forEach directly. The current implementation will throw TypeError: iterator.filter is not a function.
- this.subscriptions
- .entries()
- .filter(([, { lastPing }]) => {
- return now - lastPing > maxAgeMs;
- })
- .forEach(([sha256]) => {
+ Array.from(this.subscriptions.entries())
+ .filter(([, { lastPing }]) => now - lastPing > maxAgeMs)
+ .forEach(([sha256]) => {
this.removeSubscription(sha256);
numOfStaleSubscriptions++;
});📝 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.
| clearStaleSubscriptions({ maxAgeMs }: { maxAgeMs: number }) { | |
| const totalSubscriptions = this.subscriptions.size; | |
| let numOfStaleSubscriptions = 0; | |
| const now = Date.now(); | |
| this.subscriptions | |
| .entries() | |
| .filter(([, { lastPing }]) => { | |
| return now - lastPing > maxAgeMs; | |
| }) | |
| .forEach(([sha256]) => { | |
| this.removeSubscription(sha256); | |
| numOfStaleSubscriptions++; | |
| }); | |
| this.logger.debug( | |
| `Cleared ${numOfStaleSubscriptions}/${totalSubscriptions} subscriptions (older than ${maxAgeMs}ms)` | |
| ); | |
| } | |
| clearStaleSubscriptions({ maxAgeMs }: { maxAgeMs: number }) { | |
| const totalSubscriptions = this.subscriptions.size; | |
| let numOfStaleSubscriptions = 0; | |
| const now = Date.now(); | |
| Array.from(this.subscriptions.entries()) | |
| .filter(([, { lastPing }]) => now - lastPing > maxAgeMs) | |
| .forEach(([sha256]) => { | |
| this.removeSubscription(sha256); | |
| numOfStaleSubscriptions++; | |
| }); | |
| this.logger.debug( | |
| `Cleared ${numOfStaleSubscriptions}/${totalSubscriptions} subscriptions (older than ${maxAgeMs}ms)` | |
| ); | |
| } |
| async clear() { | ||
| this.timeoutCheckerJob.stop(); | ||
| this.subscriptionHandler.stopMothershipSubscription(); | ||
| await this.clientService.clearInstance(); | ||
| this.connectionService.resetMetadata(); | ||
| this.subscriptionHandler.clearAllSubscriptions(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await the subscription & timeout teardown to avoid race conditions
stopMothershipSubscription() returns a Promise (per previous file), but it’s called without await. The handler could proceed to reset metadata while the subscription is still active, causing unexpected events.
- this.subscriptionHandler.stopMothershipSubscription();
+ await this.subscriptionHandler.stopMothershipSubscription();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async clear() { | |
| this.timeoutCheckerJob.stop(); | |
| this.subscriptionHandler.stopMothershipSubscription(); | |
| await this.clientService.clearInstance(); | |
| this.connectionService.resetMetadata(); | |
| this.subscriptionHandler.clearAllSubscriptions(); | |
| } | |
| async clear() { | |
| this.timeoutCheckerJob.stop(); | |
| await this.subscriptionHandler.stopMothershipSubscription(); | |
| await this.clientService.clearInstance(); | |
| this.connectionService.resetMetadata(); | |
| this.subscriptionHandler.clearAllSubscriptions(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts (1)
59-65:stop()throws if the interval was never registered.
SchedulerRegistry.getInterval()throws aNotFoundExceptionwhen the key is missing.
Becausestart()callsstop()pre-emptively, the very firststart()will crash the service.- const interval = this.schedulerRegistry.getInterval(this.jobName); - if (isDefined(interval)) { - clearInterval(interval); - this.schedulerRegistry.deleteInterval(this.jobName); - } + try { + const interval = this.schedulerRegistry.getInterval(this.jobName); + clearInterval(interval); + this.schedulerRegistry.deleteInterval(this.jobName); + } catch { + // Interval was not registered – nothing to stop + this.logger.debug(`No active interval found for ${this.jobName}`); + }packages/unraid-api-plugin-connect/src/config.entity.ts (1)
204-210:⚠️ Potential issueGeneric type mismatch –
ConnectConfigdoes not declaremothershipordynamicRemoteAccess
registerAs<ConnectConfig>('connect', …)returns an object withmothershipanddynamicRemoteAccessproperties, yetConnectConfig(fromconfig.demo.ts) only hasdemo.
This violates the generic contract and will surface as a TypeScript error.Options:
- Extend
ConnectConfigto include the new properties, or- Replace the generic with an updated interface that matches the returned shape.
-export const configFeature = registerAs<ConnectConfig>('connect', () => ({ +export const configFeature = registerAs('connect', () => ({(or update the
ConnectConfiginterface to include the new properties).
🧹 Nitpick comments (4)
packages/unraid-api-plugin-connect/src/remote-access/dynamic-remote-access-state.service.ts (1)
13-15: Consider handling missing configuration more gracefullyThe
getOrThrowmethod will throw an exception if the configuration key doesn't exist. Consider whether a more graceful fallback might be appropriate here, such as usinggetwith a default value.- return this.configService.getOrThrow<DynamicRemoteAccessState>('connect.dynamicRemoteAccess'); + return this.configService.get<DynamicRemoteAccessState>( + 'connect.dynamicRemoteAccess', + makeDisabledDynamicRemoteAccessState() + );packages/unraid-api-plugin-connect/src/config.entity.ts (3)
42-42: Fix typo in comment- // Remote Access Configurationx + // Remote Access Configuration
117-148: Ensure ConnectionMetadata has proper initialization defaultsThe
ConnectionMetadataclass has several nullable fields, but onlystatusis initialized inconfigFeature. Consider adding default values for all fields or creating a factory function similar tomakeDisabledDynamicRemoteAccessState().
173-194: Consider adding URL validation for IP addressesFor the
ipv4andipv6fields, consider adding regex pattern validation to ensure they match valid IP address formats.@Field(() => String, { nullable: true }) @IsString() @IsOptional() +@Matches(/^(\d{1,3}\.){3}\d{1,3}$/, { message: 'Must be a valid IPv4 address' }) ipv4!: string | null | undefined; @Field(() => String, { nullable: true }) @IsString() @IsOptional() +@Matches(/^([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$/, { message: 'Must be a valid IPv6 address' }) ipv6!: string | null | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
packages/unraid-api-plugin-connect/src/config.entity.ts(1 hunks)packages/unraid-api-plugin-connect/src/helpers/consts.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts(1 hunks)packages/unraid-api-plugin-connect/src/remote-access/dynamic-remote-access-state.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/unraid-api-plugin-connect/src/helpers/consts.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/unraid-api-plugin-connect/src/remote-access/dynamic-remote-access-state.service.ts (3)
packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts (1)
Injectable(12-71)packages/unraid-api-plugin-connect/src/helpers/consts.ts (1)
ONE_MINUTE_MS(1-1)packages/unraid-api-plugin-connect/src/config.entity.ts (1)
makeDisabledDynamicRemoteAccessState(196-202)
packages/unraid-api-plugin-connect/src/config.entity.ts (3)
packages/unraid-api-plugin-connect/src/helpers/my-servers-config.ts (1)
MyServersConfig(29-53)packages/unraid-api-plugin-generator/src/templates/config.entity.ts (1)
configFeature(16-20)packages/unraid-api-plugin-connect/src/config.demo.ts (1)
ConnectConfig(3-6)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/unraid-api-plugin-connect/src/remote-access/dynamic-remote-access-state.service.ts (1)
17-26: LGTM! Timeout checking logic works as expected.The implementation correctly checks if the last ping is older than one minute and disables dynamic remote access if needed.
packages/unraid-api-plugin-connect/src/config.entity.ts (1)
150-171: LGTM! Well-structured state entity with validationThe
DynamicRemoteAccessStateclass is well-structured with appropriate validation decorators and field definitions.
packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts
Show resolved
Hide resolved
| async checkForTimeouts() { | ||
| this.subscriptionHandler.clearStaleSubscriptions({ maxAgeMs: THREE_MINUTES_MS }); | ||
| this.checkMothershipClientTimeout(); | ||
| this.dynamicRemoteAccess.checkForTimeout(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling potential errors in checkForTimeouts
Since this method is called in a periodic interval, any unhandled errors could cause the interval to stop working. Consider adding try/catch blocks around each method call.
async checkForTimeouts() {
+ try {
this.subscriptionHandler.clearStaleSubscriptions({ maxAgeMs: THREE_MINUTES_MS });
this.checkMothershipClientTimeout();
this.dynamicRemoteAccess.checkForTimeout();
+ } catch (error) {
+ this.logger.error('Error checking for timeouts', error);
+ }
}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/unraid-api-plugin-connect/src/config.persistence.ts (1)
37-48: Subscription never unsubscribed (repeat feedback)
Previous review already highlighted the potential leak; the code still doesn’t keep the Subscription reference or clean it up inonModuleDestroy().
🧹 Nitpick comments (11)
packages/unraid-api-plugin-connect/src/system/nginx.service.ts (1)
8-16: Minor inconsistency in error messageThe error message states "Failed to restart Nginx" but the command is actually running
reload.- this.logger.warn('Failed to restart Nginx with error: ', err); + this.logger.warn('Failed to reload Nginx with error: ', err);packages/unraid-api-plugin-connect/src/system/dns.service.ts (1)
9-17: Consider adding a comment explaining what the UpdateDNS.php script doesThe service calls an external PHP script, but without any context about its purpose or expected behavior.
async update() { + // Updates DNS settings by running the Dynamix plugin's PHP script try { await execa('/usr/bin/php', ['/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php']); return true; } catch (err: unknown) { this.logger.warn('Failed to call Update DNS with error: ', err); return false; } }packages/unraid-api-plugin-connect/src/system/network.service.ts (1)
13-16: Consider handling the return valuesBoth
nginxService.reload()anddnsService.update()return booleans indicating success/failure, but this method doesn't use those values.async reloadNetworkStack() { - await this.nginxService.reload(); - await this.dnsService.update(); + const nginxResult = await this.nginxService.reload(); + const dnsResult = await this.dnsService.update(); + return nginxResult && dnsResult; }packages/unraid-api-plugin-connect/package.json (3)
2-18: Update package descriptionThe description still says "Example Health plugin for Unraid API" but this package is for the Connect plugin.
"license": "GPL-2.0-or-later", - "description": "Example Health plugin for Unraid API", + "description": "Connect plugin for Unraid API",
13-13: Hardcoded staging URL in codegen scriptThe codegen script has a hardcoded URL to the staging environment. Consider making this configurable.
- "codegen": "MOTHERSHIP_GRAPHQL_LINK='https://staging.mothership.unraid.net/ws' graphql-codegen --config codegen.ts" + "codegen": "graphql-codegen --config codegen.ts"Then set the URL in the codegen.ts file or use an .env file for environment-specific configuration.
19-77: Lots of duplicated dependenciesMany packages appear in both devDependencies and peerDependencies with identical version constraints. Consider reviewing if all of these need to be in both sections.
Typically, peerDependencies are for packages that your library expects to be provided by the consuming application, while devDependencies are only needed during development.
packages/unraid-api-plugin-connect/src/remote-access/static-remote-access.service.ts (1)
15-18:getRemoteAccessUrlis a stub that can break callers
beginRemoteAccess()returns the result of this method, yet the current implementation always yieldsnull. Any consumer expecting a usable URL will mis-behave.
Consider at least returning a sensible placeholder or throwingNotImplementedExceptionuntil the full logic is in place.packages/unraid-api-plugin-connect/src/system/upnp.service.ts (1)
143-148: Nit: prefer early return to avoid nested blocks
A quick readability win—exit early when no WAN port can be determined.-const wanPortToUse = await this.getWanPortToUse(args); -if (!wanPortToUse) { - await this.disableUpnp(); - ... -} +const wanPortToUse = await this.getWanPortToUse(args); +if (!wanPortToUse) { + await this.disableUpnp(); + this.logger.error('No WAN port found. Disabled UPNP.'); + throw new Error('No WAN port found. Disabled UPNP.'); +}packages/unraid-api-plugin-connect/src/config.entity.ts (3)
42-42: Fix typo in commentThere's a typo in the comment: "Remote Access Configurationx" should be "Remote Access Configuration".
- // Remote Access Configurationx + // Remote Access Configuration
107-109: Commented code should be removedThese commented lines appear to be no longer needed since the connection status tracking has been moved to the dedicated
ConnectionMetadataclass.- // @Field(() => MinigraphStatus) - // @IsEnum(MinigraphStatus) - // minigraph!: MinigraphStatus;
210-213: Consider using more specific types instead ofanyThe
store: anyreduces type safety. Consider using a more specific type if possible.- store: any; + store: Record<string, unknown>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/config.demo.ts(1 hunks)packages/unraid-api-plugin-connect/src/config.entity.ts(1 hunks)packages/unraid-api-plugin-connect/src/config.persistence.ts(1 hunks)packages/unraid-api-plugin-connect/src/helpers/consts.ts(1 hunks)packages/unraid-api-plugin-connect/src/helpers/dependency-injection.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts(1 hunks)packages/unraid-api-plugin-connect/src/remote-access/static-remote-access.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/dns.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/network.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/nginx.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/upnp.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/unraid-api-plugin-connect/src/config.demo.ts
- packages/unraid-api-plugin-connect/src/helpers/dependency-injection.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/unraid-api-plugin-connect/src/mothership/timeout-checker.job.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/unraid-api-plugin-connect/src/system/nginx.service.ts (3)
packages/unraid-api-plugin-connect/src/remote-access/static-remote-access.service.ts (1)
Injectable(6-36)packages/unraid-api-plugin-connect/src/system/dns.service.ts (1)
Injectable(5-18)packages/unraid-api-plugin-connect/src/system/network.service.ts (1)
Injectable(6-17)
packages/unraid-api-plugin-connect/src/system/dns.service.ts (3)
packages/unraid-api-plugin-connect/src/remote-access/static-remote-access.service.ts (1)
Injectable(6-36)packages/unraid-api-plugin-connect/src/system/nginx.service.ts (1)
Injectable(4-17)packages/unraid-api-plugin-connect/src/system/network.service.ts (1)
Injectable(6-17)
packages/unraid-api-plugin-connect/src/config.entity.ts (3)
packages/unraid-api-plugin-connect/src/helpers/my-servers-config.ts (1)
MyServersConfig(29-53)packages/unraid-api-plugin-connect/src/config.demo.ts (1)
ConnectDemoConfig(3-6)packages/unraid-api-plugin-generator/src/templates/config.entity.ts (1)
configFeature(16-20)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
packages/unraid-api-plugin-connect/src/helpers/consts.ts (1)
1-3: Nice implementation of time constants!The constants are well-structured, using calculation rather than hardcoded values and featuring clear naming conventions that indicate the units (MS for milliseconds, SECS for seconds). Using
ONE_MINUTE_MSto calculateTHREE_MINUTES_MSis especially good practice.Consider adding JSDoc comments to these constants to provide context for their intended usage across the Connect Plugin.
packages/unraid-api-plugin-connect/src/system/nginx.service.ts (2)
1-2: Good use of importsImports are clean and appropriately specified.
4-7: Good implementation of loggerThe service correctly uses NestJS injection and implements a private logger with the class name.
packages/unraid-api-plugin-connect/src/system/dns.service.ts (1)
1-7: Good class structureThe service is properly decorated as injectable and implements a class-specific logger.
packages/unraid-api-plugin-connect/src/system/network.service.ts (1)
1-11: Good dependency injectionThe service correctly injects its dependencies through the constructor.
packages/unraid-api-plugin-connect/src/config.entity.ts (2)
204-208: Type definition resolves previous issueThis
ConnectConfigtype correctly includes themothershipproperty, which resolves the generic type mismatch issue mentioned in the previous review.
215-237: Verify all config properties are initializedThe config feature properly initializes the new structure with default values. Ensure that all properties in the various classes have proper defaults to prevent runtime errors.
| async beginRemoteAccess() { | ||
| const { dynamicRemoteAccessType } = this.configService.getOrThrow<MyServersConfig>('connect.config'); | ||
| if (dynamicRemoteAccessType !== DynamicRemoteAccessType.STATIC) { | ||
| this.logger.error('Invalid Dynamic Remote Access Type: %s', dynamicRemoteAccessType); | ||
| return null; | ||
| } | ||
| this.logger.log('Enabling Static Remote Access'); | ||
| this.configService.set('connect.config.wanaccess', true); | ||
| await this.networkService.reloadNetworkStack(); | ||
| return this.getRemoteAccessUrl(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling is one-sided
reloadNetworkStack() can throw but the promise chain isn’t wrapped in try/catch, so the service may bubble an unhandled rejection and leave the config in an inconsistent state (wanaccess=true while the network stack never reloaded).
Wrap the mutation & reload in a guarded block and roll back on failure.
try {
this.configService.set('connect.config.wanaccess', true);
await this.networkService.reloadNetworkStack();
return this.getRemoteAccessUrl();
-}
+} catch (err) {
+ this.logger.error('Static RA enable failed', err);
+ this.configService.set('connect.config.wanaccess', false);
+ throw err;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async beginRemoteAccess() { | |
| const { dynamicRemoteAccessType } = this.configService.getOrThrow<MyServersConfig>('connect.config'); | |
| if (dynamicRemoteAccessType !== DynamicRemoteAccessType.STATIC) { | |
| this.logger.error('Invalid Dynamic Remote Access Type: %s', dynamicRemoteAccessType); | |
| return null; | |
| } | |
| this.logger.log('Enabling Static Remote Access'); | |
| this.configService.set('connect.config.wanaccess', true); | |
| await this.networkService.reloadNetworkStack(); | |
| return this.getRemoteAccessUrl(); | |
| async beginRemoteAccess() { | |
| const { dynamicRemoteAccessType } = this.configService.getOrThrow<MyServersConfig>('connect.config'); | |
| if (dynamicRemoteAccessType !== DynamicRemoteAccessType.STATIC) { | |
| this.logger.error('Invalid Dynamic Remote Access Type: %s', dynamicRemoteAccessType); | |
| return null; | |
| } | |
| this.logger.log('Enabling Static Remote Access'); | |
| try { | |
| this.configService.set('connect.config.wanaccess', true); | |
| await this.networkService.reloadNetworkStack(); | |
| return this.getRemoteAccessUrl(); | |
| } catch (err) { | |
| this.logger.error('Static RA enable failed', err); | |
| this.configService.set('connect.config.wanaccess', false); | |
| throw err; | |
| } | |
| } |
| async enableUpnp(args?: { sslPort?: number; wanPort?: number }) { | ||
| const { sslPort, wanPort } = args ?? {}; | ||
| if (wanPort !== this.#wanPort || this.#localPort !== sslPort) { | ||
| await this.removeUpnpLease(); | ||
| } | ||
|
|
||
| // todo: start the renewal job | ||
|
|
||
| const wanPortToUse = await this.getWanPortToUse(args); | ||
| this.#wanPort = wanPortToUse; | ||
| const localPortToUse = sslPort ?? this.#localPort; | ||
| if (wanPortToUse && localPortToUse) { | ||
| await this.renewUpnpLease({ | ||
| publicPort: wanPortToUse, | ||
| privatePort: localPortToUse, | ||
| }); | ||
| } else { | ||
| await this.disableUpnp(); | ||
| this.logger.error('No WAN port found %o. Disabled UPNP.', { | ||
| wanPort: wanPortToUse, | ||
| localPort: localPortToUse, | ||
| }); | ||
| throw new Error('No WAN port found. Disabled UPNP.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Race condition & missing renewal job
enableUpnp() mutates internal state before renewUpnpLease() resolves. If the latter fails, the class remains in a half-enabled state. Also, the TODO about starting a renewal task is crucial—without it the lease expires after an hour.
Wrap the call in try/catch and revert the fields on error; schedule the renewal before returning.
Implement atomic enableUpnp with rollback and schedule renewal
The current implementation in
packages/unraid-api-plugin-connect/src/system/upnp.service.ts (lines 137–161)
mutates this.#wanPort (and by extension this.#localPort) before renewUpnpLease() succeeds, leaving the service in a half-enabled state if renewal fails. Additionally, the // todo: start the renewal job comment indicates that leases aren’t being refreshed, so they’ll expire after their TTL.
Please:
- Wrap the call to
renewUpnpLease({ publicPort, privatePort })in atry/catch.- On error, revert
this.#wanPortandthis.#localPortto their previous values and rethrow or handle the failure gracefully.
- On error, revert
- Replace the
// todo: start the renewal jobwith an actual scheduled renewal (e.g., asetIntervalor background timer) so the lease is refreshed before expiration.
This ensures that:
- State changes only persist on success.
- Leases are automatically renewed.
| const excludedPorts = new Set(mappings?.map((val) => val.public.port)); | ||
| // Generate a random port between minPort and maxPort up to maxAttempts times | ||
| for (let i = 0; i < maxAttempts; i++) { | ||
| const port = Math.floor(Math.random() * (maxPort - minPort + 1)) + minPort; | ||
| if (!excludedPorts.has(port)) { | ||
| return port; | ||
| } |
There was a problem hiding this comment.
new Set(undefined) will crash – guard against missing mappings
If getMappings() returns undefined, the spread into new Set() throws TypeError.
-const excludedPorts = new Set(mappings?.map((val) => val.public.port));
+const excludedPorts = new Set(
+ (mappings ?? []).map((val) => val.public.port),
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const excludedPorts = new Set(mappings?.map((val) => val.public.port)); | |
| // Generate a random port between minPort and maxPort up to maxAttempts times | |
| for (let i = 0; i < maxAttempts; i++) { | |
| const port = Math.floor(Math.random() * (maxPort - minPort + 1)) + minPort; | |
| if (!excludedPorts.has(port)) { | |
| return port; | |
| } | |
| const excludedPorts = new Set( | |
| (mappings ?? []).map((val) => val.public.port), | |
| ); | |
| // Generate a random port between minPort and maxPort up to maxAttempts times | |
| for (let i = 0; i < maxAttempts; i++) { | |
| const port = Math.floor(Math.random() * (maxPort - minPort + 1)) + minPort; | |
| if (!excludedPorts.has(port)) { | |
| return port; |
| async persist(config = this.configService.get<MyServersConfig>('connect.config')) { | ||
| try { | ||
| if (isEqual(config, await this.loadConfig())) { | ||
| this.logger.verbose(`Config is unchanged, skipping persistence`); | ||
| return false; | ||
| } | ||
| } catch (error) { | ||
| this.logger.error(`Error loading config (will overwrite file):`, error); | ||
| } | ||
| const data = JSON.stringify(config, null, 2); |
There was a problem hiding this comment.
persist() writes "undefined" when config is missing
If connect.config hasn’t been set, config is undefined; JSON.stringify(undefined) serialises to the literal string "undefined" which isn’t valid JSON for later reads.
Guard against empty configs.
async persist(config = this.configService.get<MyServersConfig>('connect.config')) {
+ if (config == null) {
+ this.logger.warn('No config in memory – skipping persistence');
+ return false;
+ }
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async persist(config = this.configService.get<MyServersConfig>('connect.config')) { | |
| try { | |
| if (isEqual(config, await this.loadConfig())) { | |
| this.logger.verbose(`Config is unchanged, skipping persistence`); | |
| return false; | |
| } | |
| } catch (error) { | |
| this.logger.error(`Error loading config (will overwrite file):`, error); | |
| } | |
| const data = JSON.stringify(config, null, 2); | |
| async persist(config = this.configService.get<MyServersConfig>('connect.config')) { | |
| if (config == null) { | |
| this.logger.warn('No config in memory – skipping persistence'); | |
| return false; | |
| } | |
| try { | |
| if (isEqual(config, await this.loadConfig())) { | |
| this.logger.verbose(`Config is unchanged, skipping persistence`); | |
| return false; | |
| } | |
| } catch (error) { | |
| this.logger.error(`Error loading config (will overwrite file):`, error); | |
| } | |
| const data = JSON.stringify(config, null, 2); |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/unraid-api-plugin-connect/src/system/upnp.service.ts (1)
125-126:new Set(undefined)will still explode – previous feedback not applied
Passingundefinedto theSetconstructor raisesTypeError.
Use an empty array fallback as suggested earlier.-const excludedPorts = new Set(mappings?.map((val) => val.public.port)); +const excludedPorts = new Set((mappings ?? []).map((val) => val.public.port));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
packages/unraid-api-plugin-connect/src/helpers/dependency-injection.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/upnp.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/unraid-api-plugin-connect/src/helpers/dependency-injection.ts (1)
2-3: Looks good – thanks for centralising the tokens
Clear constant names, all-caps convention is consistent with the rest of the helpers.packages/unraid-api-plugin-connect/src/system/upnp.service.ts (1)
175-183: Cron frequency vs TTL – intentional?
The job renews every 30 min while the TTL is 60 min. That’s fine, but if you ever shorten the TTL remember to adjust the cron to trigger sooner than half the TTL to avoid race conditions.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/remote-access/upnp-remote-access.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
packages/unraid-api-plugin-connect/src/remote-access/upnp-remote-access.service.ts (2)
1-6: Imports look goodThe imports are clear and appropriate for the service's functionality.
8-17: Service declaration and logger setup are well-structuredThe class correctly uses NestJS dependency injection and establishes a properly scoped logger instance.
packages/unraid-api-plugin-connect/src/remote-access/upnp-remote-access.service.ts
Show resolved
Hide resolved
packages/unraid-api-plugin-connect/src/remote-access/upnp-remote-access.service.ts
Show resolved
Hide resolved
pujitm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts (3)
136-150: IPv6 hosts need brackets –new URL()mis-parses raw IPv6 literals
new URL('http://2001:db8::1')throws because2001:db8::1is read as a scheme.
Wrap bare IPv6 addresses in[].- const urlString = `${httpMode}${url}${portToUse}`; + const host = /:/.test(url) && !url.startsWith('[') ? `[${url}]` : url; + const urlString = `${httpMode}${host}${portToUse}`;
191-201:sslMode: "no"not handled – valid configs will falsely errorWhen
sslEnabledis true butsslModeis"no", the function falls through to the generic error branch.- if (nginx.sslMode === 'yes') { + if (nginx.sslMode === 'yes') { ... + } else if (nginx.sslMode === 'no') { + return this.getUrlForField({ + url: nginx[field] as string, + port: nginx.httpPort, + }); }
308-315: IPv6 URL stored inipv4field – likely copy-paste bug
lanIp6result is assigned toipv4, leavingipv6empty.- urls.push({ - name: 'LAN IPv6', - type: URL_TYPE.LAN, - ipv4: lanIp6Url, - }); + urls.push({ + name: 'LAN IPv6', + type: URL_TYPE.LAN, + ipv6: lanIp6Url, + });packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)
121-146:restartRequiredflag is never setYou initialise
let restartRequired = falsebut never mutate it, yet the method's docstring says it may returntrue. Either implement the logic or returnvoidto avoid misleading callers.
🧹 Nitpick comments (1)
api/docs/developer/api-plugins.md (1)
13-13: Clarify the use of “vendor” as a verb.The phrasing “we vendor them” may confuse readers, since “vendor” is more commonly a noun. Consider rewording for clarity.
Proposed diff:
-To solve this, we vendor them during the build process. Here's the complete process: +To solve this, we bundle (vendor) them during the build process. Here's the complete process:🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Check that the noun ‘vendor’ after the pronoun ‘we’ is correct. It’s possible that you may need to switch to a possessive pronoun, or use another part of speech.
Context: ...a npm during production. To solve this, we vendor them during the build process. Here's t...(PRP_VB)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/docs/developer/api-plugins.md(1 hunks)api/src/unraid-api/plugin/global-deps.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/mothership-subscription.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/nginx.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/upnp.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts(1 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/util/data.ts(2 hunks)packages/unraid-shared/src/util/key-order.ts(1 hunks)plugin/.gitignore(1 hunks)unraid-ui/src/forms/UnraidSettingsLayout.vue(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- plugin/.gitignore
- packages/unraid-api-plugin-connect/src/service/upnp.service.ts
- packages/unraid-api-plugin-connect/src/service/mothership-subscription.handler.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/unraid-shared/src/index.ts
- unraid-ui/src/forms/UnraidSettingsLayout.vue
- packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts
- packages/unraid-api-plugin-connect/src/service/nginx.service.ts
- packages/unraid-shared/src/util/key-order.ts
- api/src/unraid-api/plugin/global-deps.module.ts
- packages/unraid-shared/src/util/data.ts
- packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts
🧰 Additional context used
🪛 LanguageTool
api/docs/developer/api-plugins.md
[grammar] ~13-~13: Check that the noun ‘vendor’ after the pronoun ‘we’ is correct. It’s possible that you may need to switch to a possessive pronoun, or use another part of speech.
Context: ...a npm during production. To solve this, we vendor them during the build process. Here's t...
(PRP_VB)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
api/docs/developer/api-plugins.md (8)
15-15: Section header is clear.The “Configure the build system” heading accurately introduces step 1.
20-25: Vendoring configuration example.The snippet clearly demonstrates how to add workspace packages to the build vendoring configuration.
31-36: Vite configuration for workspace dependencies.This mapping correctly covers exclusion from optimization and externalization for SSR.
51-53: Peer dependency declaration snippet.The use of
"workspace:*"specifiers and optional peer dependencies is correctly illustrated.
56-61: peerDependenciesMeta snippet is accurate.Marking the workspace dependencies as optional ensures smooth development installs.
74-88: Plugin package entry configuration.The JSON snippet effectively outlines
main,types,exports, andfilesfields.
91-99: Plugin peerDependencies snippet.The recommended peer dependencies align with best practices to avoid duplication.
102-107: Plugin build script instructions.The
buildandpreparescripts correctly set up TypeScript compilation on install.packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)
274-391: Well-structured settings UI configurationThe JSON Forms configuration is well-implemented with:
- Clear preconditions for enabling remote access
- Proper conditional field visibility rules
- Appropriate port validation constraints
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts
Show resolved
Hide resolved
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts
Show resolved
Hide resolved
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Removals
Tests
Style
Chores