Feat: Better identify safemode #2445
Conversation
WalkthroughThe ArrayOperation.page reboot path now composes richer confirmation text when safemode is selected by evaluating nginx.ini to detect Tailscale UI or Unraid Connect Remote Access and by checking for plugin-related log files; it conditionally appends warnings about plugins not loading and potential remote/webGUI access disruption. If safemode is not selected the original generic reboot message is used. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ArrayOp as ArrayOperation.page
participant nginx as nginx.ini
participant Logs as plugin log files
User->>ArrayOp: Initiate reboot
ArrayOp->>ArrayOp: Is safemode selected?
alt Safemode Selected
ArrayOp->>nginx: Read/evaluate nginx.ini
nginx-->>ArrayOp: Config info
ArrayOp->>Logs: Check for plugin-related log files
Logs-->>ArrayOp: Presence/absence
ArrayOp->>ArrayOp: Detect Tailscale UI or Unraid Connect Remote Access
ArrayOp->>ArrayOp: Compose contextual message (plugins, remote/webGUI access)
else Safemode Not Selected
ArrayOp->>ArrayOp: Use generic reboot message
end
ArrayOp->>User: Show confirmation dialog with composed text
User->>ArrayOp: Confirm or cancel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
351-377: Safe mode logic is functional but could use minor cleanup.The tailscale detection and conditional messaging work correctly. However, the string concatenation pattern redeclares
textwithvaron line 364, which is unnecessary:var text = "..."; var text = text.concat(...); // redeclarationFor cleaner code, consider:
- var text = "_(This will reboot the system in safe mode)_\n\n"; - var text = text.concat("_(No plugins will be loaded in safe mode.)_"); + var text = "_(This will reboot the system in safe mode)_\n\n"; + text = text.concat("_(No plugins will be loaded in safe mode.)_");Or use string concatenation operator:
var text = "_(This will reboot the system in safe mode)_\n\n" + "_(No plugins will be loaded in safe mode.)_";The tailscale detection logic (reading
nginx.iniand checkingHTTP_HOST) is appropriate for identifying if the GUI is accessed via Tailscale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/ArrayOperation.page(1 hunks)emhttp/plugins/dynamix/styles/default-base.css(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Squidly271
Repo: unraid/webgui PR: 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.
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-09-03T07:28:01.902Z
Learning: Squidly271's preference for not reviewing drafts is specifically because they find reviews annoying during active development phases with multiple commits, but they do want reviews once the PR is ready and more stable.
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
📚 Learning: 2025-05-28T20:54:31.390Z
Learnt from: zackspear
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-28T20:54:31.390Z
Learning: In the unraid/webgui repository, the CSS rule `div.title.ud, div#title.ud { margin-top: 0 !important; }` uses `!important` specifically to override CSS that's added by a plugin, which is a necessary approach for ensuring proper title positioning and preventing overlap with tabs.
Applied to files:
emhttp/plugins/dynamix/styles/default-base.css
📚 Learning: 2025-06-21T00:10:40.789Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2258
File: emhttp/plugins/dynamix/DashStats.page:0-0
Timestamp: 2025-06-21T00:10:40.789Z
Learning: In the Unraid webgui codebase (emhttp/plugins/dynamix), replacing `<i>` elements with `<button>` elements for accessibility would require extensive CSS refactoring due to legacy CSS having direct button styles that would conflict with icon-based toggles.
Applied to files:
emhttp/plugins/dynamix/styles/default-base.css
🔇 Additional comments (2)
emhttp/plugins/dynamix/styles/default-base.css (1)
1905-1907: LGTM! Clean utility class addition.The
.font-boldutility class follows the established TailwindCSS-inspired pattern and will be used to emphasize the tailscale warning in the safe mode confirmation dialog.emhttp/plugins/dynamix/ArrayOperation.page (1)
873-873: LGTM! Checkbox properly integrated.The safe mode checkbox is well-integrated with proper label structure, internationalization, and is correctly referenced in the
shutdown_nowJavaScript function.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
358-365: Consider more precise key matching.Using
strpos($key, 'NGINX_TAILSCALE')could match unintended keys if 'NGINX_TAILSCALE' appears as a substring in other key names. While this may be acceptable given the expected structure of nginx.ini, consider using exact prefix matching for robustness.If you want more precise matching, apply this diff:
- if (strpos($key, 'NGINX_TAILSCALE') !== false && $value == $_SERVER['HTTP_HOST']) { + if (str_starts_with($key, 'NGINX_TAILSCALE_') && $value == $_SERVER['HTTP_HOST']) { $tailscaleUI = true; break; } - if (strpos($key, 'NGINX_WANFQDN') !== false && $value == $_SERVER['HTTP_HOST']) { + if (str_starts_with($key, 'NGINX_WANFQDN_') && $value == $_SERVER['HTTP_HOST']) { $connectRemote = true; break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/ArrayOperation.page(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Learnt from: Squidly271
Repo: unraid/webgui PR: 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.
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-09-03T07:28:01.902Z
Learning: Squidly271's preference for not reviewing drafts is specifically because they find reviews annoying during active development phases with multiple commits, but they do want reviews once the PR is ready and more stable.
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
🔇 Additional comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
351-384: Good contextual warning implementation.The logic to detect and warn users about remote access implications when rebooting in safe mode is well-designed. The code appropriately checks the user's current access method and provides targeted warnings, which will help prevent users from inadvertently losing access to their server.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
357-366: Prefer strict comparison operator for string matching.Lines 358 and 362 use
==to compare$valuewith$_SERVER['HTTP_HOST']. While both should be strings, using===would be more strict and avoid potential type coercion issues.Apply this diff to use strict comparison:
foreach ($nginx as $key => $value) { - if (strpos($key, 'NGINX_TAILSCALE') !== false && $value == $_SERVER['HTTP_HOST']) { + if (strpos($key, 'NGINX_TAILSCALE') !== false && $value === $_SERVER['HTTP_HOST']) { $tailscaleUI = true; break; } - if (strpos($key, 'NGINX_WANFQDN') !== false && $value == $_SERVER['HTTP_HOST']) { + if (strpos($key, 'NGINX_WANFQDN') !== false && $value === $_SERVER['HTTP_HOST']) { $connectRemote = true; break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/ArrayOperation.page(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Squidly271
Repo: unraid/webgui PR: 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.
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-09-03T07:28:01.902Z
Learning: Squidly271's preference for not reviewing drafts is specifically because they find reviews annoying during active development phases with multiple commits, but they do want reviews once the PR is ready and more stable.
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
📚 Learning: 2025-02-10T20:28:41.294Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2011
File: etc/rc.d/rc.S:0-0
Timestamp: 2025-02-10T20:28:41.294Z
Learning: When modifying files that are managed outside the repository (like `/etc/php.d/errors-php.ini`), always include a file existence check to ensure the script doesn't fail if the file is missing.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
📚 Learning: 2025-02-27T21:52:42.541Z
Learnt from: dlandon
Repo: unraid/webgui PR: 2035
File: emhttp/plugins/dynamix/SyslogFilter.page:19-26
Timestamp: 2025-02-27T21:52:42.541Z
Learning: In the unraid/webgui repository, basic error handling is sufficient for file operations like checking file existence and using the @ operator to suppress errors, without additional error messages or notifications.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/ArrayOperation.page
🔇 Additional comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
351-380: Excellent enhancement for safe mode warnings!The implementation successfully addresses the PR objective of better identifying safe mode implications. The code:
- Correctly detects whether the user is currently accessing via Tailscale or Unraid Connect Remote Access
- Provides contextual warnings that help users understand the impact of rebooting in safe mode
- Handles missing or invalid nginx.ini file gracefully with the
@parse_ini_file() ?: []pattern- Includes both generic warnings (if plugins are installed) and specific warnings (if currently accessing via those methods)
The previous review concerns have been properly addressed.
Based on learnings
2 participants
Summary by CodeRabbit