Refactor: Allow nchan scripts to exit if no listeners#2333
Conversation
WalkthroughRefactors the Nchan publish API (new publish signature and publish_noDupe replacing publish_md5), updates many callers to the new API and explicit args, removes monitor()/focus-based live-update pause behavior and related UI/config, adds a dummy client-side Nchan listener, and changes docker_load sleep cadence. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Nchan Script
participant NoDupe as publish_noDupe()
participant Pub as publish()
participant NChan as Nchan Server
participant Clients as Subscribers
Caller->>NoDupe: publish_noDupe(endpoint,message)
alt message changed or no known listener
NoDupe->>Pub: publish(endpoint,message,len,abort,abortTime)
Pub->>NChan: POST message
NChan-->>Pub: ack / no-subscriber
Pub-->>NoDupe: true/false
NoDupe-->>Caller: true/false
else duplicate and listener known
NoDupe-->>Caller: skip publish (no-op)
end
sequenceDiagram
participant DL as docker_load
loop periodic
DL->>DL: check output
alt output present
DL->>DL: publish("dockerload", output)
DL->>DL: sleep(1)
else no output
DL->>DL: sleep(10)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
emhttp/plugins/dynamix.docker.manager/nchan/docker_load (1)
22-28: Logic bug: docker stats “no containers” case won’t hit the slow path.shell_exec() returns an empty string when there’s no output; it’s rarely null. As written, you’ll almost always sleep(1), not sleep(10). Fix by checking for empty/whitespace output (and optionally common docker error strings you redirected to stdout with 2>&1).
Apply:
- publish_noDupe("dockerload", $output); - if ( $output === null ) { - // slow down publishing if no containers are running - sleep(10); - } else { - sleep(1); - } + publish_noDupe("dockerload", $output); + // Slow down if there's no meaningful output (no running containers or daemon not reachable) + if (!is_string($output) || trim($output) === '' || + stripos($output, 'error response from daemon') !== false || + stripos($output, 'cannot connect to the docker daemon') !== false) { + sleep(10); + } else { + sleep(1); + }This preserves the intent, reduces needless loop iterations, and still leverages publish_noDupe()’s default abort=true to exit when no listeners.
emhttp/plugins/dynamix/nchan/wg_poller (1)
18-28: Fix out-of-range unit index and overflow in scale()When $value is very large, $base can equal count($units), causing an out-of-bounds index. Also, the 1000-rounding bump can overflow $base without re-clamping. Clamp against the last valid index and guard the post-rounding increment.
Apply this diff:
-function scale($value, &$unit) { - $units = ['','Ki','Mi','Gi','Ti','Pi','Ei','Zi','Yi']; - $size = count($units); - $base = $value ? floor(log($value, 1024)) : 0; - if ($base>$size) $base = $size-1; - $value /= pow(1024, $base); - $decimals = $value>=100 ? 0 : ($value>=10 ? 1 : (round($value*100)%100===0 ? 0 : 2)); - if (round($value,-1)==1000) {$value = 1; $base++;} - $unit = $units[$base].'B'; - return number_format($value, $decimals, '.', $value>9999 ? ',':''); -} +function scale($value, &$unit) { + $units = ['','Ki','Mi','Gi','Ti','Pi','Ei','Zi','Yi']; + $last = count($units) - 1; // last valid index + $base = $value ? (int)floor(log($value, 1024)) : 0; + if ($base > $last) $base = $last; + $value /= pow(1024, $base); + $decimals = $value>=100 ? 0 : ($value>=10 ? 1 : (round($value*100)%100===0 ? 0 : 2)); + if (round($value,-1) == 1000 && $base < $last) { + $value = 1; + $base++; + } + $unit = $units[$base].'B'; + return number_format($value, $decimals, '.', $value>9999 ? ',':''); +}
🧹 Nitpick comments (8)
emhttp/plugins/dynamix/include/publish.php (1)
123-133: Make the “no-op” path explicit: return false when not publishing.Right now the function falls through and returns null when the message is unchanged and a listener is present. Returning false explicitly keeps a consistent boolean interface with publish() and simplifies consumers.
Apply this diff:
- if ( ($message !== ($msg_old[$endpoint]??null)) || !$listener[$endpoint]) { + if ( ($message !== ($msg_old[$endpoint]??null)) || !$listener[$endpoint]) { $msg_old[$endpoint] = $message; $msg_time[$endpoint] = time(); $return = publish($endpoint, $message, $len, $abort, $abortTime); // if no listener, keep publishing whether or not its the same message. $listener[$endpoint] = $return ? true : false; return $return; - } + } else { + return false; + }emhttp/plugins/dynamix/nchan/update_1 (1)
126-126: Switch to publish_noDupe() aligns with dedupe + “exit if no listeners” objective — confirm semantics.
- With publish_noDupe('update1', $echo) defaulting to abort=true, this script will exit when there are no subscribers (after the abort window). Verify this is intended for update_1.
- Minor: $md5_old defined earlier is now unused after moving away from MD5-based publishing. Consider removing it as cleanup.
emhttp/plugins/dynamix/nchan/ups_status (1)
135-135: publish_noDupe(): behavior change to exit if no listeners — validate intent and do small cleanup.
- This call uses defaults (len=1, abort=true), so the script will terminate when there are no subscribers. Confirm this matches the desired behavior for ups_status.
- Nit: $md5_old is still declared above but unused; safe to remove.
- Nit: You cast OVERRIDE_UPS_CAPACITY after htmlspecialchars(). Since you cast to (int) anyway, htmlspecialchars() is redundant here.
emhttp/plugins/dynamix/nchan/file_manager (2)
255-255: Prefer publish()’s internal param overloading to avoid hard-coding len=1.Per team convention, publish() sorts out overloads. You can pass only the abort flag and let len default to 1.
Apply:
- publish('filemonitor', file_exists($active) ? 1 : 0,1,false); + publish('filemonitor', file_exists($active) ? 1 : 0, false);
258-258: Same here: rely on publish()’s overloads; pass only abort=false.Apply:
- publish('filemanager', json_encode($reply),1,false); + publish('filemanager', json_encode($reply), false);emhttp/plugins/dynamix/nchan/wg_poller (2)
38-38: Prevent unit cross-talk; use distinct vars for RX/TX unitsReusing $unit by reference for both values is easy to misread and brittle. Using separate variables improves clarity.
Apply this diff:
- if (count($row)>5) $echo[] = [$row[0], $row[5]?$now-$row[5]:0, scale($row[6],$unit)." $unit", scale($row[7],$unit)." $unit"]; + if (count($row)>5) { + $rx = scale($row[6], $unit_rx)." $unit_rx"; + $tx = scale($row[7], $unit_tx)." $unit_tx"; + $echo[] = [$row[0], $row[5] ? $now - $row[5] : 0, $rx, $tx]; + }
30-30: Remove unused legacy variable $md5_oldLeftover from publish_md5 path; it’s now unused.
Apply this diff:
-$md5_old = -1;emhttp/plugins/dynamix/nchan/vm_dashusage (1)
24-24: Remove unused legacy variable $md5_oldThis isn’t used anymore after migrating away from publish_md5.
Apply this diff:
-$md5_old = -1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
emhttp/plugins/dynamix.docker.manager/nchan/docker_load(1 hunks)emhttp/plugins/dynamix.vm.manager/nchan/vm_usage(2 hunks)emhttp/plugins/dynamix/include/publish.php(5 hunks)emhttp/plugins/dynamix/nchan/file_manager(1 hunks)emhttp/plugins/dynamix/nchan/session_check(1 hunks)emhttp/plugins/dynamix/nchan/update_1(2 hunks)emhttp/plugins/dynamix/nchan/update_2(1 hunks)emhttp/plugins/dynamix/nchan/update_3(1 hunks)emhttp/plugins/dynamix/nchan/ups_status(2 hunks)emhttp/plugins/dynamix/nchan/vm_dashusage(2 hunks)emhttp/plugins/dynamix/nchan/wg_poller(3 hunks)emhttp/plugins/dynamix/nchan/wlan0(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#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.
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
PR: unraid/webgui#2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/nchan/update_2emhttp/plugins/dynamix/nchan/vm_dashusageemhttp/plugins/dynamix.docker.manager/nchan/docker_loademhttp/plugins/dynamix/include/publish.php
📚 Learning: 2025-08-10T23:33:40.338Z
Learnt from: Squidly271
PR: unraid/webgui#2326
File: emhttp/plugins/dynamix/include/publish.php:46-55
Timestamp: 2025-08-10T23:33:40.338Z
Learning: In the unraid/webgui codebase, the publish() and publish_md5() functions in emhttp/plugins/dynamix/include/publish.php deliberately do not support 0-second timeouts for the abort mechanism. The use of the elvis operator (?:) for timeout parameters is intentional, as timeout values of 0 are not relevant and not supported. The minimum valid timeout should be greater than 0 seconds.
Applied to files:
emhttp/plugins/dynamix.docker.manager/nchan/docker_load
🔇 Additional comments (14)
emhttp/plugins/dynamix/nchan/session_check (1)
3-3: Header year bump only — LGTMNo functional changes. Safe and consistent with PR-wide copyright updates.
emhttp/plugins/dynamix/include/publish.php (3)
82-99: Abort-on-no-subscribers behavior is clear and robust — LGTMForcing $reply=false when no subscribers and gating exit on elapsed $abortTime meets the PR objective and simplifies upstream logic that checks publish() return values.
103-110: Per-endpoint dedupe state — LGTMStatic endpoint-scoped msg_old/msg_time/listener is a clean replacement for MD5-based dedupe and reduces CPU work on hot loops.
33-41: No legacypublish()calls detected—please verify external usageOur search didn’t find any internal calls to
publish()with a boolean as the third argument, so there’s no immediate breakage within this repo. However, per team preference for backwards compatibility, you can reintroduce minimal overload-handling in emhttp/plugins/dynamix/include/publish.php:function publish($endpoint, $message, $len=1, $abort=true, $abortTime=30) { static $abortStart = [], $com = [], $lens = []; + // Back-compat: treat a boolean third arg as the legacy “abort” flag + if (func_num_args() === 3 && is_bool($len)) { + $abort = $len; + $len = 1; + }Please confirm whether any external plugins or scripts call
publish()with only three arguments and a boolean third parameter. If they do, keep the shim; otherwise it can safely be omitted.emhttp/plugins/dynamix/nchan/update_3 (1)
162-162: Switch to publish_noDupe — LGTMMigration away from publish_md5 aligns with the new dedupe approach and default abort-on-no-listeners behavior.
emhttp/plugins/dynamix.vm.manager/nchan/vm_usage (2)
3-4: Header year updates — LGTM
90-90: Switch to publish_noDupe — LGTMMatches the new API and desired “exit when no listeners” semantics.
emhttp/plugins/dynamix/nchan/wlan0 (1)
69-69: Switch to publish_noDupe — LGTMConsistent with the refactor across nchan scripts.
emhttp/plugins/dynamix/nchan/update_1 (1)
3-3: LGTM: header year update is correct.emhttp/plugins/dynamix/nchan/ups_status (1)
3-3: LGTM: header year update is correct.emhttp/plugins/dynamix/nchan/update_2 (1)
502-502: Migration to publish_noDupe() looks good; dedupe + abort-on-no-listeners achieved.This change matches the refactor goal and should reduce redundant messages while letting the script exit when no subscribers.
emhttp/plugins/dynamix/nchan/wg_poller (1)
40-40: Verified:publish_noDupe()Defaults Ensure Dedupe and Abort SemanticsI’ve confirmed in
emhttp/plugins/dynamix/include/publish.php(lines 103–108) that the signature is:function publish_noDupe($endpoint, $message, $len = 1, $abort = true, $abortTime = 30) { … }–
$abortdefaults totrue, and$lento1, matching the PR’s goal to dedupe messages and abort when there are no listeners.
Everything looks correct—approving these changes.emhttp/plugins/dynamix/nchan/vm_dashusage (2)
3-4: LGTM: Updated copyrightYear ranges updated correctly.
112-112: publish_noDupe defaults confirmedThe signature in
emhttp/plugins/dynamix/include/publish.phpis:function publish_noDupe($endpoint, $message, $len=1, $abort=true, $abortTime=30) { … }–
$len=1ensures single‐byte length
–$abort=trueenforces “exit if no listeners”
–$abortTime=30seconds for the abort timeoutNo changes needed—this call already uses the correct defaults.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/nchan/parity_list (1)
156-157: Typo in comment.Minor readability nit: “timeoutto” should be “timeout to”.
Apply this diff:
- //publish to the dummy listener with the timeoutto see if there are any subscribers + // publish to the dummy listener with the timeout to see if there are any subscribersemhttp/plugins/dynamix/ArrayOperation.page (1)
509-513: Optional: Stop the dummy listener on page unload (symmetry with other subscribers).For consistency with mymonitor/devices/paritymonitor cleanup and to prevent dangling sockets during navigation, stop mainPingListener on unload.
Add this inside window.onunload alongside the others:
try { mainPingListener.stop(); } catch(e) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/ArrayOperation.page(1 hunks)emhttp/plugins/dynamix/nchan/device_list(1 hunks)emhttp/plugins/dynamix/nchan/parity_list(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#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.
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
PR: unraid/webgui#2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix/nchan/device_list
🔇 Additional comments (2)
emhttp/plugins/dynamix/nchan/device_list (1)
569-576: Ignore JSON-encoding change – publish_noDupe already encodes arrays
Thepublish_noDupe()implementation (include/publish.php lines 112–114) checksis_array($message)and appliesjson_encode()before sending. Since$echois a PHP array, it will automatically be serialized to JSON bypublish_noDupe, matching the frontend’s use ofJSON.parse. No update to the call site indevice_listis required.Likely an incorrect or invalid review comment.
emhttp/plugins/dynamix/ArrayOperation.page (1)
509-513: LGTM: Dummy ping listener initialization is correct.The added no-op WebSocket subscriber is straightforward and aligns with the “exit if no listeners” design.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/include/publish.php (1)
103-134: Enforce JSON‐encode error handling and consistent boolean return in publish_noDupeThe proposed changes can be applied safely—none of the ~20 call sites branch on a
nullreturn—so we should:• Add
JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODEtojson_encode()
• Log and returnfalseon JSON failures for symmetry withpublish()
• Always return abool:
–truewhen a listener is known or the publish succeeded
–falseotherwise (including skipped publishes)Apply:
--- a/emhttp/plugins/dynamix/include/publish.php +++ b/emhttp/plugins/dynamix/include/publish.php @@ -103,11 +103,22 @@ function publish_noDupe($endpoint, $message, $len=1, $abort=true, $abortTime=30) { static $listener = []; if (!isset($listener[$endpoint])) { $listener[$endpoint] = false; } - if (is_array($message)) { - $message = json_encode($message); - } + if (is_array($message)) { + $json = json_encode( + $message, + JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE + ); + if ($json === false) { + my_logger( + 'publish_noDupe: json_encode() failed: '.json_last_error_msg(), + 'publish_noDupe' + ); + return false; + } + $message = $json; + } // if abort is set, republish the message even if it hasn't changed // after $timeout seconds to check for subscribers and exit accordingly @@ -133,6 +144,8 @@ function publish_noDupe($endpoint, $message, $len=1, $abort=true, $abortTime=30) { $listener[$endpoint] = $return ? true : false; return $return; } + + return (bool)$listener[$endpoint]; }No callers rely on
nullvs.false—this makes the API predictable.
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/publish.php (1)
13-15: Resolved: Avoid undefined-index on DOCUMENT_ROOT.Thanks for addressing the earlier feedback by using the null-coalescing operator. This prevents CLI notices and keeps logs clean.
🧹 Nitpick comments (5)
emhttp/plugins/dynamix/include/publish.php (5)
39-41: Handle json_encode failures explicitly (invalid UTF-8).json_encode can return false; logging and short-circuiting avoids posting "false" or empty bodies.
Apply:
- if ( is_array($message) ) { - $message = json_encode($message); - } + if ( is_array($message) ) { + $json = json_encode($message, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + if ($json === false) { + my_logger('json_encode() failed: '.json_last_error_msg(), 'publish'); + return false; + } + $message = $json; + }
51-57: Use a standard JSON Accept header.'Accept: application/json' is the conventional media type; improves interoperability with proxies/middleware.
Apply:
- CURLOPT_HTTPHEADER => ['Accept:text/json'], + CURLOPT_HTTPHEADER => ['Accept: application/json'],
61-78: Derive HTTP status via curl_getinfo instead of regex; capture before closing handle.Parsing curl error text is brittle and locale-dependent. Use CURLINFO_RESPONSE_CODE and only then close/unset the handle.
Apply:
- $reply = curl_exec($com[$endpoint]); - $err = curl_error($com[$endpoint]); + $reply = curl_exec($com[$endpoint]); + // Must get HTTP code before closing the handle + $http = curl_getinfo($com[$endpoint], CURLINFO_RESPONSE_CODE); + $err = curl_error($com[$endpoint]); if ($err) { my_logger("curl error: $err endpoint: $endpoint"); - curl_close($com[$endpoint]); - unset($com[$endpoint]); - - preg_match_all("/[0-9]+/",$err,$matches); - // 500: out of shared memory when creating a channel - // 507: out of shared memory publishing a message - - if ( ($matches[0][0] ?? "") == 507 || ($matches[0][0] ?? "") == 500 ) { + curl_close($com[$endpoint]); + unset($com[$endpoint]); + // 500: out of shared memory when creating a channel + // 507: out of shared memory publishing a message + if (in_array((int)$http, [500, 507], true)) { my_logger("Nchan out of shared memory. Reloading nginx"); // prevent multiple attempts at restarting from other scripts using publish.php touch("/tmp/publishPaused"); exec("/etc/rc.d/rc.nginx restart"); @unlink("/tmp/publishPaused"); } }
82-99: Minor: prefer unset over assigning null for timers to avoid stale keys.This keeps the per-endpoint state arrays a bit cleaner over long runtimes.
Apply:
- // a subscriber is present. Reset the abort timer if it's set - $abortStart[$endpoint] = null; + // a subscriber is present. Reset the abort timer if it's set + unset($abortStart[$endpoint]);
18-22: Edge case: allow "0" payloads in curl_socket().The current truthiness check drops numeric zero or empty JSON strings. Use a length check to permit valid zero-ish payloads.
Apply:
- if ($message) curl_setopt_array($com, [CURLOPT_POSTFIELDS => $message, CURLOPT_POST => 1]); + if ($message !== '') curl_setopt_array($com, [CURLOPT_POSTFIELDS => $message, CURLOPT_POST => 1]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/publish.php(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#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.
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
PR: unraid/webgui#2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix/include/publish.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
emhttp/plugins/dynamix/DashStats.page (1)
2423-2423: Switch to start() only is fine; optional visibility-based gating can reduce background loadDropping monitor() removes focus/visibility throttling, so streams will continue while the tab is hidden. If that’s intentional per the refactor, no action needed. If you want to keep backend and client load down when the tab isn’t visible, consider gating subscribers with document.visibilityState (start on visible, stop on hidden) as a small enhancement.
Example approach (outside the shown ranges):
// Once at init: function startStreams() { dashboard.start(); <?if ($vmusage == "Y"):?> vmdashusage.start(); <?endif;?> <?if ($apcupsd):?> apcups.start(); <?endif;?> } function stopStreams() { if (dashboard.stop) dashboard.stop(); <?if ($vmusage == "Y"):?> if (vmdashusage.stop) vmdashusage.stop(); <?endif;?> <?if ($apcupsd):?> if (apcups.stop) apcups.stop(); <?endif;?> } document.addEventListener('visibilitychange', () => { if (document.visibilityState === 'visible') startStreams(); else stopStreams(); }); // On load: startStreams();Also applies to: 2425-2425, 2428-2428
emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php (2)
550-563: Auto-reload should also respect Shadowbox modals (and similar overlays)With the nchanPaused guard removed, an active Shadowbox modal (used by openBox) won’t be detected by dialogOpen(), so the page may reload mid-operation. Expand the gate to include Shadowbox (or its container) visibility.
Apply this minimal diff to avoid reload while a Shadowbox is open:
- if (! dialogOpen() ) { + if (!dialogOpen() && !$('#sb-container').is(':visible')) { location.reload(); }Alternatively (preferred for cohesion), broaden dialogOpen() itself to centralize the logic:
function dialogOpen() { return ( $('.sweet-alert').is(':visible') || $('.swal-overlay--show-modal').is(':visible') || ($('#sb-container').length && $('#sb-container').is(':visible')) || // Shadowbox (window.Shadowbox && Shadowbox.isOpen === true) // extra safety ); }Verification request:
- Please verify that Shadowbox renders with #sb-container (or update the selector accordingly) in current builds.
15-17: Harden legacy Nchan monitor shim and verify load orderThe inline snippet in HeadInlineJS.php runs before we’ve seen any evidence that the NchanSubscriber library (dynamix.js) is loaded. As a result, wrapping the prototype assignment in a one-time guard and deferring installation via
window.loadis the safest approach to avoid a ReferenceError.Please verify that your page includes
dynamix.js(which defineswindow.NchanSubscriber) either before this inline block or via your deferred install listener. If it does, the immediateattach()call will take effect; otherwise theload-event listener ensures compatibility.Locations to check:
- emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php (the inline shim)
- The
<script src=".../dynamix.js">include in your DefaultPageLayout templatesApplied diff:
-// Legacy code. No longer used in webGUI, and its purpose is removed, but plugins might still reference this prototype and its a fatal error to remove it. -var subscribers = []; -NchanSubscriber.prototype.monitor = function(){subscribers.push(this);} +// Legacy shim. No longer used in webGUI. Kept for compatibility because plugins might still reference this prototype and it's a fatal error to remove it. +// Preserve/initialize the global subscribers array for legacy consumers. +var subscribers = window.subscribers || []; +window.subscribers = subscribers; + +// Safely install the legacy monitor method only if/when NchanSubscriber is available. +(function installLegacyNchanMonitor() { + function attach() { + if (window.NchanSubscriber && window.NchanSubscriber.prototype && !window.NchanSubscriber.prototype.monitor) { + window.NchanSubscriber.prototype.monitor = function(){ subscribers.push(this); }; + try { + console.warn( + 'Deprecated: NchanSubscriber.prototype.monitor is a no-op shim for compatibility and may be removed in a future release.' + ); + } catch(e) {} + } + } + // Attempt now, and also once on window load in case NchanSubscriber loads later. + attach(); + window.addEventListener('load', function lateAttach() { + attach(); + window.removeEventListener('load', lateAttach); + }); +}());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
emhttp/plugins/dynamix.docker.manager/DockerContainers.page(2 hunks)emhttp/plugins/dynamix.vm.manager/VMUsageStats.page(2 hunks)emhttp/plugins/dynamix/ArrayOperation.page(3 hunks)emhttp/plugins/dynamix/DashStats.page(2 hunks)emhttp/plugins/dynamix/DisplaySettings.page(0 hunks)emhttp/plugins/dynamix/default.cfg(0 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php(0 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php(2 hunks)
💤 Files with no reviewable changes (3)
- emhttp/plugins/dynamix/DisplaySettings.page
- emhttp/plugins/dynamix/default.cfg
- emhttp/plugins/dynamix/include/DefaultPageLayout/BodyInlineJS.php
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/ArrayOperation.page
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#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.
🔇 Additional comments (4)
emhttp/plugins/dynamix/DashStats.page (1)
2-2: Removing ":stop" on ups_status aligns with exit-on-no-listeners; verify publisher/subscriber naming stays consistentThis matches the new lifecycle where scripts self-terminate without explicit stop hints. Please double-check that the ups_status publisher still emits on the apcups channel (the JS subscribes to /sub/apcups) and that it cleanly exits when no client subscribes. A naming drift between publisher (ups_status) and subscriber (apcups) would lead to either no updates or an idle script.
emhttp/plugins/dynamix.vm.manager/VMUsageStats.page (3)
3-3: LGTM: Channel name simplified correctly.The channel name change from "vm_usage:stop" to "vm_usage" aligns with the broader refactoring to use the new publish_noDupe mechanism, which handles deduplication internally without requiring channel suffixes.
35-35: LGTM: Client-side initialization simplified.Removing the
.monitor()call is consistent with the PR objective to eliminate monitor/focus-based live-update pause behavior. The simplifiedvmusage.start()call maintains the subscription functionality while removing the deprecated monitoring feature.
28-32: Ensure there’s a matching publisher for thevm_usagechannelI wasn’t able to find any server-side publish calls for
/pub/vm_usage(nopublish_noDupe,publish_md5or direct Nchan API). Before shipping, please confirm that:
- A backend routine actually invokes something like
at the same cadence as your stats collector (e.g. in a cron or daemon).publish_noDupe('/pub/vm_usage', json_encode($htmlSnippet), …);- The payload remains a JSON-encoded HTML string (so
JSON.parse(msg)yields a<tr>…</tr>snippet, not an[object Object]).Locations of interest:
- emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php →
get_vm_usage_stats()builds the raw data- (wherever you wire that into an Nchan publish call)
Please verify or add the publish logic so the client’s
NchanSubscriber('/sub/vm_usage')actually receives the expected HTML-wrapped JSON.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/nchan/wg_poller (1)
18-28: Prevent unit-index overflow and clamp inputs in wg_scale()Rounding up to the next unit can push $base past the last index (Yi), causing undefined offset. Also clamp/cast $value defensively and fix the boundary check.
Apply this diff:
function wg_scale($value, &$unit) { $units = ['','Ki','Mi','Gi','Ti','Pi','Ei','Zi','Yi']; $size = count($units); - $base = $value ? floor(log($value, 1024)) : 0; - if ($base>$size) $base = $size-1; - $value /= pow(1024, $base); + $value = max(0, (float)$value); + $base = $value ? (int)floor(log($value, 1024)) : 0; + if ($base >= $size) $base = $size - 1; + $value /= pow(1024, $base); $decimals = $value>=100 ? 0 : ($value>=10 ? 1 : (round($value*100)%100===0 ? 0 : 2)); - if (round($value,-1)==1000) {$value = 1; $base++;} + if (round($value, -1) == 1000 && $base < $size - 1) { $value = 1; $base++; } $unit = $units[$base].'B'; return number_format($value, $decimals, '.', $value>9999 ? ',':''); }
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php (1)
549-556: Avoid potential duplicate reload timers when setTimerReload is called repeatedly.Clear any existing interval before creating a new one to prevent multiple intervals running concurrently.
Apply this diff:
function setTimerReload() { - timers.reload = setInterval(function(){ + if (timers.reload) clearInterval(timers.reload); + timers.reload = setInterval(function(){ if (! dialogOpen() ) { location.reload(); } },<?=$myPage['Load'] * 60000?>); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/include/DefaultPageLayout/HeadInlineJS.php(2 hunks)emhttp/plugins/dynamix/nchan/wg_poller(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#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.
📚 Learning: 2025-05-31T05:10:13.120Z
Learnt from: Squidly271
PR: unraid/webgui#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.
Applied to files:
emhttp/plugins/dynamix/nchan/wg_poller
🔇 Additional comments (2)
emhttp/plugins/dynamix/nchan/wg_poller (2)
3-3: LGTM: Copyright year updateHeader year bump looks correct.
14-16: Use long open tags for CLI portabilityShort open tags (
<?) can be disabled; prefer<?phpto avoid environment dependency (especially for CLI).Apply this diff:
-<? +<?phpPlease confirm short_open_tag is enabled in the target runtime if you prefer to keep
<?.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/nchan/update_3 (1)
74-85: Fix inter-sample delta ($ts) logic; current code under-reports speeds over time$ts uses ($time1 - $time0) where $time0 is never updated, so the denominator grows each iteration while the numerator is only the last-sample delta. This progressively understates rx/tx rates.
Suggested refactor:
while (true) { - // check for language changes + // roll timestamps for this sample + $time0 = $time1; + $time1 = microtime(true); + // check for language changes extract(parse_plugin_cfg('dynamix',true)); ... - sleep(1); - $time1 = microtime(true); + sleep(1); }Also applies to: 164-166
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/nchan/parity_list (1)
151-151: Double-check frontend handling of the completed case ($echo === "").When parity completes, $echo is an empty string. If the frontend always JSON.parse’s the message body for the 'parity' topic, JSON.parse('') will throw. Arrays are handled automatically server-side, but empty strings are not auto-encoded. Please verify the subscriber code guards against empty payloads before JSON.parse or expects a JSON string ("").
Run this script to locate the relevant handlers and confirm they gate JSON.parse on non-empty payloads:
#!/bin/bash set -euo pipefail # Find parity-topic handlers and nearby JSON.parse calls rg -n --line-number -A 5 -B 5 '(parity|mainPingListener|JSON\.parse)' \ emhttp 2>/dev/null || true # If ArrayOperation.page exists, show focused context fd -a 'ArrayOperation.page' 2>/dev/null | xargs -r -I{} rg -n -A 5 -B 5 '(parity|JSON\.parse)' {}
🧹 Nitpick comments (9)
emhttp/plugins/dynamix.vm.manager/nchan/vm_usage (3)
68-71: Clean up $echo initialization and assign once after the loop$echo is initialized twice to an array and then repeatedly reassigned inside the loop. Initialize to a string and assign once after the foreach to improve clarity (and avoid type flip).
Apply:
- $echo = []; - $echo = []; $echodata = ""; $running = 0; @@ - $echo = $echodata ; - } + } + $echo = $echodata; if ($running < 1) $echo = "<tr><td colspan='7' style='text-align:center;padding-top:12px'>"._('No VMs running')."</td></tr>";Also applies to: 86-88
17-17: Remove unused variable from legacy md5 flow$md5_old is no longer used after dropping publish_md5(); remove to avoid confusion.
-$md5_old = -1;
13-15: Avoid closing and reopening PHP in a CLI scriptThe early closing tag followed by a second open tag is unnecessary and can be dropped to keep a single PHP block.
-?> -<?emhttp/plugins/dynamix/nchan/parity_list (1)
150-157: Move the ping publish first to short‑circuit when there are no listeners (and fix comment typo).Send the dummy ping before the other publishes so the script exits immediately when there are no subscribers, avoiding unnecessary work and publishes. Also fixes the “timeoutto” typo and improves comment clarity.
- // publish without a timeout for no listeners - publish('parity', $echo,1,false); - publish('paritymonitor', $spot > 0 ? 1 : 0,1,false); - publish('fsState', $fsState,1,false); - publish('mymonitor', $process,1,false); - - //publish to the dummy listener with the timeoutto see if there are any subscribers - publish('mainPingListener',"ping"); + // Ping a dummy listener (with default exit-on-no-listener semantics) to short-circuit this loop if there are no subscribers + publish('mainPingListener', "ping"); + // Publish without a timeout for no listeners + publish('parity', $echo, 1, false); + publish('paritymonitor', $spot > 0 ? 1 : 0, 1, false); + publish('fsState', $fsState, 1, false); + publish('mymonitor', $process, 1, false);emhttp/plugins/dynamix/nchan/update_3 (2)
162-162: Confirm dedupe parity vs prior publish_md5(..., true) usageThe previous call used md5-based dedupe. If dedupe is still desired for this channel, consider publish_noDupe() to keep parity with the old behavior; otherwise, the channel may receive repetitive identical payloads.
Proposed change:
- publish('update3',$echo); + publish_noDupe('update3', $echo);
162-162: Optional: Tune abortTime to reduce idle-exit latencyDefault abortTime is 30s. Given this script publishes every 1s, consider a shorter abort time (e.g., 10s) to stop sooner when there are no subscribers.
Example:
- publish('update3',$echo); + publish('update3', $echo, 1, true, 10);emhttp/plugins/dynamix/nchan/update_1 (2)
126-126: Confirm dedupe parity vs prior publish_md5(..., true) usageIf the intent was to suppress redundant updates like before, switch to publish_noDupe(); otherwise identical payloads will be published each tick.
Proposed change:
- publish('update1',$echo); + publish_noDupe('update1', $echo);Also consider explicit abortTime if you want faster idle-exit for a 5s cadence, e.g. 15–20s:
- publish('update1',$echo); + publish('update1', $echo, 1, true, 20);
3-5: Verify copyright year consistencyThe topline was updated to 2005–2025; if Bergware International contributions continued in 2024–2025, consider aligning the second line as well.
Example:
- * Copyright 2012-2023, Bergware International. + * Copyright 2012-2025, Bergware International.emhttp/plugins/dynamix/nchan/vm_dashusage (1)
24-24: Remove leftover md5-related variable$md5_old appears to be a vestige from the previous md5-based publish flow and is now unused. Please remove to avoid confusion.
-$md5_old = -1;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
emhttp/plugins/dynamix.docker.manager/nchan/docker_load(1 hunks)emhttp/plugins/dynamix.vm.manager/nchan/vm_usage(2 hunks)emhttp/plugins/dynamix/nchan/device_list(1 hunks)emhttp/plugins/dynamix/nchan/parity_list(1 hunks)emhttp/plugins/dynamix/nchan/update_1(2 hunks)emhttp/plugins/dynamix/nchan/update_2(1 hunks)emhttp/plugins/dynamix/nchan/update_3(1 hunks)emhttp/plugins/dynamix/nchan/ups_status(2 hunks)emhttp/plugins/dynamix/nchan/vm_dashusage(2 hunks)emhttp/plugins/dynamix/nchan/wg_poller(3 hunks)emhttp/plugins/dynamix/nchan/wlan0(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- emhttp/plugins/dynamix.docker.manager/nchan/docker_load
- emhttp/plugins/dynamix/nchan/ups_status
- emhttp/plugins/dynamix/nchan/wg_poller
- emhttp/plugins/dynamix/nchan/device_list
- emhttp/plugins/dynamix/nchan/wlan0
- emhttp/plugins/dynamix/nchan/update_2
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/nchan/wg_poller:39-41
Timestamp: 2025-08-13T05:09:31.948Z
Learning: In the Unraid webGUI nchan system, the publish() function will automatically exit the script when there are no listeners, eliminating the need to manually check return values and break out of loops in nchan poller scripts.
Learnt from: Squidly271
PR: unraid/webgui#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.
📚 Learning: 2025-08-13T02:06:47.712Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/nchan/parity_list:150-158
Timestamp: 2025-08-13T02:06:47.712Z
Learning: In the unraid/webgui codebase, both publish() and publish_noDupe() functions in emhttp/plugins/dynamix/include/publish.php automatically detect arrays passed as the $message parameter and convert them to JSON strings using json_encode() internally. This eliminates the need for manual json_encode() calls at call sites - arrays are automatically converted to JSON while strings are passed through unchanged.
Applied to files:
emhttp/plugins/dynamix/nchan/parity_list
📚 Learning: 2025-08-13T02:06:47.712Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/nchan/parity_list:150-158
Timestamp: 2025-08-13T02:06:47.712Z
Learning: In the unraid/webgui codebase, the publish() and publish_noDupe() functions in emhttp/plugins/dynamix/include/publish.php automatically handle JSON encoding when arrays are passed to them, eliminating the need for manual json_encode() calls at the call sites.
Applied to files:
emhttp/plugins/dynamix/nchan/parity_list
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
PR: unraid/webgui#2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix/nchan/parity_list
📚 Learning: 2025-08-13T03:13:53.283Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Applied to files:
emhttp/plugins/dynamix/nchan/parity_list
📚 Learning: 2025-05-31T05:10:13.120Z
Learnt from: Squidly271
PR: unraid/webgui#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.
Applied to files:
emhttp/plugins/dynamix/nchan/parity_list
🔇 Additional comments (5)
emhttp/plugins/dynamix.vm.manager/nchan/vm_usage (1)
3-4: LGTM: copyright year updates are correctYears updated consistently; no issues.
emhttp/plugins/dynamix/nchan/parity_list (1)
151-154: LGTM: publish() usage aligns with the new API and array auto-JSON behavior.Passing $echo directly (array or string) is correct given publish()’s internal array→JSON handling in publish.php. The signature and per-topic flags look consistent.
emhttp/plugins/dynamix/nchan/update_3 (1)
162-162: Switch to new publish() is correct and aligns with auto-exit semanticsLGTM. This matches the updated publish API and the PR goal of letting scripts exit when there are no listeners. Per our prior learning for this project, publish() will auto-exit when there are no subscribers, so no return checks or loop breaks are needed.
emhttp/plugins/dynamix/nchan/update_1 (1)
126-126: Switch to new publish() is correct and aligns with auto-exit semanticsLGTM. This matches the new API and the PR objective (scripts exiting when no listeners), so no extra loop control is required.
emhttp/plugins/dynamix/nchan/vm_dashusage (1)
3-4: Copyright year updates — LGTMHeader years updated correctly. No further action needed.
…nd auto exit when finished
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
emhttp/plugins/dynamix/scripts/install_key (1)
17-21: Optional: clarify intent and use boolean over “1” for readability (if supported)If publish() supports boolean true as an alias for len=1 (as per recent overloading work), this improves readability. A brief comment also makes the no-abort rationale obvious to future readers.
- publish('plugins', $message,1,false); + // Keep running even if there are no listeners (do not abort this CLI-driven flow) + publish('plugins', $message, true, false);emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
181-190: Optional: centralize nchan write behavior to avoid duplicationThe write(...$messages) + publish('plugins', …, 1, false) pattern is repeated in multiple scripts (plugin, language, multiplugin). Consider factoring this into a small helper (e.g., publish_plugins($message)) in a shared include to keep behavior consistent and simplify future tweaks.
Example of a thin wrapper you could add (in an appropriate shared include):
function publish_plugins($message, $abort=false, $len=true) { // boolean $len=true can be treated as 1 by publish()’s internal overloading return publish('plugins', $message, $len, $abort); }Then here:
- publish('plugins', $message,1,false); + publish_plugins($message);emhttp/plugins/dynamix.plugin.manager/scripts/language (1)
70-79: Optional: prefer boolean true over magic number and document intent (if overloading is enabled)For consistency with the publish() overloading work and readability, consider using true in place of 1, and add a short comment explaining abort=false.
- publish('plugins', $message,1,false); + // Do not abort the script if there are no listeners (UI closed) + publish('plugins', $message, true, false);emhttp/plugins/dynamix.plugin.manager/scripts/multiplugin (1)
23-32: Optional: deduplicate write/publish logic across scriptsSame suggestion as in plugin/language: consider a shared helper (e.g., publish_plugins($message)) to reduce duplication and ensure consistent len/abort behavior everywhere.
Within this file:
- publish('plugins', $message,1,false); + publish_plugins($message);Helper (placed in a common include such as publish.php or Wrappers.php):
function publish_plugins($message, $abort=false, $len=true) { return publish('plugins', $message, $len, $abort); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
emhttp/plugins/dynamix.docker.manager/scripts/container_size(1 hunks)emhttp/plugins/dynamix.docker.manager/scripts/update_container(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/language(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/multiplugin(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/plugin(1 hunks)emhttp/plugins/dynamix/scripts/diagnostics(1 hunks)emhttp/plugins/dynamix/scripts/install_key(1 hunks)emhttp/plugins/dynamix/scripts/newperms(1 hunks)emhttp/plugins/dynamix/scripts/ssd_trim(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/nchan/wg_poller:39-41
Timestamp: 2025-08-13T05:09:31.948Z
Learning: In the Unraid webGUI nchan system, the publish() function will automatically exit the script when there are no listeners, eliminating the need to manually check return values and break out of loops in nchan poller scripts.
Learnt from: Squidly271
PR: unraid/webgui#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.
📚 Learning: 2025-08-13T05:09:31.948Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix/nchan/wg_poller:39-41
Timestamp: 2025-08-13T05:09:31.948Z
Learning: In the Unraid webGUI nchan system, the publish() function will automatically exit the script when there are no listeners, eliminating the need to manually check return values and break out of loops in nchan poller scripts.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/languageemhttp/plugins/dynamix.plugin.manager/scripts/pluginemhttp/plugins/dynamix/scripts/newpermsemhttp/plugins/dynamix.plugin.manager/scripts/multipluginemhttp/plugins/dynamix.docker.manager/scripts/update_container
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
PR: unraid/webgui#2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/pluginemhttp/plugins/dynamix/scripts/diagnosticsemhttp/plugins/dynamix.docker.manager/scripts/container_sizeemhttp/plugins/dynamix.plugin.manager/scripts/multipluginemhttp/plugins/dynamix/scripts/ssd_trimemhttp/plugins/dynamix/scripts/install_key
🔇 Additional comments (9)
emhttp/plugins/dynamix.docker.manager/scripts/container_size (1)
30-30: Confirm abort=false is intentional for non-poller scriptBased on the new publish() behavior (auto-exit when no listeners), setting abort=false here looks correct so the script completes even if the UI disconnects. Please confirm this endpoint is meant to keep running regardless of listeners.
Note: I leveraged your recent learning that publish() auto-exits when there are no listeners in pollers; this script appears to be a non-poller workflow.
emhttp/plugins/dynamix/scripts/ssd_trim (1)
36-36: LGTM: Keep TRIM running even if no listenerPassing abort=false is appropriate so TRIM operations aren’t killed if the client disconnects. This aligns with the new auto-exit semantics for pollers while ensuring maintenance tasks complete.
emhttp/plugins/dynamix/scripts/newperms (1)
30-30: LGTM: Consistent publish semantics for long-running jobUsing abort=false under the nchan path ensures newperms won’t be terminated mid-run if the panel closes, while still streaming logs when listeners exist.
emhttp/plugins/dynamix.docker.manager/scripts/update_container (1)
36-36: LGTM: Don’t abort container updates on disconnectFor image pulls and container updates, abort=false is the safe choice—prevents terminating critical operations if the UI goes away mid-stream, while still publishing logs when available.
emhttp/plugins/dynamix/scripts/diagnostics (1)
50-50: LGTM: Diagnostics should finish even without listenersabort=false makes sense here so the diagnostics ZIP completes even if the user closes the page; logs are still published when a listener is present.
emhttp/plugins/dynamix/scripts/install_key (1)
19-19: LGTM: publish() now uses explicit len=1 and abort=false appropriatelyUsing abort=false is correct here so the key install flow isn’t terminated when no UI listener is attached. The explicit len=1 aligns with keeping only the latest message in the channel.
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
185-185: LGTM: explicit publish() parameters fit the plugin manager flowPassing len=1 and abort=false is appropriate so the install/update operations proceed even if the UI disconnects. This is consistent with the new publish() signature and the PR objective to only auto-exit in poller-type scripts.
emhttp/plugins/dynamix.plugin.manager/scripts/language (1)
74-74: LGTM: publish(..., 1, false) avoids unintended script terminationThis ensures language operations continue when no listeners are present, which is the desired behavior for these non-poller scripts.
emhttp/plugins/dynamix.plugin.manager/scripts/multiplugin (1)
27-27: LGTM: parameters match desired semantics (keep last message, don’t abort)The change aligns with the updated publish() API and the intent for multiplugin operations to continue independent of listener presence.
3 participants
Summary by CodeRabbit
New Features
Enhancements
UX Changes
Chores