fix(Boot Settings): Syslinux gui mode fix #2631
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFrontend preview and backend write flows were updated to generate and render GUI-aware initrd values ( ChangesBoot Parameters GUI-Safe Enhancement
System Information Chassis Display
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as BootParameters.page
participant Script as manage_boot_params.sh
participant Loader as Syslinux/GRUB
User->>UI: toggle APPLY / change default
UI->>UI: compute per-entry initrd / updatePreview()
UI->>Script: trigger write with new append/kernel args
Script->>Loader: write guarded append lines, normalize CRLF
Loader->>Script: validation result
Script->>Loader: install final config
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (1)
945-952:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGUI Safe Mode still falls back to non-GUI initrd in one branch.
At Line 951,
gui_safe_appendis built fromnew_appendunchanged, so it keepsinitrd=/bzrootinstead ofinitrd=/bzroot,/bzroot-guiwhenEXCLUDE_FRAMEBUFFER_FROM_GUI != 1. That breaks GUI Safe Mode semantics.Suggested fix
if [[ "${EXCLUDE_FRAMEBUFFER_FROM_GUI}" == "1" ]]; then local gui_safe_append=" append $(build_append_line_gui_safe)" else - local gui_safe_append=" append $new_append" + local gui_safe_append=" append $(echo "$new_append" | sed 's/^initrd=\/bzroot\([ ]\|$\)/initrd=\/bzroot,\/bzroot-gui\1/')" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh` around lines 945 - 952, The GUI safe-mode branch incorrectly uses the plain new_append which leaves initrd=/bzroot instead of the GUI-safe initrd; change the else branch so gui_safe_append is built from the GUI-safe builder (use build_append_line_gui_safe) or otherwise ensure new_append is transformed to include the GUI initrd (initrd=/bzroot,/bzroot-gui); update the code that sets gui_safe_append (the variable and the else branch around APPLY_TO_GUI_SAFE_MODE / EXCLUDE_FRAMEBUFFER_FROM_GUI) to call build_append_line_gui_safe instead of using new_append so GUI Safe Mode gets the correct initrd.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 945-952: The GUI safe-mode branch incorrectly uses the plain
new_append which leaves initrd=/bzroot instead of the GUI-safe initrd; change
the else branch so gui_safe_append is built from the GUI-safe builder (use
build_append_line_gui_safe) or otherwise ensure new_append is transformed to
include the GUI initrd (initrd=/bzroot,/bzroot-gui); update the code that sets
gui_safe_append (the variable and the else branch around APPLY_TO_GUI_SAFE_MODE
/ EXCLUDE_FRAMEBUFFER_FROM_GUI) to call build_append_line_gui_safe instead of
using new_append so GUI Safe Mode gets the correct initrd.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7fb76cb5-9849-4030-9df8-c3495fa475ec
📒 Files selected for processing (3)
emhttp/plugins/dynamix/BootParameters.pageemhttp/plugins/dynamix/scripts/manage_boot_params.shemhttp/plugins/dynamix/scripts/system_information
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (1)
959-989:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGUI Safe Mode missing
/bzroot-guiwhen framebuffer exclusion is disabled.When
EXCLUDE_FRAMEBUFFER_FROM_GUIis0(the else branch at line 966),gui_safe_appenduses$new_appenddirectly, which hasinitrd=/bzrootwithout/bzroot-gui. This differs from the "GUI Mode" handling (line 910) which applies a sed transformation to add/bzroot-gui.For consistency, "GUI Safe Mode" should also include
/bzroot-guisince it's a GUI-enabled mode.Proposed fix
# Update "Unraid OS GUI Safe Mode" label if enabled in UI if [[ "${APPLY_TO_GUI_SAFE_MODE}" == "1" ]]; then # Optionally exclude framebuffer parameters for GUI mode entries if [[ "${EXCLUDE_FRAMEBUFFER_FROM_GUI}" == "1" ]]; then local gui_safe_append=" append $(build_append_line_gui_safe)" else - local gui_safe_append=" append $new_append" + local gui_safe_append=" append $(echo "$new_append" | sed 's/^initrd=\/bzroot\([ ]\|$\)/initrd=\/bzroot,\/bzroot-gui\1/')" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh` around lines 959 - 989, The GUI Safe Mode branch currently sets gui_safe_append=" append $new_append" which leaves initrd=/bzroot instead of the GUI initrd; change the else branch so gui_safe_append uses the same GUI transformation as the regular GUI Mode (i.e., produce an append line where initrd=/bzroot is replaced with initrd=/bzroot-gui or call build_append_line_gui_safe) so the awk replacement writes the GUI initrd for "Unraid OS GUI Safe Mode"; update the else branch that assigns gui_safe_append to perform that substitution rather than using $new_append unchanged.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (1)
747-749: 💤 Low valueRemove useless echo.
The
echo "$(cmd)"pattern is redundant. The command output can be used directly via command substitution.Proposed fix
build_kernel_args_gui_safe() { - echo "$(build_append_line_gui_safe | sed 's/^initrd=\/bzroot,\/bzroot-gui[ ]*//')" + build_append_line_gui_safe | sed 's/^initrd=\/bzroot,\/bzroot-gui[ ]*//' }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh` around lines 747 - 749, In build_kernel_args_gui_safe, remove the redundant echo and command-substitution wrapper around build_append_line_gui_safe; instead have the function directly pipe build_append_line_gui_safe's output into sed to strip the initrd prefix (the sed expression 's/^initrd=\/bzroot,\/bzroot-gui[ ]*//'), so replace the echo "$(build_append_line_gui_safe | sed ...)" form with a direct pipeline using build_append_line_gui_safe | sed ... to emit the cleaned kernel args.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 959-989: The GUI Safe Mode branch currently sets gui_safe_append="
append $new_append" which leaves initrd=/bzroot instead of the GUI initrd;
change the else branch so gui_safe_append uses the same GUI transformation as
the regular GUI Mode (i.e., produce an append line where initrd=/bzroot is
replaced with initrd=/bzroot-gui or call build_append_line_gui_safe) so the awk
replacement writes the GUI initrd for "Unraid OS GUI Safe Mode"; update the else
branch that assigns gui_safe_append to perform that substitution rather than
using $new_append unchanged.
---
Nitpick comments:
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 747-749: In build_kernel_args_gui_safe, remove the redundant echo
and command-substitution wrapper around build_append_line_gui_safe; instead have
the function directly pipe build_append_line_gui_safe's output into sed to strip
the initrd prefix (the sed expression 's/^initrd=\/bzroot,\/bzroot-gui[ ]*//'),
so replace the echo "$(build_append_line_gui_safe | sed ...)" form with a direct
pipeline using build_append_line_gui_safe | sed ... to emit the cleaned kernel
args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48e58db9-72fb-4907-a5ec-e4c87abbaea3
📒 Files selected for processing (2)
emhttp/plugins/dynamix/BootParameters.pageemhttp/plugins/dynamix/scripts/manage_boot_params.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/BootParameters.page
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (1)
955-962:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGUI Safe Mode still rewrites to the non-GUI initrd.
When
EXCLUDE_FRAMEBUFFER_FROM_GUI=0, this branch reuses$new_append, which still starts withinitrd=/bzroot. Any save through this path will drop/bzroot-guifrom theUnraid OS GUI Safe Modeentry, so one GUI boot target still regresses.Suggested fix
if [[ "${EXCLUDE_FRAMEBUFFER_FROM_GUI}" == "1" ]]; then local gui_safe_append=" append $(build_append_line_gui_safe)" else - local gui_safe_append=" append $new_append" + local gui_safe_append=" append $(echo "$new_append" | sed 's/^initrd=\/bzroot\([ ]\|$\)/initrd=\/bzroot,\/bzroot-gui\1/')" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh` around lines 955 - 962, The GUI Safe Mode branch currently reuses new_append (which contains initrd=/bzroot) when EXCLUDE_FRAMEBUFFER_FROM_GUI==0, causing the GUI entry to lose /bzroot-gui; change the assignment that sets gui_safe_append so it generates an append line that preserves the GUI initrd—either call build_append_line_gui_safe to produce the full append string instead of reusing new_append or explicitly replace the initrd token in new_append with initrd=/bzroot-gui before assigning gui_safe_append (update the code around APPLY_TO_GUI_SAFE_MODE, EXCLUDE_FRAMEBUFFER_FROM_GUI, gui_safe_append, build_append_line_gui_safe and new_append accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 955-962: The GUI Safe Mode branch currently reuses new_append
(which contains initrd=/bzroot) when EXCLUDE_FRAMEBUFFER_FROM_GUI==0, causing
the GUI entry to lose /bzroot-gui; change the assignment that sets
gui_safe_append so it generates an append line that preserves the GUI
initrd—either call build_append_line_gui_safe to produce the full append string
instead of reusing new_append or explicitly replace the initrd token in
new_append with initrd=/bzroot-gui before assigning gui_safe_append (update the
code around APPLY_TO_GUI_SAFE_MODE, EXCLUDE_FRAMEBUFFER_FROM_GUI,
gui_safe_append, build_append_line_gui_safe and new_append accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4211979-f316-4caf-993e-374a734f0bf9
📒 Files selected for processing (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 280-284: The syslinux stanza matching is too strict: in the AWK
block(s) that start with the /^label[[:space:]]+/ pattern (used by read_config
and related parsing code), normalize whitespace and relax equality so labels
match regardless of tabs/multiple spaces or trailing/leading spacing, and accept
"append" lines without requiring leading indentation; specifically, in the block
that lowercases and trims the line (the /^label[[:space:]]+/ handler), change
the equality check that compares to "label " label to a regex or
normalized-compare that matches "label" followed by any whitespace then the
target label, and mirror the same relaxed matching logic in the other identical
blocks (the one around the append handling and the duplicate section at lines
~833–840) so label and append stanza detection is whitespace-agnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e68231bc-e78a-41c7-95b0-23651db1d04e
📒 Files selected for processing (2)
emhttp/plugins/dynamix/BootParameters.pageemhttp/plugins/dynamix/scripts/manage_boot_params.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/BootParameters.page
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh (1)
749-751: 💤 Low valueRemove unnecessary echo wrapper.
The
echo "$(cmd)"pattern is redundant—the pipeline already outputs to stdout.♻️ Suggested fix
build_kernel_args_gui_safe() { - echo "$(build_append_line_gui_safe | sed 's/^initrd=\/bzroot,\/bzroot-gui[ ]*//')" + build_append_line_gui_safe | sed 's/^initrd=\/bzroot,\/bzroot-gui[ ]*//' }Note: The same pattern exists in
build_kernel_args()on line 746.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh` around lines 749 - 751, The function build_kernel_args_gui_safe uses an unnecessary echo wrapper around a command substitution; replace the body so it directly pipes build_append_line_gui_safe into sed (i.e., remove echo and the "$(…)" wrapper) to let the pipeline write to stdout; also apply the same simplification to the build_kernel_args function which uses the identical echo "$(…)" pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@emhttp/plugins/dynamix/scripts/manage_boot_params.sh`:
- Around line 749-751: The function build_kernel_args_gui_safe uses an
unnecessary echo wrapper around a command substitution; replace the body so it
directly pipes build_append_line_gui_safe into sed (i.e., remove echo and the
"$(…)" wrapper) to let the pipeline write to stdout; also apply the same
simplification to the build_kernel_args function which uses the identical echo
"$(…)" pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63c6f028-ef84-4f73-a200-5e5f684e2e4e
📒 Files selected for processing (1)
emhttp/plugins/dynamix/scripts/manage_boot_params.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When changing from Unraid OS to gui mode the bzroot-gui entry on initrd is being removed. This PR is to fix this so the entry is retained.
Summary by CodeRabbit
New Features
Improvements