fix: auto-uninstallation of connect api plugin #1791
Conversation
WalkthroughCentralizes writing the nested Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as API Startup
participant ConfigModule as API Config Loader
participant Cleanup as Connect Cleanup
participant FS as Filesystem
participant CLI as CLI (unraid-api)
participant Config as Config Service
Startup->>Cleanup: pruneStaleConnectPluginEntryIfNecessary()
Cleanup->>FS: isConnectPluginInstalled() (check SKIP env or plugin file)
alt plugin installed
FS-->>Cleanup: true
Cleanup-->>Startup: complete (no-op)
else plugin not installed
FS-->>Cleanup: false
Cleanup->>CLI: run "unraid-api plugins remove -b unraid-api-plugin-connect [--no-restart]"
CLI->>Config: mutate/persist config (remove plugin entry)
CLI-->>Cleanup: ExecaReturnValue
Cleanup-->>Startup: complete
end
note right of ConfigModule `#DDEBF7`: During config load
ConfigModule->>FS: isConnectPluginInstalled()
alt plugin not installed
ConfigModule->>Config: filter out 'unraid-api-plugin-connect' from diskConfig.plugins
ConfigModule->>Config: persist updated api config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1791 +/- ##
==========================================
- Coverage 52.27% 52.13% -0.14%
==========================================
Files 874 874
Lines 50561 50431 -130
Branches 5063 5027 -36
==========================================
- Hits 26430 26293 -137
- Misses 24056 24063 +7
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/plugin/__test__/plugin-management.service.spec.ts (1)
30-36: Remove@ts-expect-errorwith explicit typing.The multiple
@ts-expect-errorsuppressions can be avoided by asserting the type after runtime checks.Apply this diff to improve type safety:
- set: vi.fn((key: string, value: unknown) => { - if (key === 'api' && typeof value === 'object' && value !== null) { - // @ts-expect-error - value is an object - if (Array.isArray(value.plugins)) { - // @ts-expect-error - value is an object - configStore = [...value.plugins]; - } - } + set: vi.fn((key: string, value: unknown) => { + if (key === 'api' && typeof value === 'object' && value !== null) { + const apiValue = value as Record<string, unknown>; + if (Array.isArray(apiValue.plugins)) { + configStore = [...apiValue.plugins]; + } + } if (key === 'api.plugins' && Array.isArray(value)) { configStore = [...value]; } }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/unraid-api/plugin/__test__/plugin-management.service.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/unraid-api/plugin/__test__/plugin-management.service.spec.ts
🔇 Additional comments (1)
api/src/unraid-api/plugin/__test__/plugin-management.service.spec.ts (1)
24-26: LGTM: Test supports nested api object reads.The mock correctly returns the api object structure with the plugins array, aligning with the production change to nested configuration storage.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🧬 Code graph analysis (1)
packages/unraid-api-plugin-connect/src/index.ts (2)
api/src/unraid-api/plugin/plugin-management.service.ts (1)
plugins(17-19)api/src/unraid-api/config/api-config.module.ts (1)
apiConfig(45-45)
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
41-41: LGTM: Proper dependency injection.The constructor correctly injects ConfigService following NestJS patterns.
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : 'Unknown error while restarting API.'; | ||
| this.logger.error('Failed to restart API: %s', message); | ||
| } |
There was a problem hiding this comment.
Major: Failed restart leaves system in inconsistent state.
If the restart fails, the configuration has been updated to remove the plugin, but the current running instance still has the plugin loaded in memory. This creates an inconsistency between the persisted configuration and the running system state.
Consider adding recovery logic:
- Restore the original config if restart fails
- Set a flag or state indicating manual intervention is needed
- Provide clear guidance to operators on how to complete the removal manually
pujitm
force-pushed
the
fix/connect-install
branch
from
f294678 to
4d3c810
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/unraid-api-plugin-connect/src/index.ts (2)
74-78: Self-restart during module initialization remains problematic.As flagged in the previous review, triggering an API restart from within
onModuleInit()creates race conditions and lifecycle issues:
- The module may be mid-initialization when restart occurs
- The restart command lacks proper oversight of the current process lifecycle
- No mechanism to verify restart completed successfully
The concern raised previously about using a signal file, controlled shutdown mechanism, or supervisor-based restart remains valid.
79-85: Failed restart leaves system in inconsistent state.As noted in the previous review, if the restart fails after the configuration has been updated (by the CLI command), the running instance will still have the plugin loaded in memory while the persisted configuration has it removed. This creates an inconsistency between the runtime state and the persisted configuration.
The previous suggestion to add recovery logic remains valid:
- Restore the original config if restart fails, or
- Set a flag indicating manual intervention is needed, or
- Provide clear guidance to operators on how to complete the removal manually
🧹 Nitpick comments (2)
packages/unraid-api-plugin-connect/src/index.ts (2)
40-41: Remove unnecessary empty constructor.TypeScript and NestJS do not require an explicit empty constructor. You can safely remove these lines.
- - constructor() {}
52-56: Consider removing redundant execa options.The
extendEnv: trueoption is the default behavior in execa 9.6.0, making it redundant. Additionally,shell: 'bash'is likely unnecessary for these simple command executions—execa can execute them directly without spawning a shell, which is more secure and efficient.const { stdout, stderr } = await execa( 'unraid-api', ['plugins', 'remove', '-b', 'unraid-api-plugin-connect', '--no-restart'], - { shell: 'bash', extendEnv: true } );await execa('unraid-api', ['restart'], { - shell: 'bash', - extendEnv: true, });Also applies to: 75-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/index.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
43-69: Config persistence is properly implemented viaapiConfigPersistence.persist()— no changes needed.The CLI command at line 135 of
api/src/unraid-api/cli/plugins/plugin.command.tsexplicitly callsawait this.apiConfigPersistence.persist()after removing the plugin. This method writes the updated configuration to disk viaConfigFileHandler.writeConfigFile()before the CLI process returns. Since your code awaits theexeca()call, the configuration is persisted to disk before execution continues. The--no-restartflag prevents any restart loop. The implementation is correct.
| // Wait a bit for any potential file system flushes or other ops to settle | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
Replace arbitrary delay with proper synchronization.
The 1-second delay is an unreliable synchronization mechanism. If you're waiting for configuration persistence or filesystem operations to complete, you should instead:
- Ensure the CLI command's promise resolution indicates persistence is complete, or
- Explicitly verify the configuration file has been updated before proceeding, or
- Use proper filesystem sync operations (e.g., fsync) if needed
Arbitrary time-based delays can fail under load, on slower systems, or due to timing variations, leading to race conditions.
Example of explicit verification:
// After CLI removal completes, verify the config file reflects the change
const configPath = '/path/to/api/config.json'; // adjust path as needed
const configContent = await fs.promises.readFile(configPath, 'utf-8');
const config = JSON.parse(configContent);
if (config.api?.plugins?.includes('unraid-api-plugin-connect')) {
throw new Error('Plugin removal was not persisted to config file');
}
this.logger.log('Verified plugin removal persisted to disk');🤖 Prompt for AI Agents
In packages/unraid-api-plugin-connect/src/index.ts around lines 71-72, replace
the arbitrary 1s setTimeout with deterministic synchronization: after the CLI
removal promise resolves, either rely on the CLI to guarantee persistence
(ensure you await its returned promise) or explicitly verify the config file has
been updated by reading and parsing the config file and checking the plugin
entry is gone, retrying with short backoff until success or a sensible timeout;
alternatively, open the file descriptor and call fsync to ensure data is flushed
before proceeding. Ensure the code throws a clear error on timeout/failure and
logs successful verification instead of using a fixed delay.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/unraid-api-plugin-connect/src/index.ts (3)
40-41: Remove unnecessary empty constructor.The empty constructor adds no value and should be removed. TypeScript automatically provides a default constructor when none is defined.
Apply this diff:
- - constructor() {} - async onModuleInit() {
52-56: Remove unnecessaryshell: 'bash'option.The
shell: 'bash'option is likely unnecessary for this straightforward command execution. Execa can execute the command directly without spawning a shell, which is more secure and efficient. TheextendEnv: trueoption is also the default and can be omitted.Apply this diff:
const { stdout, stderr } = await execa( 'unraid-api', - ['plugins', 'remove', '-b', 'unraid-api-plugin-connect', '--no-restart'], - { shell: 'bash', extendEnv: true } + ['plugins', 'remove', '-b', 'unraid-api-plugin-connect', '--no-restart'] );
43-86: Clarify the intended behavior and document required operator action.Since the restart logic is commented out, operators need clear guidance on what happens after this automatic removal:
- Update the warning message (line 46-49) to inform operators that a manual restart is required to fully unload the plugin
- Consider adding a success message that explicitly tells operators to run
unraid-api restartmanually- Document the decision: Add a comment explaining why automatic restart was removed and manual intervention is preferred
Example enhanced logging:
this.logger.warn( 'Connect plugin is not installed, but is listed as an API plugin. ' + 'Will attempt to remove it from configuration. ' + 'A manual API restart will be required to fully unload the plugin.' ); // ... after successful removal ... this.logger.log( 'Successfully removed stale connect plugin entry from configuration. ' + 'Please run `unraid-api restart` to fully unload the plugin from memory.' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/index.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
55-58: Duplicate: Arbitrary delay should be removed.This 1-second delay is unreliable synchronization. The comment claims it allows "config subscription to flush to disk," but
configService.set()does not trigger asynchronous persistence. Since the in-memory config update should be removed (as noted in the previous comment), this delay is unnecessary.
🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
43-84: Consider the implications of plugin removal without restart.The module attempts to remove itself from the configuration during initialization, but without restarting, the plugin remains loaded in memory for the current session. This creates a temporary inconsistency where:
- The configuration no longer lists the plugin (after CLI succeeds)
- The plugin module is still active in the current process
- Future startups will correctly exclude the plugin
If this behavior is intentional (automatic cleanup without service disruption), consider adding a log message to clarify that the plugin will be fully removed on next restart. If immediate removal is required, you'll need to implement a graceful restart mechanism.
Example clarifying log message:
this.logger.log( 'Successfully completed `%s` to prune the stale connect plugin entry. Plugin will be fully unloaded on next API restart.', removalCommand );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/index.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🧬 Code graph analysis (1)
packages/unraid-api-plugin-connect/src/index.ts (2)
api/src/unraid-api/config/api-config.module.ts (1)
apiConfig(45-45)api/src/unraid-api/plugin/plugin-management.service.ts (1)
plugins(17-19)
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
41-41: LGTM: ConfigService injection.The constructor properly injects ConfigService, which is necessary for reading and updating the API plugin configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/unraid-api-plugin-connect/src/index.ts (3)
63-67: Remove unnecessaryshell: 'bash'option.The
shell: 'bash'option is unnecessary for this straightforward command execution. The command doesn't use shell-specific features (like&&,||, or pipes), so direct execution is preferred for better performance and security.Apply this diff:
const { stdout, stderr } = await execa( 'unraid-api', ['plugins', 'remove', '-b', 'unraid-api-plugin-connect', '--no-restart'], - { shell: 'bash', extendEnv: true } + { extendEnv: true } );
52-60: Critical: In-memory config update is not persisted and may be unnecessary.The code updates in-memory configuration via
configService.set(), but NestJS ConfigService does not persist changes to disk. The subsequent CLI command with--no-restartshould handle both the plugin removal and configuration persistence. This in-memory update creates potential inconsistencies:
- If the CLI succeeds, it will persist the correct configuration, making this update redundant
- If the CLI fails, the in-memory state shows the plugin removed while disk config still contains it
- The 1-second delay is arbitrary and unreliable for synchronization
Consider removing this in-memory update entirely and relying on the CLI command. If you need to update in-memory state to reflect the CLI's changes, do so after the CLI succeeds.
Based on learnings from past reviews about config persistence issues.
88-92: Fix inconsistent error messages.The error messages reference "restarting API" but the command uses
--no-restart, so it only removes the plugin without restarting. The error handling should accurately describe what failed.Apply this diff:
} catch (error) { const message = - error instanceof Error ? error.message : 'Unknown error while restarting API.'; - this.logger.error('Failed to restart API: %s', message); + error instanceof Error ? error.message : 'Unknown error during plugin removal.'; + this.logger.error('Failed to run `%s`: %s', removalCommand, message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/index.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-02-18T21:39:10.313Z
Learnt from: pujitm
Repo: unraid/api PR: 1145
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:57-60
Timestamp: 2025-02-18T21:39:10.313Z
Learning: When using Node.js `rm` with `{ force: true }` option or Unix/Linux `rm -f`, existence checks are redundant as the command continues silently if the file doesn't exist.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🧬 Code graph analysis (1)
packages/unraid-api-plugin-connect/src/index.ts (2)
api/src/unraid-api/config/api-config.module.ts (1)
apiConfig(45-45)api/src/unraid-api/plugin/plugin-management.service.ts (1)
plugins(17-19)
🔇 Additional comments (3)
packages/unraid-api-plugin-connect/src/index.ts (3)
11-11: LGTM: Import needed for scheduled method.The import of
Timeoutfrom@nestjs/scheduleis required for the@Timeout(1_000)decorator used on line 44.
42-42: LGTM: ConfigService injection.The constructor injection of
ConfigServiceis necessary for the plugin removal logic and follows NestJS best practices.
44-50: LGTM: Method initialization and warning.The
@Timeout(1_000)decorator appropriately delays the removal attempt until after module initialization completes. The warning message clearly communicates the automatic removal attempt.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/unraid-api-plugin-connect/src/index.ts (3)
3-61: Avoid in‑memoryapimutation + fixed delay; rely on CLI for persistenceThis block updates
api.pluginsin memory, waits 1s “to flush to disk,” then runs the CLI which is what actually persists config. With the customConfigService.set()here being in‑memory only, this has a few issues:
- If the CLI fails, runtime state now shows the plugin removed while disk config still contains it.
- If the CLI succeeds, the pre‑CLI
set()and 1s sleep are redundant.- The fixed 1s delay is a brittle, non-deterministic way to synchronize with any background persistence.
Given the CLI is the source of truth for plugin removal and config persistence, consider removing this in‑memory mutation and delay (or, if you really need to mirror the CLI result in memory, do it only after a successful CLI run).
A minimal fix is to drop this whole block:
- const apiConfig = this.configService.get('api') || {}; - const plugins = apiConfig.plugins || []; - const updatedPlugins = plugins.filter((p: string) => p !== 'unraid-api-plugin-connect'); - this.configService.set('api', { ...apiConfig, plugins: updatedPlugins }); - - // Allow config subscription to flush to disk (25ms buffer + margin) - await new Promise((resolve) => setTimeout(resolve, 1000)); - const debugPlugins = this.configService.get<string[]>('api.plugins') || []; - this.logger.debug('Plugins before running removal command: %o', debugPlugins);and rely on the CLI (and whatever persists
apiconfig) instead. If you still want pre‑removal diagnostics, logapi.pluginswithout mutating it first.Based on learnings that this
ConfigService.set()is in‑memory only.
64-68: Consider droppingshell: 'bash'for theexecacallThe
execainvocation passes a simple command plus argument list and doesn’t appear to use any shell features. Running viashell: 'bash'adds overhead and broadens the attack surface unnecessarily.Unless
unraid-apispecifically requires a shell, you can simplify to:- const { stdout, stderr } = await execa( - 'unraid-api', - ['plugins', 'remove', '-b', 'unraid-api-plugin-connect', '--no-restart'], - { shell: 'bash', extendEnv: true } - ); + const { stdout, stderr } = await execa( + 'unraid-api', + ['plugins', 'remove', '-b', 'unraid-api-plugin-connect', '--no-restart'], + { extendEnv: true } + );Please confirm in your environment/docs that
unraid-apidoesn’t depend on being launched via a shell.
98-99: Align error message with actual operation (plugin removal, not restart)The catch block still talks about “restarting API,” but this code only runs the plugin removal CLI with
--no-restart. The log message is misleading.Consider updating it to reflect the actual failure:
- const message = - error instanceof Error ? error.message : 'Unknown error while restarting API.'; - this.logger.error('Failed to restart API: %s', message); + const message = + error instanceof Error ? error.message : 'Unknown error during plugin removal.'; + this.logger.error('Failed to run `%s`: %s', removalCommand, message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/index.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-02-18T21:39:10.313Z
Learnt from: pujitm
Repo: unraid/api PR: 1145
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:57-60
Timestamp: 2025-02-18T21:39:10.313Z
Learning: When using Node.js `rm` with `{ force: true }` option or Unix/Linux `rm -f`, existence checks are redundant as the command continues silently if the file doesn't exist.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom ConfigService implementation that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-03-13T16:17:55.820Z
Learnt from: elibosley
Repo: unraid/api PR: 1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🧬 Code graph analysis (1)
packages/unraid-api-plugin-connect/src/index.ts (2)
api/src/unraid-api/config/api-config.module.ts (1)
apiConfig(45-45)api/src/unraid-api/plugin/plugin-management.service.ts (1)
plugins(17-19)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.ts (2)
35-39: Consider replacing the arbitrary startup delay with proper synchronization.The 1-second delay uses an arbitrary timeout to work around parallel startup tasks. While the comment explains the reasoning, this approach could fail if startup takes longer or creates unnecessary delays when startup completes faster.
Consider using NestJS module initialization ordering (e.g., importing dependent modules first) or implementing a condition-based wait that checks for specific initialization markers rather than relying on a fixed delay.
80-94: Remove unnecessaryshell: 'bash'option.The
shell: 'bash'option on line 92 is unnecessary for this straightforward command execution.execacan executeunraid-apidirectly without spawning a shell, which is more efficient and cross-platform.Apply this diff:
const { stdout, stderr } = await execa('unraid-api', removalCommand.split(' ').slice(1), { - shell: 'bash', extendEnv: true, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/unraid-api-plugin-connect/src/__test__/connect-plugin.service.test.ts(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(2 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.utils.ts(1 hunks)packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.utils.tspackages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.tspackages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-05-08T19:28:54.365Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:19-24
Timestamp: 2025-05-08T19:28:54.365Z
Learning: The directory `/usr/local/emhttp/plugins/dynamix.my.servers` is a valid directory that exists as part of the Unraid API plugin installation and should be included in verification checks.
Applied to files:
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.utils.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
Repo: unraid/api PR: 1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
packages/unraid-api-plugin-connect/src/__test__/connect-plugin.service.test.ts
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-02-18T21:39:10.313Z
Learnt from: pujitm
Repo: unraid/api PR: 1145
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:57-60
Timestamp: 2025-02-18T21:39:10.313Z
Learning: When using Node.js `rm` with `{ force: true }` option or Unix/Linux `rm -f`, existence checks are redundant as the command continues silently if the file doesn't exist.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-09-04T15:26:34.416Z
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-01-30T19:38:02.478Z
Learnt from: pujitm
Repo: unraid/api PR: 1075
File: unraid-ui/src/register.ts:15-34
Timestamp: 2025-01-30T19:38:02.478Z
Learning: In the web components registration process for unraid-ui, use a soft-fail approach (logging warnings/errors) instead of throwing errors, to ensure other components can still register successfully even if one component fails.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
Repo: unraid/api PR: 942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-03-13T16:17:55.820Z
Learnt from: elibosley
Repo: unraid/api PR: 1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🧬 Code graph analysis (2)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.ts (2)
packages/unraid-api-plugin-connect/src/config/connect.config.ts (1)
ConfigType(187-190)packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.utils.ts (1)
isConnectPluginInstalled(7-12)
packages/unraid-api-plugin-connect/src/index.ts (1)
packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts (1)
Module(15-34)
🔇 Additional comments (5)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.utils.ts (1)
7-12: LGTM!The utility function correctly checks plugin installation status synchronously, which is appropriate for module load-time decisions. The environment variable escape hatch is useful for development.
packages/unraid-api-plugin-connect/src/unraid-connect/connect.module.ts (1)
13-13: LGTM!ConnectPluginService is correctly wired as a provider in the module. Keeping it internal (not exported) is appropriate for a lifecycle management service.
Also applies to: 23-23
packages/unraid-api-plugin-connect/src/__test__/connect-plugin.service.test.ts (1)
1-100: LGTM!The test suite correctly uses Vitest (with
vimocking per project standards) and provides good coverage of the service's lifecycle and pruning behaviors. The tests appropriately verify behavior based on plugin installation status and theshouldRestartparameter.packages/unraid-api-plugin-connect/src/index.ts (1)
7-8: LGTM!The refactoring successfully moves complex plugin management logic from module initialization to the dedicated
ConnectPluginService, resulting in cleaner module structure and better separation of concerns. The simplifiedDisabledConnectPluginModuleappropriately delegates lifecycle management to the service.Also applies to: 36-39, 46-46
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.ts (1)
24-27: LGTM!The
onModuleDestroyhook appropriately handles cleanup when the module is destroyed, avoiding a restart since the process is already shutting down.
| async pruneStaleConnectPluginEntry({ shouldRestart = true }: { shouldRestart?: boolean } = {}) { | ||
| if (isConnectPluginInstalled()) { | ||
| this.logger.warn( | ||
| 'Connect plugin is installed, skipping pruning. This should not have been invoked.' | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // First, correct the in-memory config, so we don't persist an incorrect config when this api instance shuts down. | ||
| this.removeConnectPluginFromNestConfig(); | ||
|
|
||
| // Then, allow config subscription to flush to disk (25ms buffer + margin) | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
|
|
||
| // Finally, remove the stale plugin entry. | ||
| await this.removeConnectPluginFromBootConfig(shouldRestart); | ||
| } |
There was a problem hiding this comment.
Replace arbitrary config flush delay with explicit persistence.
The 1-second setTimeout at line 61 assumes configuration changes will be persisted within that timeframe, but there's no explicit persistence call or verification. The comment mentions "25ms buffer + margin," yet uses 1000ms—a massive margin that still provides no guarantee.
Issues:
- No explicit persistence:
configService.set()on line 58 (viaremoveConnectPluginFromNestConfig) updates in-memory state only. Per project learnings, NestJS ConfigService doesn't automatically persist changes to disk. - Race condition: If
removeConnectPluginFromBootConfigexecutes before persistence completes, and if a restart occurs, the API will reload the old configuration containing the plugin entry, potentially causing a restart loop. - Arbitrary timeout: 1 second provides no deterministic guarantee, especially under system load.
Solution: Explicitly invoke the persistence mechanism (e.g., ConnectConfigPersister or equivalent) and await its completion before proceeding to line 64.
Based on learnings about NestJS ConfigService.set() and past review feedback on config persistence.
🤖 Prompt for AI Agents
In
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.ts
around lines 49 to 65, the method uses an arbitrary 1s setTimeout to wait for an
in-memory config change to "flush" but never explicitly persists the change,
risking a race and restart loop; replace the timeout by calling and awaiting the
project's config persistence API (e.g., ConnectConfigPersister.persist/save or
the equivalent persistence method) after removeConnectPluginFromNestConfig
returns, or make removeConnectPluginFromNestConfig return a Promise that
performs the persistence, then await that Promise before calling
removeConnectPluginFromBootConfig(shouldRestart) so that disk persistence is
guaranteed before proceeding.
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : 'Unknown error while restarting API.'; | ||
| this.logger.error('Failed to restart API: %s', message); | ||
| } |
There was a problem hiding this comment.
Fix misleading error messages.
The error messages reference "restarting API" and "while restarting API," but when shouldRestart is false, the command uses --no-restart and doesn't restart at all. The messages should accurately reflect that the plugin removal failed, not specifically the restart.
Apply this diff:
} catch (error) {
const message =
- error instanceof Error ? error.message : 'Unknown error while restarting API.';
- this.logger.error('Failed to restart API: %s', message);
+ error instanceof Error ? error.message : 'Unknown error during plugin removal.';
+ this.logger.error('Failed to remove plugin via `%s`: %s', removalCommand, message);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/unraid-api-plugin-connect/src/unraid-connect/connect-plugin.service.ts
around lines 121 to 125: the catch message incorrectly says "restarting API"
even when shouldRestart is false (command uses --no-restart), so change the
error text to reflect plugin removal failure; update the const message and
logger.error call to use a neutral description like "Failed to remove plugin"
(include the actual error.message when available) so logs correctly describe the
failure regardless of restart behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/src/connect-plugin-cleanup.ts (1)
24-38: Remove unnecessaryshell: 'bash'from execa and simplify command constructionFor this straightforward
unraid-apiinvocation, spawning an extra Bash shell is unnecessary and reintroduces a pattern that was previously removed elsewhere. You can invoke the CLI directly with explicit args, which is simpler, faster, and has less surface area:- let removalCommand = 'unraid-api plugins remove -b unraid-api-plugin-connect'; - if (!shouldRestart) { - removalCommand += ' --no-restart'; - } - return await execa('unraid-api', removalCommand.split(' ').slice(1), { - shell: 'bash', - extendEnv: true, - }); + const args = ['plugins', 'remove', '-b', 'unraid-api-plugin-connect']; + if (!shouldRestart) { + args.push('--no-restart'); + } + return await execa('unraid-api', args, { + extendEnv: true, + });This preserves behavior while avoiding the shell dependency.
api/src/index.ts (1)
16-16: Avoid restarting the API from within its own startup path
pruneStaleConnectPluginEntryIfNecessary()defaultsshouldRestart = true, so calling it here can trigger a self‑initiatedunraid-apirestart duringviteNodeApp, recreating the lifecycle/race risks you previously had inonModuleInit. For a startup‑time cleanup/migration, you can safely prune the stale plugin entry without restarting and let this process continue booting.Consider explicitly disabling restart at this call site:
- await pruneStaleConnectPluginEntryIfNecessary(); + await pruneStaleConnectPluginEntryIfNecessary({ shouldRestart: false });This keeps cleanup behavior while avoiding self‑managed restarts from inside the API process.
Also applies to: 64-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/connect-plugin-cleanup.ts(1 hunks)api/src/index.ts(2 hunks)packages/unraid-api-plugin-connect/src/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
api/src/connect-plugin-cleanup.tspackages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-02-18T21:39:10.313Z
Learnt from: pujitm
Repo: unraid/api PR: 1145
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:57-60
Timestamp: 2025-02-18T21:39:10.313Z
Learning: When using Node.js `rm` with `{ force: true }` option or Unix/Linux `rm -f`, existence checks are redundant as the command continues silently if the file doesn't exist.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-09-04T15:26:34.416Z
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-01-30T19:38:02.478Z
Learnt from: pujitm
Repo: unraid/api PR: 1075
File: unraid-ui/src/register.ts:15-34
Timestamp: 2025-01-30T19:38:02.478Z
Learning: In the web components registration process for unraid-ui, use a soft-fail approach (logging warnings/errors) instead of throwing errors, to ensure other components can still register successfully even if one component fails.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
Repo: unraid/api PR: 942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
Repo: unraid/api PR: 1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom ConfigService implementation that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
packages/unraid-api-plugin-connect/src/index.ts
🧬 Code graph analysis (2)
api/src/index.ts (1)
api/src/connect-plugin-cleanup.ts (1)
pruneStaleConnectPluginEntryIfNecessary(24-39)
packages/unraid-api-plugin-connect/src/index.ts (1)
packages/unraid-api-plugin-generator/src/templates/index.ts (1)
ApiModule(28-28)
🔇 Additional comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
30-30: Static ApiModule export looks goodExporting
ApiModuleasConnectPluginModulealigns with the generator template and removes the dynamic/disabled fallback complexity. Just ensure there are no remaining imports of the oldDisabledConnectPluginModuleorisConnectPluginInstalledelsewhere in the repo or tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { type NestFastifyApplication } from '@nestjs/platform-fastify'; | ||
| import { unlinkSync } from 'fs'; | ||
| import { mkdir } from 'fs/promises'; | ||
| import { mkdir, readFile } from 'fs/promises'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 hours ago
The best way to fix this problem is to remove the unused readFile import from line 7 in api/src/index.ts. This should be done by editing the import statement so that only the actually used mkdir named import remains. No other changes need to be made elsewhere in the code since readFile is never referenced.
| @@ -4,7 +4,7 @@ | ||
|
|
||
| import { type NestFastifyApplication } from '@nestjs/platform-fastify'; | ||
| import { unlinkSync } from 'fs'; | ||
| import { mkdir, readFile } from 'fs/promises'; | ||
| import { mkdir } from 'fs/promises'; | ||
| import http from 'http'; | ||
| import https from 'https'; | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/src/unraid-api/config/api-config.module.ts (2)
33-39: Consider refactoring: Side effect in a "load" function violates single responsibility.The
loadApiConfigfunction name suggests it only reads configuration, but it now also modifies the config file on disk. While the comment acknowledges this as a "Hack," this approach:
- Violates the principle of least surprise
- Makes the function harder to test and reason about
- Could cause issues if called in unexpected contexts
Consider one of these alternatives:
- Move the cleanup logic to
onApplicationBootstrapinApiConfigPersistenceclass- Create a separate cleanup function that's explicitly called during startup
- Add a separate cleanup pass in the main bootstrap sequence
This would maintain clearer separation between loading and cleanup operations.
33-33: Improve documentation: Explain the "Hack" comment more thoroughly.The comment acknowledges this is a "Hack" but doesn't explain why it's necessary, what the ideal solution would be, or what problem it solves. More context would help future maintainers understand the tradeoffs.
Consider expanding the comment to something like:
// HACK: Auto-cleanup stale connect plugin entry during config load. // This runs here (outside NestJS DI) to ensure the plugin list is clean // before module initialization. Ideally, this would be handled by a dedicated // cleanup service that runs during bootstrap, but that would require refactoring // the plugin loading order.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/connect-plugin-cleanup.ts(1 hunks)api/src/index.ts(1 hunks)api/src/unraid-api/config/api-config.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/index.ts
- api/src/connect-plugin-cleanup.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-03-14T16:10:38.562Z
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: api/src/graphql/schema/types/connect/connect.graphql:142-146
Timestamp: 2025-03-14T16:10:38.562Z
Learning: The updateApiSettings mutation in api/src/unraid-api/graph/connect/connect.resolver.ts is protected with the UsePermissions decorator that requires UPDATE permission on the CONFIG resource.
Applied to files:
api/src/unraid-api/config/api-config.module.ts
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
api/src/unraid-api/config/api-config.module.ts
🧬 Code graph analysis (1)
api/src/unraid-api/config/api-config.module.ts (1)
api/src/connect-plugin-cleanup.ts (1)
isConnectPluginInstalled(7-12)
🔇 Additional comments (2)
api/src/unraid-api/config/api-config.module.ts (2)
9-9: LGTM: Import statement is correct.The imported function
isConnectPluginInstalledis properly defined in the cleanup module and the import path is correct.
38-38: Type assertion justifiably raises concern about missing required fields.The type assertion
as ApiConfigon line 38 does bypass type safety. WhileloadConfig()theoretically returns a fullApiConfigwith defaults merged,diskConfigis explicitly typed asPartial<ApiConfig>, and after the optional-chaining filter onplugins, there's no guarantee all required fields are present.The codebase demonstrates the correct pattern elsewhere: merge with defaults before writing (lines 50-53). The problematic code should either:
- Merge with defaults before writing:
await apiHandler.writeConfigFile({...createDefaultConfig(), ...diskConfig})- Update
writeConfigFilesignature to acceptPartial<ApiConfig>with proper merging logic, as shown inupdateConfigThe assertion hides the risk that required fields could be missing when persisted to disk.
| // Hack: cleanup stale connect plugin entry if necessary | ||
| if (!isConnectPluginInstalled()) { | ||
| diskConfig.plugins = diskConfig.plugins?.filter( | ||
| (plugin) => plugin !== 'unraid-api-plugin-connect' | ||
| ); | ||
| await apiHandler.writeConfigFile(diskConfig as ApiConfig); | ||
| } |
There was a problem hiding this comment.
Add error handling for the config write operation.
The writeConfigFile operation can fail (e.g., due to disk I/O errors or permission issues), but there's no error handling in place. Since this runs during application startup outside the NestJS DI container, an unhandled error could crash the application before proper error handling is initialized.
Consider adding error handling:
// Hack: cleanup stale connect plugin entry if necessary
if (!isConnectPluginInstalled()) {
- diskConfig.plugins = diskConfig.plugins?.filter(
- (plugin) => plugin !== 'unraid-api-plugin-connect'
- );
- await apiHandler.writeConfigFile(diskConfig as ApiConfig);
+ const originalPlugins = diskConfig.plugins ?? [];
+ const cleanedPlugins = originalPlugins.filter(
+ (plugin) => plugin !== 'unraid-api-plugin-connect'
+ );
+ if (cleanedPlugins.length !== originalPlugins.length) {
+ diskConfig.plugins = cleanedPlugins;
+ try {
+ await apiHandler.writeConfigFile(diskConfig as ApiConfig);
+ } catch (error) {
+ console.error('Failed to cleanup stale connect plugin entry:', error);
+ // Continue with startup even if cleanup fails
+ }
+ }
}🤖 Prompt for AI Agents
In api/src/unraid-api/config/api-config.module.ts around lines 33 to 39, the
call to await apiHandler.writeConfigFile(diskConfig as ApiConfig) is unprotected
and can throw during startup; wrap this call in a try/catch, catch any error,
log the error details (e.g., console.error with a clear message and the error
object) and avoid rethrowing so startup doesn't crash; if you need to treat this
as fatal, log the error then call process.exit(1) explicitly—otherwise just log
and continue.
Optimize: Only write config if the plugin was actually removed.
The current implementation writes to disk even if the 'unraid-api-plugin-connect' entry was not present in the plugins array. This causes unnecessary I/O operations on every startup when the plugin is not installed but also not in the config.
Apply this diff to optimize the write operation:
// Hack: cleanup stale connect plugin entry if necessary
if (!isConnectPluginInstalled()) {
- diskConfig.plugins = diskConfig.plugins?.filter(
- (plugin) => plugin !== 'unraid-api-plugin-connect'
- );
- await apiHandler.writeConfigFile(diskConfig as ApiConfig);
+ const originalPlugins = diskConfig.plugins ?? [];
+ const cleanedPlugins = originalPlugins.filter(
+ (plugin) => plugin !== 'unraid-api-plugin-connect'
+ );
+ if (cleanedPlugins.length !== originalPlugins.length) {
+ diskConfig.plugins = cleanedPlugins;
+ await apiHandler.writeConfigFile(diskConfig as ApiConfig);
+ }
}🤖 Prompt for AI Agents
In api/src/unraid-api/config/api-config.module.ts around lines 33 to 39, the
code always writes diskConfig back to disk even when 'unraid-api-plugin-connect'
wasn't present; change the logic to check whether the plugin was actually
removed before calling writeConfigFile: compute the filtered plugins array,
compare its length (or use includes to detect presence) against the original
plugins, and only assign diskConfig.plugins and await
apiHandler.writeConfigFile(diskConfig as ApiConfig) when the filtered array
differs from the original (i.e., the plugin existed and was removed).
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.27.0](v4.26.2...v4.27.0) (2025-11-19) ### Features * remove Unraid API log download functionality ([#1793](#1793)) ([e4a9b82](e4a9b82)) ### Bug Fixes * auto-uninstallation of connect api plugin ([#1791](#1791)) ([e734043](e734043)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Refactor
Chores