Next release#1690
Open
jokob-sk wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds a shared light-gray theme token, updates dark-theme text colors to use it, makes skeleton loading elements reusable spinner targets, wires spinner/skeleton behavior into device pages, and stores localized timestamps in rendered device link attributes. ChangesUI theme, loading skeletons, and link timestamps
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Comment on lines
+972
to
+975
| function resolveSpinnerTarget(explicitTarget = null) { | ||
| if (explicitTarget) { | ||
| return $(explicitTarget); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Invalid explicit target currently bypasses fallback target resolution.
When explicitTarget is provided but matches nothing, the function returns early and never tries active/visible .spinnerTarget fallbacks, causing unintended full-screen spinner placement.
Suggested fix
function resolveSpinnerTarget(explicitTarget = null) {
if (explicitTarget) {
- return $(explicitTarget);
+ const explicit = $(explicitTarget);
+ if (explicit.length) {
+ return explicit;
+ }
}📝 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
| function resolveSpinnerTarget(explicitTarget = null) { | |
| if (explicitTarget) { | |
| return $(explicitTarget); | |
| } | |
| function resolveSpinnerTarget(explicitTarget = null) { | |
| if (explicitTarget) { | |
| const explicit = $(explicitTarget); | |
| if (explicit.length) { | |
| return explicit; | |
| } | |
| } |
🤖 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/js/common.js` around lines 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.
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
Improvements