Refactor PR script generation#2376
Conversation
Fix update procedure Change banners to be non dismissable Add notes saying reboots might be necessary Wrap Changes within CDATA
WalkthroughThe generator script bumps the PLUGIN manifest min to 7.1.0, adds a CHANGES CDATA, removes the separate update method and merges update handling into install with backup/restore and deployment summary, changes banner/UI and uninstall flow, prints reboot notes, and ensures a trailing newline. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant PLG_Install as PLG Install Flow
participant Tarball as PR Tarball
participant FS as Filesystem
participant Manifest as installed_files.txt
User->>PLG_Install: Trigger install/update
PLG_Install->>Manifest: Read previous installed files
alt prior installation exists
PLG_Install->>FS: Restore originals or remove NEW files
end
PLG_Install->>Manifest: Clear manifest
PLG_Install->>Tarball: List tarball contents
loop per file
PLG_Install->>FS: Backup existing target (if present)
PLG_Install->>FS: Extract file to /
PLG_Install->>Manifest: Record file as NEW or MOD
end
PLG_Install-->>User: Report per-file actions and summary
Note over PLG_Install,User: Reboot may be required for code changes
sequenceDiagram
autonumber
participant User
participant Banner as UI Banner
participant PluginMgr as Plugin Manager
User->>Banner: Click "Uninstall"
Banner->>User: Show confirmation (swal)
alt confirm
Banner->>PluginMgr: Call plugin removal
PluginMgr-->>User: Removal complete + reboot note
else cancel
Banner-->>User: Dismiss
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/generate-pr-plugin.sh (1)
148-260: Hardcoded gzip flags; abort on extraction failure and validate pathsDetect tar compression (gz/xz/none) and use the selected list/extract commands for listing/counting/extraction; fail-fast if extraction returns non‑zero to avoid a half-installed state. Fix in .github/scripts/generate-pr-plugin.sh (tar usage around lines ~165–216).
Apply:
@@ -# Get file list first -echo "Examining tarball contents..." -tar -tzf "$TARBALL" | head -20 +echo "Examining tarball contents..." +# Detect compression and select tar commands +case "$TARBALL" in + *.tar.gz|*.tgz) TAR_LIST='tar -tzf'; TAR_EXTRACT='tar -xzvf' ;; + *.tar.xz|*.txz) TAR_LIST='tar -tJf'; TAR_EXTRACT='tar -xJvf' ;; + *) TAR_LIST='tar -tf'; TAR_EXTRACT='tar -xvf' ;; +esac +$TAR_LIST "$TARBALL" | head -20 @@ -# Count total files -FILE_COUNT=$(tar -tzf "$TARBALL" | grep -v '/$' | wc -l) +# Count total files +FILE_COUNT=$($TAR_LIST "$TARBALL" | grep -v '/$' | wc -l) @@ -# Get file list -tar -tzf "$TARBALL" > /tmp/plugin_files.txt +# Get file list +$TAR_LIST "$TARBALL" > /tmp/plugin_files.txt @@ -echo "Extracting files to system (verbose mode)..." +echo "Extracting files to system (verbose mode)..." echo "----------------------------------------" -tar -xzvf "$TARBALL" -C / +${TAR_EXTRACT} "$TARBALL" -C / EXTRACT_STATUS=$? echo "----------------------------------------" echo "Extraction completed with status: $EXTRACT_STATUS" echo "" +if [ "$EXTRACT_STATUS" -ne 0 ]; then + echo "❌ Extraction failed — aborting install" + exit 1 +fiOptional: validate every path from the tar listing starts with "usr/local/emhttp/" before backing up/extracting and abort if any path escapes that prefix.
🧹 Nitpick comments (1)
.github/scripts/generate-pr-plugin.sh (1)
263-304: Banner APIs: confirm option signatures; prefer const; minor UX
- Verify caPluginUpdateCheck supports {noDismiss: true} and addBannerWarning(message, true, true) parameter order in current WebGUI JS.
- bannerMessage is never reassigned; use const.
Apply:
- let bannerMessage = "Modified GUI installed via <b>webgui-pr-PR_PLACEHOLDER</b> plugin. " + + const bannerMessage = "Modified GUI installed via <b>webgui-pr-PR_PLACEHOLDER</b> plugin. " +If the API expects different option names/positions, adjust accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/generate-pr-plugin.sh(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Learnt from: Squidly271
PR: unraid/webgui#0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
🔇 Additional comments (5)
.github/scripts/generate-pr-plugin.sh (5)
52-52: Min version bumped to 7.1.0 — verify rollout expectationsConfirm this is aligned with Unraid’s supported upgrade path and that test environments running <7.1 are not expected to consume these PR plugins.
56-64: Wrapping CHANGES in CDATA looks goodSafer for arbitrary changelog chars.
257-258: Reboot note added — LGTMClear guidance for users.
353-355: Removal flow messaging — LGTMGood to remind about potential reboot post-uninstall.
383-384: Trailing newline on final message — LGTMNice polish for CI logs.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/scripts/generate-pr-plugin.sh (2)
163-176: Harden tar extraction: validate paths and prevent traversal/absolute entries.Currently you only “examine” contents, then extract to /. Add enforcement to reject entries with .. or absolute paths and whitelist expected prefixes (e.g., usr/local/emhttp/). Also prevent ownership surprises.
echo "Examining tarball contents..." tar -tzf "$TARBALL" | head -20 echo "" +# Validate tarball paths +INVALID=$(tar -tzf "$TARBALL" | grep -E '(^/|(^|/)\.\.(/|$))' || true) +if [ -n "$INVALID" ]; then + echo "ERROR: Tarball contains invalid paths:" + echo "$INVALID" + exit 1 +fi +ALLOW_PREFIX='^usr/local/emhttp/' +if ! tar -tzf "$TARBALL" | grep -Eq "$ALLOW_PREFIX"; then + echo "ERROR: Tarball does not contain expected paths under usr/local/emhttp/" + exit 1 +fi + @@ -echo "Extracting files to system (verbose mode)..." +echo "Extracting files to system (verbose mode)..." echo "----------------------------------------" -tar -xzvf "$TARBALL" -C / +tar --no-same-owner -xzvf "$TARBALL" -C / EXTRACT_STATUS=$?Also applies to: 208-217
196-199: Backups should preserve symlinks; verification should count symlinks.cp -p dereferences symlinks; restore may change link semantics. Use cp -a. Also use -e instead of -f when verifying installs to include symlinks.
- cp -p "$SYSTEM_FILE" "$BACKUP_FILE" + cp -a "$SYSTEM_FILE" "$BACKUP_FILE" @@ - if [ -f "$file" ]; then + if [ -e "$file" ]; then INSTALLED_COUNT=$((INSTALLED_COUNT + 1)) fiAlso applies to: 223-226
🧹 Nitpick comments (6)
.github/scripts/generate-pr-plugin.sh (6)
160-162: Duplicate manifest truncation.You truncate the manifest again here. Keep a single, well-placed truncate (after directories are created). If you remove the earlier one per the critical fix, this is the correct spot.
21-25: PLUGIN_URL derivation is brittle and variable naming is misleading.Replacing only .tar.gz will fail for other extensions; also TXZ_URL suggests .txz. Derive robustly and consider renaming the var (or updating the usage line).
- # Extract base URL from TXZ_URL and use consistent filename - PLUGIN_URL="${TXZ_URL%.tar.gz}.plg" + # Derive .plg alongside the archive regardless of extension + PLUGIN_URL="${TXZ_URL%.*}.plg"
29-29: Add macOS-friendly SHA256 fallback.You already special-case sed for macOS; do the same for hash to ease local testing.
-TARBALL_SHA256=$(sha256sum "$LOCAL_TARBALL" | awk '{print $1}') +if command -v sha256sum >/dev/null 2>&1; then + TARBALL_SHA256=$(sha256sum "$LOCAL_TARBALL" | awk '{print $1}') +else + TARBALL_SHA256=$(shasum -a 256 "$LOCAL_TARBALL" | awk '{print $1}') +fi
174-176: Use a safe temp file and ensure cleanup.Avoid fixed /tmp path and add a trap to clean up on failures.
-# Get file list -tar -tzf "$TARBALL" > /tmp/plugin_files.txt +TMP_LIST="$(mktemp -t pr_files.XXXXXX)" +trap 'rm -f "$TMP_LIST"' EXIT +# Get file list +tar -tzf "$TARBALL" > "$TMP_LIST" @@ -done < /tmp/plugin_files.txt +done < "$TMP_LIST" -# Clean up temp file -rm -f /tmp/plugin_files.txt +# Clean up temp file (also handled by trap) +rm -f "$TMP_LIST"Also applies to: 205-207
271-279: Guard optional CA function to avoid JS ReferenceError.caPluginUpdateCheck may not exist on systems without CA. Wrap the call.
- // Check for updates (non-dismissible) - caPluginUpdateCheck("webgui-pr-PR_PLACEHOLDER.plg", {noDismiss: true}); + // Check for updates (non-dismissible) if CA is available + if (typeof caPluginUpdateCheck === 'function') { + caPluginUpdateCheck("webgui-pr-PR_PLACEHOLDER.plg", {noDismiss: true}); + }
213-213: Consider adding tar verbosity behind a flag.-xzvf on large sets is noisy. Optional: gate -v via an env (e.g., PR_INSTALL_VERBOSE) to keep logs readable by default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/scripts/generate-pr-plugin.sh(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Learnt from: Squidly271
PR: unraid/webgui#0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
🔇 Additional comments (3)
.github/scripts/generate-pr-plugin.sh (3)
52-52: Min version bump: confirm gating for users < 7.1.0.Looks correct. Please confirm this aligns with support policy and existing CI publish flows.
253-258: Reboot notes: good addition.Clear user comms; no issues.
Also applies to: 351-355
66-119: Update pre-restore flow is solid after the manifest fix.Restoring originals then installing is safer and idempotent. After you fix the early manifest truncate, this sequence LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/generate-pr-plugin.sh (1)
210-219: Extraction failure isn’t fatal — can leave system in partial stateYou record success and proceed even if tar fails. Bail out on non‑zero status.
Apply:
tar -xzvf "$TARBALL" -C / EXTRACT_STATUS=$? echo "----------------------------------------" echo "Extraction completed with status: $EXTRACT_STATUS" echo "" +if [ "$EXTRACT_STATUS" -ne 0 ]; then + echo "ERROR: Extraction failed — aborting install" + exit 1 +fiAlso add
set -euo pipefailat the top of this Run block as suggested above.
♻️ Duplicate comments (1)
.github/scripts/generate-pr-plugin.sh (1)
81-104: Past blocker resolved: manifest dir now ensured and metadata preserved on restoremkdir -p added before truncation and cp now preserves metadata. Nice.
Also applies to: 113-116
🧹 Nitpick comments (5)
.github/scripts/generate-pr-plugin.sh (5)
21-25: Make PLG URL derivation robust for .txz and .tar.gzCovers both archive types and avoids silent mismatch with the “TXZ_URL” var name.
Apply:
-if [ -z "$PLUGIN_URL" ]; then - # Extract base URL from TXZ_URL and use consistent filename - PLUGIN_URL="${TXZ_URL%.tar.gz}.plg" -fi +if [ -z "$PLUGIN_URL" ]; then + base="${TXZ_URL%.tar.gz}" + if [ "$base" = "$TXZ_URL" ]; then + base="${TXZ_URL%.txz}" + fi + PLUGIN_URL="${base}.plg" +fi
68-83: Good: pre‑install restore now creates dirs first; add strict mode to this Run blockYou fixed the fresh‑install failure. To avoid silent partial updates, enable strict shell mode here (and mirror in other Run blocks).
Apply at the top of the CDATA:
<
2 participants
Fix update procedure
Change banners to be non dismissable
Add notes saying reboots might be necessary
Wrap Changes within CDATA
Bump the min version to 7.1.0
Summary by CodeRabbit
New Features
Refactor
Chores
Removal