fix(connect): disable api plugin if unraid plugin is absent #1773
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRuntime gating was added so importing ApiModule chooses either the real ConnectPluginModule or a new DisabledConnectPluginModule based on a filesystem check, with a development env var SKIP_CONNECT_PLUGIN_CHECK to override the check. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Runtime as packages/.../src/index.ts
participant Env as Environment
participant FS as File System
App->>Runtime: import ApiModule
Runtime->>Env: read SKIP_CONNECT_PLUGIN_CHECK
alt SKIP_CONNECT_PLUGIN_CHECK == true
Runtime->>Runtime: select ConnectPluginModule (env override)
else
Runtime->>FS: existsSync(pluginPath)?
alt plugin exists
Runtime->>Runtime: select ConnectPluginModule
else plugin missing
Runtime->>Runtime: select DisabledConnectPluginModule
Runtime->>App: console.warn("Connect plugin not installed") on init
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
|
Discovered a more significant issue: the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1773 +/- ##
==========================================
- Coverage 52.75% 52.23% -0.52%
==========================================
Files 868 872 +4
Lines 49708 50219 +511
Branches 5005 5009 +4
==========================================
+ Hits 26224 26234 +10
- Misses 23410 23909 +499
- Partials 74 76 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
api/.env.development(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
packages/unraid-api-plugin-connect/src/index.ts
🧠 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-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
📚 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 (1)
packages/unraid-api-plugin-connect/src/index.ts (3)
api/src/unraid-api/plugin/plugin.module.ts (2)
Module(11-37)Module(39-65)packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (1)
isConnectPluginInstalled(73-75)packages/unraid-api-plugin-generator/src/templates/index.ts (1)
ApiModule(28-28)
🔇 Additional comments (4)
api/.env.development (1)
35-35: LGTM! Development override is appropriately placed.The
SKIP_CONNECT_PLUGIN_CHECKenvironment variable aligns with the runtime check inpackages/unraid-api-plugin-connect/src/index.tsand allows developers to bypass the plugin presence validation during local development.packages/unraid-api-plugin-connect/src/index.ts (3)
3-3: LGTM! Import is correctly structured.The
existsSyncimport fromnode:fsis appropriate for the synchronous plugin detection logic.
12-15: Well-documented conditional initialization pattern.The comments clearly explain the rationale for the two-module approach and the development override mechanism, making the code's intent immediately clear to future maintainers.
Also applies to: 31-34, 45-47, 55-59
60-60: No issues found with the conditional export pattern.The original concern was that
DisabledConnectPluginModuleexports nothing. However, this is not a problem because:
- Both
ConnectPluginModuleandDisabledConnectPluginModuleare valid NestJS modules decorated with@Module- Downstream code in
plugin.module.tsfilters plugins by checking ifApiModuleexists, then spreads the modules into theimportsarray — it doesn't consume exports from the module- The schema validation requires
ApiModuleto be a valid class constructor, which both modules satisfy- An empty module with no exports still functions correctly when imported into NestJS
The conditional export cleanly avoids dynamic module plumbing and is safe for both the installed and disabled code paths.
|
Waiting on #1774 |
pujitm
force-pushed
the
fix/connect-init
branch
from
d8a8caf to
26a1526
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/index.ts (1)
82-87: Environment variable check is correctly implemented.The strict equality check for
'true'properly addresses the previous review concern about falsy string values.For enhanced developer ergonomics, consider accepting additional truthy values:
const isConnectPluginInstalled = () => { - if (process.env.SKIP_CONNECT_PLUGIN_CHECK === 'true') { + const skip = process.env.SKIP_CONNECT_PLUGIN_CHECK?.toLowerCase(); + if (skip === 'true' || skip === '1' || skip === 'yes') { return true; } return existsSync('/boot/config/plugins/dynamix.unraid.net.plg'); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/.env.development(1 hunks)packages/unraid-api-plugin-connect/src/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/.env.development
🧰 Additional context used
🧠 Learnings (6)
📚 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
📚 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-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: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
Repo: unraid/api PR: 1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
packages/unraid-api-plugin-connect/src/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 (1)
packages/unraid-api-plugin-connect/src/index.ts (3)
api/src/unraid-api/plugin/plugin.module.ts (2)
Module(11-37)Module(39-65)packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (1)
isConnectPluginInstalled(73-75)packages/unraid-api-plugin-generator/src/templates/index.ts (1)
ApiModule(28-28)
🔇 Additional comments (4)
packages/unraid-api-plugin-connect/src/index.ts (4)
3-5: Imports are appropriate for the new functionality.The addition of
existsSyncfor file checks andexecafor command execution are suitable choices for the runtime detection logic.
14-31: Documentation clarifies module purpose.The added comments effectively explain when the full plugin module is used, improving code maintainability.
89-94: Conditional export pattern is valid and properly implemented.Both
ConnectPluginModuleandDisabledConnectPluginModuleare properly decorated with@Moduleand follow NestJS conventions. The plugin system correctly handles the pattern by filtering plugins with anApiModuleproperty and importing them into the module graph. The static branching at module load time keeps the export surface predictable while avoiding dynamic module complexity.
40-76: Rewrite focus: The foundational claims in the review are factually incorrect based on git history.The review comment asserts that "PR comments state the
plugins removecommand is malfunctioning" and references PR #1774 as pending. However:
- PR #1774 was merged today (commit 64eb9ce) with improvements:
--bypass-npm,--npm, andconfig-onlysupport for the remove command- The command is not broken; it was just enhanced to be "scriptable"
- DisabledConnectPluginModule was added in a recent commit specifically to auto-prune desynced plugins
However, there is one valid technical concern:
- The
execacall lacks a timeout, while other calls in the codebase consistently use timeouts (10s–60s). Add{ timeout: 10000 }to prevent indefinite blocking during module initialization.Likely an incorrect or invalid review comment.
pujitm
changed the title
pujitm
changed the title
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.26.0](v4.25.3...v4.26.0) (2025-11-17) ### Features * add cpu power query & subscription ([#1745](#1745)) ([d7aca81](d7aca81)) * add schema publishing to apollo studio ([#1772](#1772)) ([7e13202](7e13202)) * add workflow_dispatch trigger to schema publishing workflow ([818e7ce](818e7ce)) * apollo studio readme link ([c4cd0c6](c4cd0c6)) * **cli:** make `unraid-api plugins remove` scriptable ([#1774](#1774)) ([64eb9ce](64eb9ce)) * use persisted theme css to fix flashes on header ([#1784](#1784)) ([854b403](854b403)) ### Bug Fixes * **api:** decode html entities before parsing notifications ([#1768](#1768)) ([42406e7](42406e7)) * **connect:** disable api plugin if unraid plugin is absent ([#1773](#1773)) ([c264a18](c264a18)) * detection of flash backup activation state ([#1769](#1769)) ([d18eaf2](d18eaf2)) * re-add missing header gradient styles ([#1787](#1787)) ([f8a6785](f8a6785)) * respect OS safe mode in plugin loader ([#1775](#1775)) ([92af3b6](92af3b6)) --- 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>
Mitigates an edge case where the connect api plugin does not uninstall itself when Unraid version < 7.2.0, resulting in retention of undesired connect functionality on stock unraid after upgrading to 7.2.0+.
Summary by CodeRabbit