plugin/source/dynamix.unraid.net/install/scripts/verify_install.sh (2)

5-6: Consider adding set -u for better error handling

The script currently uses set -e to exit on errors, but adding set -u would help catch unbound variables as well.

# Exit on errors
set -e
+set -u

---

96-108: Quote variables in the if condition

Variables in the if condition should be quoted to handle empty values correctly.

-if [ $EXEC_ERRORS -eq 0 ] && [ $DIR_ERRORS -eq 0 ]; then
+if [ "$EXEC_ERRORS" -eq 0 ] && [ "$DIR_ERRORS" -eq 0 ]; then

plugin/builder/utils/nodejs-helper.ts (2)

39-41: Use absolute path for NODE_FILENAME

The code checks if NODE_FILENAME exists, but doesn't specify a full path, which might lead to checking in the wrong directory.

-    if (!existsSync(NODE_FILENAME)) {
-      await fetchFile(NODE_URL, NODE_FILENAME);
+    const nodeFilePath = join(startingDir, NODE_FILENAME);
+    if (!existsSync(nodeFilePath)) {
+      await fetchFile(NODE_URL, nodeFilePath);

---

42-43: Add error handling for the tar extraction

The tar command execution should include error handling to manage potential extraction failures.

-    await $`tar --strip-components=1 -xf ${NODE_FILENAME} -C ${NODE_DEST}`;
+    try {
+      await $`tar --strip-components=1 -xf ${NODE_FILENAME} -C ${NODE_DEST}`;
+    } catch (error) {
+      console.error(`Failed to extract Node.js: ${error.message}`);
+      throw error;
+    }

.cursor/rules/plugin-migration-to-slackware-package.mdc (1)

6-11: Consider expanding migration guidelines

While the file establishes the migration goal, it would benefit from more detailed migration steps or resources.

Consider expanding this section with:

• Links to Slackware package documentation
• Specific migration steps or a checklist
• Examples of correctly migrated components

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh (3)

7-7: Remove or use the UNRAID_BINARY_PATH variable

The UNRAID_BINARY_PATH variable is defined but never used in the script.

Either remove this unused variable or utilize it in the script where appropriate.

🧰 Tools

🪛 Shellcheck (0.10.0)

[warning] 7-7: UNRAID_BINARY_PATH appears unused. Verify use (or export if used externally).

(SC2034)

---

18-23: Add error handling for symlink creation

The symlink creation process lacks error handling and doesn't verify successful operations.

Consider adding error checking for the symlink creation:

if [ -f usr/local/unraid-api/dist/cli.js ]; then
-  ln -sf usr/local/unraid-api/dist/cli.js usr/local/bin/unraid-api
-  ln -sf usr/local/bin/unraid-api usr/local/sbin/unraid-api
-  ln -sf usr/local/bin/unraid-api usr/bin/unraid-api
+  if ! ln -sf usr/local/unraid-api/dist/cli.js usr/local/bin/unraid-api; then
+    echo "Warning: Failed to create symlink in usr/local/bin" >&2
+  fi
+  if ! ln -sf usr/local/bin/unraid-api usr/local/sbin/unraid-api; then
+    echo "Warning: Failed to create symlink in usr/local/sbin" >&2
+  fi
+  if ! ln -sf usr/local/bin/unraid-api usr/bin/unraid-api; then
+    echo "Warning: Failed to create symlink in usr/bin" >&2
+  fi
fi

---

35-46: Simplify flash backup configuration logic

The flash backup configuration has multiple nested if statements and could be simplified for better readability.

Consider consolidating the directory creation and symlink creation:

# Configure flash backup to stop when the system starts shutting down
-if [ ! -d etc/rc.d/rc6.d ]; then
-  mkdir -p etc/rc.d/rc6.d
-fi
-
-if [ ! -h etc/rc.d/rc0.d ]; then
-  ln -sf etc/rc.d/rc6.d etc/rc.d/rc0.d
-fi
-
-if [ ! -h etc/rc.d/rc6.d/K10_flash_backup ] && [ -f etc/rc.d/rc.flash_backup ]; then
-  ln -sf etc/rc.d/rc.flash_backup etc/rc.d/rc6.d/K10_flash_backup
-fi
+# Ensure rc6.d directory exists
+mkdir -p etc/rc.d/rc6.d
+
+# Link rc0.d to rc6.d if needed
+[ ! -h etc/rc.d/rc0.d ] && ln -sf etc/rc.d/rc6.d etc/rc.d/rc0.d
+
+# Create flash backup shutdown script link if needed
+if [ ! -h etc/rc.d/rc6.d/K10_flash_backup ] && [ -f etc/rc.d/rc.flash_backup ]; then
+  ln -sf etc/rc.d/rc.flash_backup etc/rc.d/rc6.d/K10_flash_backup
+fi

plugin/source/dynamix.unraid.net/install/scripts/file_patches.sh (1)

4-13: Add error handling for nginx configuration patching

The nginx configuration patching lacks error handling and doesn't verify the success of the sed operation.

Add error handling to verify the sed command execution:

# Patch nginx config if needed
NGINX_CHANGED=0
FILE=/etc/nginx/nginx.conf
if grep -q "SAMEORIGIN" "${FILE}" >/dev/null 2>&1; then
  cp "$FILE" "$FILE-" 
  OLD="add_header X-Frame-Options 'SAMEORIGIN';" 
  NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";"
-  sed -i "s#${OLD}#${NEW}#" "${FILE}"
+  if sed -i "s#${OLD}#${NEW}#" "${FILE}"; then
+    NGINX_CHANGED=1
+    echo "Updated nginx config: replaced X-Frame-Options with Content-Security-Policy"
+  else
+    echo "Failed to update nginx config" >&2
+    cp "$FILE-" "$FILE"  # Restore from backup
+  fi
-  NGINX_CHANGED=1
fi

plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (2)

61-66: Improve restart function to ensure service is fully stopped

The restart function uses a fixed sleep duration which might not be sufficient for all scenarios.

Implement a more robust restart mechanism:

restart() {
  stop
-  sleep 2
+  # Wait for service to fully stop (up to 10 seconds)
+  local max_wait=10
+  local count=0
+  while [ $count -lt $max_wait ]; do
+    if ! "$unraid_binary_path" service status &>/dev/null; then
+      # Service is stopped
+      break
+    fi
+    sleep 1
+    count=$((count + 1))
+  done
+  
+  # If service is still running after max wait, force stop
+  if [ $count -eq $max_wait ] && "$unraid_binary_path" service status &>/dev/null; then
+    echo "Warning: Service did not stop gracefully, forcing stop..."
+    killall -9 node 2>/dev/null || true
+    sleep 1
+  fi
+  
  start
}

---

27-31: Add optimization for dependency restoration

The dependency restoration check could be optimized to avoid unnecessary file operations.

Add a check for an empty node_modules directory:

# Check if node modules need restoration
-if [ ! -d "${dependencies_dir}" ] && [ -f "${vendor_archive}" ]; then
+if { [ ! -d "${dependencies_dir}" ] || [ -z "$(ls -A "${dependencies_dir}" 2>/dev/null)" ]; } && [ -f "${vendor_archive}" ]; then
  echo "Restoring node modules from vendor archive..."
  restore_dependencies "${vendor_archive}"
fi

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/file_patches.sh (1)

1-3: Add defensive shell options for safer execution

Consider enabling set -euo pipefail (or at least set -e) right after the she-bang.
This makes the script abort on the first failing command or on an undefined variable, preventing half-applied patches that can leave the system in an inconsistent state.

 #!/bin/sh
+# Abort on error/unset vars, fail on pipe errors
+set -euo pipefail
 # Script to handle file patches

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/cleanup.sh (1)

66-70: echo "\n…": ANSI escape not interpreted

echo (BusyBox/GNU) will print the literal “\n”. Use printf '\n…' or echo with -e (GNU-only) for portable newlines.

-echo "\n**********************************"
+printf '\n**********************************\n'

plugin/plugins/dynamix.unraid.net.plg (2)

33-55: Disk-space check: make failure explicit & POSIX-portable

1. df -m fails on systems where -m is unsupported (BusyBox). Use df -P and convert.
2. exit 0 on success is redundant (the script already exits 0).
3. Surplus blank lines can be trimmed.

---

115-120: Service start should tolerate missing rc functionality

Some users disable init scripts. Consider testing /etc/rc.d/rc.unraid-api status || true before starting, or at least capturing failure to aid debugging.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/cleanup.sh (2)

8-62: Consider refactoring the long file-list for maintainability
The inline for FILE in \ … block works, but as the list grows it will be harder to update and review. You could extract the paths into a here-document or an external variable/file, for example:

 perform_file_restoration() {
-  for FILE in \
-    "/usr/local/emhttp/plugins/…/DisplaySettings.page" \
-    … 
-  do
+  # List of files to restore
+  set -- \
+    "/usr/local/emhttp/plugins/…/DisplaySettings.page" \
+    …
+  # Iterate over backups
+  for FILE; do
     [ -f "$FILE-" ] && mv -f "$FILE-" "$FILE"
   done

This keeps the function body lean and the file list easy to manage.

---

176-178: Combine sed invocations for conciseness
You’re already removing two patterns from rc.nginx. You can merge them:

-sed -i '/scripts\/makestate/d' /etc/rc.d/rc.nginx
-sed -i '/#robots.txt any origin/d' /etc/rc.d/rc.nginx
+sed -i -e '/scripts\/makestate/d' \
+       -e '/#robots.txt any origin/d' /etc/rc.d/rc.nginx

This reduces disk writes and keeps the intent clear.

plugin/docs/migration/progress.md (1)

19-27: Fix nested list indentation under “Installation Scripts”
The sub-items for modularized scripts should be indented further to clearly denote hierarchy. For example:

-  - Modularized the installation logic into separate scripts for better maintainability:
-  - `setup_api.sh`: Handles API setup, symlinks, and service startup
-  - `file_patches.sh`: Manages file patches and system configurations
-  - `cleanup.sh`: Performs uninstallation and cleanup operations
+  - Modularized the installation logic into separate scripts for better maintainability:
+    - `setup_api.sh`: Handles API setup, symlinks, and service startup
+    - `file_patches.sh`: Manages file patches and system configurations
+    - `cleanup.sh`: Performs uninstallation and cleanup operations

This makes the nesting level unambiguous in Markdown renderers.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh (2)

5-8: Remove or use the unused UNRAID_BINARY_PATH variable

The UNRAID_BINARY_PATH constant is defined but never referenced. To reduce confusion and maintainability overhead, either remove this variable or leverage it in your symlink creation instead of hard-coding paths.

🧰 Tools

🪛 Shellcheck (0.10.0)

[warning] 7-7: UNRAID_BINARY_PATH appears unused. Verify use (or export if used externally).

(SC2034)

---

36-62: Harden configure_script against unquoted expansions and improve insertion logic

• The grep -q "$2 $4" "$1" check may misfire if $2 or $4 contain spaces or regex metacharacters; consider quoting or escaping these arguments.
• The sed -i "10i…" insertion always targets line 10, which could be out of bounds for short files; you may want to insert immediately after the shebang (line 1) or detect the first executable section.

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/cleanup.sh (2)

13-45: Make Firefox profile restoration dynamic

The hard-coded path

"/usr/share/mozilla/firefox/9n35r0i1.default/user.js"

may not exist on all systems. Consider detecting the active profile directory under ~/.mozilla/firefox/*.default* at runtime or parameterizing this path to ensure restoration works across Unraid versions.

---

1-4: Consider enabling strict error handling

Adding at the top:

set -eu

would catch typos and unset variables early. If you prefer to continue on individual command failures, selectively trap errors instead.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/test-setup-api.sh (6)

1-3: Enable strict error handling
To catch unexpected failures and undefined variables, add strict mode after the shebang:

 #!/bin/sh
+set -euo pipefail
 # Test script to validate setup_api.sh functionality

---

7-11: Consolidate and quote path variables
You declare RC_SCRIPT on line 9 but never use it. Either remove it or reference it in your RC checks. Also, consistently quote all path variables to avoid word-splitting issues:

-SETUP_SCRIPT="/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh"
-BINARY_PATH="/usr/local/bin/unraid-api"
-RC_SCRIPT="/etc/rc.d/rc.unraid-api"
-ENV_FILE="/boot/config/plugins/dynamix.my.servers/env"
+SETUP_SCRIPT="/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh"
+BINARY_PATH="/usr/local/bin/unraid-api"
-# RC_SCRIPT is unused – consider removing
+ENV_FILE="/boot/config/plugins/dynamix.my.servers/env"

🧰 Tools

🪛 Shellcheck (0.10.0)

[warning] 9-9: RC_SCRIPT appears unused. Verify use (or export if used externally).

(SC2034)

---

21-27: Handle non-executable script more robustly
You auto-chmod the setup script if it’s not executable. It may be better to fail the test rather than modify system state in a validation tool. Alternatively, log to a separate file instead of printing to stdout.

---

30-32: Quote and log the setup script invocation
Wrap the variable in quotes and redirect output to a log for post-mortem analysis:

-echo "Running setup_api.sh..."
-"$SETUP_SCRIPT"
+echo "Running setup_api.sh..." | tee -a /tmp/test-setup-api.log
+"$SETUP_SCRIPT" 2>&1 | tee -a /tmp/test-setup-api.log

---

37-53: DRY up symlink checks
You repeat the same block three times. Consider looping over an array of target paths to reduce duplication:

-# Check symlinks
-if [ -L "$BINARY_PATH" ]; then
-  echo "✓ Symlink created: $BINARY_PATH -> $(readlink $BINARY_PATH)"
-else
-  echo "✗ Symlink not created: $BINARY_PATH"
-fi
-
-if [ -L "/usr/local/sbin/unraid-api" ]; then
-  echo "✓ Symlink created: /usr/local/sbin/unraid-api -> $(readlink /usr/local/sbin/unraid-api)"
-else
-  echo "✗ Symlink not created: /usr/local/sbin/unraid-api"
-fi
-
-if [ -L "/usr/bin/unraid-api" ]; then
-  echo "✓ Symlink created: /usr/bin/unraid-api -> $(readlink /usr/bin/unraid-api)"
-else
-  echo "✗ Symlink not created: /usr/bin/unraid-api"
-fi
+for link in "$BINARY_PATH" "/usr/local/sbin/unraid-api" "/usr/bin/unraid-api"; do
+  if [ -L "$link" ]; then
+    echo "✓ Symlink created: $link -> $(readlink "$link")"
+  else
+    echo "✗ Symlink not created: $link"
+  fi
+done

---

70-76: Use RC_SCRIPT variable in runlevel checks
You declared RC_SCRIPT but used a hardcoded list. Replace the list with a reference to $RC_SCRIPT or remove the unused variable.

plugin/source/dynamix.unraid.net/install/doinst.sh (11)

1-4: Add strict mode for robust failure detection
Enabling strict shell options helps catch errors early:

 #!/bin/sh
+set -euo pipefail
 # Main installation script for dynamix.unraid.net package
 # This script calls specialized external scripts to handle different aspects of installation

---

6-9: Quote variable expansions
Wrap $1 and $SCRIPTS_DIR in quotes when referenced to prevent word splitting:

-INSTALL_MODE="${1:-install}"
+INSTALL_MODE="${1:-install}"
 SCRIPTS_DIR="/usr/local/share/dynamix.unraid.net/install/scripts"

Also, use "$SCRIPTS_DIR" in downstream tests.

---

10-15: Redirect all output to log file
Instead of manually echoing to $LOGFILE, consider redirecting stdout and stderr at the top:

-date > "$LOGFILE"
-echo "Starting installation with mode: $INSTALL_MODE" >> "$LOGFILE"
-echo "Script directory: $SCRIPTS_DIR" >> "$LOGFILE"
+exec > >(tee "$LOGFILE") 2>&1
+echo "Starting installation with mode: $INSTALL_MODE"
+echo "Script directory: $SCRIPTS_DIR"

This captures every command’s output automatically.

---

17-24: Quote paths and check after chmod
Always quote variables in tests and commands:

-if [ -d $SCRIPTS_DIR ]; then
-  chmod +x $SCRIPTS_DIR/*.sh
+if [ -d "$SCRIPTS_DIR" ]; then
+  chmod +x "$SCRIPTS_DIR"/*.sh

Optionally verify that chmod succeeded.

---

27-35: Fail fast when cleanup.sh is missing
Currently you log an error but continue. If cleanup.sh is required, exit with non-zero:

-else
-  echo "ERROR: cleanup.sh not found or not executable" >> "$LOGFILE"
+else
+  echo "ERROR: cleanup.sh not found or not executable" >> "$LOGFILE"
+  exit 1

---

38-44: Validate installation mode
Handle unexpected modes explicitly to avoid silent no-ops:

-elif [ "$INSTALL_MODE" = "remove" ]; then
+else
+  echo "ERROR: Unknown mode '$INSTALL_MODE'. Supported: install, upgrade, remove." >> "$LOGFILE"
+  exit 1
+fi
+elif [ "$INSTALL_MODE" = "remove" ]; then

---

42-49: Quote and error-check file_patches.sh
Wrap the path in quotes and bail on failure if it’s critical:

-if [ -x $SCRIPTS_DIR/file_patches.sh ]; then
+if [ -x "$SCRIPTS_DIR/file_patches.sh" ]; then
     echo "Applying system patches and configurations..."
     echo "Running file_patches.sh" >> "$LOGFILE"
-    $SCRIPTS_DIR/file_patches.sh
+    "$SCRIPTS_DIR/file_patches.sh" || { echo "file_patches.sh failed" >> "$LOGFILE"; exit 1; }

---

51-73: DRY and harden setup_api.sh invocation
You repeat manual symlink fallback logic. Extract it into a function and reuse. Also quote all paths:

-    if [ -L "/usr/local/bin/unraid-api" ]; then
-      echo "Symlink created successfully" >> "$LOGFILE"
-    else
-      echo "ERROR: Symlink not created, attempting to create manually" >> "$LOGFILE"
-      # Create the symlink manually as fallback
-      if [ -f "/usr/local/unraid-api/dist/cli.js" ]; then
-        ln -sf "/usr/local/unraid-api/dist/cli.js" "/usr/local/bin/unraid-api"
-        …

Refactor into:

create_symlinks() {
  local src="/usr/local/unraid-api/dist/cli.js"
  for dst in "/usr/local/bin/unraid-api" "/usr/local/sbin/unraid-api" "/usr/bin/unraid-api"; do
    ln -sf "$src" "$dst"
  done
}
…
if [ -x "$SCRIPTS_DIR/setup_api.sh" ]; then
  "$SCRIPTS_DIR/setup_api.sh"
  if [ ! -L "$BINARY_PATH" ]; then
    echo "ERROR: Symlink missing, running fallback" >> "$LOGFILE"
    create_symlinks
  fi

---

92-98: Quote and bail on verify_install.sh failure
For consistent error handling:

-if [ -x $SCRIPTS_DIR/verify_install.sh ]; then
-  echo "Running verify_install.sh" >> "$LOGFILE"
-  $SCRIPTS_DIR/verify_install.sh
+if [ -x "$SCRIPTS_DIR/verify_install.sh" ]; then
+  echo "Running verify_install.sh" >> "$LOGFILE"
+  "$SCRIPTS_DIR/verify_install.sh" || { echo "verify_install.sh failed" >> "$LOGFILE"; exit 1; }

---

103-133: Capture and propagate test script exit status
Instead of parsing output for failures only, check the actual exit code of test-setup-api.sh and fail the installation if critical tests fail:

-    test_output=$($SCRIPTS_DIR/test-setup-api.sh)
+    if ! test_output=$("$SCRIPTS_DIR/test-setup-api.sh"); then
+      echo "ERROR: test-setup-api.sh returned non-zero status" >> "$LOGFILE"
+      echo "$test_output" >> "$LOGFILE"
+      exit 1
+    fi

This gives you a deterministic way to detect test failures.

---

139-145: Quote cleanup.sh in removal branch
Ensure paths are quoted and handle cleanup errors:

-if [ -x $SCRIPTS_DIR/cleanup.sh ]; then
+if [ -x "$SCRIPTS_DIR/cleanup.sh" ]; then
     "$SCRIPTS_DIR/cleanup.sh" cleanup
+    || { echo "cleanup.sh cleanup failed" >> "$LOGFILE"; exit 1; }

plugin/docs/migration/progress.md (1)

61-67: Mention test-setup-api.sh in verification section
The docs describe verify_install.sh under “Verification and TAG Handling” but omit the new test-setup-api.sh test script. For completeness, add a bullet:

- `test-setup-api.sh`: Validates setup_api.sh by checking symlinks, environment files, and init script entries.

This ensures readers know about both verification tools.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh (1)

159-173: Consider adding execution permissions check for rc.unraid-api

The script checks if the file exists but should also verify it has execution permissions before attempting to add it to the startup scripts.

-if [ -f "/etc/rc.d/rc.unraid-api" ]; then
+if [ -x "/etc/rc.d/rc.unraid-api" ]; then
   echo "Configuring startup for unraid-api" >> "$LOGFILE"
   # Configure for runlevel 3 (multi-user mode, standard Slackware default)
   # Slackware uses rc.M for runlevel 3 (multi-user mode)
   configure_script "/etc/rc.d/rc.M" "/etc/rc.d/rc.unraid-api" "Start Unraid API service" "start" "multi-user startup"
   
   # Also add to rc.local as a fallback 
   configure_script "/etc/rc.d/rc.local" "/etc/rc.d/rc.unraid-api" "Start Unraid API service" "start" "local startup"
   
   # Configure unraid-api shutdown in rc.0 and rc.6
   configure_script "/etc/rc.d/rc.0" "/etc/rc.d/rc.unraid-api" "Stop Unraid API service" "stop" "shutdown"
   configure_script "/etc/rc.d/rc.6" "/etc/rc.d/rc.unraid-api" "Stop Unraid API service" "stop" "reboot"
 else
-  echo "ERROR: rc.unraid-api not found" >> "$LOGFILE"
+  echo "ERROR: rc.unraid-api not found or not executable" >> "$LOGFILE"
 fi

plugin/docker-compose.yml (1)

23-23: Add newline at end of file

The file is missing a newline character at the end, which is a common convention for text files in Unix-based systems.

 - CI=${CI:-false}
 - TAG=${TAG}
 - API_VERSION=${API_VERSION}
+

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 23-23: no new line character at the end of file

(new-line-at-end-of-file)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/plugins/dynamix.unraid.net.plg (1)

98-108: Enhanced package removal logic with dynamic detection

The updated removal script now correctly detects the installed package name from Slackware package logs before removal, with a proper fallback to the basic package name. This is more robust than hardcoding the package name.

Consider adding error handling for the removepkg command to detect and report any failures during package removal:

        echo "Removing package: $pkg_basename"
-        removepkg --terse "$pkg_basename"
+        if ! removepkg --terse "$pkg_basename"; then
+          echo "Error: Failed to remove package $pkg_basename"
+          exit 1
+        fi

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/test-setup-api.sh (3)

9-9: Variable RC_SCRIPT is defined but never used

The RC_SCRIPT variable is defined but not referenced anywhere else in the script. Either use it or remove it to avoid confusion.

-RC_SCRIPT="/etc/rc.d/rc.unraid-api"

🧰 Tools

🪛 Shellcheck (0.10.0)

[warning] 9-9: RC_SCRIPT appears unused. Verify use (or export if used externally).

(SC2034)

---

29-32: Consider checking the return code of setup_api.sh

The script runs setup_api.sh without checking its return code. This could lead to continuing with validation even if the setup script failed.

 # Run the setup script
 echo "Running setup_api.sh..."
 "$SETUP_SCRIPT"
+SETUP_RESULT=$?
+if [ $SETUP_RESULT -ne 0 ]; then
+  echo "✗ Setup script failed with exit code: $SETUP_RESULT"
+  exit 1
+fi

---

69-76: Add specific messages for each rc file check

The current implementation uses the same success/failure message for all rc files, but they serve different purposes - rc.M/rc.local are for startup while rc.0/rc.6 are for shutdown.

 # Check if startup/shutdown configurations were added
 for RCFILE in "/etc/rc.d/rc.M" "/etc/rc.d/rc.local" "/etc/rc.d/rc.0" "/etc/rc.d/rc.6"; do
   if [ -f "$RCFILE" ] && grep -q "rc.unraid-api" "$RCFILE"; then
-    echo "✓ rc.unraid-api entry found in $RCFILE"
+    case "$RCFILE" in
+      "/etc/rc.d/rc.M"|"/etc/rc.d/rc.local")
+        echo "✓ rc.unraid-api startup entry found in $RCFILE"
+        ;;
+      "/etc/rc.d/rc.0"|"/etc/rc.d/rc.6")
+        echo "✓ rc.unraid-api shutdown entry found in $RCFILE"
+        ;;
+    esac
   else
-    echo "✗ rc.unraid-api entry not found in $RCFILE"
+    case "$RCFILE" in
+      "/etc/rc.d/rc.M"|"/etc/rc.d/rc.local")
+        echo "✗ rc.unraid-api startup entry not found in $RCFILE"
+        ;;
+      "/etc/rc.d/rc.0"|"/etc/rc.d/rc.6")
+        echo "✗ rc.unraid-api shutdown entry not found in $RCFILE"
+        ;;
+    esac
   fi
 done

plugin/docs/migration/progress.md (1)

5-5: Incomplete date in progress report

The date "2024-05-XX" should be updated with the actual completion date.

-### 2024-05-XX
+### 2024-05-15

plugin/plugins/dynamix.unraid.net.plg (3)

108-109: Use consistent XML entity references

TAG="&tag;" directly references the XML entity inline, which is inconsistent with how XML entities are typically used in this file.

-      TAG="&tag;" MAINTXZ="&source;.txz"
-      VENDOR_ARCHIVE="/boot/config/plugins/dynamix.my.servers/&vendor_store_filename;"
+      TAG="&tag;" 
+      MAINTXZ="&source;.txz"
+      VENDOR_ARCHIVE="/boot/config/plugins/dynamix.my.servers/&vendor_store_filename;"

---

117-117: Improve error message clarity

The error message "files missing from main txz" doesn't clearly indicate which specific file is missing.

-[[ ! -f /usr/local/emhttp/plugins/dynamix.my.servers/scripts/gitflash_log ]] && echo "⚠️ files missing from main txz" && exit 1
+[[ ! -f /usr/local/emhttp/plugins/dynamix.my.servers/scripts/gitflash_log ]] && echo "⚠️ Critical file missing: /usr/local/emhttp/plugins/dynamix.my.servers/scripts/gitflash_log" && exit 1

---

120-122: Add error handling for service start operation

The script starts the unraid-api service but doesn't check if it started successfully.

 # Start the Unraid API service using the rc script
 if [[ -x /etc/rc.d/rc.unraid-api ]]; then
   /etc/rc.d/rc.unraid-api start
+  if [ $? -ne 0 ]; then
+    echo "⚠️ Warning: Failed to start the Unraid API service"
+    # Continue installation despite service start failure
+  else
+    echo "✅ Unraid API service started successfully"
+  fi
 fi

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/install/doinst.sh (2)

76-79: Quote absolute path for rc.unraid-api check
The absolute path /etc/rc.d/rc.unraid-api is unquoted in both the test and the chmod invocation. To prevent word-splitting or globbing on unusual directory names, wrap the literal path in quotes.

Proposed diff:

-  if [ -f /etc/rc.d/rc.unraid-api ]; then
-    chmod 755 /etc/rc.d/rc.unraid-api
+  if [ -f "/etc/rc.d/rc.unraid-api" ]; then
+    chmod 755 "/etc/rc.d/rc.unraid-api"

---

1-13: Consider enforcing strict error handling
Adding set -euo pipefail near the top (after the shebang) would cause the script to exit on failures, undefined variables, or pipeline errors. This can help catch errors early and avoid silent failures in later steps.

Example addition:

 #!/bin/sh
+set -euo pipefail
 # Main installation script for dynamix.unraid.net package

plugin/docs/migration/progress.md (4)

5-9: Replace placeholder date with actual date
The header ### 2024-05-XX still contains a placeholder. Please update it to the correct date of the completed tasks to maintain accurate historical records.

---

54-67: Remove duplicate Next Steps and Progress sections
The document has two ## Next Steps and two high-level migration progress headings (## Next Steps at line 54 and again at line 126; # Plugin Migration to Slackware Package at line 113). Consolidate these to avoid redundancy and improve readability.

---

71-93: Unify API Build Process headings
You have both ## Building the API Package and ## API Build Process covering the same content. Merging these into a single heading will streamline the section and prevent confusion.

---

7-13: Standardize bullet list indentation and style
Nested lists currently mix single-space indentation and inconsistent list markers. For clarity, use two-space indents for sub-bullets and choose either - or 1. style uniformly.

Also applies to: 30-37

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (3)

107-115: Fix plugin environment file path
The script checks /boot/config/plugins/dynamix.my.servers/env but should target dynamix.unraid.net.

Suggested patch:

@@ -107,1 +107,1 @@ ENV_FILE="/boot/config/plugins/dynamix.my.servers/env"
-ENV_FILE="/boot/config/plugins/dynamix.my.servers/env"
+ENV_FILE="/boot/config/plugins/dynamix.unraid.net/env"

---

122-131: Correct shutdown script name for flash_backup
Checks /etc/rc.d/rc.flash_backup (underscore) but the actual script uses a dash.

Suggested patch:

@@ -124,1 +124,1 @@ if [ -f "/etc/rc.d/rc.flash_backup" ]; then
-if [ -f "/etc/rc.d/rc.flash_backup" ]; then
+if [ -f "/etc/rc.d/rc.flash-backup" ]; then

---

17-22: ⚠️ Potential issue

Incorrect plugin path in CRITICAL_FILES
Paths reference dynamix.my.servers instead of dynamix.unraid.net, causing false negatives.

Apply this patch:

@@ -19,1 +19,1 @@ CRITICAL_FILES="/usr/local/bin/unraid-api 
-/usr/local/emhttp/plugins/dynamix.my.servers/scripts/gitflash_log
+/usr/local/emhttp/plugins/dynamix.unraid.net/scripts/gitflash_log

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (3)

6-6: Reconsider set -e for a diagnostic script
Global set -e will exit on any unexpected non-zero status outside conditionals, which may prematurely terminate this verification. Since the script handles errors explicitly and always exits 0, consider removing set -e to avoid unintended exits.

---

161-167: Consider treating missing log file as an error
Currently, absence of the log file only warns without incrementing TOTAL_ERRORS. For consistency, you may want to count it as an error.

---

178-188: Exit code always zero; may not integrate with CI
Script exits with 0 even when errors are found. If this will be used in automated tests, consider exiting non-zero on TOTAL_ERRORS > 0.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (2)

100-110: ⚠️ Potential issue

Fix plugin environment file path
The script checks /boot/config/plugins/dynamix.my.servers/env, but your plugin uses dynamix.unraid.net. Update ENV_FILE accordingly.

@@ -101,1 +101,1 @@ ENV_FILE="/boot/config/plugins/dynamix.my.servers/env"
-ENV_FILE="/boot/config/plugins/dynamix.my.servers/env"
+ENV_FILE="/boot/config/plugins/dynamix.unraid.net/env"

---

19-24: 🛠️ Refactor suggestion

Remove outdated CRITICAL_DIRS entry
The directory /usr/local/emhttp/plugins/dynamix.my.servers is not part of this plugin. Please remove or update it to /usr/local/emhttp/plugins/dynamix.unraid.net.

@@ -21,1 +21,0 @@ CRITICAL_DIRS="/usr/local/unraid-api
-/usr/local/emhttp/plugins/dynamix.my.servers
+ # removed outdated plugin directory

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (3)

6-7: Consider using bash and running shellcheck
Switching the shebang to #!/usr/bin/env bash can improve portability (e.g., consistent behavior of built-ins like readlink) and enable the use of more robust Bash features. Running shellcheck will also catch potential pitfalls early.

---

155-160: Treat missing installation log as an error
Currently a missing log file only triggers a warning without incrementing TOTAL_ERRORS. Counting it as an error ensures that log creation is validated.

@@ -158,3 +158,4 @@ if [ -f "/var/log/unraid-api/dynamix-unraid-install.log" ]; then
-  printf '✓ Installation log file exists\n'
+  printf '✓ Installation log file exists\n'
 else
-  printf '⚠ Installation log file not found\n'
+  printf '✗ Installation log file not found\n'
+  TOTAL_ERRORS=$((TOTAL_ERRORS + 1))
 fi

---

172-182: Exit non-zero on failures for CI integration
Since this is a verification script, you may want a non-zero exit code when errors are detected to integrate with automated pipelines.

@@ -174,2 +174,8 @@ else
   echo "Installation verification completed with issues."
-  # We don't exit with error as this is just a verification script
-  exit 0
+  echo "See log file for details: /var/log/unraid-api/dynamix-unraid-install.log"
+  # Exit with failure to signal issues in CI
+  exit 1
 fi

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (2)

77-81: Consider adding error handling in restart function.

While functionally correct, the restart function doesn't check the return code from stop before attempting to start.

restart() {
-  stop
+  if ! stop; then
+    echo "Warning: Error stopping service, attempting to start anyway"
+  fi
   sleep 2
   start
}

---

28-64: Consider checking for log directory creation errors.

The script creates a log directory but doesn't verify success.

# Create log directory if it doesn't exist
-mkdir -p /var/log/unraid-api
+mkdir -p /var/log/unraid-api || {
+  echo "Warning: Failed to create log directory"
+}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/scripts/dependencies.sh (7)

10-12: Allow overriding the dependencies directory
Make DEPENDENCIES_DIR configurable via environment variable, falling back to the default.

-DEPENDENCIES_DIR="/usr/local/unraid-api/node_modules"
+: "${DEPENDENCIES_DIR:=/usr/local/unraid-api/node_modules}"

---

16-49: Enhance download reliability with retries and timeouts
Adding retry logic and a timeout to curl helps mitigate transient network failures.

   # Attempt to download the vendor archive
   echo "Downloading vendor archive from $vendor_store_url to $vendor_store_path"
-  if curl -f -L "$vendor_store_url" -o "$vendor_store_path"; then
+  if curl -f -L --retry 3 --retry-delay 5 --max-time 60 \
+           "$vendor_store_url" -o "$vendor_store_path"; then
     echo "Successfully downloaded vendor archive to $vendor_store_path"
     # Return the path to the downloaded archive
     echo "$vendor_store_path"
     return 0

---

54-90: Unify logging destinations for info vs. errors
Currently informational messages are mixed between stdout and stderr. Reserve stderr only for errors.

-  echo "Looking for vendor archive at $vendor_store_path" >&2
+  echo "Looking for vendor archive at $vendor_store_path"
 
   # Check if the expected archive exists

---

98-137: Localize the backup file variable in restore_dependencies
Prevent unintended global variable leakage by declaring backup_file as local.

 restore_dependencies() {
-  backup_file="$1"
+  local backup_file="$1"
 
   # Check if backup file exists
   if [ ! -f "$backup_file" ]; then

---

139-182: Preserve ownership when archiving dependencies
Ensure the archive retains root ownership metadata by adding --owner and --group flags to tar.

-  if XZ_OPT=-5 tar -cJf "$archive_file" -C "$(dirname "$source_dir")" "$(basename "$source_dir")"; then
+  if XZ_OPT=-5 tar --owner=0 --group=0 \
+           -cJf "$archive_file" \
+           -C "$(dirname "$source_dir")" "$(basename "$source_dir")"; then
     echo "node_modules archive created successfully."
     return 0

---

184-210: Error out if provided archive path is invalid
Currently, ensure silently falls back when the user passes a non-existent archive. Fail early to surface mis-typed paths.

 ensure() {
-  if [ -n "$1" ] && [ -f "$1" ]; then
-    echo "Using provided vendor archive: $1"
-    restore_dependencies "$1"
-    return $?
+  if [ -n "$1" ]; then
+    if [ -f "$1" ]; then
+      echo "Using provided vendor archive: $1"
+      restore_dependencies "$1"
+      return $?
+    else
+      echo "Error: Provided archive '$1' not found." >&2
+      return 1
+    fi
   fi

---

212-245: Redirect usage help to stderr and remove trailing whitespace
Usage messages should go to stderr to distinguish them from valid script output, and avoid stray spaces.

   *)
-    echo "Usage: $0 {restore|archive|ensure|redownload}"
+    echo "Usage: $0 {restore|archive|ensure|redownload}" >&2
     exit 1
     ;;
 esac 

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

plugin/builder/build-vendor-store.ts (1)

59-61: Good implementation of directory creation guard.

This properly addresses the previous review comment by ensuring the deploy directory exists before attempting to write files to it.

plugin/builder/build-vendor-store.ts (2)

22-24: Consider adding directory existence check.

The function correctly builds the path, but it would be more robust to verify that the vendor store directory exists before returning the path.

export function getVendorFullPath(version: string): string {
+    // Ensure the directory exists
+    if (!existsSync(vendorStorePath)) {
+        mkdirSync(vendorStorePath, { recursive: true });
+    }
    return join(vendorStorePath, getVendorBundleName(version));
}

---

30-46: Potential fragility in API node_modules path resolution.

The function assumes a specific directory structure relative to the current working directory, which might be brittle if the directory structure changes.

Consider making the API directory path configurable or using a more robust discovery mechanism:

async function createNodeModulesTarball(outputPath: string): Promise<void> {
    console.log(`Creating node_modules tarball at ${outputPath}`);
    try {
-        // Create a tarball of the node_modules directly from the API directory
-        const apiNodeModules = join(process.cwd(), "..", "api", "node_modules");
+        // Use a configurable path or environment variable for API location
+        const apiDir = process.env.API_DIR || join(process.cwd(), "..", "api");
+        const apiNodeModules = join(apiDir, "node_modules");
        if (existsSync(apiNodeModules)) {
            console.log(`Found API node_modules at ${apiNodeModules}, creating tarball...`);
-            execSync(`tar -cJf "${outputPath}" -C "${join(process.cwd(), "..", "api")}" node_modules`);
+            execSync(`tar -cJf "${outputPath}" -C "${apiDir}" node_modules`);
            console.log(`Successfully created node_modules tarball at ${outputPath}`);
            return;
        }
        throw new Error(`API node_modules not found at ${apiNodeModules}`);
    } catch (error) {
        console.error(`Failed to create node_modules tarball: ${error}`);
        throw error;
    }
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)