Fixed: Multiple Tabs on Main would cause issues on parity sync changes#2338
Conversation
WalkthroughRefactors Nchan publishing: updates publish() defaults and signatures, introduces ping(), switches many publishers to publish_noDupe with JSON-encoded payloads, and adds lightweight websocket subscribers for “ping” channels. Several call sites drop extra publish arguments and/or move to JSON. No broader control flow altered beyond heartbeat/ping emissions. Changes
Sequence Diagram(s)sequenceDiagram
participant Pub as Publisher Script
participant N as Nchan
participant Sub as Web UI Subscriber
participant Ping as Ping Subscriber
Pub->>N: publish_noDupe(topic, JSON)
alt New data
N-->>Sub: message(JSON)
else Duplicate
N-->>Sub: (no message)
end
Pub->>N: ping(pingTopic)
N-->>Ping: "ping"
sequenceDiagram
participant Caller as Caller
participant publish as publish(endpoint,msg,len,abort=false)
participant N as Nchan
Caller->>publish: msg (string)
publish->>N: POST message
alt No listeners and abort=true
publish-->>Caller: abort after timeout + log
else Default (abort=false)
publish-->>Caller: continue loop
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
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: 4
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix/include/publish.php (1)
98-121: Bug: abortTime parameter is ignored by publish_noDupe → publish path.publish_noDupe($endpoint, $message, $noListenerAbort=false, $abortTime=30) never passes $abortTime down to publish(). That forces publish() to use its default (30s) regardless of the caller’s explicit $abortTime.
Apply this diff:
- $return = publish($endpoint, $message,1,$noListenerAbort); + $return = publish($endpoint, $message, 1, $noListenerAbort, $abortTime);
🧹 Nitpick comments (12)
emhttp/plugins/dynamix/DashStats.page (1)
2414-2418: Ping subscriber addition looks good; consider const for immutablesThe lightweight ping subscriber is correctly initialized and started early. Minor nit: dashboardPing is never reassigned, so prefer const.
Apply:
- var dashboardPing = new NchanSubscriber('/sub/dashboardPing',{subscriber:'websocket', reconnectTimeout:5000}); + const dashboardPing = new NchanSubscriber('/sub/dashboardPing',{subscriber:'websocket', reconnectTimeout:5000});emhttp/plugins/dynamix/nchan/ups_status (1)
135-137: Avoid manual json_encode; publish_noDupe handles arrayspublish_noDupe() auto-serializes arrays to JSON. Passing a pre-encoded string adds redundancy and double-encapsulation risk if future helpers evolve.
Use:
- publish_noDupe('apcups',json_encode($echo)); + publish_noDupe('apcups',$echo); ping('dashboardPing');emhttp/plugins/dynamix/nchan/wlan0 (1)
69-69: Let publish() handle JSON encodingpublish() already JSON-encodes arrays; no need to call json_encode at the call site.
Apply:
- publish('wlan0',json_encode($echo)); + publish('wlan0',$echo);emhttp/plugins/dynamix/nchan/update_3 (1)
162-164: Simplify: remove json_encode; keep pingpublish_noDupe() auto-encodes arrays. Dropping json_encode reduces noise and keeps call sites consistent with helper semantics. The ping addition is appropriate.
Apply:
- publish_noDupe('update3',json_encode($echo)); - ping('dashboardPing'); + publish_noDupe('update3',$echo); + ping('dashboardPing');emhttp/plugins/dynamix/nchan/update_2 (1)
502-502: Avoid manual json_encode; publish_noDupe already handles arrayspublish_noDupe() auto-encodes arrays to JSON internally. Passing a pre-encoded string is redundant and obscures intent.
- publish_noDupe('update2',json_encode($echo)); + publish_noDupe('update2', $echo);emhttp/plugins/dynamix/nchan/wg_poller (1)
39-39: Drop explicit json_encode; publisher will encode arrayspublish_noDupe() converts arrays to JSON internally. Passing $echo directly keeps call sites simpler and consistent with other modules.
- publish_noDupe('wireguard',json_encode($echo)); + publish_noDupe('wireguard', $echo);emhttp/plugins/dynamix/nchan/notify_poller (1)
21-21: Nit: tighten the comment wordingMinor readability tweak; reduce spacing and clarify the sentence.
-// Not necessary to check for listeners. This will automatically exit if there are no listeners anywhere +// No need to check for listeners; this poller exits automatically when there are no listeners anywhere.emhttp/plugins/dynamix/nchan/update_1 (1)
126-126: Let publish_noDupe handle JSON encodingAs per our codebase convention, pass arrays to publish_noDupe() and let it JSON-encode internally.
- publish_noDupe('update1',json_encode($echo)); + publish_noDupe('update1', $echo);emhttp/plugins/dynamix/nchan/device_list (1)
569-576: Update the comment to reflect new abort semantics and consider dedupe for 'devices'.
- The comment “publish without a timeout for no listeners” is ambiguous now that abort defaults to false and we use ping() for listener checks. Clarify this to reduce confusion.
- Optional: devices is high-churn but can still benefit from not republishing identical payloads every second. publish_noDupe would reduce traffic without losing updates, since i/o metrics still change frequently.
Apply this diff to clarify and optionally dedupe:
- // publish without a timeout for no listeners - publish('devices',json_encode($echo)); + // publish devices; listener exit is governed by ping('mainPingListener'), not publish() timeout + // Consider dedupe to avoid republishing identical payloads every tick + publish_noDupe('devices', json_encode($echo));If you prefer to keep continuous publishing for devices, update only the comment:
- // publish without a timeout for no listeners + // publish; do not auto-exit on no listeners (ping('mainPingListener') handles this)emhttp/plugins/dynamix/include/publish.php (3)
28-33: Fix docblock: message type and abort default mismatch.The doc says “$message (string)” which is fine, but it still claims “$abort … (default true)”. The signature sets $abort=false. Update the doc to avoid confusion at callsites.
Apply this diff:
-// $abort: if true, the script will exit if the endpoint is without subscribers on the next publish attempt after $abortTime seconds (default true) +// $abort: if true, the script will exit if the endpoint is without subscribers on the next publish attempt after $abortTime seconds (default false)
60-61: Use consistent logger tag for curl errors.Elsewhere in this file you log under 'publish'; here it falls back to default 'webgui'. Use a consistent tag to simplify filtering.
Apply this diff:
- my_logger("curl error: $err endpoint: $endpoint"); + my_logger("curl error: $err endpoint: $endpoint", 'publish');
48-54: Nit: Prefer standards-compliant Accept header.Accept:text/json is nonstandard; prefer application/json.
Apply this diff:
- CURLOPT_HTTPHEADER => ['Accept:text/json'], + CURLOPT_HTTPHEADER => ['Accept: application/json'],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
emhttp/plugins/dynamix.docker.manager/nchan/docker_load(1 hunks)emhttp/plugins/dynamix.vm.manager/VMUsageStats.page(1 hunks)emhttp/plugins/dynamix.vm.manager/nchan/vm_usage(1 hunks)emhttp/plugins/dynamix/ArrayOperation.page(1 hunks)emhttp/plugins/dynamix/DashStats.page(1 hunks)emhttp/plugins/dynamix/include/publish.php(7 hunks)emhttp/plugins/dynamix/nchan/device_list(1 hunks)emhttp/plugins/dynamix/nchan/file_manager(1 hunks)emhttp/plugins/dynamix/nchan/notify_poller(1 hunks)emhttp/plugins/dynamix/nchan/parity_list(1 hunks)emhttp/plugins/dynamix/nchan/session_check(1 hunks)emhttp/plugins/dynamix/nchan/update_1(1 hunks)emhttp/plugins/dynamix/nchan/update_2(1 hunks)emhttp/plugins/dynamix/nchan/update_3(1 hunks)emhttp/plugins/dynamix/nchan/ups_status(1 hunks)emhttp/plugins/dynamix/nchan/vm_dashusage(1 hunks)emhttp/plugins/dynamix/nchan/wg_poller(1 hunks)emhttp/plugins/dynamix/nchan/wlan0(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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-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/nchan/notify_polleremhttp/plugins/dynamix/nchan/session_checkemhttp/plugins/dynamix.docker.manager/nchan/docker_loademhttp/plugins/dynamix/nchan/device_listemhttp/plugins/dynamix/nchan/parity_listemhttp/plugins/dynamix/include/publish.php
📚 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_3emhttp/plugins/dynamix/nchan/update_1emhttp/plugins/dynamix/nchan/update_2emhttp/plugins/dynamix/nchan/vm_dashusageemhttp/plugins/dynamix.docker.manager/nchan/docker_loademhttp/plugins/dynamix/nchan/device_listemhttp/plugins/dynamix/nchan/wg_polleremhttp/plugins/dynamix/nchan/parity_listemhttp/plugins/dynamix/nchan/ups_statusemhttp/plugins/dynamix/include/publish.php
📚 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/wlan0emhttp/plugins/dynamix/nchan/update_3emhttp/plugins/dynamix/nchan/update_1emhttp/plugins/dynamix/nchan/update_2emhttp/plugins/dynamix/nchan/vm_dashusageemhttp/plugins/dynamix/nchan/device_listemhttp/plugins/dynamix/nchan/wg_polleremhttp/plugins/dynamix/nchan/ups_status
📚 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/update_3emhttp/plugins/dynamix/nchan/update_1emhttp/plugins/dynamix/nchan/update_2emhttp/plugins/dynamix/nchan/vm_dashusageemhttp/plugins/dynamix/nchan/wg_polleremhttp/plugins/dynamix/nchan/ups_status
📚 Learning: 2025-08-14T02:58:16.005Z
Learnt from: Squidly271
PR: unraid/webgui#2333
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:90-90
Timestamp: 2025-08-14T02:58:16.005Z
Learning: In the unraid/webgui codebase, Squidly271 specifically does not want deduplication behavior for the vm_usage channel in emhttp/plugins/dynamix.vm.manager/nchan/vm_usage. The standard publish() function should be used instead of publish_noDupe() for this channel.
Applied to files:
emhttp/plugins/dynamix/nchan/update_3emhttp/plugins/dynamix/nchan/update_1emhttp/plugins/dynamix/nchan/update_2emhttp/plugins/dynamix.vm.manager/nchan/vm_usageemhttp/plugins/dynamix/nchan/vm_dashusageemhttp/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-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/include/publish.php
🧬 Code Graph Analysis (1)
emhttp/plugins/dynamix/include/publish.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
my_logger(220-222)
🔇 Additional comments (13)
emhttp/plugins/dynamix/ArrayOperation.page (1)
509-509: Comment clarification acknowledgedThe comment now accurately reflects the ping-based listener-detection approach. No functional changes; LGTM.
emhttp/plugins/dynamix/nchan/update_2 (1)
503-503: Heartbeat ping addition looks goodAdding ping('dashboardPing') after publishing is aligned with the multi-tab heartbeat approach and should help avoid stale listeners on Main.
emhttp/plugins/dynamix.vm.manager/VMUsageStats.page (1)
35-39: Lightweight ping subscriber is appropriateSubscribing to /sub/vmPing via websocket with a no-op handler is a minimal way to assert listener presence without adding page work. This aligns with the heartbeat pattern used elsewhere.
emhttp/plugins/dynamix/nchan/wg_poller (1)
40-40: Dashboard heartbeat ping is consistent with the new patternEmitting ping('dashboardPing') after publishing is fine here and should help maintain consistent listener checks across tabs.
emhttp/plugins/dynamix/nchan/update_1 (1)
127-127: Ping addition is appropriateping('dashboardPing') after the publish supports the heartbeat approach for Main/Dashboard multi-tab scenarios.
emhttp/plugins/dynamix/nchan/session_check (1)
21-21: Comment aligns with Nchan behavior—good clarificationAccurate note. The nchan publish path will terminate when there are no listeners, so extra checks are unnecessary here.
emhttp/plugins/dynamix/nchan/file_manager (1)
255-255: Simplified publish() usage LGTMDropping the trailing args to rely on publish() defaults is consistent with the updated API and keeps call sites clean. JSON-encoding the reply is appropriate if publish() now expects strings.
Also applies to: 258-258
emhttp/plugins/dynamix/nchan/vm_dashusage (1)
112-113: Switch to publish_noDupe + dashboardPing is consistent with the new heartbeat patternDeduped payloads with an explicit dashboard ping aligns with the broader changes in this PR. Assuming the front-end subscriber expects JSON, this looks correct.
emhttp/plugins/dynamix/nchan/parity_list (2)
152-155: LGTM: Deduplicated parity and state monitors reduce chatter.Using publish_noDupe for paritymonitor, fsState, and mymonitor aligns with the PR’s objective to minimize duplicate publishes across steady-state loops.
156-157: LGTM: Ping-based lifecycle control is consistent with new semantics.ping("mainPingListener") centralizes the no-listener abort behavior, matching the updated publish/publish_noDupe defaults.
emhttp/plugins/dynamix/nchan/device_list (2)
573-573: LGTM: arraymonitor publish with defaults.Removing explicit params is correct under the new publish() signature and abort defaults.
575-576: LGTM: Ping-based abort is consistent with the repo-wide pattern.This ensures the script exits when nobody is listening, independent of the publish() calls above.
emhttp/plugins/dynamix/include/publish.php (1)
128-132: LGTM: ping wrapper cleanly encapsulates no-listener abort.This keeps callsites concise and centralizes lifecycle semantics.
3 participants
Summary by CodeRabbit