feat(bootmode): move boot mode settings (UEFI) to new page in Flash #2498
Conversation
…ve boot mode options from Syslinux.page
WalkthroughAdds a new Boot Mode configuration page that manages UEFI/Legacy selection and removes the boot-mode UI and hidden form args from the Syslinux configuration page. The new page uses a client-side prepare function and posts to /update.php. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant Filesystem
rect rgb(240,248,255)
note over Server,Filesystem: Page render phase
Server->>Filesystem: check `/sys/firmware/efi` and `/boot/EFI`
Filesystem-->>Server: report presence/absence
Server-->>Browser: render `BootMode.page` (current mode + checkbox default)
end
rect rgb(245,255,240)
note over Browser,Server: User action and submission
Browser->>Browser: user toggles "Permit UEFI" checkbox
Browser->>Browser: onsubmit → prepareBootMode(form) sets `#arg[1]` (0/1)
Browser->>Server: POST `/update.php` with command and `#arg[1]`
Server->>Server: apply boot-mode update based on `#arg[1]`
Server-->>Browser: response (success/failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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
|
elibosley
changed the title
![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
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/BootMode.page (1)
28-36: Consider clarifying the relationship between current boot mode and the checkbox.The page displays two related but distinct concepts:
- Current boot mode (line 29): How the system currently booted (read-only)
- Permit UEFI checkbox (line 33): Whether UEFI boot capability should be enabled
This distinction might confuse users who could expect that unchecking the box immediately switches to Legacy mode, when in fact it requires a reboot and may need BIOS/UEFI firmware changes.
Consider adding clearer help text explaining:
- What happens when the checkbox is unchecked (EFI boot files are removed/disabled)
- That a system reboot is required for changes to take effect
- That BIOS/UEFI firmware settings may also need adjustment
🔎 Suggested clarification
_(Boot system in UEFI mode)_: : <label> <input type="checkbox" name="boot" <?=is_dir('/boot/EFI')?'checked':''?>> _(Permit UEFI boot mode)_ </label> - *_(Please check your system settings to support UEFI boot mode)_.* + *_(Enabling UEFI boot mode requires BIOS/UEFI firmware support. Disabling will remove EFI boot files and requires system reboot. Check your motherboard settings to configure the boot mode)_.*
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/BootMode.pageemhttp/plugins/dynamix/Syslinux.page
💤 Files with no reviewable changes (1)
- emhttp/plugins/dynamix/Syslinux.page
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
emhttp/plugins/dynamix/BootMode.page (1)
23-26: The bootmode script exists atemhttp/plugins/dynamix/scripts/bootmodeand correctly handles the#arg[1]argument. The script properly processes values 0 and 1 to toggle between legacy mode (EFI-) and UEFI mode (EFI), as expected.
| <script> | ||
| function prepareBootMode(form) { | ||
| form['#arg[1]'].value = form.boot.checked ? 1 : 0; | ||
| } | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive null checks in JavaScript function.
The function accesses form.boot and form['#arg[1]'] without verifying they exist, which could cause runtime errors if the form structure changes.
🔎 Proposed fix with defensive checks
function prepareBootMode(form) {
+ if (!form.boot || !form['#arg[1]']) {
+ console.error('Required form elements not found');
+ return false;
+ }
form['#arg[1]'].value = form.boot.checked ? 1 : 0;
+ return true;
}Also update the form's onsubmit to return the validation result:
-<form markdown="1" method="POST" action="/update.php" target="progressFrame" onsubmit="prepareBootMode(this)">
+<form markdown="1" method="POST" action="/update.php" target="progressFrame" onsubmit="return prepareBootMode(this)">Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/BootMode.page around lines 18 to 22, the
prepareBootMode(form) function assumes form.boot and form['#arg[1]'] exist which
can throw if the form structure changes; add defensive checks that verify both
elements exist (and are of expected types) before reading or setting values, and
ensure the function returns a boolean indicating success so the caller can
decide to submit; also update the form's onsubmit to return the result of
prepareBootMode(this) so submission is prevented when validation fails.
|
@limetech - I made this change to facilitate Ed's upcoming PR for config changes. It seems to work, let me know if you have any questions. |
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.