chore: include node_modules in api slackware package #1415
Conversation
WalkthroughThe build process was simplified by removing the creation, archiving, and movement of a compressed Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 0
🧹 Nitpick comments (1)
api/scripts/build.ts (1)
128-128: Confirm intentional retention of node_modules
The removal ofnode_moduleshas been commented out, so the directory will persist in the pack and release artifacts. Ensure this aligns with the Slackware packaging expectations.Consider toggling this behavior via an env var (e.g.,
KEEP_NODE_MODULES) to keep builds reproducible:-// await $`rm -rf node_modules`; +if (!process.env.KEEP_NODE_MODULES) { + await $`rm -rf node_modules`; +} else { + console.log('Retaining node_modules as per KEEP_NODE_MODULES flag'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/scripts/build.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build Unraid UI Library (Webcomponent Version)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
218-222:VENDOR_ARCHIVEis now dead code—drop it to avoid driftThe vendor-store
<FILE>stanza was removed, but the shell variable persists:VENDOR_ARCHIVE="/boot/config/plugins/dynamix.my.servers/&vendor_store_filename;"It’s never referenced below, so keeping it around:
- confuses future readers,
- risks someone assuming the archive is still used, and
- slightly inflates the env.
Safe to delete these two lines altogether.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build Unraid UI Library (Webcomponent Version)
- GitHub Check: Cloudflare Pages
| # Clean up any old node_modules archives (on the boot drive) that don't match our current version | ||
| /etc/rc.d/rc.unraid-api cleanup-dependencies | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard rc.unraid-api cleanup-dependencies with existence & exit-status checks
/etc/rc.d/rc.unraid-api might be missing or return a non-zero status (e.g., corrupted script, permission issue).
Right now the installer would sail past the failure silently and continue, leaving stale archives behind or, worse, terminating later with a less obvious error.
-# Clean up any old node_modules archives (on the boot drive) that don't match our current version
-/etc/rc.d/rc.unraid-api cleanup-dependencies
+# Clean up any old node_modules archives (on the boot drive) that don't match our current version
+if [ -x "/etc/rc.d/rc.unraid-api" ]; then
+ /etc/rc.d/rc.unraid-api cleanup-dependencies || \
+ echo "⚠️ Dependency cleanup failed, continuing install anyway"
+else
+ echo "⚠️ /etc/rc.d/rc.unraid-api not found or not executable, skipping dependency cleanup"
+fiMinimal defensive code prevents broken upgrades on edge systems without adding much complexity.
🤖 Prompt for AI Agents
In plugin/plugins/dynamix.unraid.net.plg around lines 234 to 236, the call to
/etc/rc.d/rc.unraid-api cleanup-dependencies lacks checks for the script's
existence and its exit status. To fix this, first verify that
/etc/rc.d/rc.unraid-api exists and is executable before running the cleanup
command. Then, after running it, check the exit status and handle any non-zero
result appropriately, such as logging an error or aborting the installation to
prevent silent failures and stale archives.
There was a problem hiding this comment.
this file is bundled with the plugin, so it will always exist
There was a problem hiding this comment.
@pujitm Got it—since the plugin ships its own /etc/rc.d/rc.unraid-api, the file’s absence isn’t a realistic scenario. Thanks for clarifying.
If you’re comfortable relying on that guarantee, feel free to keep the current line as-is. For belt-and-suspenders robustness you could still echo a warning when the cleanup command itself returns non-zero, but that’s purely optional:
# Optional: surface a failure while still proceeding
/etc/rc.d/rc.unraid-api cleanup-dependencies || \
echo "⚠️ Dependency cleanup failed (non-zero exit), continuing install"Totally fine to leave it out if you’d rather keep things concise. Let me know if you’d like me to drop the suggestion entirely.
✏️ Learnings added
Learnt from: pujitm
PR: unraid/api#1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.305Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
since node_modules are included with the slackware package now, `ensure` is much less useful, but `restore` is necessary to load any api plugins, as they must be vendored after installation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugin/builder/build-plugin.ts (1)
15-15: Deprecation dependency: verify continued import ofgetVendorBundleName.
build-vendor-store.tsis marked deprecated, yet this file still imports its public helper.
If that module is slated for removal, refactor the helper into a dedicated util before de-leting the source to avoid a future breaking change.plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/scripts/dependencies.sh (1)
310-311: Usage string outdated forredownloadarguments.The help still lists
redownloadwithout its two parameters; update to avoid user confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build-plugin.yml(0 hunks).github/workflows/main.yml(0 hunks)api/scripts/build.ts(0 hunks)plugin/builder/build-plugin.ts(3 hunks)plugin/builder/build-txz.ts(1 hunks)plugin/builder/build-vendor-store.ts(1 hunks)plugin/docker-compose.yml(0 hunks)plugin/plugins/dynamix.unraid.net.plg(1 hunks)plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api(1 hunks)plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/scripts/dependencies.sh(3 hunks)
💤 Files with no reviewable changes (4)
- .github/workflows/main.yml
- .github/workflows/build-plugin.yml
- plugin/docker-compose.yml
- api/scripts/build.ts
✅ Files skipped from review due to trivial changes (1)
- plugin/builder/build-vendor-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/plugins/dynamix.unraid.net.plg
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/scripts/dependencies.sh (1)
plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/scripts/api_utils.sh (1)
get_archive_information(28-70)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Web App
- GitHub Check: Build Unraid UI Library (Webcomponent Version)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
plugin/builder/build-plugin.ts (1)
79-81:vendor_store_urlremoved — confirm manifest & downstream scripts are aligned.You dropped the
vendor_store_urlentity but keptvendor_store_filename.
Double-check that:
dynamix.unraid.net.plgno longer declares thevendor_store_urlentity (otherwiseupdateEntityValuewill throw).- No runtime scripts still need the remote URL; if they do, leave the key with an empty value instead of deleting it.
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (1)
32-36: Service starts even when dependencies are missing – risk of runtime failure.
dependencies.sh restorenow returns success on any error; the API may start withoutnode_modules, leading to late-crash behaviour that’s harder to diagnose.Consider:
"$scripts_dir/dependencies.sh" restore || { - echo "Failed to restore API plugin dependencies! Continuing with start, but API plugins may be unavailable." -} + echo "Failed to restore API plugin dependencies! Aborting start." + return 1 +}or at least exit non-zero only when the API binary subsequently fails.
plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/scripts/dependencies.sh
Show resolved
Hide resolved
pujitm
force-pushed
the
chore/node_modules
branch
from
22529b6 to
0dff2cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
justfile (1)
32-38: Minor: collapse innerfindloop for speed & clarityRepeatedly spawning a subshell for each
build_diris unnecessary. A singlefindcan cover all patterns:-build_dirs=("dist" ".nuxt" ".output" "coverage" "deploy") -for build_dir in "${build_dirs[@]}"; do - find . -name "$build_dir" -type d | while read dir; do - echo "Removing: $dir" - rm -rf "$dir" - done -done +find . \( -name dist -o -name .nuxt -o -name .output -o -name coverage -o -name deploy \) \ + -type d -print0 | while IFS= read -r -d '' dir; do + echo "Removing: $dir" + rm -rf "$dir" +doneFewer
findinvocations and null-delimited handling for robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
justfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
1 participant
Summary by CodeRabbit
node_modulesdirectory, removing compression and archiving steps.node_modulesdirectories and common build artifact folders for easier maintenance.