More refinements#2102
Conversation
…ling + align syntax formatting and remove trailing spaces
WalkthroughThis pull request updates wireless and nchan functionality across several components. The Wireless page now provides enhanced user guidance with additional HTML attributes and improved focus behavior for manual input. Backend scripts and PHP files for wireless and Nginx handling have modified logic for setting regulatory domains and managing nchan subscribers. Several NchanSubscriber instances now include a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Wireless.page
participant ManualInput as Manual Input Field
User->>UI: Select "00" as region
UI->>ManualInput: Display manual input section
UI->>ManualInput: Set focus on the input field
ManualInput-->>User: Accepts manual region entry
sequenceDiagram
participant Admin
participant Nginx as Nginx Service
participant Monitor as monitor_nchan
Admin->>Nginx: Start service
Nginx->>Monitor: Execute 'monitor_nchan start'
Note right of Monitor: Nchan subscribers now use reconnectTimeout
Admin->>Nginx: Stop service
Nginx->>Monitor: Execute 'monitor_nchan stop'
Monitor->>Nginx: Terminate nchan processes
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (2)
577-583: Consider documenting deprecation plans.This function is marked with a note suggesting it "can likely be removed", but it's still included in the codebase. If this is planned for future removal, consider adding a formal deprecation notice with timeline.
-function showUpgrade(text, noDismiss=false) { /** @note can likely be removed, not used in webgui or api repos */ +/** + * @deprecated Since version X.X.X. Will be removed in version Y.Y.Y. + * Not used in webgui or api repos. + */ +function showUpgrade(text, noDismiss=false) {
584-591: Consider documenting deprecation plans.Similar to the previous function, this one is also marked for potential removal.
-function hideUpgrade(set) { /** @note can likely be removed, not used in webgui or api repos */ +/** + * @deprecated Since version X.X.X. Will be removed in version Y.Y.Y. + * Not used in webgui or api repos. + */ +function hideUpgrade(set) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/DefaultPageLayout.php(25 hunks)
🔇 Additional comments (7)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (7)
108-109: Enhanced subscriber tracking with monitor method.The prototype extension adds a tracking mechanism for Nchan subscribers, pushing each instance to the
nchanSubsarray. This is a core component of the new focus-based control for live updates.
144-152: Clean implementation of resumeEvents function.The resumeEvents function has been properly refactored to handle delayed resumption of page events, with consistent spacing and improved parameter handling.
153-182: Well-implemented focus-based control for live updates.This new code block successfully implements the core functionality described in the PR objectives - pausing Nchan communications when the page loses focus and resuming when it regains focus. The implementation:
- Tracks Nchan subscribers in an array
- Manages pausing/resuming based on visibility and user interaction
- Implements a 30-minute inactivity timer as specified in requirements
- Provides clear visual feedback to users with a warning banner
The event handlers for window clicks and visibility changes ensure a seamless user experience, particularly valuable for users with the page open on a secondary screen.
264-266: LGTM! Minor formatting improvement in makeWindow function.The code has been reformatted for better readability while maintaining functionality.
311-313: LGTM! Consistent formatting in bannerAlert function.The indentation has been improved for better code readability.
295-297: LGTM! Improved makeWindow usage in openWindow function.The code correctly utilizes the makeWindow function, maintaining consistent functionality.
174-179:Details
✅ Verification successful
Verify handling of multiple browser tabs.
The current implementation pauses Nchan updates when the document becomes hidden and resumes when it becomes visible again. This correctly handles the case where a user switches tabs/applications. However, consider what happens if a user has multiple tabs of the Unraid interface open simultaneously - ensure the visibility events don't conflict between tabs.
🏁 Script executed:
#!/bin/bash # Look for any other places in the codebase where document.addEventListener('visibilitychange', ...) is used # to check for potential conflicts with this implementation rg -n "document\.addEventListener\('visibilitychange'" --type php --type jsLength of output: 199
No conflicts found with the visibilitychange event handler.
I verified using a repository-wide search and confirmed that the only usage of
document.addEventListener('visibilitychange', …)is inemhttp/plugins/dynamix/include/DefaultPageLayout.php. This means each browser tab independently handles its own visibility state, so there are no cross-tab conflicts within the codebase.Please ensure via manual or functional tests that the pause/resume behavior for Nchan updates works as expected when multiple Unraid interface tabs are open concurrently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/Wireless.php (1)
201-202: Input validation improvement for wireless credentials.The addition of the
maxlength="63"attribute to both username and password input fields enforces compliance with IEEE 802.11i standards, which specify a maximum of 63 characters for Wi-Fi credentials. This prevents users from entering overly long values that would be rejected by the wireless system.While the maxlength constraint is good, consider also adding a minimum length requirement for the password field to enhance security. You could add both a
minlengthattribute and pattern validation:-if ($attr3 || $manual || $safe) echo "<tr id=\"password\" class=\"$hide1\"><td>"._('Password').":</td><td><input type=\"password\" name=\"PASSWORD\" class=\"narrow\" maxlength=\"63\" autocomplete=\"off\" spellcheck=\"false\" value=\"$passwd\"><i id=\"showPass\" class=\"fa fa-eye\" onclick=\"showPassword()\"></i></td></tr>"; +if ($attr3 || $manual || $safe) echo "<tr id=\"password\" class=\"$hide1\"><td>"._('Password').":</td><td><input type=\"password\" name=\"PASSWORD\" class=\"narrow\" minlength=\"8\" maxlength=\"63\" title=\""._('Password must be 8-63 characters')."\" autocomplete=\"off\" spellcheck=\"false\" value=\"$passwd\"><i id=\"showPass\" class=\"fa fa-eye\" onclick=\"showPassword()\"></i></td></tr>";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
emhttp/plugins/dynamix/Wireless.page(3 hunks)emhttp/plugins/dynamix/include/Wireless.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/Wireless.page
This reverts commit 49598e2.
This reverts commit 674b73d.
This reverts commit 0bdfad0.
This reverts commit 9d43ac8.
This reverts commit 4232cc1.
This reverts commit d86ee94.
These changes prevent Nchan issues when nginx is restarted. In my testing it works most of the time, but opening many browsers to the same GUI still gives the issue, though this is definitely an improvement over the current implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (1)
1040-1040: Consider centralizing the reconnect timeout value.
Multiple Nchan subscribers hardcodereconnectTimeout: 5000. As a minor enhancement, you might define a single constant or variable for the timeout and reference it, reducing duplication and easing future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
emhttp/plugins/dynamix.docker.manager/DockerContainers.page(1 hunks)emhttp/plugins/dynamix/ArrayOperation.page(5 hunks)emhttp/plugins/dynamix/DashStats.page(4 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout.php(26 hunks)etc/rc.d/rc.nginx(5 hunks)sbin/monitor_nchan(2 hunks)
🔇 Additional comments (27)
emhttp/plugins/dynamix.docker.manager/DockerContainers.page (1)
195-195: Improved page behavior for inactive tabsThe removal of the
.monitor()method call aligns with the PR's objective to modify Nchan communication behavior. Instead of refreshing the page after inactivity, this change supports the new approach where communication is paused when inactive and resumed when the user interacts with the page.sbin/monitor_nchan (5)
7-7: Added temporary file for tracking nchan processesThis new variable defines a temporary file that will be used to store the list of nchan processes, enabling the system to track and restore them when needed.
10-17: Well-implemented function for stopping nchan processesThe
nchan_stop()function is well-designed to:
- Clear the temporary tracking file
- Read running nchan processes
- Store them in the temporary file for later restoration
- Kill each process
This implementation properly supports the PR's goal to pause Nchan communication when a page becomes inactive.
19-25: Well-implemented function for restarting nchan processesThe
nchan_start()function effectively:
- Checks if the tracking file exists
- Reads and restarts each previously stopped nchan process
- Cleans up the temporary file afterward
This implementation properly supports the PR's goal to resume Nchan communication when a page becomes visible again.
27-37: Added command-line interface for controlling nchan processesThis code block adds explicit command-line handling for stop/start operations, allowing external scripts to trigger these actions. This is important for integration with visibility events from the web interface.
56-56: Improved process handling using new functionChanged direct kill commands to use the new
nchan_stop()function, which provides a more robust way to stop nchan processes while preserving the ability to restart them later.emhttp/plugins/dynamix/DashStats.page (4)
1617-1617: Added reconnection timeout to improve resilienceAdding a
reconnectTimeoutparameter set to 5000 milliseconds for thevmdashusageNchanSubscriber instance improves connection handling. If the connection is lost, it will attempt to reconnect after 5 seconds rather than immediately, which prevents excessive reconnection attempts.
1628-1628: Added reconnection timeout to dashboard subscriberSimilar to the previous change, adding a 5-second reconnection timeout to the dashboard subscriber enhances connection resilience when the page loses focus or network connectivity is temporarily disrupted.
1773-1773: Added reconnection timeout to UPS monitoring subscriberThe same 5-second reconnection timeout was added to the UPS monitoring subscriber, ensuring consistent behavior across all Nchan subscribers in the dashboard.
1798-1804: Streamlined subscriber initializationRemoved the
.monitor()method calls for all NchanSubscriber instances, now only calling.start(). This change is consistent with the modifications in other files and supports the PR objective of pausing communication when the page is inactive instead of automatically refreshing.etc/rc.d/rc.nginx (6)
152-152: Enhanced reliability with infinite message timeout settingSetting
nchan_message_timeoutto0configures messages to never expire, which supports the PR objective of improving nchan communication behavior and ensuring messages are retained regardless of page visibility state.
391-391: Enhanced subscriber reliability with infinite timeoutSetting
nchan_subscriber_timeoutto0ensures that subscribers won't time out, which complements the publisher-side changes and supports the goal of maintaining connections that can be paused and resumed without data loss.
689-689: Improved function clarity with explicit returnAdding an explicit
return 0enhances code readability and prevents any potential issues with implicit return values.
715-716: Correctly manage nchan publishers on Nginx startThese lines implement the start portion of the nchan publisher management, ensuring that nchan communication is properly resumed when the Nginx service starts.
729-730: Properly pause nchan publishers before stopping NginxThis change ensures nchan publishers are properly stopped before shutting down Nginx, preventing potential issues with lingering connections.
734-736: Robust Nginx shutdown with failsafe mechanismAdding a safety "hammer" with
pkilland an additional wait for shutdown provides a more robust approach to ensure Nginx is fully stopped, which helps prevent issues on restart.emhttp/plugins/dynamix/ArrayOperation.page (6)
117-117: Simplified device start without monitoringRemoving the
.monitor()call aligns with the PR objectives by allowing the toggle_state functionality to work without forcing continuous monitoring. This supports the new behavior where monitoring can be paused when pages are inactive.
322-322: Enhanced connection resilience for mymonitorAdding a 5-second
reconnectTimeoutparameter provides a consistent reconnection strategy when the page becomes visible again after being inactive, supporting the PR's goal of a seamless transition.
370-370: Improved arraymonitor reconnection behaviorThe 5-second reconnection timeout for the arraymonitor subscriber helps ensure quick recovery after periods of inactivity, contributing to the PR's objective of a more seamless user experience.
375-375: Enhanced devices subscriber reconnection strategySetting a consistent reconnection timeout for the devices subscriber aligns with the overall changes to make nchan communication more resilient to page visibility changes.
426-426: Standardized reconnection behavior for fsStateAdding the reconnection timeout parameter to the fsState subscriber maintains consistency with the other subscriber instances and supports the PR's goal of improved communication handling.
441-441: Consistent reconnection settings for paritymonitorThe 5-second reconnection timeout for paritymonitor completes the set of changes to standardize nchan subscriber behavior across all components.
emhttp/plugins/dynamix/include/DefaultPageLayout.php (5)
108-108: Initialization use of Shadowbox looks fine.
No issues found with callingShadowbox.init({skipSetup:true})at this point.
143-143: Resume event logic appears correct.
The approach to iterating overtimersand scheduling timeouts per event is straightforward and consistent with the pause logic.
970-970: Consistent reconnectTimeout usage.
UsingreconnectTimeout: 5000fornchan_wlan0maintains uniform behavior across subscribers.
978-978: Reconnect timeout added.
The addition ofreconnectTimeout: 5000is consistent with other Nchan subscribers, improving resilience in case of transient connection issues.
991-991: docker subscriber update looks good.
AddingreconnectTimeout: 5000here aligns well with the other subscribers and ensures consistent reconnection logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (1)
121-125: Revisit forced page reload.
Force-refreshing the page may disrupt users who are in the middle of editing or performing tasks. Consider making this behavior optional or providing warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/DefaultPageLayout.php(28 hunks)
🔇 Additional comments (5)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (5)
761-761: Minor addition appears fine.
The new<div class='tabs'>wrapper is straightforward and does not introduce issues.
974-974: Consistent reconnection setting.
AddingreconnectTimeout: 5000aligns with other subscribers. No issues noted.
982-982: Maintaining uniformity.
Good job applying consistent reconnection settings to the plugins subscriber as well.
995-995: Consistent timeout usage.
This follows the same reconnection pattern as elsewhere. Looks good.
1044-1044: Uniform reconnect strategy.
IncludingreconnectTimeouthere helps maintain consistent behavior across all subscribers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sbin/monitor_nchan (1)
10-17: Well-structured nchan_stop function with potential improvement opportunity.The function correctly captures running nchan processes and stops them, but consider two improvements:
- The grep pattern might be too broad - consider using a more specific pattern to avoid matching unintended processes.
- Consider adding a graceful termination option before using pkill.
nchan_stop() { echo -n >$nchan_list while IFS=$'\n' read -r nchan; do [[ ${nchan##*/} == '.*' ]] && continue echo $nchan >>$nchan_list - pkill -f $nchan + # Try graceful termination first, then force kill after timeout + pkill -TERM -f "$nchan" + sleep 0.5 + pkill -9 -f "$nchan" 2>/dev/null done <<< $(ps -eo cmd | grep -Po '/usr/local/emhttp/.*/nchan/.*') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
sbin/monitor_nchan(2 hunks)
🔇 Additional comments (5)
sbin/monitor_nchan (5)
7-7: Added temporary file for nchan process tracking.Good addition - this temporary file provides a mechanism to persist the list of nchan processes between stop and start operations.
19-27: Good implementation of nchan_start with proper process checking.The function correctly checks if processes are already running before starting them, avoiding duplicate processes. The cleanup of the temporary file is also properly handled.
29-39: Clear command-line argument handling for start/stop operations.The implementation of dedicated start/stop functionality provides better control over the nchan processes and improves script usability.
46-46: Improved curl command with proper error redirection.The updated curl command now properly redirects stderr to /dev/null, preventing error messages from appearing in logs or output.
Also applies to: 50-50
58-58: Refactored process stopping logic to use the new nchan_stop function.Good refactoring - replacing the previous immediate kill logic with the more structured nchan_stop function improves code maintainability and reuse.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (6)
109-109: Avoid direct prototype manipulation.
ExtendingNchanSubscriber.prototypedirectly can complicate debugging and maintenance if done repeatedly or in multiple places. Consider wrapping this functionality in a standard helper or dedicated class method instead.
114-114: Consider using block scope.
Usingvar subscribers = [];makessubscribersfunction-scoped instead of block-scoped. Preferletorconstwhere possible for enhanced readability and fewer scope pitfalls, unless reassignments are strictly required.
549-549: Check compatibility of default parameters.
Modern browsers support default parameter syntax (noDismiss=false), but older ones might not. Ensure you transpile for any legacy environments where needed.
759-759: Reloading the page unconditionally could disrupt user actions.
If the user is in the middle of a form, forcing a reload might cause data loss. Consider verifyingformHasUnsavedChangesbefore reloading, or prompt the user for confirmation.
975-975: Reduce code duplication for Nchan subscribers.
Multiple instances use the same config{subscriber:'websocket', reconnectTimeout:5000}. A helper function could simplify updates and ensure consistency.Possible approach:
- var nchan_wlan0 = new NchanSubscriber('/sub/wlan0',{subscriber:'websocket', reconnectTimeout:5000}); - var nchan_plugins = new NchanSubscriber('/sub/plugins',{subscriber:'websocket', reconnectTimeout:5000}); - var nchan_docker = new NchanSubscriber('/sub/docker',{subscriber:'websocket', reconnectTimeout:5000}); - var nchan_vmaction = new NchanSubscriber('/sub/vmaction',{subscriber:'websocket', reconnectTimeout:5000}); + function createNchanSubscriber(path) { + return new NchanSubscriber(path, {subscriber:'websocket', reconnectTimeout:5000}); + } + var nchan_wlan0 = createNchanSubscriber('/sub/wlan0'); + var nchan_plugins = createNchanSubscriber('/sub/plugins'); + var nchan_docker = createNchanSubscriber('/sub/docker'); + var nchan_vmaction = createNchanSubscriber('/sub/vmaction');Also applies to: 983-983, 996-996, 1045-1045
1274-1274: Consolidate focus/visibility handling.
Currently,$(window).focus()anddocument.visibilitychangeboth managenchanFocusStart()andnchanFocusStop(). Consider unifying them to ensure consistent behavior and avoid potential edge cases or conflicting timers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
emhttp/plugins/dynamix/ArrayOperation.page(4 hunks)emhttp/plugins/dynamix/DashStats.page(3 hunks)emhttp/plugins/dynamix/include/DefaultPageLayout.php(28 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix/DashStats.page
- emhttp/plugins/dynamix/ArrayOperation.page
🔇 Additional comments (3)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (3)
147-147: Implementation looks good.
No specific issues found in this function.
155-155: Straightforward utility function.
This minor grammar helper function is clear and works as expected.
762-762: LGTM on structural markup.
The added<div class='tabs'>is straightforward and consistent with the existing layout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (2)
109-109: Global array usage might lead to memory leaks
When callingmonitor(), each new subscriber is pushed to the globalsubscribersarray, but there’s no apparent cleanup if one unsubscribes or is replaced. Over time, this could lead to stale references lingering. Consider removing entries for unsubscribed instances to prevent potential memory leaks.
115-115: Global scope variable could be encapsulated
var subscribers = [];pollutes the global namespace. For maintainability, place this array behind a module or closure to avoid name collisions and keep state management more organized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/DefaultPageLayout.php(30 hunks)
🔇 Additional comments (7)
emhttp/plugins/dynamix/include/DefaultPageLayout.php (7)
147-147: Minor spacing inclusion
This empty line can improve readability by clearly separating logical blocks.
548-548: Trivial formatting enhancement
Inserting a blank line before the next function helps visually delineate code sections.
761-762: Clarify auto-reload logic while paused
The script won’t reload ifnchanPausedremains true for an extended period, which could inadvertently prevent periodic refresh. Confirm whether it’s intentional to skip reloading when the page isn’t focused or consider adjusting this logic.
975-975: Consistent use of reconnectTimeout
AddingreconnectTimeout: 5000aligns with the approach in other subscriber setups and improves overall resilience of the WebSocket connection.
1296-1296: No concerns with closing brace
No issues here—this line simply finalizes the conditional block.
1315-1315: Blank line for readability
This additional line helps visually distinguish the preceding block from the next logic.
1319-1319: Minor spacing adjustment
Another small spacing tweak that makes the code easier to scan without functional changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sbin/monitor_nchan (3)
10-17: Consider adding error handling to the nchan_stop function.The function effectively captures and terminates nchan processes, but lacks error handling if the
pkillcommand fails. Additionally, the pattern matching with grep could potentially match unintended processes if the path structure changes.nchan_stop() { echo -n >$nchan_list while IFS=$'\n' read -r nchan; do [[ ${nchan##*/} == '.*' ]] && continue echo $nchan >>$nchan_list - pkill -f $nchan + if ! pkill -f $nchan; then + logger -t $nchan_id -- "Failed to stop process: $nchan" + fi done <<< $(ps -eo cmd | grep -Po '/usr/local/emhttp/.*/nchan/.*') }
19-27: Consider adding validation and error handling to the nchan_start function.The function starts each process listed in the temporary file without validating that the commands are safe to execute. Additionally, there's no error handling if a process fails to start.
nchan_start() { [[ -e $nchan_list ]] || return while IFS=$'\n' read -r nchan; do if ! pgrep -f $nchan >/dev/null; then - $nchan &>/dev/null & + if [[ $nchan == /usr/local/emhttp/*/nchan/* ]]; then + $nchan &>/dev/null & + if ! pgrep -f $nchan >/dev/null; then + logger -t $nchan_id -- "Failed to start process: $nchan" + fi + else + logger -t $nchan_id -- "Skipped suspicious process: $nchan" + fi fi done < $nchan_list rm -f $nchan_list }
10-27: Consider handling potential race conditions.The current implementation might face race conditions if multiple instances of this script run simultaneously, as they would concurrently modify the same temporary file.
Consider using file locking mechanisms or PID files to prevent concurrent execution:
+LOCKFILE=/var/run/monitor_nchan.lock +# Function to acquire lock +acquire_lock() { + exec 200>$LOCKFILE + flock -n 200 || { echo "Another instance is running"; exit 1; } + echo $$ >&200 +} nchan_stop() { + acquire_lock echo -n >$nchan_list # Rest of the function... } nchan_start() { + acquire_lock [[ -e $nchan_list ]] || return # Rest of the function... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
etc/rc.d/rc.nginx(8 hunks)sbin/monitor_nchan(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- etc/rc.d/rc.nginx
🔇 Additional comments (6)
sbin/monitor_nchan (6)
7-7: Temporary file established for nchan process tracking.The addition of this temporary file helps maintain state between the stopping and starting of nchan processes.
29-34: Clean implementation of the stop command.The handler for the 'stop' command is well structured, calling the new
nchan_stopfunction and conditionally cleaning up associated files.
36-39: Clean implementation of the start command.The handler for the 'start' command is well structured, calling the new
nchan_startfunction.
47-47: Improved curl command pattern matching.The updated grep pattern using
-Pom1with the\Koperator is more precise and efficient for extracting just the subscriber count.Also applies to: 51-51
59-59: Improved code reuse with function call.Replacing inline process termination with a call to the new
nchan_stopfunction improves code maintainability.
12-16: Added robust process management structure.The implementation of dedicated functions for stopping and starting nchan processes with state persistence is a significant improvement. The approach of saving process information before stopping them allows for proper recovery when needed.
Also applies to: 21-24
2 participants
Summary by CodeRabbit
New Features
Refactor