Next release - #1690
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds a shared light-gray theme token, updates loading skeleton markup and spinner targeting, changes several skeleton hide/show paths to reuse DOM nodes, localizes rendered device timestamps, and reorganizes multiple documentation guides and navigation entries. ChangesFrontend theme, loading, and timestamps
Documentation and navigation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/deviceDetailsEdit.php (1)
487-523: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHide the details skeleton on all save completion paths.
showDetailsTabSkeleton()is called at save start (Line 426), buthideDetailsTabSkeleton()only runs on the success branch (Line 491). Onresp.success === falseand AJAX error (Line 515), the skeleton stays visible and can block editing.Suggested fix
success: function(resp) { if (resp && resp.success) { showMessage(getString("Device_Saved_Success")); - hideDetailsTabSkeleton(); } else { console.log(resp); errorMessage = resp?.error; showMessage(`${getString("Device_Saved_Unexpected")}: ${errorMessage}`, 5000, "modal_red"); } // Remove navigation prompt window.onbeforeunload = null; somethingChanged = false; // Refresh API updateApi("devices,appevents"); // Callback if (typeof refreshCallback === "function") { refreshCallback(direction); } + hideDetailsTabSkeleton(); hideSpinner(); }, error: function(xhr) { if (xhr.status === 403) { showMessage(getString("Device_Save_Unauthorized")); } else { showMessage(getString("Device_Save_Failed") + " (" + xhr.status + ")"); } + hideDetailsTabSkeleton(); hideSpinner(); }🤖 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 `@front/deviceDetailsEdit.php` around lines 487 - 523, The save flow in deviceDetailsEdit.php leaves the details skeleton visible on failure paths because hideDetailsTabSkeleton() is only called in the resp.success branch. Update the success and error handlers in the save callback so hideDetailsTabSkeleton() is invoked after any save completion, including resp.success === false and xhr error cases, alongside the existing cleanup like hideSpinner(), window.onbeforeunload, and refreshCallback.
🤖 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 `@front/css/dark-patch.css`:
- Around line 17-24: Remove the duplicate --color-white declaration in the :root
block of dark-patch.css so each CSS variable is defined only once. Update the
color token list to keep a single --color-white entry alongside the other
--color-* variables, and ensure no other duplicate custom properties remain in
that block.
In `@front/devices.php`:
- Around line 924-925: Guard empty devFirstConnection and devLastConnection
values before calling localizeTimestamp in the hover-card markup builder so
blank timestamps stay empty instead of becoming the "Failed conversion" string.
Update the data-firstseen and data-lastseen attribute expressions to check the
row values from mapIndx(COL.devFirstConnection) and
mapIndx(COL.devLastConnection) first, and only format non-empty timestamps;
otherwise fall back to the existing "Unknown" behavior used by the UI.
In `@front/php/templates/skel_tab_maint_logging.php`:
- Line 1: The global .spinnerTarget usage makes spinner anchoring depend on DOM
order, so showSpinner()/hideSpinner() in common.js may target the wrong pane.
Update the spinner selection logic to scope to the active tab/container or
accept an explicit target instead of using $(".spinnerTarget").first(). Adjust
the templates around skel-tab-maint-logging and any other spinnerTarget markup
so the intended container is identifiable by active context, and keep the
spinner APIs consistent with that target resolution.
In `@front/php/templates/skel_tab_tools.php`:
- Line 1: The spinner overlay is being anchored to the first .spinnerTarget
instead of the active visible skeleton, which can target the wrong container
when multiple exist. Update showSpinner() and hideSpinner() to resolve the
currently active/visible spinner target explicitly, using the relevant
spinnerTarget element for the current view rather than .first(), and keep the
logic aligned with the existing spinner handling in those functions.
---
Outside diff comments:
In `@front/deviceDetailsEdit.php`:
- Around line 487-523: The save flow in deviceDetailsEdit.php leaves the details
skeleton visible on failure paths because hideDetailsTabSkeleton() is only
called in the resp.success branch. Update the success and error handlers in the
save callback so hideDetailsTabSkeleton() is invoked after any save completion,
including resp.success === false and xhr error cases, alongside the existing
cleanup like hideSpinner(), window.onbeforeunload, and refreshCallback.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c14f703f-2fec-4f4a-85a7-ef9607730ba3
📒 Files selected for processing (35)
front/css/app.cssfront/css/dark-patch.cssfront/css/system-dark-patch.cssfront/deviceDetails.phpfront/deviceDetailsEdit.phpfront/deviceDetailsEvents.phpfront/deviceDetailsPresence.phpfront/deviceDetailsSessions.phpfront/deviceDetailsTools.phpfront/devices.phpfront/js/network-tree.jsfront/js/ui_components.jsfront/php/templates/skel_devices.phpfront/php/templates/skel_events.phpfront/php/templates/skel_notifications.phpfront/php/templates/skel_plugins.phpfront/php/templates/skel_presence.phpfront/php/templates/skel_report.phpfront/php/templates/skel_settings.phpfront/php/templates/skel_tab_details.phpfront/php/templates/skel_tab_events.phpfront/php/templates/skel_tab_maint_backup.phpfront/php/templates/skel_tab_maint_dbtools.phpfront/php/templates/skel_tab_maint_logging.phpfront/php/templates/skel_tab_maint_multiedit.phpfront/php/templates/skel_tab_plugins_table.phpfront/php/templates/skel_tab_presence.phpfront/php/templates/skel_tab_sessions.phpfront/php/templates/skel_tab_sysinfo_initcheck.phpfront/php/templates/skel_tab_sysinfo_network.phpfront/php/templates/skel_tab_sysinfo_server.phpfront/php/templates/skel_tab_sysinfo_storage.phpfront/php/templates/skel_tab_tools.phpfront/php/templates/skel_workflows.phpfront/pluginsCore.php
Comment on lines
+924
to
+925
| ` data-firstseen="${localizeTimestamp(row[mapIndx(COL.devFirstConnection)])}"` + | ||
| ` data-lastseen="${localizeTimestamp(row[mapIndx(COL.devLastConnection)])}"` + |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard empty timestamps before formatting.
At Line 924 and Line 925, empty values now become "Failed conversion" (truthy), so hover cards show that literal string instead of "Unknown".
💡 Proposed fix
+ const firstSeenRaw = row[mapIndx(COL.devFirstConnection)];
+ const lastSeenRaw = row[mapIndx(COL.devLastConnection)];
return (
`<b class="anonymizeDev ">` +
`<a href="deviceDetails.php?mac=${row[mapIndx(COL.devMac)]}" class="hover-node-info"` +
@@
- ` data-firstseen="${localizeTimestamp(row[mapIndx(COL.devFirstConnection)])}"` +
- ` data-lastseen="${localizeTimestamp(row[mapIndx(COL.devLastConnection)])}"` +
+ ` data-firstseen="${firstSeenRaw ? localizeTimestamp(firstSeenRaw) : ''}"` +
+ ` data-lastseen="${lastSeenRaw ? localizeTimestamp(lastSeenRaw) : ''}"` +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ` data-firstseen="${localizeTimestamp(row[mapIndx(COL.devFirstConnection)])}"` + | |
| ` data-lastseen="${localizeTimestamp(row[mapIndx(COL.devLastConnection)])}"` + | |
| const firstSeenRaw = row[mapIndx(COL.devFirstConnection)]; | |
| const lastSeenRaw = row[mapIndx(COL.devLastConnection)]; | |
| return ( | |
| `<b class="anonymizeDev ">` + | |
| `<a href="deviceDetails.php?mac=${row[mapIndx(COL.devMac)]}" class="hover-node-info"` + | |
| @@ | |
| ` data-firstseen="${firstSeenRaw ? localizeTimestamp(firstSeenRaw) : ''}"` + | |
| ` data-lastseen="${lastSeenRaw ? localizeTimestamp(lastSeenRaw) : ''}"` + |
🤖 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 `@front/devices.php` around lines 924 - 925, Guard empty devFirstConnection and
devLastConnection values before calling localizeTimestamp in the hover-card
markup builder so blank timestamps stay empty instead of becoming the "Failed
conversion" string. Update the data-firstseen and data-lastseen attribute
expressions to check the row values from mapIndx(COL.devFirstConnection) and
mapIndx(COL.devLastConnection) first, and only format non-empty timestamps;
otherwise fall back to the existing "Unknown" behavior used by the UI.
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 `@front/js/common.js`:
- Around line 972-975: The resolveSpinnerTarget function currently returns the
explicitTarget result immediately, so when it matches nothing it skips the
fallback lookup and can place the spinner full-screen. Update
resolveSpinnerTarget to validate the explicitTarget selection before returning,
and if the jQuery result is empty continue on to the existing active/visible
.spinnerTarget fallback logic instead of exiting early.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54f51c01-8b47-457b-8b33-732454109eda
📒 Files selected for processing (2)
front/css/dark-patch.cssfront/js/common.js
💤 Files with no reviewable changes (1)
- front/css/dark-patch.css
Contributor
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 (3)
front/deviceDetailsEvents.php (1)
183-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe reusable skeleton still only runs once.
Because the loading state is gated behind
eventsPageInitialized, laterloadEventsData()calls (for example from#chkHideConnectionEvents) refresh the table without ever showing the skeleton/spinner again. That defeats the new hide/show reuse path after the first render.Proposed fix
function loadEventsData() { const mac = getMac(); if (!mac) { console.warn("loadEventsData: mac not set, skipping"); return; } + + showEventsTabSkeleton(); + showSpinner(); const hideConnections = $('`#chkHideConnectionEvents`')[0].checked; ... } ... - showEventsTabSkeleton(); - showSpinner(); - var eventsRows = 10;🤖 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 `@front/deviceDetailsEvents.php` around lines 183 - 186, The loading UI in loadEventsData() is still bypassed on subsequent refreshes because it is guarded by eventsPageInitialized, so later calls such as from `#chkHideConnectionEvents` never show the skeleton/spinner. Update the load flow around showEventsTabSkeleton() and showSpinner() so the reusable loading state runs on every data fetch, while keeping the one-time initialization in the existing eventsPageInitialized path separate from the refresh UI.front/deviceDetailsEdit.php (1)
487-522: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHide the details skeleton on failed saves too.
showDetailsTabSkeleton()now runs before the request, buthideDetailsTabSkeleton()only runs in the success branch. Any API error orresp.success === falseleaves the skeleton over the form and blocks the retry path.Suggested fix
success: function(resp) { if (resp && resp.success) { showMessage(getString("Device_Saved_Success")); - hideDetailsTabSkeleton(); } else { console.log(resp); errorMessage = resp?.error; showMessage(`${getString("Device_Saved_Unexpected")}: ${errorMessage}`, 5000, "modal_red"); } @@ hideSpinner(); + hideDetailsTabSkeleton(); }, error: function(xhr) { if (xhr.status === 403) { showMessage(getString("Device_Save_Unauthorized")); } else { showMessage(getString("Device_Save_Failed") + " (" + xhr.status + ")"); } hideSpinner(); + hideDetailsTabSkeleton(); }🤖 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 `@front/deviceDetailsEdit.php` around lines 487 - 522, The save flow in deviceDetailsEdit.php only clears the details skeleton inside the success branch of the AJAX callback, so failed saves leave the form blocked. Update the save handling around the success and error callbacks to always call hideDetailsTabSkeleton() on any completed request path, including when resp.success is false and in the xhr error handler, while keeping the existing success behavior in the save callback.front/deviceDetailsTools.php (1)
214-219: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t show the tools spinner before the tab is visible.
These calls run as soon as the script is parsed, but the matching hide path is behind
#panTools:visible. On any initial load that opens another tab, the spinner can stay active until Tools is visited.Suggested fix
- showSpinner(); - showToolsTabSkeleton(); - // ---------------------------------------------------------------- function manualnmapscan(targetip, mode) { @@ function initDeviceToolsPage() { // Only proceed if .panTools is visible if (!$('`#panTools`:visible').length) { return; // exit early if nothing is visible @@ if (toolsPageInitialized) return; toolsPageInitialized = true; + + showSpinner(); + showToolsTabSkeleton(); initNmapButtons(); initCopyFromDevice(); hideSpinner();Also applies to: 695-710
🤖 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 `@front/deviceDetailsTools.php` around lines 214 - 219, The Tools spinner/skeleton is being shown too early because the initialization block runs on script parse, while the hide logic is gated behind `#panTools`:visible. Move the showSpinner and showToolsTabSkeleton behavior in deviceDetailsTools.php behind a visibility check for the Tools tab, and keep it paired with the existing hide path so it only activates when the Tools tab is actually displayed; apply the same adjustment in the duplicated initialization path around the matching symbols used later in the file.
🤖 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 `@front/php/templates/skel_device_details.php`:
- Around line 63-218: The device details skeleton markup now uses floated
Bootstrap columns inside skel_device_details.php, but the spinner target
showSpinner() measures can collapse without a clearing wrapper. Wrap the main
skel-detail-form column groups in a .row or clearfix container so the floated
col-lg-* / col-sm-* sections expand correctly and `#device-details-skeleton` keeps
the full form height. Use skel-detail-form and showSpinner() as the key
references when updating the structure.
---
Outside diff comments:
In `@front/deviceDetailsEdit.php`:
- Around line 487-522: The save flow in deviceDetailsEdit.php only clears the
details skeleton inside the success branch of the AJAX callback, so failed saves
leave the form blocked. Update the save handling around the success and error
callbacks to always call hideDetailsTabSkeleton() on any completed request path,
including when resp.success is false and in the xhr error handler, while keeping
the existing success behavior in the save callback.
In `@front/deviceDetailsEvents.php`:
- Around line 183-186: The loading UI in loadEventsData() is still bypassed on
subsequent refreshes because it is guarded by eventsPageInitialized, so later
calls such as from `#chkHideConnectionEvents` never show the skeleton/spinner.
Update the load flow around showEventsTabSkeleton() and showSpinner() so the
reusable loading state runs on every data fetch, while keeping the one-time
initialization in the existing eventsPageInitialized path separate from the
refresh UI.
In `@front/deviceDetailsTools.php`:
- Around line 214-219: The Tools spinner/skeleton is being shown too early
because the initialization block runs on script parse, while the hide logic is
gated behind `#panTools`:visible. Move the showSpinner and showToolsTabSkeleton
behavior in deviceDetailsTools.php behind a visibility check for the Tools tab,
and keep it paired with the existing hide path so it only activates when the
Tools tab is actually displayed; apply the same adjustment in the duplicated
initialization path around the matching symbols used later in the file.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8c69c6b-3449-4f24-8ac2-356b98789537
📒 Files selected for processing (12)
front/deviceDetailsEdit.phpfront/deviceDetailsEvents.phpfront/deviceDetailsPresence.phpfront/deviceDetailsSessions.phpfront/deviceDetailsTools.phpfront/js/common.jsfront/php/templates/skel_device_details.phpfront/php/templates/skel_device_details_tab_details.phpfront/php/templates/skel_device_details_tab_events.phpfront/php/templates/skel_device_details_tab_presence.phpfront/php/templates/skel_device_details_tab_sessions.phpfront/php/templates/skel_device_details_tab_tools.php
💤 Files with no reviewable changes (5)
- front/php/templates/skel_device_details_tab_presence.php
- front/php/templates/skel_device_details_tab_details.php
- front/php/templates/skel_device_details_tab_events.php
- front/php/templates/skel_device_details_tab_sessions.php
- front/php/templates/skel_device_details_tab_tools.php
🚧 Files skipped from review as they are similar to previous changes (1)
- front/js/common.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/deviceDetails.php (1)
266-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the spinner on the confirm-discard path too.
The new spinner call only runs when
somethingChangedis false. If the user confirms the discard modal,performSwitchstill runs without the spinner, so this navigation path keeps the old blank-state behavior.Suggested fix
+function switchRecord(direction) { + showSpinner(); + performSwitch(direction); +} + function recordSwitch(direction) { if(somethingChanged) { showModalDefaultStrParam ('Unsaved changes', 'Do you want to discard your changes?', - '<?= lang('Gen_Cancel');?>', '<?= lang('Gen_Okay');?>', performSwitch, direction); + '<?= lang('Gen_Cancel');?>', '<?= lang('Gen_Okay');?>', switchRecord, direction); } else { - showSpinner(); - performSwitch(direction) + switchRecord(direction); } }🤖 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 `@front/deviceDetails.php` around lines 266 - 273, The spinner is only shown in the direct switch path, so the confirm-discard flow from showModalDefaultStrParam to performSwitch skips it. Update the switch logic in deviceDetails.php so the same showSpinner behavior runs when the user confirms discarding changes, not just when somethingChanged is false. Use the existing performSwitch callback/direction flow to place the spinner call in the shared path so both navigation branches behave consistently.
🧹 Nitpick comments (1)
docs/PIHOLE_GUIDE.md (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAbsolute
/docs/PLUGINS.mdand./REMOTE_NETWORKS.mdmixed link styles.Line 78 uses an absolute path for PLUGINS.md and a relative path for REMOTE_NETWORKS.md. Standardize to relative links for portability.
📝 Suggested fix
-Explore other [plugins](/docs/PLUGINS.md) to discover additional information about your network, or learn how to scan [remote networks](./REMOTE_NETWORKS.md). +Explore other [plugins](PLUGINS.md) to discover additional information about your network, or learn how to scan [remote networks](REMOTE_NETWORKS.md).🤖 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 `@docs/PIHOLE_GUIDE.md` at line 78, The markdown link styles in the PIHOLE_GUIDE content are inconsistent, with an absolute link to PLUGINS.md and a relative link to REMOTE_NETWORKS.md. Update the sentence in the guide to use the same relative-link style for both targets so navigation remains portable; adjust the links in the affected markdown text rather than changing the document structure.
🤖 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 `@docs/HOME_ASSISTANT.md`:
- Around line 50-58: The “Missing devices” guidance is unclear about where the
first arp-scan command should be run. Update the HOME_ASSISTANT.md flow so
container access instructions come before any command that must run inside
NetAlertX, or explicitly state which scan runs on the host versus inside the
container. Keep the wording around arp-scan and the NetAlertX container
consistent so readers know the correct execution context before trying the
command.
- Line 87: Remove the redundant sudo from the container shell example in
HOME_ASSISTANT.md: the prompt already shows root access, so the arp-scan command
should be documented without sudo. Update the example to keep only the direct
arp-scan invocation so it matches how commands are run inside the container.
In `@docs/PIHOLE_GUIDE.md`:
- Around line 72-75: The Pi-hole database mount example is missing the host-side
path in the mount mapping. Update the table entry for the pihole-FTL.db database
so it shows a complete host-to-container path, matching the style used in the
other mount examples in PIHOLE_GUIDE.md and clearly identifying the source path
and the container path.
- Around line 48-51: The Docker volume examples in PIHOLE_GUIDE.md are missing
the host-side source path, so the mount syntax is invalid and confusing. Update
the examples for the `dhcp.leases` and `pihole-FTL.db` rows to include a
representative host path before the colon, keeping the existing container paths
intact. Use the table entries in the Pi-hole guide that describe the
`dhcp.leases` and `pihole-FTL.db` mounts so readers can copy a working
`host:container` compose example.
- Line 3: The link in PIHOLE_GUIDE.md currently uses an absolute
/docs/PLUGINS.md path that breaks in common renderers; update the markdown
reference in the guide to use a relative link instead. Adjust the existing
PLUGINS.md link in the PIHOLE_GUIDE.md content so it resolves correctly from the
docs folder and remains portable across GitHub and MkDocs.
- Around line 15-20: The Pi-hole guide currently recommends an insecure default
for PIHOLEAPI_SSL_VERIFY. Update the documentation entry in the PIHOLEAPI_*
settings table to recommend True as the default, and mention that False should
only be used for self-signed certificates with an explicit security warning.
Keep the wording aligned with the existing PIHOLEAPI_SSL_VERIFY description so
readers understand the safe default and the limited exception.
In `@front/systeminfoInitCheck.php`:
- Around line 10-32: The layout in systeminfoInitCheck.php uses Bootstrap 4/5
utility classes that are not available in the shipped Bootstrap 3.3.7/AdminLTE
setup, so the section won’t render as intended. Update the markup around the
check status, file list, and init-check button blocks by replacing classes like
col-12, d-flex, gap-2, and w-100 with Bootstrap 3-compatible grid/utility
patterns or equivalent custom CSS, keeping the existing structure in place and
adjusting the affected containers and button styling consistently.
In `@mkdocs.yml`:
- Line 69: The navigation entry for “Incorrect offline detection” is in the
wrong section of mkdocs.yml and should be moved out of Setup › Advanced guides
into the Troubleshooting list. Update the mkdocs navigation structure by
removing the FIX_OFFLINE_DETECTION.md item from the current Advanced guides
group and adding it under the Troubleshooting section so the entry is grouped
with related troubleshooting content.
---
Outside diff comments:
In `@front/deviceDetails.php`:
- Around line 266-273: The spinner is only shown in the direct switch path, so
the confirm-discard flow from showModalDefaultStrParam to performSwitch skips
it. Update the switch logic in deviceDetails.php so the same showSpinner
behavior runs when the user confirms discarding changes, not just when
somethingChanged is false. Use the existing performSwitch callback/direction
flow to place the spinner call in the shared path so both navigation branches
behave consistently.
---
Nitpick comments:
In `@docs/PIHOLE_GUIDE.md`:
- Line 78: The markdown link styles in the PIHOLE_GUIDE content are
inconsistent, with an absolute link to PLUGINS.md and a relative link to
REMOTE_NETWORKS.md. Update the sentence in the guide to use the same
relative-link style for both targets so navigation remains portable; adjust the
links in the affected markdown text rather than changing the document structure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 268f7088-f7df-484d-b8be-cea2b00e9d75
⛔ Files ignored due to path filters (1)
docs/img/NOTIFICATIONS/Device-notification-settings.pngis excluded by!**/*.png
📒 Files selected for processing (26)
docs/HOME_ASSISTANT.mddocs/NOTIFICATIONS.mddocs/PIHOLE_GUIDE.mddocs/REST_IMPORT.mdfront/appEventsCore.phpfront/css/app.cssfront/deviceDetails.phpfront/deviceDetailsEdit.phpfront/deviceDetailsEvents.phpfront/deviceDetailsPresence.phpfront/deviceDetailsSessions.phpfront/deviceDetailsTools.phpfront/devices.phpfront/events.phpfront/maintenance.phpfront/php/templates/skel_device_details_tab_details.phpfront/php/templates/skel_plugins.phpfront/plugins.phpfront/pluginsCore.phpfront/presence.phpfront/report.phpfront/systeminfo.phpfront/systeminfoInitCheck.phpfront/userNotifications.phpfront/workflowsCore.phpmkdocs.yml
💤 Files with no reviewable changes (1)
- front/plugins.php
✅ Files skipped from review due to trivial changes (7)
- front/report.php
- front/presence.php
- front/events.php
- front/systeminfo.php
- docs/NOTIFICATIONS.md
- front/php/templates/skel_device_details_tab_details.php
- docs/REST_IMPORT.md
🚧 Files skipped from review as they are similar to previous changes (5)
- front/deviceDetailsSessions.php
- front/deviceDetailsEvents.php
- front/deviceDetailsPresence.php
- front/deviceDetailsEdit.php
- front/deviceDetailsTools.php
Comment on lines
+50
to
+58
| If some devices do not appear in Home Assistant, first verify that NetAlertX can detect them by running: | ||
|
|
||
| [configuration]: ./img/HOME_ASISSTANT/HomeAssistant-Configuration.png "configuration" | ||
| [sensors]: ./img/HOME_ASISSTANT/HomeAssistant-Device-as-Sensors.png "sensors" | ||
| [history]: ./img/HOME_ASISSTANT/HomeAssistant-Device-Presence-History.png "history" | ||
| [list]: ./img/HOME_ASISSTANT/HomeAssistant-Devices-List.png "list" | ||
| [overview]: ./img/HOME_ASISSTANT/HomeAssistant-Overview-Card.png "overview" | ||
| ```bash | ||
| sudo arp-scan --interface=eth0 192.168.1.0/24 | ||
| ``` | ||
|
|
||
| ## Troubleshooting | ||
| Replace the interface and subnet with values appropriate for your environment (see the [Subnets](./SUBNETS.md) documentation). | ||
|
|
||
| If you can't see all devices detected, run `sudo arp-scan --interface=eth0 192.168.1.0/24` (change these based on your setup, read [Subnets](./SUBNETS.md) docs for details). This command has to be executed the NetAlertX container, not in the Home Assistant container. | ||
| Run this command **inside the NetAlertX container**, not inside the Home Assistant container. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify where the arp-scan command should run.
The "Missing devices" subsection shows an arp-scan command (lines 52–53) before explaining how to access the NetAlertX container (lines 60–84). However, line 58 instructs to run it "inside the NetAlertX container." This creates confusion—readers may try to run the first command before they know how to enter the container. Reorder so container access precedes the command, or clarify that the first command can run on the host while the test scan (line 87) runs inside the container.
🤖 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 `@docs/HOME_ASSISTANT.md` around lines 50 - 58, The “Missing devices” guidance
is unclear about where the first arp-scan command should be run. Update the
HOME_ASSISTANT.md flow so container access instructions come before any command
that must run inside NetAlertX, or explicitly state which scan runs on the host
versus inside the container. Keep the wording around arp-scan and the NetAlertX
container consistent so readers know the correct execution context before trying
the command.
| 4. Run a test `arp-scan`. | ||
|
|
||
| ```bash | ||
| / # sudo arp-scan --ignoredups --retry=6 192.168.1.0/24 --interface=eth0 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove unnecessary sudo inside container shell.
Line 87 shows sudo arp-scan run at a / # prompt inside the container. Since the shell is already root (/ # prompt), sudo is redundant and may fail if not installed. Remove sudo from the container-internal example.
📝 Suggested fix
- / # sudo arp-scan --ignoredups --retry=6 192.168.1.0/24 --interface=eth0
+ / # arp-scan --ignoredups --retry=6 192.168.1.0/24 --interface=eth0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| / # sudo arp-scan --ignoredups --retry=6 192.168.1.0/24 --interface=eth0 | |
| / # arp-scan --ignoredups --retry=6 192.168.1.0/24 --interface=eth0 |
🤖 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 `@docs/HOME_ASSISTANT.md` at line 87, Remove the redundant sudo from the
container shell example in HOME_ASSISTANT.md: the prompt already shows root
access, so the arp-scan command should be documented without sudo. Update the
example to keep only the direct arp-scan invocation so it matches how commands
are run inside the container.
| # Integration with Pi-hole | ||
|
|
||
| NetAlertX comes with 3 plugins suitable for integrating with your existing PiHole instance. The first plugin uses the v6 API, the second plugin is using a direct SQLite DB connection, the other leverages the `DHCP.leases` file generated by PiHole. You can combine multiple approaches and also supplement scans with other [plugins](/docs/PLUGINS.md). | ||
| NetAlertX includes three plugins for integrating with an existing Pi-hole installation. The first plugin imports devices through the Pi-hole v6 API, the second parses the `dhcp.leases` file generated by Pi-hole, and the third reads the Pi-hole SQLite database directly. You can use any of these approaches individually or combine them with each other and other [plugins](/docs/PLUGINS.md). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Absolute /docs/PLUGINS.md link will break in most renderers.
From docs/PIHOLE_GUIDE.md, the absolute path /docs/PLUGINS.md resolves incorrectly on GitHub (to github.com/docs/PLUGINS.md) and in MkDocs. Use a relative link: PLUGINS.md or ./PLUGINS.md.
📝 Suggested fix
-You can use any of these approaches individually or combine them with each other and other [plugins](/docs/PLUGINS.md).
+You can use any of these approaches individually or combine them with each other and other [plugins](PLUGINS.md).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| NetAlertX includes three plugins for integrating with an existing Pi-hole installation. The first plugin imports devices through the Pi-hole v6 API, the second parses the `dhcp.leases` file generated by Pi-hole, and the third reads the Pi-hole SQLite database directly. You can use any of these approaches individually or combine them with each other and other [plugins](/docs/PLUGINS.md). | |
| NetAlertX includes three plugins for integrating with an existing Pi-hole installation. The first plugin imports devices through the Pi-hole v6 API, the second parses the `dhcp.leases` file generated by Pi-hole, and the third reads the Pi-hole SQLite database directly. You can use any of these approaches individually or combine them with each other and other [plugins](PLUGINS.md). |
🤖 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 `@docs/PIHOLE_GUIDE.md` at line 3, The link in PIHOLE_GUIDE.md currently uses
an absolute /docs/PLUGINS.md path that breaks in common renderers; update the
markdown reference in the guide to use a relative link instead. Adjust the
existing PLUGINS.md link in the PIHOLE_GUIDE.md content so it resolves correctly
from the docs folder and remains portable across GitHub and MkDocs.
Merged
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
UI/Styling
Documentation