[WIP] extract mothership to connect plugin #1367
Conversation
to create a new convention of storing and loading environment-related config from that file.
WalkthroughThis update refactors Unraid's API and Connect plugin, modularizing connect, mothership, and remote access logic into new NestJS modules and services. It removes legacy Redux store code, replaces direct state management with ConfigService, and centralizes shared types and utilities in a new Changes
Changes Table (Abbreviated)
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant ConnectPlugin
participant ConfigService
participant MothershipModule
participant RemoteAccessModule
participant Shared
Client->>API: GraphQL Query/Mutation
API->>ConnectPlugin: Delegate to Connect Resolver/Service
ConnectPlugin->>ConfigService: Read/Update Config
ConnectPlugin->>MothershipModule: Setup/Manage Mothership Connection
ConnectPlugin->>RemoteAccessModule: Enable/Disable Remote Access
ConnectPlugin->>Shared: Use shared types/utilities
MothershipModule->>ConfigService: Update connection metadata
RemoteAccessModule->>ConfigService: Update remote access state
API-->>Client: Return GraphQL Response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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.
| export const loadLegacyStore = () => { | ||
| return { | ||
| store: store.getState(), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Legacy store loading is clear but consider serialization implications.
The function correctly provides the initial store state but doesn't handle synchronization (which appears to be managed by StoreSyncService per the summary). Consider adding validation to ensure the store state contains only serializable values.
🏁 Script executed:
#!/bin/bash
# Check if the store uses any middleware for serialization checks
rg "serializableCheck" --type tsLength of output: 93
Enforce serialization checks when loading the legacy store
The application currently disables Redux’s serializable-value middleware (serializableCheck: false in api/src/store/index.ts), which means non‑serializable data could slip into your config loader. To guard against this, add a runtime validation step (or re‑enable the middleware) before returning the state:
• File: api/src/unraid-api/config/config.loader.ts
export const loadLegacyStore = () => {
const state = store.getState();
// runtime serialization guard
try {
JSON.stringify(state);
} catch {
throw new Error(
'loadLegacyStore failed: legacy store contains non-serializable values'
);
}
return { store: state };
};• Alternatively, re-enable serializableCheck in api/src/store/index.ts or validate against a zod/schema before shipping the state.
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts
Outdated
Show resolved
Hide resolved
| 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.
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts
Outdated
Show resolved
Hide resolved
| 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
Outdated
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
Outdated
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); |
| // Remote Access Configurationx | ||
| @Field(() => Boolean) | ||
| @IsBoolean() | ||
| wanaccess!: boolean; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify type change compatibility
The wanaccess field changed from string to boolean. This is a more appropriate type but verify this change won't break existing code that expects a string.
🏁 Script executed:
#!/bin/bash
# Find all usages of wanaccess in the codebase
rg "wanaccess" --type tsLength of output: 2689
Type change for wanaccess requires updates across the codebase
We switched wanaccess from a string to a boolean in packages/unraid-api-plugin-connect/src/config.entity.ts. However, many modules still treat it as a string ('yes'/'no'/''), which will break at runtime. Please update the following locations to use a boolean instead of string:
- packages/unraid-api-plugin-connect/src/helpers/my-servers-config.ts:
wanaccess: string - api/src/types/my-servers-config.ts: Zod schema (
wanaccess: z.string()) - Store modules & actions:
- api/src/store/modules/config.ts: initial state (
wanaccess: '') and assignments ('yes'/'no') - api/src/store/actions/setup-remote-access.ts: setting
'yes'/'no'
- api/src/store/modules/config.ts: initial state (
- Store listeners:
- api/src/store/listeners/upnp-listener.ts:
wanaccess === 'yes' - api/src/store/listeners/wan-access-change-listener.ts: comparisons against
''
- api/src/store/listeners/upnp-listener.ts:
- GraphQL resolver:
- api/src/unraid-api/graph/resolvers/connect/connect-settings.service.ts:
getters.config().remote.wanaccess === 'yes'
- api/src/unraid-api/graph/resolvers/connect/connect-settings.service.ts:
- Core types & utils:
- api/src/core/types/ini.ts:
wanaccess: string - Tests:
- api/src/test/store/modules/config.test.ts (
expect.any(String),toBe('yes')) - api/src/test/core/utils/files/config-file-normalizer.test.ts (empty/string values)
- api/src/test/store/modules/config.test.ts (
- api/src/core/types/ini.ts:
Ensure schemas, comparisons, defaults, and test assertions all use boolean (or map string values at the boundary) to align with this change.
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.
| get renewalJob(): ReturnType<typeof this.scheduleRegistry.getCronJob> { | ||
| return this.scheduleRegistry.getCronJob(UPNP_RENEWAL_JOB_TOKEN); | ||
| } |
There was a problem hiding this comment.
Getter may throw when the job hasn’t been registered yet
SchedulerRegistry#getCronJob throws if the job name is unknown.
Accessing renewalJob before the first @Cron registration (e.g. during application bootstrap or in tests) will blow up the service.
Consider:
-getCronJob(UPNP_RENEWAL_JOB_TOKEN);
+this.scheduleRegistry.doesExist('cron', UPNP_RENEWAL_JOB_TOKEN)
+ ? this.scheduleRegistry.getCronJob(UPNP_RENEWAL_JOB_TOKEN)
+ : undefined;Committable suggestion skipped: line range outside the PR's diff.
| const wanPortToUse = await this.getWanPortToUse(args); | ||
| this.#wanPort = wanPortToUse; | ||
| const localPortToUse = sslPort ?? this.#localPort; |
There was a problem hiding this comment.
State is mutated before the mapping is confirmed – possible half-enabled state
this.#wanPort is set before createUpnpMapping() succeeds.
If createUpnpMapping throws, the service ends up disabled (#enabled is still false) but with a non-undefined #wanPort, making later logic confusing.
Suggestion:
-const wanPortToUse = await this.getWanPortToUse(args);
-this.#wanPort = wanPortToUse;
+const wanPortToUse = await this.getWanPortToUse(args);and only assign inside createUpnpMapping on success (it already does so).
Optionally wrap the call in try/catch and roll back #localPort as well.
📝 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 wanPortToUse = await this.getWanPortToUse(args); | |
| this.#wanPort = wanPortToUse; | |
| const localPortToUse = sslPort ?? this.#localPort; | |
| const wanPortToUse = await this.getWanPortToUse(args); | |
| const localPortToUse = sslPort ?? this.#localPort; |
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.
| async stop() { | ||
| await this.upnpService.disableUpnp(); | ||
| await this.networkService.reloadNetworkStack(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the stop() method
The method calls two async operations but doesn't handle potential errors.
async stop() {
+ try {
await this.upnpService.disableUpnp();
await this.networkService.reloadNetworkStack();
+ return true;
+ } catch (error) {
+ this.logger.error('Failed to stop UPnP remote access: %O', error);
+ return false;
+ }
}| async begin() { | ||
| const sslPort = this.configService.get<string | undefined>('store.emhttp.var.portssl'); | ||
| try { | ||
| await this.upnpService.createOrRenewUpnpLease({ | ||
| sslPort: Number(sslPort), | ||
| }); | ||
| await this.networkService.reloadNetworkStack(); | ||
| } catch (error) { | ||
| this.logger.error( | ||
| 'Failed to begin UPNP Remote Access using port %s: %O', | ||
| String(sslPort), | ||
| error | ||
| ); | ||
| this.stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and input validation in the begin() method
The method retrieves and uses an SSL port without proper validation:
- There's no check if
sslPortis undefined before converting to Number - No validation that the port is in a valid range
- The call to
this.stop()is not awaited, potentially allowing unhandled promise rejections
async begin() {
const sslPort = this.configService.get<string | undefined>('store.emhttp.var.portssl');
+ if (!sslPort) {
+ this.logger.error('SSL port is not configured');
+ return;
+ }
+ const numericPort = Number(sslPort);
+ if (isNaN(numericPort) || numericPort <= 0 || numericPort > 65535) {
+ this.logger.error(`Invalid SSL port: ${sslPort}`);
+ return;
+ }
try {
await this.upnpService.createOrRenewUpnpLease({
- sslPort: Number(sslPort),
+ sslPort: numericPort,
});
await this.networkService.reloadNetworkStack();
} catch (error) {
this.logger.error(
'Failed to begin UPNP Remote Access using port %s: %O',
String(sslPort),
error
);
- this.stop();
+ await this.stop().catch(stopError => {
+ this.logger.error('Error while stopping UPnP after initial failure: %O', stopError);
+ });
}
}📝 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 begin() { | |
| const sslPort = this.configService.get<string | undefined>('store.emhttp.var.portssl'); | |
| try { | |
| await this.upnpService.createOrRenewUpnpLease({ | |
| sslPort: Number(sslPort), | |
| }); | |
| await this.networkService.reloadNetworkStack(); | |
| } catch (error) { | |
| this.logger.error( | |
| 'Failed to begin UPNP Remote Access using port %s: %O', | |
| String(sslPort), | |
| error | |
| ); | |
| this.stop(); | |
| } | |
| } | |
| async begin() { | |
| const sslPort = this.configService.get<string | undefined>('store.emhttp.var.portssl'); | |
| if (!sslPort) { | |
| this.logger.error('SSL port is not configured'); | |
| return; | |
| } | |
| const numericPort = Number(sslPort); | |
| if (isNaN(numericPort) || numericPort <= 0 || numericPort > 65535) { | |
| this.logger.error(`Invalid SSL port: ${sslPort}`); | |
| return; | |
| } | |
| try { | |
| await this.upnpService.createOrRenewUpnpLease({ | |
| sslPort: numericPort, | |
| }); | |
| await this.networkService.reloadNetworkStack(); | |
| } catch (error) { | |
| this.logger.error( | |
| 'Failed to begin UPNP Remote Access using port %s: %O', | |
| String(sslPort), | |
| error | |
| ); | |
| await this.stop().catch(stopError => { | |
| this.logger.error('Error while stopping UPnP after initial failure: %O', stopError); | |
| }); | |
| } | |
| } |
pujitm
force-pushed
the
refactor/mothership
branch
from
8888587 to
5a063ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts (1)
14-14: Address the architectural question.The comment raises a valid concern about whether this resolver should remain here or move to the connect plugin. This decision impacts the overall architecture and should be resolved.
Consider the following factors:
- Is owner information always needed regardless of connect plugin status?
- Does this create a dependency that could affect modularity?
- How does this align with the plugin architecture goals?
Please clarify the architectural decision or create an issue to track this design question.
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php (3)
182-184: Add validation for JSON parsing.The code reads and parses JSON but doesn't validate if the JSON is well-formed. If the file contains invalid JSON,
json_decode()will returnnull, and the?:operator will use an empty array, which is good. However, consider logging when JSON parsing fails for debugging purposes.-$connectConfig = file_exists($connectJsonPath) ? @json_decode(file_get_contents($connectJsonPath), true) : []; +$connectConfig = []; +if (file_exists($connectJsonPath)) { + $jsonContent = @file_get_contents($connectJsonPath); + $connectConfig = @json_decode($jsonContent, true); + if (json_last_error() !== JSON_ERROR_NONE) { + error_log("Failed to parse connect.json: " . json_last_error_msg()); + $connectConfig = []; + } +}
210-210: SSO enabled logic could be more explicit.The current logic
!empty($connectConfig['ssoSubIds'] ?? '')works but could be clearer about what constitutes "enabled" SSO.-$this->ssoEnabled = !empty($connectConfig['ssoSubIds'] ?? ''); +$this->ssoEnabled = !empty($connectConfig['ssoSubIds'] ?? null);This makes it clearer that we're checking for the existence of non-empty
ssoSubIdsrather than falling back to an empty string.
182-210: Consider backwards compatibility with existing INI config.The migration to JSON config appears to completely replace the INI-based config for remote connection settings. Consider adding a fallback mechanism that reads from the old INI config if the new JSON config doesn't exist or is empty.
This would ensure a smoother migration path:
$connectJsonPath = '/boot/config/plugins/dynamix.my.servers/configs/connect.json'; $connectConfig = file_exists($connectJsonPath) ? @json_decode(file_get_contents($connectJsonPath), true) : []; + +// Fallback to INI config if JSON config is not available +if (empty($connectConfig) && !empty($this->myServersFlashCfg)) { + $connectConfig = [ + 'apikey' => $this->myServersFlashCfg['remote']['apikey'] ?? '', + 'avatar' => $this->myServersFlashCfg['remote']['avatar'] ?? '', + 'email' => $this->myServersFlashCfg['remote']['email'] ?? '', + // Add other mappings as needed + ]; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/connect/connect-api-key.service.ts(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/unraid-api-plugin-connect/src/connect/connect-api-key.service.ts
🧰 Additional context used
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts
[failure] 11-11:
Delete ';⏎import·{·ConfigService·}·from·'@nestjs/config
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/graph/resolvers/owner/owner.resolver.ts (4)
3-3: Good migration to shared module.The import change from local path to
@unraid/sharedaligns well with the architectural refactoring to centralize shared types and utilities.
17-17: Correct NestJS dependency injection pattern.The ConfigService injection follows proper NestJS patterns and correctly replaces the previous store access approach.
12-12:⚠️ Potential issueFix import formatting issue.
Static analysis flagged a formatting issue with this import statement.
Apply this fix:
-import { Owner } from '@app/unraid-api/graph/resolvers/owner/owner.model.js'; -import { ConfigService } from '@nestjs/config'; +import { Owner } from '@app/unraid-api/graph/resolvers/owner/owner.model.js'; +import { ConfigService } from '@nestjs/config';⛔ 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.
25-38:✅ Verification successful
Verify configuration structure consistency.
The migration from store access to ConfigService looks correct, but ensure the
connect.configstructure matches expectations across the codebase.Run this script to verify the configuration structure is consistent:
🏁 Script executed:
#!/bin/bash # Description: Verify connect.config usage patterns across the codebase # Expected: Consistent usage of connect.config.username and connect.config.avatar echo "=== Searching for connect.config usage ===" rg -A 3 "connect\.config" --type ts echo "=== Searching for config.username usage ===" rg -A 2 "config\.username" --type ts echo "=== Searching for config.avatar usage ===" rg -A 2 "config\.avatar" --type tsLength of output: 14454
connect.config usage verified
Verified thatconnect.config.usernameandconnect.config.avatarare both set inconnect-settings.service.tsand consistently retrieved (including inowner.resolver.ts). No discrepancies found—this migration is good to merge.plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php (2)
206-207: Logic for hasRemoteApikey and registered properties looks correct.The separation of concerns is well implemented -
hasRemoteApikeychecks for the presence of an API key, whileregisteredrequires both an API key and the connect plugin to be installed.
189-190:✅ Verification successful
Data type change may break backwards compatibility.
The default values for
wanaccessandupnpEnabledchanged from string"no"to booleanfalse. This could break other parts of the system that expect string values when checking these configuration options.Also applies to: 195-196
🏁 Script executed:
#!/bin/bash # Search for code that might expect string values for these config options rg -A 3 -B 3 'wanaccess.*=.*"no"' rg -A 3 -B 3 'upnpEnabled.*=.*"no"' rg -A 3 -B 3 'wanaccess.*===.*"no"' rg -A 3 -B 3 'upnpEnabled.*===.*"no"'Length of output: 1118
🏁 Script executed:
#!/bin/bash # Search for usage of boolean defaults in PHP code to catch any string comparisons rg -A3 -B3 "\$connectConfig\['wanaccess'\]" -t php rg -A3 -B3 "\$connectConfig\['upnpEnabled'\]" -t phpLength of output: 117
🏁 Script executed:
#!/bin/bash # List all occurrences of these keys across the repo for context rg -n "wanaccess" -C5 rg -n "upnpEnabled" -C5Length of output: 69491
Defaults-to-boolean false is compatible—no breakage found
I searched through the PHP plugin and supporting JS/TypeScript code and found:
• In Connect.page, both
wanaccessandupnpEnabledare only ever checked via=== 'yes', so a boolean false (from the new default) behaves identically to the old"no".
• There are no other strict comparisons against"no"anywhere else in the PHP or JS/TS code.No changes are needed here.
pujitm
force-pushed
the
refactor/mothership
branch
from
5a063ef to
fa59b6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/core/log.ts(2 hunks)packages/unraid-api-plugin-connect/src/config.entity.ts(1 hunks)packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/connect/connect.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/unraid-api-plugin-connect/src/connect/connect.module.ts
- packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
api/src/core/log.ts (1)
api/src/environment.ts (1)
LOG_LEVEL(85-89)
packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts (2)
packages/unraid-api-plugin-connect/src/mothership/mothership.handler.ts (1)
Injectable(12-86)packages/unraid-api-plugin-connect/src/config.entity.ts (2)
ConfigType(207-210)emptyMyServersConfig(212-227)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test API
- GitHub Check: Build API
🔇 Additional comments (16)
api/src/core/log.ts (2)
4-4: LGTM! Proper import of LOG_LEVEL constant.The import correctly brings in the LOG_LEVEL constant from the environment configuration, which is used in the serializers section.
46-50: Good security enhancement with additional sensitive data redaction.The new redact paths properly handle the additional sensitive tokens introduced in the connect plugin refactor:
apikeyandlocalApiKeyfor API authenticationaccesstoken,idtoken,refreshtokenfor OAuth/SSO tokensThese align with the sensitive fields defined in the MyServersConfig class.
packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts (4)
1-11: Well-structured NestJS service with proper dependencies.The service follows good practices:
- Proper dependency injection with ConfigService
- Clear separation of concerns with event handling
- Immutable config key definition
13-15: Safe configuration retrieval with proper error handling.Using
getOrThrowensures the service fails fast if the required configuration is missing, which is appropriate for a critical service dependency.
23-30: Smart user reset logic that preserves non-identity data.The destructuring approach correctly separates infrastructure settings (
wanaccess,wanport,upnpEnabled,ssoSubIds) from user identity data, ensuring only user-specific fields are cleared on logout.
32-35: Proper event-driven logout handling.The async event listener correctly calls the resetUser method, maintaining consistency with the broader logout flow seen in MothershipHandler.
packages/unraid-api-plugin-connect/src/config.entity.ts (10)
1-17: Comprehensive import of validation and GraphQL decorators.The imports properly bring in all necessary validation, transformation, and GraphQL decorators for the robust type system being implemented.
20-32: Improved enum definitions with clearer state representation.The updated enums provide better granularity:
MinigraphStatusnow includesPRE_INITandERROR_RETRYINGfor more precise connection state trackingDynamicRemoteAccessTypesimplified to three clear options:STATIC,UPNP,DISABLED
34-112: Well-structured MyServersConfig class with comprehensive validation.The class improvements include:
- Proper GraphQL and validation decorators for all fields
- Type safety with boolean for
wanaccess(addressing past review concerns)- Nullable
wanportfor optional configuration- Strong validation for sensitive fields like email and SSO IDs
- Clear field grouping with comments
The SSO ID regex validation is particularly good for security.
114-145: Appropriate separation of connection metadata concerns.The
ConnectionMetadataclass properly encapsulates mothership connection state with:
- Clear status tracking with MinigraphStatus enum
- Optional error and timing fields for debugging
- Proper nullable types for optional metadata
147-168: Good URL access object modeling.The
AccessUrlObjectclass properly models network access with:
- Support for both IPv4 and IPv6
- URL type classification from shared package
- Optional name field for user-friendly identification
170-191: Comprehensive dynamic remote access state tracking.The
DynamicRemoteAccessStateclass provides good state management for remote access with proper typing and validation.
193-199: Clean factory function for default state.The helper function properly uses
plainToInstancefor type-safe object creation with appropriate default values.
201-210: Proper type composition for configuration hierarchy.The type definitions correctly compose the configuration structure, extending demo config and providing proper typing for the nested configuration object.
212-227: Comprehensive default configuration factory.The
emptyMyServersConfigfunction provides appropriate defaults for all fields, ensuring consistent initialization across the system.
229-236: Proper configuration registration with type safety.The config feature registration correctly initializes all components:
- Mothership with PRE_INIT status
- Dynamic remote access in disabled state
- Default MyServersConfig instance
This provides a clean foundation for the configuration system.
| serializers: { | ||
| context: (context) => { | ||
| // For DEBUG and ERROR levels, return full context | ||
| if (LOG_LEVEL === 'DEBUG' || LOG_LEVEL === 'ERROR') { | ||
| return context; | ||
| } | ||
| // For other levels, only return the context key if it exists | ||
| return context?.context || context; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Context serializer logic looks good but verify LOG_LEVEL constant usage.
The conditional serialization based on log level is a smart approach for performance and security. However, ensure the LOG_LEVEL constant usage is consistent.
🏁 Script executed:
#!/bin/bash
# Verify LOG_LEVEL usage consistency across the codebase
echo "Checking LOG_LEVEL usage in log.ts and environment.ts..."
rg "LOG_LEVEL" --type ts -A 2 -B 2Length of output: 5560
Unify LOG_LEVEL source in core/log.ts
The mapping to Pino levels currently reads directly from process.env.LOG_LEVEL while elsewhere you use the imported LOG_LEVEL constant. This inconsistency can lead to mismatches if defaults or casing logic change in environment.ts.
• api/src/core/log.ts (around the const level = … definition)
Suggested diff:
- const level =
- levels[
- levels.indexOf(process.env.LOG_LEVEL?.toLowerCase() as (typeof levels)[number])
- ] ?? 'info';
+ const level =
+ levels[
+ levels.indexOf(LOG_LEVEL.toLowerCase() as (typeof levels)[number])
+ ] ?? 'info';📝 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.
| serializers: { | |
| context: (context) => { | |
| // For DEBUG and ERROR levels, return full context | |
| if (LOG_LEVEL === 'DEBUG' || LOG_LEVEL === 'ERROR') { | |
| return context; | |
| } | |
| // For other levels, only return the context key if it exists | |
| return context?.context || context; | |
| }, | |
| }, | |
| const level = | |
| levels[ | |
| levels.indexOf( | |
| - process.env.LOG_LEVEL?.toLowerCase() as (typeof levels)[number] | |
| + LOG_LEVEL.toLowerCase() as (typeof levels)[number] | |
| ) | |
| ] ?? 'info'; |
🤖 Prompt for AI Agents
In api/src/core/log.ts around lines 54 to 63, the LOG_LEVEL constant is used
inconsistently by reading directly from process.env.LOG_LEVEL in some places
while using the imported LOG_LEVEL constant elsewhere. To fix this, unify the
LOG_LEVEL source by always using the imported LOG_LEVEL constant throughout the
file, including in the serializers and level mapping logic, to ensure consistent
casing and default handling as defined in environment.ts.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
125-130:⚠️ Potential issue
wsClient.terminate()is still invoked – API mismatch
terminate()exists on the raw WebSocket, not ongraphql-ws’sClient. Calling it throwsTypeError: client.terminate is not a function.- this.clearClientEventHandlers(); - this.wsClient.terminate(); - await this.wsClient.dispose(); + this.clearClientEventHandlers(); + await this.wsClient.dispose(); // disposes & closes internallypackages/unraid-api-plugin-connect/src/system/url-resolver.service.ts (2)
306-314: IPv6 URL is stored inipv4field – same bug as previously reported
The IPv6 URL resolved fromlanIp6is still assigned toipv4instead ofipv6.- ipv4: lanIp6Url, + ipv6: lanIp6Url,Same issue was flagged in the last review – please amend.
127-156: 🛠️ Refactor suggestionHandle raw IPv6 literals – wrap with square brackets before constructing the URL
new URL()rejects bare IPv6 literals such ashttps://2001:db8::1.
Detect IPv6 literals and wrap them in[]so the constructor receives an RFC-compliant host part.- const urlString = `${httpMode}${url}${portToUse}`; + const isIpv6Literal = url.includes(':') && !url.startsWith('['); + const hostPart = isIpv6Literal ? `[${url}]` : url; + const urlString = `${httpMode}${hostPart}${portToUse}`;This fixes failures when the nginx config contains an IPv6 address instead of a hostname.
🧹 Nitpick comments (3)
packages/unraid-api-plugin-connect/src/helpers/consts.ts (1)
1-6: Well-structured time constants with minor consistency improvement opportunity.The constants are mathematically correct and follow clear naming conventions. Good reuse pattern where
THREE_MINUTES_MSbuilds onONE_MINUTE_MS, andONE_DAY_SECS/FIVE_DAYS_SECSbuild on previous constants.Consider improving consistency by making
ONE_HOUR_SECSbuild onONE_MINUTE_SECS:-export const ONE_HOUR_SECS = 60 * 60; +export const ONE_HOUR_SECS = 60 * ONE_MINUTE_SECS;This maintains the established pattern of building constants from previously defined ones.
packages/unraid-api-plugin-connect/src/system/network.service.ts (1)
25-45: Normalise & de-duplicate origins more robustly
map((origin) => (origin.endsWith('/') ? origin.slice(0, -1) : origin))trims only the trailing slash and keeps case differences (HTTP://…vshttp://…) or leading/trailing spaces, causing silent duplicates. A stricter normaliser would:-].map((origin) => (origin.endsWith('/') ? origin.slice(0, -1) : origin)); +].map((origin) => + origin.trim().replace(/\/$/, '').toLowerCase(), +);That guarantees canonical form before the
Setis built.packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts (1)
18-27:getExtraOrigins()may emit duplicates & invalid URIsAfter
.split(',')you could:
map((o) => o.trim())to remove accidental whitespace around commas (newline/tab not just spaces).new Set()to guarantee uniqueness.- Use
URLconstructor for basic validation instead ofstartsWith.- return extraOrigins - .replaceAll(' ', '') - .split(',') - .filter((origin) => origin.startsWith('http://') || origin.startsWith('https://')); + return [...new Set( + extraOrigins + .split(',') + .map((o) => o.trim()) + .filter((o) => { + try { new URL(o); return true; } catch { return false; } + }), + )];
📜 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 (25)
api/src/__test__/store/modules/config.test.ts(1 hunks)api/src/graphql/resolvers/query/cloud/check-api.ts(0 hunks)api/src/graphql/resolvers/query/cloud/check-cloud.ts(0 hunks)api/src/graphql/resolvers/query/cloud/check-minigraphql.ts(0 hunks)api/src/store/getters/index.ts(0 hunks)api/src/store/index.ts(1 hunks)api/src/store/modules/cache.ts(0 hunks)api/src/store/root-reducer.ts(0 hunks)api/src/unraid-api/graph/resolvers/cloud/cloud.model.ts(0 hunks)api/src/unraid-api/graph/resolvers/cloud/cloud.resolver.spec.ts(0 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(0 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/connect/connect-api-key.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/helpers/consts.ts(1 hunks)packages/unraid-api-plugin-connect/src/helpers/my-servers-config.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/cloud.model.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership-subscription.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/mothership/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/resolver/cloud.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/service/cloud.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/network.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/system.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/system/url-resolver.service.ts(1 hunks)
💤 Files with no reviewable changes (9)
- api/src/unraid-api/graph/resolvers/cloud/cloud.resolver.spec.ts
- api/src/graphql/resolvers/query/cloud/check-api.ts
- api/src/store/root-reducer.ts
- api/src/graphql/resolvers/query/cloud/check-minigraphql.ts
- api/src/store/modules/cache.ts
- api/src/unraid-api/graph/resolvers/cloud/cloud.model.ts
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/store/getters/index.ts
- api/src/graphql/resolvers/query/cloud/check-cloud.ts
✅ Files skipped from review due to trivial changes (1)
- packages/unraid-api-plugin-connect/src/model/cloud.model.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/unraid-api-plugin-connect/src/mothership/mothership.module.ts
- packages/unraid-api-plugin-connect/src/system/system.module.ts
- api/src/store/index.ts
- api/src/test/store/modules/config.test.ts
- packages/unraid-api-plugin-connect/package.json
- packages/unraid-api-plugin-connect/src/connect/connect-api-key.service.ts
- packages/unraid-api-plugin-connect/src/mothership/mothership-subscription.handler.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/unraid-api-plugin-connect/src/system/url-resolver.service.ts (6)
packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts (1)
Injectable(8-57)packages/unraid-api-plugin-connect/src/system/network.service.ts (1)
Injectable(8-46)packages/unraid-api-plugin-connect/src/connect/connect-settings.service.ts (1)
Injectable(37-521)packages/unraid-api-plugin-connect/src/remote-access/static-remote-access.service.ts (1)
Injectable(8-35)packages/unraid-api-plugin-connect/src/remote-access/upnp-remote-access.service.ts (1)
Injectable(9-42)packages/unraid-api-plugin-connect/src/config.entity.ts (1)
ConfigType(207-210)
🔇 Additional comments (7)
packages/unraid-api-plugin-connect/src/helpers/my-servers-config.ts (5)
3-3: Good addition for GraphQL integration.The import aligns well with the NestJS GraphQL integration mentioned in the summary.
5-11: Excellent conversion to explicit string enums.Converting from implicit to explicit string literal enums improves type safety and makes enum values explicit, which is a best practice for maintainability and debugging.
13-17: Consistent enum pattern applied.The same explicit string literal pattern is properly applied to
DynamicRemoteAccessType, maintaining consistency across the codebase.
19-22: Proper GraphQL enum registration.The
registerEnumTypecall is correctly implemented with appropriate metadata for GraphQL schema generation.
25-47: Clean formatting improvements.The type definitions maintain their structure while achieving consistent indentation and string literal style. This improves code readability without affecting functionality.
Also applies to: 52-55
packages/unraid-api-plugin-connect/src/connect/connect-config.service.ts (1)
43-51:⚠️ Potential issue
resetUser()silently overwrites non-identity fieldsThe spread order means every property in
identity(defaults) overrides the current config, potentially reverting unrelated settings the user changed after login. Invert the merge order:- this.configService.set(this.configKey, { - ...this.getConfig(), - ...identity, - }); + this.configService.set(this.configKey, { + ...identity, + ...this.getConfig(), // keep latest user settings + });(or update individual keys).
Likely an incorrect or invalid review comment.
packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts (1)
215-221:⚠️ Potential issue
ConfigServicehas no.set()– compilation will fail
@nestjs/config’sConfigServiceexposes read-only getters. The call below will error at compile time:this.configService.set('connect.mothership.timeout', getDelay);Persist the timeout another way (e.g., dedicated state service or in-memory variable).
⛔ 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.
| async reloadNetworkStack() { | ||
| await this.nginxService.reload(); | ||
| await this.dnsService.update(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Shield reloadNetworkStack() with error handling or run services in parallel
Both reloads are awaited sequentially; if nginxService.reload() rejects, dnsService.update() never runs and the error bubbles out unhandled.
Either guard each await in a try / catch and log, or fire them concurrently:
- async reloadNetworkStack() {
- await this.nginxService.reload();
- await this.dnsService.update();
- }
+ async reloadNetworkStack() {
+ await Promise.allSettled([
+ this.nginxService.reload(),
+ this.dnsService.update(),
+ ]);
+ }This prevents partial-state updates and shortens total latency.
📝 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 reloadNetworkStack() { | |
| await this.nginxService.reload(); | |
| await this.dnsService.update(); | |
| } | |
| async reloadNetworkStack() { | |
| await Promise.allSettled([ | |
| this.nginxService.reload(), | |
| this.dnsService.update(), | |
| ]); | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/system/network.service.ts around lines
17 to 20, the reloadNetworkStack method awaits nginxService.reload() and
dnsService.update() sequentially without error handling, causing
dnsService.update() to be skipped if the first fails. Modify the method to run
both service calls concurrently using Promise.all and add try/catch to handle
and log any errors, ensuring both services attempt to reload regardless of
individual failures and reducing total execution time.
| apiKey: { valid: true }, | ||
| minigraphql, | ||
| cloud, | ||
| allowedOrigins: getAllowedOrigins(), | ||
| allowedOrigins: this.networkService.getAllowedOrigins(), | ||
| error: | ||
| `${apiKey.error ? `API KEY: ${apiKey.error}` : ''}${ | ||
| `${ | ||
| cloud.error ? `NETWORK: ${cloud.error}` : '' | ||
| }${minigraphql.error ? `CLOUD: ${minigraphql.error}` : ''}` || undefined, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
apiKey hard-codes “valid: true” & error message concatenation is brittle
-
Returning
{ valid: true }regardless of the actual key state may mislead consumers. Consider delegating this tocloudServiceso the resolver reflects real validity. -
Error string construction lacks delimiters and may yield
"NETWORK: …CLOUD: …"without spacing. Simpler:
- error:
- `${ cloud.error ? `NETWORK: ${cloud.error}` : '' }${
- minigraphql.error ? `CLOUD: ${minigraphql.error}` : ''}` || undefined,
+ error: [cloud.error && `NETWORK: ${cloud.error}`,
+ minigraphql.error && `CLOUD: ${minigraphql.error}`]
+ .filter(Boolean)
+ .join(' | ') || undefined,This avoids empty substrings and is easier to read.
📝 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.
| apiKey: { valid: true }, | |
| minigraphql, | |
| cloud, | |
| allowedOrigins: getAllowedOrigins(), | |
| allowedOrigins: this.networkService.getAllowedOrigins(), | |
| error: | |
| `${apiKey.error ? `API KEY: ${apiKey.error}` : ''}${ | |
| `${ | |
| cloud.error ? `NETWORK: ${cloud.error}` : '' | |
| }${minigraphql.error ? `CLOUD: ${minigraphql.error}` : ''}` || undefined, | |
| }; | |
| apiKey: { valid: true }, | |
| minigraphql, | |
| cloud, | |
| allowedOrigins: this.networkService.getAllowedOrigins(), | |
| error: [ | |
| cloud.error && `NETWORK: ${cloud.error}`, | |
| minigraphql.error && `CLOUD: ${minigraphql.error}`, | |
| ] | |
| .filter(Boolean) | |
| .join(' | ') || undefined, | |
| }; |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/resolver/cloud.resolver.ts around
lines 37 to 45, the apiKey field is hardcoded to { valid: true } which does not
reflect the actual validity state and may mislead consumers; update this to
retrieve the real validity status from cloudService instead. Additionally, the
error message concatenation combines network and cloud errors without
delimiters, causing readability issues; refactor the error construction to join
only existing error messages with a clear separator like a space or comma to
avoid empty substrings and improve clarity.
| max: Infinity, | ||
| retryIf: (error, operation) => { | ||
| const { retryCount = 0 } = operation.getContext(); | ||
| // i.e. retry api key errors up to 3 times (4 attempts total) | ||
| return !this.isInvalidApiKeyError(error) || retryCount < MAX_AUTH_RETRIES; | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Infinite retry loop may hang the service
attempts.max = Infinity combined with exponential back-off means an unreachable Mothership keeps the process alive forever. Consider a sane upper bound or expose it via config.
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/mothership/graphql.client.ts around
lines 224 to 231, the retry configuration sets max attempts to Infinity, which
can cause an infinite retry loop and hang the service if the Mothership is
unreachable. Fix this by replacing Infinity with a reasonable upper limit for
max retries, such as a configurable constant or a fixed number, to prevent
endless retries and allow the process to fail gracefully after a set number of
attempts.
| const mothershipGqlUri = this.configService.getOrThrow<string>('MOTHERSHIP_GRAPHQL_LINK'); | ||
| const hostname = new URL(mothershipGqlUri).host; | ||
| const lookup = promisify(lookupDNS); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use .hostname instead of .host to strip port before DNS look-up
new URL(mothershipGqlUri).host includes the port if present (e.g. api.example.com:443), causing DNS resolution to fail.
Swap to .hostname so only the domain part is sent to dns.lookup / dns.resolve.
- const hostname = new URL(mothershipGqlUri).host;
+ const hostname = new URL(mothershipGqlUri).hostname;📝 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 mothershipGqlUri = this.configService.getOrThrow<string>('MOTHERSHIP_GRAPHQL_LINK'); | |
| const hostname = new URL(mothershipGqlUri).host; | |
| const lookup = promisify(lookupDNS); | |
| // File: packages/unraid-api-plugin-connect/src/service/cloud.service.ts | |
| const mothershipGqlUri = this.configService.getOrThrow<string>('MOTHERSHIP_GRAPHQL_LINK'); | |
| - const hostname = new URL(mothershipGqlUri).host; | |
| + const hostname = new URL(mothershipGqlUri).hostname; | |
| const lookup = promisify(lookupDNS); |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/cloud.service.ts around lines
205 to 207, replace the use of `.host` with `.hostname` on the URL object to
ensure the port is excluded from the DNS lookup. This means changing `new
URL(mothershipGqlUri).host` to `new URL(mothershipGqlUri).hostname` so that only
the domain name is passed to the DNS resolution function, preventing lookup
failures caused by including the port.
| if (!local.includes(network)) | ||
| throw new Error( | ||
| `Local and network resolvers showing different IP for "${hostname}". [local="${ | ||
| local ?? 'NOT FOUND' | ||
| }"] [network="${network ?? 'NOT FOUND'}"]` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Equality check should be strict, not substring
!local.includes(network) gives false positives (e.g. "1.2.3.4" includes "2.3").
Use strict comparison to ensure both resolvers agree on the exact IP.
- if (!local.includes(network))
+ if (local !== network)📝 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 (!local.includes(network)) | |
| throw new Error( | |
| `Local and network resolvers showing different IP for "${hostname}". [local="${ | |
| local ?? 'NOT FOUND' | |
| }"] [network="${network ?? 'NOT FOUND'}"]` | |
| ); | |
| if (local !== network) | |
| throw new Error( | |
| `Local and network resolvers showing different IP for "${hostname}". [local="${ | |
| local ?? 'NOT FOUND' | |
| }"] [network="${network ?? 'NOT FOUND'}"]` | |
| ); |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/cloud.service.ts around lines
214 to 219, replace the substring check using local.includes(network) with a
strict equality check local !== network to ensure both resolvers return exactly
the same IP address. This prevents false positives where one IP string might be
a substring of the other.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/service/cloud.service.test.ts (1)
10-31: Consider improving mock isolation and test categorization.The mock setup is functional but has some areas for improvement:
- The test is labeled as "integration" but uses mocked dependencies, making it more of a unit test
- Consider using proper mocking libraries like
vi.mock()for better type safety and isolationHere's a suggested improvement for better mock isolation:
describe('CloudService.hardCheckCloud (unit)', () => { let service: CloudService; - let configService: any; - let mothership: any; - let connectConfig: any; + let configService: jest.Mocked<ConfigService>; + let mothership: jest.Mocked<MothershipService>; + let connectConfig: jest.Mocked<ConnectConfigService>; beforeEach(() => { - configService = { - getOrThrow: (key: string) => { - if (key === 'MOTHERSHIP_GRAPHQL_LINK') return MOTHERSHIP_GRAPHQL_LINK; - if (key === 'API_VERSION') return API_VERSION; - throw new Error('Unknown key'); - }, - }; + configService = { + getOrThrow: vi.fn().mockImplementation((key: string) => { + if (key === 'MOTHERSHIP_GRAPHQL_LINK') return MOTHERSHIP_GRAPHQL_LINK; + if (key === 'API_VERSION') return API_VERSION; + throw new Error('Unknown key'); + }), + } as any;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/__test__/graphql/resolvers/query/cloud/check-mothership-authentication.test.ts(0 hunks)api/src/graphql/resolvers/query/cloud/check-dns.ts(0 hunks)api/src/graphql/resolvers/query/cloud/check-mothership-authentication.ts(0 hunks)api/src/graphql/resolvers/query/cloud/create-response.ts(0 hunks)packages/unraid-api-plugin-connect/src/service/cloud.service.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- api/src/graphql/resolvers/query/cloud/check-dns.ts
- api/src/test/graphql/resolvers/query/cloud/check-mothership-authentication.test.ts
- api/src/graphql/resolvers/query/cloud/create-response.ts
- api/src/graphql/resolvers/query/cloud/check-mothership-authentication.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/unraid-api-plugin-connect/src/service/cloud.service.test.ts (1)
api/src/environment.ts (2)
MOTHERSHIP_GRAPHQL_LINK(90-94)API_VERSION(65-65)
⏰ 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: Cloudflare Pages
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/service/cloud.service.test.ts (1)
1-8: LGTM! Clean imports and well-defined test constants.The imports are appropriate for Vitest testing, and the test constants provide clear separation between test data and actual values.
| it('fails to authenticate with mothership with no credentials', async () => { | ||
| try { | ||
| await expect( | ||
| service['hardCheckCloud'](API_VERSION, BAD) | ||
| ).rejects.toThrowErrorMatchingInlineSnapshot( | ||
| `[Error: Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.]` | ||
| ); | ||
| await expect( | ||
| service['hardCheckCloud'](API_VERSION, BAD_API_KEY) | ||
| ).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Invalid credentials]`); | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.includes('Timeout')) { | ||
| // Test succeeds on timeout | ||
| return; | ||
| } | ||
| throw error; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address testing of private methods and improve error handling.
This test has several concerns that should be addressed:
- Testing private methods: Accessing
service['hardCheckCloud']tests implementation details rather than public behavior - Timeout handling: The catch block that allows tests to pass on timeout is a workaround that masks real issues
- Limited coverage: Only tests failure scenarios
Consider these improvements:
- it('fails to authenticate with mothership with no credentials', async () => {
- try {
- await expect(
- service['hardCheckCloud'](API_VERSION, BAD)
- ).rejects.toThrowErrorMatchingInlineSnapshot(
- `[Error: Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.]`
- );
- await expect(
- service['hardCheckCloud'](API_VERSION, BAD_API_KEY)
- ).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Invalid credentials]`);
- } catch (error) {
- if (error instanceof Error && error.message.includes('Timeout')) {
- // Test succeeds on timeout
- return;
- }
- throw error;
- }
- });
+ it('fails to authenticate with mothership with bad credentials', async () => {
+ await expect(
+ service['hardCheckCloud'](API_VERSION, BAD)
+ ).rejects.toThrow('Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.');
+ });
+
+ it('fails to authenticate with mothership with invalid API key', async () => {
+ await expect(
+ service['hardCheckCloud'](API_VERSION, BAD_API_KEY)
+ ).rejects.toThrow('Invalid credentials');
+ });Alternative approach: Test the public interface instead of private methods by testing methods that call hardCheckCloud internally.
📝 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.
| it('fails to authenticate with mothership with no credentials', async () => { | |
| try { | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD) | |
| ).rejects.toThrowErrorMatchingInlineSnapshot( | |
| `[Error: Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.]` | |
| ); | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD_API_KEY) | |
| ).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Invalid credentials]`); | |
| } catch (error) { | |
| if (error instanceof Error && error.message.includes('Timeout')) { | |
| // Test succeeds on timeout | |
| return; | |
| } | |
| throw error; | |
| } | |
| }); | |
| it('fails to authenticate with mothership with bad credentials', async () => { | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD) | |
| ).rejects.toThrow('Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.'); | |
| }); | |
| it('fails to authenticate with mothership with invalid API key', async () => { | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD_API_KEY) | |
| ).rejects.toThrow('Invalid credentials'); | |
| }); |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/cloud.service.test.ts around
lines 33 to 50, avoid directly testing the private method 'hardCheckCloud' by
instead writing tests against the public methods that internally invoke it.
Remove the catch block that swallows timeout errors to ensure real issues are
not masked. Expand test coverage to include successful authentication scenarios
and other relevant cases to improve overall test robustness.
78d97b7 to
de68fc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/service/cloud.service.ts (2)
203-203: Use.hostnameinstead of.hostto strip port before DNS look-up
new URL(mothershipGqlUri).hostincludes the port if present (e.g.api.example.com:443), causing DNS resolution to fail. Swap to.hostnameso only the domain part is sent todns.lookup/dns.resolve.- const hostname = new URL(mothershipGqlUri).host; + const hostname = new URL(mothershipGqlUri).hostname;
211-211: Equality check should be strict, not substring
!local.includes(network)gives false positives (e.g."1.2.3.4"includes"2.3"). Use strict comparison to ensure both resolvers agree on the exact IP.- if (!local.includes(network)) { + if (local !== network) {
🧹 Nitpick comments (3)
packages/unraid-api-plugin-connect/src/service/cloud.service.ts (3)
43-43: Consider dependency injection for cache to improve testability.The static cache makes unit testing difficult since it's shared across all instances and can't be easily mocked.
Consider injecting the cache as a dependency:
@Injectable() export class CloudService { - static cache = new NodeCache() as TypedCache<CacheSchema>; + private readonly cache: TypedCache<CacheSchema>; private readonly logger = new Logger(CloudService.name); constructor( private readonly configService: ConfigService<ConfigType>, private readonly mothership: MothershipConnectionService, - private readonly connectConfig: ConnectConfigService + private readonly connectConfig: ConnectConfigService, + cache?: TypedCache<CacheSchema> ) { + this.cache = cache ?? new NodeCache() as TypedCache<CacheSchema>; }
75-86: Cache reference inconsistency.The method uses
CloudService.cachebut should usethis.cacheif the cache becomes an instance property (related to the testability suggestion above).If implementing the cache injection suggestion, update cache references:
- const cachedCloudCheck = CloudService.cache.get('cloudCheck'); + const cachedCloudCheck = this.cache.get('cloudCheck');And similarly for line 85:
- CloudService.cache.set('cloudCheck', cloudCheck, ttl); + this.cache.set('cloudCheck', cloudCheck, ttl);
188-191: Improve type safety and logic for IP validation.The type check
typeof validIp !== 'string'is redundant sincevalidIpis already typed as string from the nullish coalescing with empty string.Simplify the logic:
- const validIp = local ?? network ?? ''; - if (typeof validIp !== 'string') { - return ''; - } + const validIp = local || network || '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/service/cloud.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 33
♻️ Duplicate comments (4)
packages/unraid-api-plugin-connect/package.json (2)
17-21: Update metadata for Connect plugin
Thedescriptionstill refers to “Example Health plugin” andkeywordsis empty. Update these fields to accurately describe the Connect plugin.- "keywords": [], - "description": "Example Health plugin for Unraid API", + "keywords": ["unraid", "plugin", "connect", "graphql"], + "description": "Unraid Connect Plugin for Mothership GraphQL integration",
21-61: Relocate runtime libraries to dependencies
Several packages (@apollo/client,class-transformer,graphql,rxjs, etc.) are imported at runtime but currently live indevDependencies. Move them intodependenciesto ensure they’re installed in production.--- package.json @@ "devDependencies": { - "@apollo/client": "^3.11.8", - "class-transformer": "^0.5.1", - "graphql": "^16.9.0", - "graphql-scalars": "^1.23.0", - "graphql-subscriptions": "^3.0.0", - "graphql-ws": "^6.0.0", - "rxjs": "^7.8.2", @@ "dependencies": { - "@unraid/shared": "workspace:*", - "ip": "^2.0.1", - "node-cache": "^5.1.2" + "@unraid/shared": "workspace:*", + "@apollo/client": "^3.11.8", + "class-transformer": "^0.5.1", + "graphql": "^16.9.0", + "graphql-scalars": "^1.23.0", + "graphql-subscriptions": "^3.0.0", + "graphql-ws": "^6.0.0", + "ip": "^2.0.1", + "node-cache": "^5.1.2", + "rxjs": "^7.8.2"packages/unraid-api-plugin-connect/src/service/cloud.service.ts (2)
202-204:⚠️ Potential issueUse
.hostnameinstead of.hostto strip port before DNS look-up
new URL(mothershipGqlUri).hostincludes the port if present (e.g.api.example.com:443), causing DNS resolution to fail.
Swap to.hostnameso only the domain part is sent todns.lookup/dns.resolve.- const hostname = new URL(mothershipGqlUri).host; + const hostname = new URL(mothershipGqlUri).hostname;
211-211:⚠️ Potential issueEquality check should be strict, not substring
!local.includes(network)gives false positives (e.g."1.2.3.4"includes"2.3").
Use strict comparison to ensure both resolvers agree on the exact IP.- if (!local.includes(network)) { + if (local !== network) {
🧹 Nitpick comments (39)
packages/unraid-api-plugin-connect/package.json (3)
11-11: Implement a real test script
Thetestscript is currently a stub. Sincevitestis already added indevDependencies, consider replacing it with:"test": "vitest"
6-9: Ensure correct casing of README
You included"readme.md"infiles, but the standard file is namedREADME.md. Confirm the actual filename or adjust for case sensitivity to avoid missing files.
67-95: Review scope of peerDependencies
ThepeerDependenciesmirror mostdevDependencies, which may force consumers to install many packages they don't need. Consider pruning to only the runtime packages that consumers must provide, or mark optional where appropriate.packages/unraid-api-plugin-connect/justfile (2)
27-27: Add fallback for bc dependency.The script assumes
bcis available for percentage calculations, but this may not be installed on all systems.- percentage=$(echo "scale=1; $lines * 100 / $total" | bc) + if command -v bc >/dev/null 2>&1; then + percentage=$(echo "scale=1; $lines * 100 / $total" | bc) + else + percentage=$(awk "BEGIN {printf \"%.1f\", $lines * 100 / $total}") + fi
33-33: Fix comment inconsistency.The comment says "Top 10 largest files" but the command uses
head -n 11. While this is technically correct (becausewc -ladds a total line), the comment should be clarified.- echo -e "${GREEN}Top 10 largest files:${NC}" - find . -type f -name "*.ts" -not -path "*/test/*" -not -path "*/graphql/generated/*" | xargs wc -l | sort -nr | head -n 11 + echo -e "${GREEN}Top 10 largest files:${NC}" + find . -type f -name "*.ts" -not -path "*/test/*" -not -path "*/graphql/generated/*" | xargs wc -l | sort -nr | head -n 10 | grep -v "total$"packages/unraid-api-plugin-connect/src/test/cloud.service.test.ts (1)
5-8: Extract test constants to improve maintainability.Hard-coded test values should be extracted as named constants for better readability and maintenance.
+const TEST_MOTHERSHIP_URL = 'https://mothership.unraid.net/ws'; +const TEST_API_VERSION = 'TEST_VERSION'; +const INVALID_API_KEY = 'BAD_API_KEY'; +const INVALID_CREDENTIALS = 'BAD'; + -const MOTHERSHIP_GRAPHQL_LINK = 'https://mothership.unraid.net/ws'; -const API_VERSION = 'TEST_VERSION'; -const BAD_API_KEY = 'BAD_API_KEY'; -const BAD = 'BAD';packages/unraid-api-plugin-connect/src/service/dns.service.ts (1)
11-11: Consider making file paths configurable.Hard-coded system paths reduce portability and testability. Consider extracting these as configuration constants.
+const PHP_BINARY_PATH = '/usr/bin/php'; +const UPDATE_DNS_SCRIPT_PATH = '/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php'; + - await execa('/usr/bin/php', ['/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php']); + await execa(PHP_BINARY_PATH, [UPDATE_DNS_SCRIPT_PATH]);packages/unraid-api-plugin-connect/src/service/nginx.service.ts (1)
10-10: Consider making Nginx control script path configurable.Hard-coded system paths should be extracted as configuration for better maintainability and testing.
+const NGINX_CONTROL_SCRIPT = '/etc/rc.d/rc.nginx'; + - await execa('/etc/rc.d/rc.nginx', ['reload']); + await execa(NGINX_CONTROL_SCRIPT, ['reload']);packages/unraid-api-plugin-connect/src/helper/delay-function.ts (1)
4-4: Consider reducing the default initial delay.The default initial delay of 10 seconds is quite aggressive for most use cases and may impact user experience.
- const { initial = 10_000, jitter = true, max = Infinity } = delayOptions ?? {}; + const { initial = 1_000, jitter = true, max = 60_000 } = delayOptions ?? {};packages/unraid-api-plugin-connect/src/readme.md (1)
20-27: Specify language for the code block.The code block should specify TypeScript for proper syntax highlighting and linting.
-``` +```typescript import { ApiModule } from '@unraid-api-plugin-connect/src'; @Module({ imports: [ApiModule], }) export class AppModule {} -``` +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
packages/unraid-api-plugin-connect/src/resolver/health.resolver.ts (1)
10-14: Health check returns demo data instead of actual health status.Consider implementing actual health check logic rather than returning a demo constant.
@Query(() => String) health() { - // You can replace the return value with your actual health check logic - return demoShared; + // Implement actual health check logic + return 'healthy'; }packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts (1)
30-30: Consider adding error handling for missing configuration.Using
getOrThrowwill throw an exception if the 'connect' configuration is missing. While this might be intentional, consider whether a GraphQL resolver should handle this more gracefully.Consider wrapping the configuration access in a try-catch block to provide a more user-friendly error:
@ResolveField(() => DynamicRemoteAccessStatus) public dynamicRemoteAccess(): DynamicRemoteAccessStatus { - const state = this.configService.getOrThrow<ConnectConfig>('connect'); + try { + const state = this.configService.getOrThrow<ConnectConfig>('connect'); + } catch (error) { + this.logger.error('Connect configuration not found', error); + throw new Error('Connect service is not properly configured'); + } return {packages/unraid-api-plugin-connect/src/event-handler/connect-login.handler.ts (2)
32-32: Consider making theurlfield configurable.The
urlfield is hardcoded to an empty string. This might need to be populated with actual data.Would you like me to investigate what the expected value for this
urlfield should be and help implement the proper logic to populate it?
17-17: Remove unnecessaryasyncmodifier.The event handler is marked as
async: truebut the method doesn't need to be async since it only awaits a single operation at the end.- @OnEvent(EVENTS.LOGIN, { async: true }) + @OnEvent(EVENTS.LOGIN)packages/unraid-api-plugin-connect/src/service/internal.client.ts (1)
64-72: Instantiate the WebSocket subclass only once
Every call togetWebsocketWithMothershipHeaders()creates a new class definition, which prevents WS-level connection pooling and slightly inflates memory. Cache the constructor instead.- private getWebsocketWithMothershipHeaders() { + private websocketCtor?: typeof WebSocket; + + private getWebsocketWithMothershipHeaders() { + if (this.websocketCtor) return this.websocketCtor; const getHeaders = () => this.connectionService.getMothershipWebsocketHeaders(); - return class WebsocketWithMothershipHeaders extends WebSocket { + this.websocketCtor = class WebsocketWithMothershipHeaders extends WebSocket { constructor(address: string | URL, protocols?: string | string[]) { super(address, protocols, { headers: getHeaders(), }); } - }; + }; + return this.websocketCtor; }packages/unraid-api-plugin-connect/src/service/network.service.ts (1)
40-44: Minor: micro-optimise de-duplication
new Set(sink)already drops duplicates; the preceding.map()can run after theSetto avoid doing the trailing-slash check twice for duplicates. Micro-level, but essentially free.packages/unraid-api-plugin-connect/src/job/timeout-checker.job.ts (1)
24-29: Timeout logic ignores the “never pinged” case
If a client connects but no ping is ever received,lastPingisundefined, and the timeout is silently skipped. Consider treatingundefinedas timed-out after the same interval or whenCONNECTEDpersists for > threshold without a ping.packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts (1)
24-33: Extract the config key into a constant
'connect.config.wanaccess'is repeated here and instopRemoteAccess(). Defining it once (e.g.const WAN_ACCESS_KEY = 'connect.config.wanaccess';) avoids typos and eases future refactors.packages/unraid-api-plugin-connect/src/service/upnp.service.ts (1)
136-144: Parameter mismatch obscures intent
getWanPortToUseonly documentswanPort, yet the caller forwards{ sslPort, wanPort }.
Although structurally allowed, it confuses readers and tooling (e.g. schema generators).-private async getWanPortToUse(args?: { wanPort?: number }) { +private async getWanPortToUse(args?: { wanPort?: number }) {…and pass only the field you need:
-const wanPortToUse = await this.getWanPortToUse(args); +const wanPortToUse = await this.getWanPortToUse({ wanPort });packages/unraid-api-plugin-connect/src/event-handler/mothership.handler.ts (1)
61-66: Remove inline question or formalise itThe comment “
// Question: do we include MinigraphStatus.ERROR_RETRYING here?” is leftover scaffolding and will confuse future readers. Either:
- Extend the array to include the status, or
- Open a tracked issue and reference it, or
- Delete the comment.
Untended TODO-style markers are noise in production code.
packages/unraid-api-plugin-connect/src/test/url-resolver.service.test.ts (1)
190-196: Hard-coded expected URL assumes trailing slash
.toString()onURLguarantees the trailing slash, but many internal helpers use.hrefvs.origin. If the implementation switches, these tests will fail for non-functional reasons.Consider comparing
new URL(...).origininstead of the full string to make tests less brittle.packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts (1)
229-238:getAllowedServerIps()silently discards IPv6 addressesCurrent logic prefers IPv4 and skips IPv6 when both exist.
- if ((curr.ipv4 && curr.ipv6) || curr.ipv4) { - acc.push(curr.ipv4.toString()); - } else if (curr.ipv6) { - acc.push(curr.ipv6.toString()); - } + if (curr.ipv4) acc.push(curr.ipv4.toString()); + if (curr.ipv6) acc.push(curr.ipv6.toString());packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts (1)
78-86: Remove unnecessary null check for required parameterThe
allowedUrlparameter is required in the input type, so the null check is redundant.- await this.stopRemoteAccess(); - if (input.allowedUrl) { - this.setAllowedUrl({ - ipv4: input.allowedUrl.ipv4?.toString() ?? null, - ipv6: input.allowedUrl.ipv6?.toString() ?? null, - type: input.allowedUrl.type, - name: input.allowedUrl.name, - }); - } + await this.stopRemoteAccess(); + this.setAllowedUrl({ + ipv4: input.allowedUrl.ipv4?.toString() ?? null, + ipv6: input.allowedUrl.ipv6?.toString() ?? null, + type: input.allowedUrl.type, + name: input.allowedUrl.name, + });packages/unraid-api-plugin-connect/src/service/graphql.client.ts (3)
59-63: Remove redundant configuration checksThe configuration values are already validated through the getter methods. These duplicate calls serve no purpose.
async onModuleInit(): Promise<void> { await this.createClientInstance(); - this.configService.getOrThrow('API_VERSION'); - this.configService.getOrThrow('MOTHERSHIP_GRAPHQL_LINK'); }
318-325: Remove unused parameterThe
eventsparameter is defined but never used in the method body.- private clearClientEventHandlers( - events: Array<'connected' | 'connecting' | 'error' | 'ping'> = [ - 'connected', - 'connecting', - 'error', - 'ping', - ] - ): void { + private clearClientEventHandlers(): void {
140-142: Consider more robust protocol replacementWhile replacing 'http' with 'ws' works for most cases, it could cause issues with URLs containing 'http' elsewhere in the string.
- url: this.mothershipGraphqlLink.replace('http', 'ws'), + url: this.mothershipGraphqlLink.replace(/^https?:/, 'ws:'),packages/unraid-api-plugin-connect/src/service/cloud.service.ts (3)
43-43: Consider making cache instance-based instead of staticUsing a static cache could cause issues in testing scenarios or if multiple service instances are needed. Consider injecting the cache or making it an instance property.
- static cache = new NodeCache() as TypedCache<CacheSchema>; + private readonly cache = new NodeCache() as TypedCache<CacheSchema>;Then update all references from
CloudService.cachetothis.cachethroughout the file.
77-78: Clean up commented codeRemove commented-out code to improve readability. If this code is needed for debugging, use proper logging levels instead.
- if (cachedCloudCheck) { - // this.logger.verbose('Cache hit for cloud check %O', cachedCloudCheck); - return cachedCloudCheck; - } + if (cachedCloudCheck) { + return cachedCloudCheck; + }- if (!local.includes(network)) { - // Question: should we actually throw an error, or just log a warning? - // - // This is usually due to cloudflare's load balancing. - // if `dig +short mothership.unraid.net` shows both IPs, then this should be safe to ignore. - // this.logger.warn( - // `Local and network resolvers showing different IP for "${hostname}". [local="${ - // local ?? 'NOT FOUND' - // }"] [network="${network ?? 'NOT FOUND'}"].` - // ); - + if (local !== network) {Also applies to: 212-220
188-191: Remove unnecessary type checkThe DNS methods return strings, and the nullish coalescing ensures
validIpis always a string. This check is redundant.const validIp = local ?? network ?? ''; - if (typeof validIp !== 'string') { - return ''; - } cache.set('cloudIp', validIp, 12 * ONE_HOUR_SECS); // 12 hours ttlpackages/unraid-api-plugin-connect/src/service/connect-api-key.service.ts (2)
13-13: Remove unused static field.The
validRolesstatic field is declared but never used in this class.- private static readonly validRoles: Set<Role> = new Set(Object.values(Role));
71-83: Improve error handling consistency.The
createLocalConnectApiKeymethod catches errors and returnsnull, whilegetOrCreateLocalApiKeythrows an error. This inconsistency could lead to confusion. Consider either throwing in both cases or returning null in both cases.public async createLocalConnectApiKey(): Promise<ApiKeyWithSecret | null> { try { return await this.create({ name: 'Connect', description: 'API key for Connect user', roles: [Role.CONNECT], overwrite: true, }); } catch (err) { - this.logger.error(`Failed to create local API key for Connect user: ${err}`); - return null; + const errorMessage = `Failed to create local API key for Connect user: ${err}`; + this.logger.error(errorMessage); + throw new Error(errorMessage); } }packages/unraid-api-plugin-connect/src/resolver/connect-settings.resolver.ts (1)
91-101: Consider improving the restart delay mechanism.Using
setTimeoutwith a hardcoded 300ms delay for API restart seems fragile. Consider using a more robust event-driven approach or at least make the delay configurable.const currentSettings = await this.connectSettingsService.getCurrentSettings(); if (restartRequired) { - setTimeout(async () => { - // Send restart out of band to avoid blocking the return of this resolver - this.logger.log('Restarting API'); - await this.connectSettingsService.restartApi(); - }, 300); + // Emit an event for API restart to be handled asynchronously + this.eventEmitter.emit('api.restart.required', { + reason: 'API settings updated', + delay: 300 // Make this configurable + }); }packages/unraid-api-plugin-connect/src/model/config.entity.ts (4)
38-38: Fix typo in comment.- // Remote Access Configurationx + // Remote Access Configuration
95-101: Enhance SSO ID validation message.The validation message could be more specific about the allowed format.
@Matches(/^[a-zA-Z0-9-]+$/, { each: true, - message: 'Each SSO ID must be alphanumeric with dashes', + message: 'Each SSO ID must contain only letters, numbers, and hyphens', })
103-106: Remove commented-out code.If the
minigraphfield is no longer needed, remove it entirely rather than leaving it commented out.- // Connection Status - // @Field(() => MinigraphStatus) - // @IsEnum(MinigraphStatus) - // minigraph!: MinigraphStatus; -
207-210: Improve type safety for ConfigType.The
store: anytype andRecord<string, string>intersection reduce type safety. Consider defining more specific types if possible.export type ConfigType = ConnectConfig & { connect: ConnectConfig; - store: any; -} & Record<string, string>; + store: Record<string, unknown>; +} & Record<string, string | number | boolean | null>;packages/unraid-api-plugin-connect/src/service/connection.service.ts (1)
102-118: Correct log-message label for clarity
The warning refers toMETADATA_CHANGED, but the emitted token isEVENTS.MOTHERSHIP_CONNECTION_STATUS_CHANGED, which can confuse log searches.- this.logger.warn('Failed to emit METADATA_CHANGED event'); + this.logger.warn('Failed to emit MOTHERSHIP_CONNECTION_STATUS_CHANGED event');packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (2)
249-266: Avoid unconditional restart flag fromupdateRemoteAccess
The method always returnstrue, so any call marksrestartRequiredeven when nothing changed. Compute deltas instead:- // Use the dynamic remote access service to handle the transition - await this.remoteAccess.enableDynamicRemoteAccess({ + const restartNeeded = await this.remoteAccess.enableDynamicRemoteAccess({ ... - }); - - return true; + }); + + return restartNeeded;
200-207: Check event-emitter result for diagnostics
UnlikeMothershipConnectionService, you ignore the boolean returned byemit. Add a check to aid troubleshooting.- // Emit login event - this.eventEmitter.emit(EVENTS.LOGIN, { + const emitted = this.eventEmitter.emit(EVENTS.LOGIN, { ... }); + if (!emitted) { + this.logger.warn('LOGIN event emitted with no listeners'); + }
📜 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 (44)
packages/unraid-api-plugin-connect/justfile(1 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/event-handler/connect-login.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/event-handler/mothership.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/helper/delay-function.ts(1 hunks)packages/unraid-api-plugin-connect/src/helper/generic-consts.ts(1 hunks)packages/unraid-api-plugin-connect/src/helper/nest-tokens.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)packages/unraid-api-plugin-connect/src/job/timeout-checker.job.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/cloud.model.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/config.demo.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/config.entity.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/connect.model.ts(3 hunks)packages/unraid-api-plugin-connect/src/model/my-servers-config.ts(1 hunks)packages/unraid-api-plugin-connect/src/module/connect.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/module/mothership.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/module/remote-access.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/module/system.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/readme.md(1 hunks)packages/unraid-api-plugin-connect/src/resolver/cloud.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/resolver/connect-settings.resolver.ts(6 hunks)packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/resolver/health.resolver.ts(1 hunks)packages/unraid-api-plugin-connect/src/resolver/network.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/service/cloud.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/config.persistence.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/connect-api-key.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/connect-config.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts(8 hunks)packages/unraid-api-plugin-connect/src/service/connection.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/dns.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/graphql.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/internal.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/mothership-subscription.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/network.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/nginx.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/sso-user.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/static-remote-access.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-api-plugin-connect/src/test/cloud.service.test.ts(1 hunks)packages/unraid-api-plugin-connect/src/test/url-resolver.service.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- packages/unraid-api-plugin-connect/src/module/remote-access.module.ts
- packages/unraid-api-plugin-connect/src/module/system.module.ts
- packages/unraid-api-plugin-connect/src/module/mothership.module.ts
- packages/unraid-api-plugin-connect/src/helper/generic-consts.ts
- packages/unraid-api-plugin-connect/src/model/config.demo.ts
- packages/unraid-api-plugin-connect/src/module/connect.module.ts
- packages/unraid-api-plugin-connect/src/helper/nest-tokens.ts
- packages/unraid-api-plugin-connect/src/model/my-servers-config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/unraid-api-plugin-connect/src/index.ts
- packages/unraid-api-plugin-connect/src/resolver/cloud.resolver.ts
- packages/unraid-api-plugin-connect/src/model/cloud.model.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
packages/unraid-api-plugin-connect/src/test/cloud.service.test.ts (1)
api/src/environment.ts (2)
MOTHERSHIP_GRAPHQL_LINK(90-94)API_VERSION(65-65)
packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts (4)
packages/unraid-api-plugin-connect/src/resolver/health.resolver.ts (1)
Resolver(6-27)packages/unraid-api-plugin-connect/src/resolver/connect-settings.resolver.ts (1)
Resolver(27-165)web/composables/gql/graphql.ts (1)
Connect(438-445)packages/unraid-api-plugin-connect/src/model/config.entity.ts (2)
ConfigType(207-210)ConnectConfig(201-205)
packages/unraid-api-plugin-connect/src/resolver/health.resolver.ts (2)
packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts (1)
Resolver(11-42)packages/unraid-shared/src/index.ts (1)
demoShared(2-2)
packages/unraid-api-plugin-connect/src/model/connect.model.ts (1)
web/composables/gql/graphql.ts (2)
ConnectSettings(447-456)Node(1030-1032)
packages/unraid-api-plugin-connect/src/test/url-resolver.service.test.ts (1)
packages/unraid-api-plugin-connect/src/model/config.entity.ts (1)
ConfigType(207-210)
packages/unraid-api-plugin-connect/src/resolver/connect-settings.resolver.ts (3)
packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts (1)
Resolver(11-42)packages/unraid-api-plugin-connect/src/resolver/network.resolver.ts (1)
Resolver(14-40)web/composables/gql/graphql.ts (1)
ConnectSettings(447-456)
packages/unraid-api-plugin-connect/src/service/connect-api-key.service.ts (5)
packages/unraid-api-plugin-connect/src/service/connect-config.service.ts (1)
Injectable(8-57)packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)
Injectable(37-521)packages/unraid-shared/src/services/api-key.ts (1)
ApiKeyService(5-61)packages/unraid-shared/src/tokens.ts (1)
API_KEY_SERVICE_TOKEN(2-2)packages/unraid-shared/src/use-permissions.directive.ts (1)
AuthActionVerb(14-14)
packages/unraid-api-plugin-connect/src/service/cloud.service.ts (8)
web/composables/gql/graphql.ts (2)
CloudResponse(415-420)MinigraphqlResponse(912-917)packages/unraid-api-plugin-connect/src/service/connect-config.service.ts (1)
Injectable(8-57)packages/unraid-api-plugin-connect/src/service/connect-api-key.service.ts (1)
Injectable(10-106)packages/unraid-api-plugin-connect/src/service/connection.service.ts (1)
Injectable(50-222)packages/unraid-api-plugin-connect/src/service/internal.client.ts (1)
Injectable(16-143)packages/unraid-api-plugin-connect/src/service/mothership-subscription.handler.ts (1)
Injectable(29-216)packages/unraid-api-plugin-connect/src/model/config.entity.ts (1)
ConfigType(207-210)packages/unraid-api-plugin-connect/src/helper/generic-consts.ts (2)
ONE_MINUTE_SECS(5-5)ONE_HOUR_SECS(6-6)
packages/unraid-api-plugin-connect/src/service/sso-user.service.ts (9)
packages/unraid-api-plugin-connect/src/service/connect-config.service.ts (1)
Injectable(8-57)packages/unraid-api-plugin-connect/src/service/connect-api-key.service.ts (1)
Injectable(10-106)packages/unraid-api-plugin-connect/src/service/config.persistence.ts (1)
Injectable(17-172)packages/unraid-api-plugin-connect/src/service/dns.service.ts (1)
Injectable(5-18)packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)
Injectable(37-521)packages/unraid-api-plugin-connect/src/service/network.service.ts (1)
Injectable(8-46)packages/unraid-api-plugin-connect/src/service/nginx.service.ts (1)
Injectable(4-17)packages/unraid-api-plugin-connect/src/model/config.entity.ts (1)
ConfigType(207-210)packages/unraid-api-plugin-connect/src/model/my-servers-config.ts (1)
MyServersConfig(24-48)
🪛 Biome (1.9.4)
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts
[error] 227-231: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🪛 markdownlint-cli2 (0.17.2)
packages/unraid-api-plugin-connect/src/readme.md
20-20: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build API
🔇 Additional comments (14)
packages/unraid-api-plugin-connect/justfile (1)
3-5: LGTM! Clean default recipe implementation.The default recipe correctly uses
just --listto show available recipes when called without arguments.packages/unraid-api-plugin-connect/src/resolver/network.resolver.ts (1)
3-12: LGTM! Clean refactor to dependency injection and shared packages.The migration to shared packages and dependency injection follows NestJS best practices and improves code modularity.
Also applies to: 16-16, 32-32
packages/unraid-api-plugin-connect/src/resolver/health.resolver.ts (1)
24-24:❓ Verification inconclusive
Verify that configuration changes persist.
Ensure that
ConfigService.set()persists changes rather than just updating in-memory state.
🏁 Script executed:
#!/bin/bash # Description: Check if ConfigService has persistence mechanism # Expected: Find configuration persistence implementation ast-grep --pattern 'class $_ { $$$ set($_, $_) { $$$ } $$$ }'Length of output: 75
Let’s locate and inspect the actual
ConfigServiceimplementation to see howset()works:
🏁 Script executed:
#!/bin/bash # 1. Find where ConfigService is defined echo "Searching for ConfigService class..." rg "class ConfigService" -n --glob "*.ts" # 2. Inspect its `set(...)` method for persistence logic file=$(rg -l "class ConfigService" -n --glob "*.ts" | head -n1) if [ -n "$file" ]; then echo "Found implementation in: $file" echo "Showing context around the `set(` method:" rg -n -B3 -A5 "set(" "$file" else echo "❌ No ConfigService class found." fiLength of output: 283
Let’s confirm where
ConfigServicecomes from in your resolver:#!/bin/bash # Locate health.resolver.ts and show its ConfigService import file=$(fd -g "health.resolver.ts" | head -n1) if [ -n "$file" ]; then echo "File: $file" rg -n "import.*ConfigService" "$file" else echo "❌ health.resolver.ts not found" fi
Let’s pinpoint where
ConfigServiceis coming from:#!/bin/bash # 1. Locate health.resolver.ts resolver=$(fd -g "health.resolver.ts" | head -n1) # 2. Show its import of ConfigService if [ -n "$resolver" ]; then echo "In $resolver:" rg -n "import.*ConfigService" "$resolver" else echo "❌ health.resolver.ts not found" fi # 3. Find all ConfigService imports across the repo echo "" echo "All ConfigService imports:" rg -n "import.*ConfigService" -g "*.ts"
Verify ConfigService.set behavior and error handling
ConfigServiceisn’t defined in this repo (imported externally). Confirm that itsset()method writes to persistent storage and doesn’t only affect in-memory state.- Add appropriate error handling or fallbacks around
ConfigService.set('connect.demo', newValue)to surface any failures.packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts (1)
30-33:✅ Verification successful
Verify configuration structure change for
dynamicRemoteAccessType.The path to access
dynamicRemoteAccessTypechanged fromstate.config.remote.dynamicRemoteAccessTypetostate.config.dynamicRemoteAccessType. This appears to be a breaking change in the configuration structure.Run the following script to verify if this configuration path change is consistent across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old configuration path # Search for old path pattern rg -A 3 "config\.remote\.dynamicRemoteAccessType" # Search for new path pattern to confirm migration rg -A 3 "config\.dynamicRemoteAccessType"Length of output: 2302
All references updated to new config path
The only occurrence ofconfig.remote.dynamicRemoteAccessTypeis in a commented-out block (api/src/store/actions/shutdown-api-event.ts). All active code now usesconfig.dynamicRemoteAccessType. No further action required.packages/unraid-api-plugin-connect/src/event-handler/connect-login.handler.ts (1)
5-6:⚠️ Potential issueImport
GRAPHQL_PUBSUB_CHANNELfrom the correct location.The
GRAPHQL_PUBSUB_CHANNELconstant is used but not imported. It should be imported from the helper file.Add the missing import:
-import { GRAPHQL_PUBSUB_TOKEN } from '../helper/nest-tokens.js'; -import { EVENTS } from '../helper/nest-tokens.js'; +import { GRAPHQL_PUBSUB_TOKEN, GRAPHQL_PUBSUB_CHANNEL, EVENTS } from '../helper/nest-tokens.js';Likely an incorrect or invalid review comment.
packages/unraid-api-plugin-connect/src/model/connect.model.ts (1)
297-301: Good implementation of the Node interface.The explicit
idfield with proper decorators correctly implements the Node interface requirements.packages/unraid-api-plugin-connect/src/service/internal.client.ts (2)
4-8: Ensurefetchis always defined in the runtime
node< 18 does not expose a globalfetch; production images often pin older LTS versions. A missing global will crash the service the first time the client is created.@@ -import { HttpLink } from '@apollo/client/link/http/index.js'; +import { HttpLink } from '@apollo/client/link/http/index.js'; +import fetch from 'cross-fetch'; // Polyfill guarantees availabilityIf you are 100 % certain the container runs on Node 18+, please ignore; otherwise importing a tiny polyfill is the safer default.
Also applies to: 79-82
82-86: QuestionableOriginheader value
Origin: /var/run/unraid-cli.sockis not a valid origin and will be rejected by many CORS checks. If the intent is to bypass the browser, consider omitting the header entirely or using a RFC-compliant scheme (e.g.http://localhost).packages/unraid-api-plugin-connect/src/service/config.persistence.ts (1)
57-66: Skip persistence when config isundefined
configService.get<MyServersConfig>('connect.config')may returnundefinedon a mis-configured system; JSON-stringifyingundefinedproduces"undefined", creating an invalid file.- async persist(config = this.configService.get<MyServersConfig>('connect.config')) { + async persist(config = this.configService.get<MyServersConfig>('connect.config')) { + if (!config) { + this.logger.warn('No config present – skipping persistence'); + return false; + }packages/unraid-api-plugin-connect/src/test/url-resolver.service.test.ts (1)
204-206: Accessing IPv6 result viaipv4field is misleadingThe test asserts:
lanIp6Url?.ipv4?.toString()If the service really returns an IPv6 address in an
ipv4slot, that is a design smell; if not, the test passes by mistake.
Adjust the contract (e.g. distinctipv6field) or the test expectation to avoid hiding issues.packages/unraid-api-plugin-connect/src/service/sso-user.service.ts (1)
17-20:ssoSubIdsis typed asstringin the model but treated asstring[]
MyServersConfig.ssoSubIdsis currently declared as a single string (seemy-servers-config.ts).
Either migrate the config schema to an array or split/join when reading/writing; otherwise runtime shape and TypeScript types mismatch.packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts (1)
1-140: Service implementation looks solid!The dynamic remote access service properly manages state transitions, timeout detection, and cleanup. The separation of concerns between static and UPnP remote access services is well-designed.
packages/unraid-api-plugin-connect/src/service/connect-api-key.service.ts (1)
85-105:✅ Verification successful
Verify API key lookups are case-sensitive.
The method searches for an API key with the exact name 'Connect'. Ensure that the API key service's
findByFieldmethod performs case-sensitive comparisons, as a key named 'connect' or 'CONNECT' would not be found.
🏁 Script executed:
#!/bin/bash # Description: Check if API key lookups in the codebase are case-sensitive # Search for findByField implementations to understand the lookup behavior ast-grep --pattern 'findByField($_, $_) { $$$ }' # Also search for any existing API keys with similar names rg -i "name.*connect" --type ts -A 3 -B 3Length of output: 52086
🏁 Script executed:
#!/bin/bash # Locate the implementation of findByField in the ApiKeyService to inspect case sensitivity rg -n "findByField" -A 20 -B 5 api/src/unraid-api/auth/api-key.service.tsLength of output: 4934
findByField lookups are already case-sensitive
ThefindByFieldimplementation inapi/src/unraid-api/auth/api-key.service.ts(lines 277–281) uses strict===on string fields, so searching for'Connect'only matches that exact casing. Since all Connect keys are created with the same capitalization, no changes are required.packages/unraid-api-plugin-connect/src/resolver/connect-settings.resolver.ts (1)
120-123: LGTM! Clean event-driven logout implementation.The change from direct store dispatch to event emission is a good architectural improvement that decouples the resolver from the state management implementation.
| echo -e "${GREEN}Lines by directory:${NC}" | ||
| cd src | ||
| # First pass to get total lines | ||
| total=$(find . -type f -name "*.ts" -not -path "*/test/*" -not -path "*/graphql/generated/*" | xargs wc -l | tail -n 1 | awk '{print $1}') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of file counting logic.
The current approach using xargs wc -l | tail -n 1 can fail when no files are found, even with error redirection.
- total=$(find . -type f -name "*.ts" -not -path "*/test/*" -not -path "*/graphql/generated/*" | xargs wc -l | tail -n 1 | awk '{print $1}')
+ total=$(find . -type f -name "*.ts" -not -path "*/test/*" -not -path "*/graphql/generated/*" -print0 | xargs -0 wc -l 2>/dev/null | tail -n 1 | awk '{print $1}' || echo "0")Similarly for line 25:
- lines=$(find "$dir" -type f -name "*.ts" -not -path "*/graphql/generated/*" | xargs wc -l 2>/dev/null | tail -n 1 | awk '{print $1}')
+ lines=$(find "$dir" -type f -name "*.ts" -not -path "*/graphql/generated/*" -print0 | xargs -0 wc -l 2>/dev/null | tail -n 1 | awk '{print $1}' || echo "0")Also applies to: 25-25
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/justfile at lines 21 and 25, the current
file counting logic using `xargs wc -l | tail -n 1` can fail if no matching
files are found. To fix this, modify the commands to handle the case when no
files are found gracefully, such as by using `find` with `-print0` and `xargs
-0` combined with a fallback or conditional check to avoid errors and ensure the
total line count is correctly set to zero if no files match.
| echo -e "${BLUE}Counting TypeScript lines in src/ (excluding test/ and graphql/generated/)...${NC}" | ||
| echo | ||
| echo -e "${GREEN}Lines by directory:${NC}" | ||
| cd src |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing src directory.
The script changes to the src directory without checking if it exists, which could cause the entire script to fail.
+ if [ ! -d "src" ]; then
+ echo -e "${RED}Error: src directory not found${NC}"
+ exit 1
+ fi
cd src🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/justfile at line 19, the script changes
directory to 'src' without verifying its existence, which may cause failure if
'src' is missing. Add a check before the 'cd src' command to confirm that the
'src' directory exists, and handle the error gracefully by either printing a
clear error message and exiting or by skipping the directory change to prevent
the script from failing unexpectedly.
| const BAD_API_KEY = 'BAD_API_KEY'; | ||
| const BAD = 'BAD'; | ||
|
|
||
| describe('CloudService.hardCheckCloud (integration)', () => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider testing public interface instead of private methods.
Testing private methods directly breaks encapsulation and makes tests brittle to implementation changes. Consider testing the public methods that use hardCheckCloud internally.
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/test/cloud.service.test.ts at line 10,
the test is directly targeting the private method hardCheckCloud, which breaks
encapsulation and can lead to brittle tests. Refactor the test to remove direct
calls to hardCheckCloud and instead test the public methods of CloudService that
internally use hardCheckCloud, ensuring the tests verify behavior through the
public interface only.
| it('fails to authenticate with mothership with no credentials', async () => { | ||
| try { | ||
| await expect( | ||
| service['hardCheckCloud'](API_VERSION, BAD) | ||
| ).rejects.toThrowErrorMatchingInlineSnapshot( | ||
| `[Error: Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.]` | ||
| ); | ||
| await expect( | ||
| service['hardCheckCloud'](API_VERSION, BAD_API_KEY) | ||
| ).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Invalid credentials]`); | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.includes('Timeout')) { | ||
| // Test succeeds on timeout | ||
| return; | ||
| } | ||
| throw error; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify timeout handling logic and add positive test cases.
The timeout handling appears to treat timeouts as test success, which is confusing. Additionally, this test only covers failure scenarios.
Consider:
- Adding explicit timeout configuration if network tests are expected
- Adding positive test cases with valid credentials
- Clarifying why timeouts are considered successful test outcomes
- } catch (error) {
- if (error instanceof Error && error.message.includes('Timeout')) {
- // Test succeeds on timeout
- return;
- }
- throw error;
- }
+ } catch (error) {
+ if (error instanceof Error && error.message.includes('Timeout')) {
+ // Skip test on timeout - network unavailable
+ console.warn('Test skipped due to network timeout');
+ return;
+ }
+ throw error;
+ }📝 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.
| it('fails to authenticate with mothership with no credentials', async () => { | |
| try { | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD) | |
| ).rejects.toThrowErrorMatchingInlineSnapshot( | |
| `[Error: Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.]` | |
| ); | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD_API_KEY) | |
| ).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Invalid credentials]`); | |
| } catch (error) { | |
| if (error instanceof Error && error.message.includes('Timeout')) { | |
| // Test succeeds on timeout | |
| return; | |
| } | |
| throw error; | |
| } | |
| }); | |
| it('fails to authenticate with mothership with no credentials', async () => { | |
| try { | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD) | |
| ).rejects.toThrowErrorMatchingInlineSnapshot( | |
| `[Error: Failed to connect to https://mothership.unraid.net/ws with a "426" HTTP error.]` | |
| ); | |
| await expect( | |
| service['hardCheckCloud'](API_VERSION, BAD_API_KEY) | |
| ).rejects.toThrowErrorMatchingInlineSnapshot(`[Error: Invalid credentials]`); | |
| } catch (error) { | |
| if (error instanceof Error && error.message.includes('Timeout')) { | |
| // Skip test on timeout - network unavailable | |
| console.warn('Test skipped due to network timeout'); | |
| return; | |
| } | |
| throw error; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/test/cloud.service.test.ts between
lines 33 and 50, the test treats timeouts as successful outcomes without clear
explanation and only covers failure cases. To fix this, explicitly configure
timeout settings for network calls if applicable, clarify in comments why
timeouts are considered successful, and add positive test cases that verify
successful authentication with valid credentials to ensure comprehensive test
coverage.
| async update() { | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling.
The method lacks proper documentation and error context for callers.
+ /**
+ * Updates DNS settings by executing the Unraid dynamix UpdateDNS script.
+ * @returns Promise<boolean> - true if DNS update succeeded, false otherwise
+ */
async update() {
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);
+ this.logger.warn('Failed to update DNS settings', { error: err });
return false;
}
}📝 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 update() { | |
| 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; | |
| } | |
| } | |
| /** | |
| * Updates DNS settings by executing the Unraid dynamix UpdateDNS script. | |
| * @returns Promise<boolean> - true if DNS update succeeded, false otherwise | |
| */ | |
| async update() { | |
| try { | |
| await execa('/usr/bin/php', ['/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php']); | |
| return true; | |
| } catch (err: unknown) { | |
| this.logger.warn('Failed to update DNS settings', { error: err }); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/dns.service.ts between lines 9
and 17, add input validation if applicable and enhance error handling by
including more detailed error context in the catch block. Also, add a clear
method-level comment describing the purpose, behavior, and return value of the
update method to improve code documentation and maintainability.
| if (!url || url === '') { | ||
| throw new Error('No URL Provided'); | ||
| } | ||
|
|
||
| if (port) { | ||
| portToUse = port === 80 ? '' : `:${port}`; | ||
| httpMode = 'http://'; | ||
| } else if (portSsl) { | ||
| portToUse = portSsl === 443 ? '' : `:${portSsl}`; | ||
| httpMode = 'https://'; | ||
| } else { | ||
| throw new Error(`No ports specified for URL: ${url}`); | ||
| } |
There was a problem hiding this comment.
IPv6 hosts need brackets – new URL() mis-parses raw IPv6 literals
new URL('http://2001:db8::1') throws because 2001:db8::1 is 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}`;📝 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 (!url || url === '') { | |
| throw new Error('No URL Provided'); | |
| } | |
| if (port) { | |
| portToUse = port === 80 ? '' : `:${port}`; | |
| httpMode = 'http://'; | |
| } else if (portSsl) { | |
| portToUse = portSsl === 443 ? '' : `:${portSsl}`; | |
| httpMode = 'https://'; | |
| } else { | |
| throw new Error(`No ports specified for URL: ${url}`); | |
| } | |
| if (!url || url === '') { | |
| throw new Error('No URL Provided'); | |
| } | |
| if (port) { | |
| portToUse = port === 80 ? '' : `:${port}`; | |
| httpMode = 'http://'; | |
| } else if (portSsl) { | |
| portToUse = portSsl === 443 ? '' : `:${portSsl}`; | |
| httpMode = 'https://'; | |
| } else { | |
| throw new Error(`No ports specified for URL: ${url}`); | |
| } | |
| // Wrap bare IPv6 literals in brackets so new URL() parses them correctly | |
| const host = /:/.test(url) && !url.startsWith('[') ? `[${url}]` : url; | |
| const urlString = `${httpMode}${host}${portToUse}`; |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts around
lines 135 to 147, the code constructs URLs using raw IPv6 addresses without
brackets, causing new URL() to misinterpret the address as a scheme. To fix
this, detect if the host is an IPv6 literal and wrap it in square brackets
before concatenating with the protocol and port. This ensures new URL() parses
the address correctly.
| if (nginx.sslMode === 'yes') { | ||
| return this.getUrlForField({ | ||
| url: nginx[field] as string, | ||
| portSsl: nginx.httpsPort, | ||
| }); | ||
| } | ||
|
|
||
| if (nginx.sslMode === 'auto') { | ||
| throw new Error(`Cannot get IP Based URL for field: "${field}" SSL mode auto`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
sslMode: "no" not handled – valid configs will falsely error
When sslEnabled is true but sslMode is "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,
+ });
}📝 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 (nginx.sslMode === 'yes') { | |
| return this.getUrlForField({ | |
| url: nginx[field] as string, | |
| portSsl: nginx.httpsPort, | |
| }); | |
| } | |
| if (nginx.sslMode === 'auto') { | |
| throw new Error(`Cannot get IP Based URL for field: "${field}" SSL mode auto`); | |
| } | |
| if (nginx.sslMode === 'yes') { | |
| return this.getUrlForField({ | |
| url: nginx[field] as string, | |
| portSsl: nginx.httpsPort, | |
| }); | |
| } else if (nginx.sslMode === 'no') { | |
| return this.getUrlForField({ | |
| url: nginx[field] as string, | |
| port: nginx.httpPort, | |
| }); | |
| } | |
| if (nginx.sslMode === 'auto') { | |
| throw new Error(`Cannot get IP Based URL for field: "${field}" SSL mode auto`); | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts around
lines 190 to 199, the code does not handle the case when sslMode is "no" while
sslEnabled is true, causing valid configurations to incorrectly throw an error.
Add a condition to properly handle sslMode "no" by returning the appropriate URL
or behavior instead of falling through to the error branch. Ensure all valid
sslMode values are explicitly handled to avoid false errors.
| // If there's no change, no need to update | ||
| if (newUserSet.symmetricDifference(currentUserSet).size === 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Set.symmetricDifference is non-standard – code will not compile
Standard JS Set has no symmetricDifference() method.
Simple replacement:
- if (newUserSet.symmetricDifference(currentUserSet).size === 0) {
+ const changed =
+ [...newUserSet].filter((x) => !currentUserSet.has(x)).length > 0 ||
+ [...currentUserSet].filter((x) => !newUserSet.has(x)).length > 0;
+ if (!changed) {
return false;
}📝 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 there's no change, no need to update | |
| if (newUserSet.symmetricDifference(currentUserSet).size === 0) { | |
| return false; | |
| } | |
| // If there's no change, no need to update | |
| const changed = | |
| [...newUserSet].filter((x) => !currentUserSet.has(x)).length > 0 || | |
| [...currentUserSet].filter((x) => !newUserSet.has(x)).length > 0; | |
| if (!changed) { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/sso-user.service.ts around
lines 32 to 35, the code uses a non-standard Set method symmetricDifference
which does not exist in JavaScript and causes compilation errors. Replace the
symmetricDifference call by manually computing the symmetric difference using
standard Set operations: create a new Set containing elements in either
newUserSet or currentUserSet but not in both, by combining the differences of
each set from the other, then check if the size of this new Set is zero to
determine if there is no change.
| getConnectionState() { | ||
| const state = this.configService.get<ConnectionMetadata>('connect.mothership'); | ||
| if (!state) { | ||
| this.logger.error( | ||
| 'connect.mothership config is not present! Preventing fatal crash; mothership is in Error state.' | ||
| ); | ||
| } | ||
| return state; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return a sane default when metadata is missing
getConnectionState() logs the missing config but still returns undefined; callers expecting a ConnectionMetadata object will crash on access. Return a minimal PRE_INIT object instead.
- if (!state) {
- this.logger.error(
- 'connect.mothership config is not present! Preventing fatal crash; mothership is in Error state.'
- );
- }
- return state;
+ if (!state) {
+ this.logger.error(
+ 'connect.mothership config is not present! Defaulting to PRE_INIT state.'
+ );
+ return { status: MinigraphStatus.PRE_INIT } as ConnectionMetadata;
+ }
+ return state;📝 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.
| getConnectionState() { | |
| const state = this.configService.get<ConnectionMetadata>('connect.mothership'); | |
| if (!state) { | |
| this.logger.error( | |
| 'connect.mothership config is not present! Preventing fatal crash; mothership is in Error state.' | |
| ); | |
| } | |
| return state; | |
| } | |
| getConnectionState() { | |
| const state = this.configService.get<ConnectionMetadata>('connect.mothership'); | |
| if (!state) { | |
| this.logger.error( | |
| 'connect.mothership config is not present! Defaulting to PRE_INIT state.' | |
| ); | |
| return { status: MinigraphStatus.PRE_INIT } as ConnectionMetadata; | |
| } | |
| return state; | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/connection.service.ts around
lines 193 to 201, the getConnectionState() method returns undefined when the
connect.mothership config is missing, which can cause callers to crash. Modify
the method to return a minimal ConnectionMetadata object with a PRE_INIT state
as a default instead of undefined, ensuring callers always receive a valid
object.
| private async setSandboxMode(sandboxEnabled: boolean): Promise<boolean> { | ||
| const { store, getters } = await import('@app/store/index.js'); | ||
| const currentSandbox = getters.config().local.sandbox; | ||
| throw new Error('Not implemented'); | ||
| const currentSandbox = this.configService.get('store.config.local.sandbox'); | ||
| const sandbox = sandboxEnabled ? 'yes' : 'no'; | ||
| if (currentSandbox === sandbox) return false; | ||
| store.dispatch(updateUserConfig({ local: { sandbox } })); | ||
| this.configService.set('store.config.local.sandbox', sandbox); | ||
| return true; |
There was a problem hiding this comment.
setSandboxMode is unreachable – breaks settings sync
The method throws "Not implemented" but is invoked from syncSettings(). Any update to sandbox mode will crash the API.
- throw new Error('Not implemented');
- const currentSandbox = this.configService.get('store.config.local.sandbox');
- const sandbox = sandboxEnabled ? 'yes' : 'no';
- if (currentSandbox === sandbox) return false;
- this.configService.set('store.config.local.sandbox', sandbox);
- return true;
+ const currentSandbox = this.configService.get('store.config.local.sandbox');
+ const sandbox = sandboxEnabled ? 'yes' : 'no';
+ if (currentSandbox === sandbox) {
+ return false;
+ }
+ this.configService.set('store.config.local.sandbox', sandbox);
+ return 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.
| private async setSandboxMode(sandboxEnabled: boolean): Promise<boolean> { | |
| const { store, getters } = await import('@app/store/index.js'); | |
| const currentSandbox = getters.config().local.sandbox; | |
| throw new Error('Not implemented'); | |
| const currentSandbox = this.configService.get('store.config.local.sandbox'); | |
| const sandbox = sandboxEnabled ? 'yes' : 'no'; | |
| if (currentSandbox === sandbox) return false; | |
| store.dispatch(updateUserConfig({ local: { sandbox } })); | |
| this.configService.set('store.config.local.sandbox', sandbox); | |
| return true; | |
| private async setSandboxMode(sandboxEnabled: boolean): Promise<boolean> { | |
| const currentSandbox = this.configService.get('store.config.local.sandbox'); | |
| const sandbox = sandboxEnabled ? 'yes' : 'no'; | |
| if (currentSandbox === sandbox) { | |
| return false; | |
| } | |
| this.configService.set('store.config.local.sandbox', sandbox); | |
| return true; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 227-231: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts
between lines 225 and 231, the setSandboxMode method currently throws a "Not
implemented" error, making it unreachable and causing crashes when called from
syncSettings. Remove the throw statement and implement the method logic to check
the current sandbox mode, compare it with the desired state, update the config
if different, and return a boolean indicating if a change was made.
tried to use them for debugging; didn't work
pujitm
force-pushed
the
refactor/mothership
branch
from
6e2ea8a to
b42d854
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/unraid-api-plugin-connect/src/service/config.persistence.ts (2)
23-26: Guard against missingPATHS_CONFIG_MODULES
get('PATHS_CONFIG_MODULES')!will throw a TypeError if the env-var is unset. Return a clear error instead so startup diagnostics are obvious.
32-48: Subscription leak – store the RxJS subscription and dispose on shutdownThe
changes$subscription is never unsubscribed. For long-running processes that restart modules (e.g. hot-reload in dev) this causes memory leaks.packages/unraid-api-plugin-connect/src/service/cloud.service.ts (2)
202-204: Use.hostnameinstead of.hostto strip port before DNS look-up
new URL(mothershipGqlUri).hostincludes the port if present (e.g.api.example.com:443), causing DNS resolution to fail. Swap to.hostnameso only the domain part is sent todns.lookup/dns.resolve.
211-211: Equality check should be strict, not substring
!local.includes(network)gives false positives (e.g."1.2.3.4"includes"2.3"). Use strict comparison to ensure both resolvers agree on the exact IP.packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)
225-232:⚠️ Potential issueFix unreachable code in setSandboxMode method.
The method throws "Not implemented" making the subsequent code unreachable. This breaks settings sync when sandbox mode is updated, as noted in previous reviews.
- throw new Error('Not implemented'); - const currentSandbox = this.configService.get('store.config.local.sandbox'); + const currentSandbox = this.configService.get('store.config.local.sandbox'); const sandbox = sandboxEnabled ? 'yes' : 'no'; if (currentSandbox === sandbox) return false; this.configService.set('store.config.local.sandbox', sandbox); return true;🧰 Tools
🪛 Biome (1.9.4)
[error] 227-231: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🧹 Nitpick comments (8)
packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts (2)
46-52: Consider adding timeout check frequency controlThe timeout check logic is correct, but there's no indication of how frequently
checkForTimeout()is called. Consider documenting the expected call frequency or implementing a scheduled job.
79-86: Simplify URL transformation logicThe allowedUrl transformation can be simplified and made more readable.
- this.setAllowedUrl({ - ipv4: input.allowedUrl.ipv4?.toString() ?? null, - ipv6: input.allowedUrl.ipv6?.toString() ?? null, - type: input.allowedUrl.type, - name: input.allowedUrl.name, - }); + this.setAllowedUrl(input.allowedUrl);The
setAllowedUrlmethod already handles the transformation and defaults appropriately.packages/unraid-api-plugin-connect/src/service/graphql.client.ts (1)
61-62: Remove redundant configuration callsThese
getOrThrowcalls are redundant since the same values are already accessed in the class getters and constructor.- this.configService.getOrThrow('API_VERSION'); - this.configService.getOrThrow('MOTHERSHIP_GRAPHQL_LINK');packages/unraid-api-plugin-connect/src/service/cloud.service.ts (2)
43-43: Consider instance-based cache instead of staticUsing a static cache shared across all service instances could lead to unexpected behavior in testing or multi-tenant scenarios. Consider using an instance-based cache.
- static cache = new NodeCache() as TypedCache<CacheSchema>; + private cache = new NodeCache() as TypedCache<CacheSchema>;And update all references from
CloudService.cachetothis.cache.
188-191: Improve type safety for IP validationThe type check
typeof validIp !== 'string'is redundant sincevalidIpis already typed as string from the nullish coalescing. Consider simplifying the logic.- const validIp = local ?? network ?? ''; - if (typeof validIp !== 'string') { - return ''; - } + const validIp = local || network || '';packages/unraid-api-plugin-connect/src/model/connect-config.model.ts (2)
38-38: Fix typo in comment.There's a typo in the comment: "Configurationx" should be "Configuration".
- // Remote Access Configurationx + // Remote Access Configuration
104-106: Clean up commented code.Consider removing the commented-out minigraph status field if it's no longer needed, or add a TODO comment explaining why it's temporarily disabled.
- // Connection Status - // @Field(() => MinigraphStatus) - // @IsEnum(MinigraphStatus) - // minigraph!: MinigraphStatus;packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (1)
150-151: Clean up commented code.Remove the commented-out import and function call if they're no longer needed.
- // const { writeConfigSync } = await import('@app/store/sync/config-disk-sync.js'); - // writeConfigSync('flash');
📜 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 (23)
api/package.json(4 hunks)api/scripts/build.ts(3 hunks)packages/unraid-api-plugin-connect/src/event-handler/mothership.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)packages/unraid-api-plugin-connect/src/job/timeout-checker.job.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/cloud.model.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/connect-config.model.ts(1 hunks)packages/unraid-api-plugin-connect/src/model/my-servers-config.model.ts(1 hunks)packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts(2 hunks)packages/unraid-api-plugin-connect/src/service/cloud.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/config.persistence.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/connect-config.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts(8 hunks)packages/unraid-api-plugin-connect/src/service/connection.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/graphql.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/internal.client.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/sso-user.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/static-remote-access.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-api-plugin-connect/src/test/url-resolver.service.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts
- packages/unraid-api-plugin-connect/src/model/my-servers-config.model.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts
- packages/unraid-api-plugin-connect/src/job/timeout-checker.job.ts
- api/package.json
- packages/unraid-api-plugin-connect/src/service/connect-config.service.ts
- packages/unraid-api-plugin-connect/src/index.ts
- packages/unraid-api-plugin-connect/src/service/internal.client.ts
- packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts
- packages/unraid-api-plugin-connect/src/event-handler/mothership.handler.ts
- packages/unraid-api-plugin-connect/src/service/upnp.service.ts
- packages/unraid-api-plugin-connect/src/test/url-resolver.service.test.ts
- packages/unraid-api-plugin-connect/src/service/url-resolver.service.ts
- packages/unraid-api-plugin-connect/src/model/cloud.model.ts
- packages/unraid-api-plugin-connect/src/service/sso-user.service.ts
- api/scripts/build.ts
- packages/unraid-api-plugin-connect/src/service/connection.service.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts
[error] 227-231: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
🔇 Additional comments (6)
packages/unraid-api-plugin-connect/src/service/config.persistence.ts (1)
57-57:❌ Incorrect review comment
Verify config parameter type consistency
The method parameter defaults to
this.configService.get<MyServersConfig>('connect.config')but the method signature accepts anyMyServersConfig. Ensure the configuration path is consistent across the service.
🏁 Script executed:
#!/bin/bash # Search for all usages of connect.config vs connect configuration keys rg -A 3 -B 3 "connect\.config|'connect'" --type tsLength of output: 24078
🏁 Script executed:
#!/bin/bash # Display the beginning of config.persistence.ts to inspect the persist method signature sed -n '1,200p' packages/unraid-api-plugin-connect/src/service/config.persistence.tsLength of output: 6893
No inconsistencies in
persistsignature or config key usage
Thepersistmethod’s default parameter is correctly inferred asMyServersConfig(viaget<MyServersConfig>), and every read/write in this service consistently uses the'connect.config'key. You can safely ignore the suggested type/path mismatch.Likely an incorrect or invalid review comment.
packages/unraid-api-plugin-connect/src/service/graphql.client.ts (2)
118-123: Potential race condition in client cleanupThe comment indicates a known race condition where
apolloClientcan become null during shutdown. Consider using proper synchronization or state management.- if (this.apolloClient) { - await this.apolloClient.clearStore(); - // some race condition causes apolloClient to be null here upon api shutdown? - this.apolloClient?.stop(); - this.apolloClient = null; - } + const client = this.apolloClient; + if (client) { + this.apolloClient = null; + await client.clearStore(); + client.stop(); + }
213-231:✅ Verification successful
Verify retry logic handles edge cases
The retry logic has complex conditions. Ensure the
retryCountcontext is properly initialized and the infinite retry limit is intentional.
🏁 Script executed:
#!/bin/bash # Search for other retry implementations to verify consistency rg -A 10 -B 5 "RetryLink|retryIf|attempts.*max" --type tsLength of output: 7186
Retry logic confirmation
The
retryCountis safely initialized to0via the default in
const { retryCount = 0 } = operation.getContext(), and it’s updated on each back‐off in yourdelaycallback. Usingmax: Infinityis intentional to allow unlimited retries for non-auth errors, while invalid API key errors are still capped atMAX_AUTH_RETRIES. No changes needed.packages/unraid-api-plugin-connect/src/model/connect-config.model.ts (1)
1-236: Well-structured model definitions.The configuration models are comprehensive and well-designed with appropriate validation decorators, GraphQL integration, and type safety. The use of class-validator and class-transformer provides robust validation and serialization capabilities.
packages/unraid-api-plugin-connect/src/service/connect-settings.service.ts (2)
39-45: Excellent dependency injection refactoring.The constructor properly injects ConfigService and other services, replacing direct Redux store access with clean dependency injection. This improves testability and follows NestJS best practices.
173-207: Good event-driven architecture implementation.The signIn method properly uses ConfigService for state management and EventEmitter2 for event publishing, replacing Redux actions with a cleaner event-driven approach.
| } catch (error) { | ||
| this.logger.error(error); | ||
| const message = error instanceof Error ? error.message : 'Unknown Error'; | ||
| this.setErrorMessage(message); | ||
| return error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling returns error object instead of throwing
The method catches errors but returns them instead of throwing, which is inconsistent with typical async error handling patterns. Consider whether callers expect thrown errors or returned error objects.
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/service/dynamic-remote-access.service.ts
around lines 88 to 94, the catch block currently returns the caught error object
instead of throwing it, which is inconsistent with standard async error
handling. Modify the catch block to throw the error after logging and setting
the error message, ensuring that the error propagates properly to callers
expecting thrown exceptions.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/main.yml(0 hunks)api/src/environment.ts(2 hunks)api/src/index.ts(2 hunks)api/src/store/actions/shutdown-api-event.ts(0 hunks)api/src/store/listeners/listener-middleware.ts(0 hunks)api/src/store/listeners/upnp-listener.ts(2 hunks)api/src/store/listeners/wan-access-change-listener.ts(0 hunks)packages/unraid-api-plugin-connect/package.json(1 hunks)packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts(1 hunks)packages/unraid-api-plugin-connect/src/helper/nest-tokens.ts(1 hunks)packages/unraid-api-plugin-connect/src/module/remote-access.module.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts(1 hunks)
💤 Files with no reviewable changes (4)
- .github/workflows/main.yml
- api/src/store/actions/shutdown-api-event.ts
- api/src/store/listeners/listener-middleware.ts
- api/src/store/listeners/wan-access-change-listener.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- api/src/store/listeners/upnp-listener.ts
- api/src/index.ts
- packages/unraid-api-plugin-connect/src/module/remote-access.module.ts
- api/src/environment.ts
- packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts
- packages/unraid-api-plugin-connect/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts (2)
packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts (1)
Injectable(9-34)packages/unraid-api-plugin-connect/src/service/upnp-remote-access.service.ts (1)
Injectable(10-43)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts (1)
16-18: Good cleanup implementation.The automatic disabling of WAN access on module destruction ensures proper cleanup and prevents dangling network configurations.
packages/unraid-api-plugin-connect/src/helper/nest-tokens.ts (1)
1-15: Excellent centralization of tokens and events.Clean organization with proper separation of job tokens, GraphQL tokens, and event constants. The consistent naming convention and use of enums reduces magic strings throughout the codebase.
packages/unraid-api-plugin-connect/src/service/static-remote-access.service.ts (2)
19-21: Clean event-driven design.Good separation of concerns - the service emits events rather than directly managing config and network state.
23-33: Solid validation and error handling.Good configuration validation with early return for invalid types. The logging provides helpful debugging information when the dynamic remote access type doesn't match expectations.
| @OnEvent(EVENTS.ENABLE_WAN_ACCESS, { async: true }) | ||
| async enableWanAccess() { | ||
| this.configService.set('connect.config.wanaccess', true); | ||
| await this.networkService.reloadNetworkStack(); | ||
| } |
There was a problem hiding this comment.
Add error handling for network stack reload failures.
If reloadNetworkStack() fails, the config will show WAN access as enabled while the network stack may not reflect this state, creating system inconsistency.
@OnEvent(EVENTS.ENABLE_WAN_ACCESS, { async: true })
async enableWanAccess() {
this.configService.set('connect.config.wanaccess', true);
- await this.networkService.reloadNetworkStack();
+ try {
+ await this.networkService.reloadNetworkStack();
+ } catch (error) {
+ // Rollback config on failure to maintain consistency
+ this.configService.set('connect.config.wanaccess', false);
+ throw error;
+ }
}📝 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.ENABLE_WAN_ACCESS, { async: true }) | |
| async enableWanAccess() { | |
| this.configService.set('connect.config.wanaccess', true); | |
| await this.networkService.reloadNetworkStack(); | |
| } | |
| @OnEvent(EVENTS.ENABLE_WAN_ACCESS, { async: true }) | |
| async enableWanAccess() { | |
| this.configService.set('connect.config.wanaccess', true); | |
| try { | |
| await this.networkService.reloadNetworkStack(); | |
| } catch (error) { | |
| // Rollback config on failure to maintain consistency | |
| this.configService.set('connect.config.wanaccess', false); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts
around lines 20 to 24, the enableWanAccess method lacks error handling for the
asynchronous reloadNetworkStack call. To fix this, wrap the await
this.networkService.reloadNetworkStack() call in a try-catch block. In the catch
block, revert the configService setting for 'connect.config.wanaccess' to false
and log or handle the error appropriately to maintain system consistency.
| @OnEvent(EVENTS.DISABLE_WAN_ACCESS, { async: true }) | ||
| async disableWanAccess() { | ||
| this.configService.set('connect.config.wanaccess', false); | ||
| await this.networkService.reloadNetworkStack(); | ||
| } |
There was a problem hiding this comment.
Add error handling for network stack reload failures.
If reloadNetworkStack() fails, the config will show WAN access as disabled while the network stack may still have WAN access enabled, creating system inconsistency.
@OnEvent(EVENTS.DISABLE_WAN_ACCESS, { async: true })
async disableWanAccess() {
this.configService.set('connect.config.wanaccess', false);
- await this.networkService.reloadNetworkStack();
+ try {
+ await this.networkService.reloadNetworkStack();
+ } catch (error) {
+ // Rollback config on failure to maintain consistency
+ this.configService.set('connect.config.wanaccess', true);
+ throw error;
+ }
}📝 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.DISABLE_WAN_ACCESS, { async: true }) | |
| async disableWanAccess() { | |
| this.configService.set('connect.config.wanaccess', false); | |
| await this.networkService.reloadNetworkStack(); | |
| } | |
| @OnEvent(EVENTS.DISABLE_WAN_ACCESS, { async: true }) | |
| async disableWanAccess() { | |
| this.configService.set('connect.config.wanaccess', false); | |
| try { | |
| await this.networkService.reloadNetworkStack(); | |
| } catch (error) { | |
| // Rollback config on failure to maintain consistency | |
| this.configService.set('connect.config.wanaccess', true); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/event-handler/wan-access.handler.ts
around lines 26 to 30, the disableWanAccess method lacks error handling for the
asynchronous reloadNetworkStack call. To fix this, wrap the await
this.networkService.reloadNetworkStack() call in a try-catch block. In the catch
block, log the error and consider reverting the configService.set call or
handling the failure to maintain system consistency between the config and
actual network state.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores
Style
Revert