Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive monitoring, device management, and real-time data collection to MoCI. It adds five new frontend JavaScript modules, three backend monitoring shell scripts, configuration management, init script integration, and installation automation. The changes significantly expand UI functionality while adding backend services for ping testing, speedtest benchmarking, and Netify flow collection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser / UI
participant RPCd as RPCd (ubus)
participant Collector as Netify Collector
participant SQLite as SQLite DB
participant Stream as Netify Stream
rect rgba(100, 150, 255, 0.5)
note over Collector,SQLite: Data Collection Phase
Collector->>Stream: Connect (nc to host:port)
Stream-->>Collector: Flow events (JSON lines)
Collector->>SQLite: INSERT flow_raw (JSON + timestamp)
Collector->>SQLite: DELETE old rows (retention pruning)
end
rect rgba(150, 200, 100, 0.5)
note over Client,RPCd: Query Phase
Client->>RPCd: uciGet moci.collector (config)
RPCd-->>Client: host, port, db_path, retention
end
rect rgba(200, 150, 100, 0.5)
note over Client,SQLite: UI Rendering Phase
Client->>SQLite: SELECT recent rows (chunked query)
SQLite-->>Client: flow_raw JSONL records
Client->>Client: Parse, aggregate, filter
Client->>Client: Render table + top apps
end
sequenceDiagram
participant Client as UI / Dashboard
participant PingMon as Ping Monitor (script)
participant FileSystem as /tmp (ping monitor.txt)
participant System as System Services
participant Storage as Storage (vnstat)
rect rgba(100, 150, 255, 0.5)
note over PingMon,FileSystem: Ping Collection Loop
PingMon->>System: ping -c 1 [target]
System-->>PingMon: latency or timeout
PingMon->>FileSystem: APPEND timestamp|status|latency|msg
PingMon->>FileSystem: PRUNE to max_lines
end
rect rgba(150, 200, 100, 0.5)
note over Client,FileSystem: Monitoring Data Load
Client->>FileSystem: READ moci-ping-monitor.txt
FileSystem-->>Client: pipe-delimited samples
Client->>Client: Parse, aggregate by 5-min buckets
end
rect rgba(200, 150, 100, 0.5)
note over Client,Storage: Traffic History (Monthly)
Client->>System: vnstat --json
System-->>Client: traffic summary (hour/day/month)
Client->>Client: Chart aggregation + rendering
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
files/moci-ping-monitor.sh-44-48 (1)
44-48:⚠️ Potential issue | 🟠 MajorClamp monitoring config to positive integers.
These values flow into Line 124 and Line 93, but
sanitize_int()currently accepts0unchanged. Aping_monitor.intervalof0turns the daemon into a tight loop, andtimeout=0/max_lines=0also produce broken runtime behavior.Suggested patch
refresh_runtime_config() { load_config - PING_INTERVAL="$(sanitize_int "$PING_INTERVAL" "$DEFAULT_INTERVAL")" - PING_TIMEOUT="$(sanitize_int "$PING_TIMEOUT" "$DEFAULT_TIMEOUT")" - PING_MAX_LINES="$(sanitize_int "$PING_MAX_LINES" "$DEFAULT_MAX_LINES")" + PING_INTERVAL="$(sanitize_positive_int "$PING_INTERVAL" "$DEFAULT_INTERVAL")" + PING_TIMEOUT="$(sanitize_positive_int "$PING_TIMEOUT" "$DEFAULT_TIMEOUT")" + PING_MAX_LINES="$(sanitize_positive_int "$PING_MAX_LINES" "$DEFAULT_MAX_LINES")" ensure_output_file } @@ sanitize_int() { case "${1:-}" in '' | *[!0-9]*) echo "$2" ;; *) echo "$1" ;; esac } + +sanitize_positive_int() { + case "${1:-}" in + '' | *[!0-9]* | 0) + echo "$2" + ;; + *) + echo "$1" + ;; + esac +}Also applies to: 52-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/moci-ping-monitor.sh` around lines 44 - 48, sanitize_int currently allows zero which leads to bad runtime behavior when PING_INTERVAL, PING_TIMEOUT, or PING_MAX_LINES are set to 0; update the validation so sanitize_int (or a new sanitize_positive_int) rejects 0 and clamps values to a positive minimum (e.g., 1) and/or returns the provided DEFAULT_* when input <= 0, then call that from refresh_runtime_config for PING_INTERVAL, PING_TIMEOUT, and PING_MAX_LINES (and the other refresh block around lines 52-61) so no zero values propagate into the ping loop or timeout/max-lines logic.Makefile-53-56 (1)
53-56:⚠️ Potential issue | 🟠 MajorPackage is missing
moci-speedtest-monitor.
scripts/watch.js, Lines 147-149 already deployfiles/moci-speedtest-monitor.shto/usr/bin/moci-speedtest-monitor, but this install recipe never does. Packaged images will ship without that binary, so speedtest-backed monitoring actions will fail outside the dev deploy path.Suggested patch
$(INSTALL_DIR) $(1)/usr/bin $(INSTALL_BIN) ./files/moci-netify-collector.sh $(1)/usr/bin/moci-netify-collector $(INSTALL_BIN) ./files/moci-ping-monitor.sh $(1)/usr/bin/moci-ping-monitor + $(INSTALL_BIN) ./files/moci-speedtest-monitor.sh $(1)/usr/bin/moci-speedtest-monitor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 53 - 56, The Makefile install recipe currently copies moci-netify-collector and moci-ping-monitor but omits moci-speedtest-monitor; add an INSTALL_BIN step to install files/moci-speedtest-monitor.sh to /usr/bin/moci-speedtest-monitor (same pattern as the existing $(INSTALL_BIN) lines) so the packaged image includes the speedtest monitor binary referenced by scripts/watch.js.moci/js/modules/vpn.js-31-35 (1)
31-35:⚠️ Potential issue | 🟠 MajorDo not expose WireGuard private-key material in the table.
Line 34 renders the first 20 characters of
private_key. Even partial secret material should not be placed in the DOM; show a neutral value likeConfiguredinstead.Suggested patch
- <td>${this.core.escapeHtml(iface.private_key?.substring(0, 20) || 'N/A')}...</td> + <td>${iface.private_key ? 'Configured' : 'N/A'}</td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/vpn.js` around lines 31 - 35, The table row in moci/js/modules/vpn.js leaks secret material by rendering iface.private_key?.substring(0, 20); instead, update the rendering logic in the row that uses this.core.escapeHtml and iface.private_key so it never prints key fragments — show a neutral value like "Configured" (escaped via this.core.escapeHtml) when iface.private_key is present, otherwise "N/A", and remove the ellipsis that implies key content.files/netify-collector.init-9-12 (1)
9-12:⚠️ Potential issue | 🟠 MajorDisabled collectors currently "start" successfully.
When
moci.collector.enabled=0, the init script at line 12 returns 0 immediately without starting the daemon. Theexec()function treats exit code 0 as success and does not throw, sorunServiceAction('start')shows a success toast despite nothing running. Either makestartfail when disabled, or have the UI flip the UCI flag before starting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/netify-collector.init` around lines 9 - 12, The start_service function currently returns 0 when moci.collector.enabled is "0", causing exec() to treat a non-start as success; change start_service so that when the UCI key moci.collector.enabled is "0" it exits with a non-zero status (e.g., return 1) and emits a clear error/log message so callers like runServiceAction('start') see a failure; update the logic in start_service to check the enabled flag, log that the collector is disabled, and return failure instead of silently returning 0 (alternative: flip the UCI flag in the UI before invoking start, but implement the non-zero exit in start_service to be safe).moci/js/modules/vpn.js-1-4 (1)
1-4:⚠️ Potential issue | 🟠 MajorNew VPNModule.loadWireGuard() is not connected to the module system. The method is defined but never called. The vpn route is not mapped in
getModuleForRoute()(moci/js/core.js:18-28), so the VPNModule won't load when navigating to a vpn page. Additionally, the constructor performs no initialization. This appears to be incomplete refactoring—the old monolithic implementation in app.js (lines 1083, 2492) still has functional vpn routing, but the new modular VPNModule is disconnected. Either add 'vpn' to the route-to-module mapping and initializeloadWireGuard()in the constructor or via route handlers, or remove the incomplete module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/vpn.js` around lines 1 - 4, VPNModule.loadWireGuard() is defined but never invoked and the module isn't registered in the app router; update the app so the VPN module is actually used: add the 'vpn' route key to getModuleForRoute() so it returns VPNModule (reference getModuleForRoute) and/or call this.loadWireGuard() from the VPNModule constructor to initialize the WireGuard UI (reference VPNModule.constructor and VPNModule.loadWireGuard), or remove the incomplete VPNModule if you intend to keep the old app.js vpn routing; ensure whichever approach you pick wires the module initialization into the existing route handling so navigating to a vpn page loads the module.files/moci-netify-collector.sh-230-238 (1)
230-238:⚠️ Potential issue | 🟠 MajorHandle one-shot DB init before the
ncdependency check.
--init-db/--init-fileonly need sqlite, butmain()callsrequire_dependencies()first. That makes/usr/bin/moci-netify-collector --init-dbfail on routers without netcat, which breaks both setup (scripts/setup-openwrt-router.shLine 211) and the UI init action (moci/js/modules/netify.jsLines 242-251).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/moci-netify-collector.sh` around lines 230 - 238, The script calls require_dependencies() before handling the one-shot init flags, causing --init-db/--init-file to fail on systems missing nc; move the case branch that detects "--init-db | --init-file" so it runs immediately after init_logging (and before require_dependencies()), and keep the existing behavior: log "netify sqlite database initialized at $NETIFY_DB" and exit 0; update main() to check the ${1:-} flags early (prior to calling require_dependencies()) so init-only runs don't require netcat.scripts/setup-openwrt-router.sh-161-190 (1)
161-190:⚠️ Potential issue | 🟠 MajorThis rerun path still resets the user's MoCI config.
Line 165 replaces
/etc/config/mociunconditionally, and Lines 171-190 then write defaults on top. Any feature toggles, paths, or schedules the user changed are lost on every rerun, which contradicts the “safe to rerun” claim on Line 4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-openwrt-router.sh` around lines 161 - 190, The script currently overwrites /etc/config/moci with install_file and unconditionally calls set_uci for many keys, wiping user changes; change this so install_file only writes the default when the config does not already exist (use the existing if present after backing it up), and modify the moci default application block to only set missing keys instead of forcing values—use uci get (or an existing helper) to test whether a key exists before calling set_uci for symbols shown (install_file, set_uci, uci commit, and the /etc/config/moci path), and keep uci commit moci after conditional updates.files/moci-netify-collector.sh-123-125 (1)
123-125:⚠️ Potential issue | 🟠 MajorHonor
moci.collector.enabledin the collector loop.
files/moci.configLines 71-77 andscripts/setup-openwrt-router.shLines 172-177 both define/usemoci.collector.enabled, but this worker only reloadsmoci.features.netify. If the UI is still enabled and the collector is disabled, the daemon keeps ingesting flows instead of shutting down.Suggested fix
NETIFY_FEATURE_ENABLED="1" +NETIFY_COLLECTOR_ENABLED="1" @@ + value="$(uci -q get moci.collector.enabled 2>/dev/null || true)" + value="$(sanitize_text "$value")" + [ -n "$value" ] && NETIFY_COLLECTOR_ENABLED="$value" + value="$(uci -q get moci.features.netify 2>/dev/null || true)" value="$(sanitize_text "$value")" [ -n "$value" ] && NETIFY_FEATURE_ENABLED="$value" @@ - if [ "$NETIFY_FEATURE_ENABLED" != "1" ]; then + if [ "$NETIFY_FEATURE_ENABLED" != "1" ] || [ "$NETIFY_COLLECTOR_ENABLED" != "1" ]; then log "netify feature disabled (moci.features.netify=$NETIFY_FEATURE_ENABLED); exiting collector" exit 0 fi @@ - if [ "$NETIFY_FEATURE_ENABLED" != "1" ]; then + if [ "$NETIFY_FEATURE_ENABLED" != "1" ] || [ "$NETIFY_COLLECTOR_ENABLED" != "1" ]; then log "netify feature disabled (moci.features.netify=$NETIFY_FEATURE_ENABLED); exiting collector" exit 0 fiAlso applies to: 212-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/moci-netify-collector.sh` around lines 123 - 125, The collector loop only respects moci.features.netify via NETIFY_FEATURE_ENABLED; add a read+sanitize of moci.collector.enabled (e.g., set a COLLECTOR_ENABLED variable from `uci -q get moci.collector.enabled` then sanitize as done for NETIFY_FEATURE_ENABLED) and use that variable in the main collector loop to stop ingestion when the collector is disabled; update the same pattern at the second block referenced (around the 212-220 area) so both initialization and subsequent reloads honor moci.collector.enabled.index.html-192-195 (1)
192-195:⚠️ Potential issue | 🟠 MajorUpdate the demo Netify mock to the SQLite collector contract.
moci/js/modules/netify.jsnow queriesflow_rawthroughsqlite3(moci/js/modules/netify.jsLines 377-385 and 471-480), but this mock still exposesoutput_file/max_lines, serves/tmp/moci-netify-flow.jsonl, and never intercepts/usr/bin/sqlite3or/usr/bin/sqlite3-cli. The Netify page will drift from production and likely render empty/broken data in demo mode.Also applies to: 202-263, 306-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` around lines 192 - 195, The demo Netify mock still returns the old JSONL file and output_file/max_lines shape instead of emulating the SQLite collector contract; update the mock branch handling (replace cases for params.path '/tmp/moci-netify-flow.jsonl' and any output_file/max_lines responses) to instead intercept '/usr/bin/sqlite3' and '/usr/bin/sqlite3-cli' invocations and return a successful result shaped like sqlite3 query output for the flow_raw table that moci/js/modules/netify.js expects (see the query handling in netify.js lines ~377-385 and ~471-480); ensure the returned payload matches the column names and row format consumed by the code so the Netify demo renders correctly.rpcd-acl.json-34-35 (1)
34-35:⚠️ Potential issue | 🟠 MajorAvoid allowlisting
/bin/shfor the MoCI role.Granting
file.execon/bin/shturns this ACL into arbitrary root shell access for any authenticated MoCI session, which bypasses the rest of the per-binary allowlist. The new monitoring/netify flows should be wrapped in dedicated helpers or narrower exec targets instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpcd-acl.json` around lines 34 - 35, The ACL currently allowlists file.exec for "/bin/sh" in the MoCI role, which grants arbitrary shell access; remove the "/bin/sh" entry from the "file": { "/bin/sh": ["exec"], } mapping and instead create narrow exec targets or dedicated helper binaries/functions for the new monitoring/netify flows (e.g., specific helper executables or wrapper scripts) and update the MoCI role to reference only those specific paths; ensure file.exec only lists the minimal binaries needed and that any new helpers are implemented and reviewed to perform the required monitoring/netify actions without exposing a general shell.scripts/setup-openwrt-router.sh-210-213 (1)
210-213:⚠️ Potential issue | 🟠 MajorDon't truncate speedtest history on every setup run.
/usr/bin/moci-speedtest-monitor --init-fileclears the output file (seefiles/moci-speedtest-monitor.shLines 140-143), so rerunning setup silently deletes existing measurements. Gate this to first install or a missing file instead.Suggested fix
log "Initializing data files" /usr/bin/moci-netify-collector --init-db || true /usr/bin/moci-ping-monitor --once || true -/usr/bin/moci-speedtest-monitor --init-file || true +SPEEDTEST_OUTPUT="$(uci -q get moci.speedtest_monitor.output_file 2>/dev/null || echo /tmp/moci-speedtest-monitor.txt)" +[ -f "$SPEEDTEST_OUTPUT" ] || /usr/bin/moci-speedtest-monitor --init-file || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-openwrt-router.sh` around lines 210 - 213, The setup script currently always runs /usr/bin/moci-speedtest-monitor --init-file which truncates the speedtest output and erases history; change the logic in setup-openwrt-router.sh to only invoke moci-speedtest-monitor --init-file when the speedtest output file does not exist (first-install) or when an explicit force-init flag is provided. Locate the call to /usr/bin/moci-speedtest-monitor --init-file in the initialization block and replace it with a conditional that checks for the presence of the expected output file (the file written/used by moci-speedtest-monitor) and only runs --init-file when absent (or when a user-supplied --force-init variable is set).README.BI.md-95-100 (1)
95-100:⚠️ Potential issue | 🟠 MajorSplit the
opkg/apkalternatives into separate commands.The install lines
opkg update or apk updateandopkg install / apk add ...are not executable as written. In bash,orand/are parsed as literal command arguments, not choice operators. When users copy-paste this block, the shell will attempt to execute a non-existent commandopkgwith argumentsupdate,or,apk, etc., causing immediate failure.Provide two separate code blocks—one for OpenWrt routers (
opkg) and one for Alpine Linux (apk)—so users can follow the appropriate instructions for their environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.BI.md` around lines 95 - 100, The current shell snippet mixes alternative package managers into single lines (using "or" and "/") so it won't execute; split into two separate blocks—one for OpenWrt using opkg and one for Alpine using apk—so each block runs valid commands (e.g., "opkg update" then "opkg install git git-http ca-bundle nano" for OpenWrt; "apk update" then "apk add git git-http ca-bundle nano" for Alpine), keeping the subsequent shared steps ("git clone https://github.com/benisai/codex-moci.git", "cd codex-moci", "sh scripts/setup-openwrt-router.sh") unchanged.moci/js/modules/netify.js-154-162 (1)
154-162:⚠️ Potential issue | 🟠 MajorDon't open
/netifyin a paused state.Line 156 forces
userPausedAutoRefresh = trueon every page entry. Because the main polling loop only runs when!this.isAutoRefreshPaused(), the recent-flows view never starts live-updating until the user manually clicks RESUME.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/netify.js` around lines 154 - 162, The load() method currently forces this.userPausedAutoRefresh = true which prevents the polling loop (guarded by isAutoRefreshPaused()) from starting; remove or change that assignment so the page does not open in a paused state (e.g. do not overwrite an existing userPausedAutoRefresh or explicitly set it to false), keep updateAutoRefreshToggleUi(), then ensure startPolling() and refresh() run as before; reference: load(), userPausedAutoRefresh, isAutoRefreshPaused(), updateAutoRefreshToggleUi(), startPolling(), refresh().moci/js/modules/netify.js-1042-1057 (1)
1042-1057:⚠️ Potential issue | 🟠 MajorThe root-domain parser is too narrow for block-scope decisions.
This hard-coded suffix list only covers a handful of multi-part TLDs. Inputs like
foo.bar.co.zaora.b.com.brfall through tolast2, returning the public suffix instead of the registrable domain, which can create invalid or overly broad DNS blocks in root-scope mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/netify.js` around lines 1042 - 1057, The current root-domain parsing logic (variables last2, last3, sldTlds, tld2) is too narrow because sldTlds only lists a few multi-part TLDs; replace this ad-hoc approach with a proper public suffix lookup (e.g., use the publicsuffix/psl or tldts library) inside the function that computes the root domain so it returns the registrable domain for inputs like foo.bar.co.za or a.b.com.br; update the code that currently computes last2/last3 and checks sldTlds to instead call the library (or a maintained public suffix list lookup), and ensure the function returns the library-derived registrable domain in all cases.moci/js/modules/devices.js-235-285 (1)
235-285:⚠️ Potential issue | 🟠 MajorInclude pinned devices even when they have no active lease or usage.
mergeRows()only emits entries backed by a DHCP lease or nlbwmon stats. A device saved through the pin dialog disappears from this page as soon as it goes offline because leftoverstaticByMacentries are never appended on their own.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/devices.js` around lines 235 - 285, mergeRows currently only adds rows from leases and totalsByClient so pinned devices in staticByMac vanish when they have no lease or usage; update mergeRows (function mergeRows, using staticByMac, totalsByClient, leases, arpMacs) to iterate staticByMac after the existing loops and append any pin entries not already in seen: for each pin (pin.mac), if not seen.has(pin.mac) and not seen.has(pin.ip), build a merged object using pin.name, pin.ip || 'N/A', pin.mac || 'N/A', look up usage = totalsByClient.get(pin.mac) || totalsByClient.get(pin.ip) for tx/rx and top apps, set online = arpMacs.has(pin.mac), pinned = true, staticSection = pin.section, push it to merged, and add its mac/ip to seen so pinned-only devices always appear.moci/js/modules/network.js-317-326 (1)
317-326:⚠️ Potential issue | 🟠 MajorProtect built-in interfaces from deletion.
loadInterfaces()renders delete actions for every runtime interface in this file, and Line 320 blindly removes the matching UCI section. That allows deletinglan,wan, orloopback, which can strand the user from the router.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/network.js` around lines 317 - 326, The deleteInterface method currently allows removing core UCI sections (e.g., lan, wan, loopback); modify deleteInterface (and optionally loadInterfaces rendering) to guard against deleting built-in/runtime-critical interfaces by checking the id against a whitelist/blacklist (for example: ['lan','wan','loopback'] or any names returned by this.core.getBuiltInInterfaces()), and if the id is protected, abort before calling this.core.uciDelete: show an error toast like "Cannot delete built-in interface" and return; keep the existing confirm/try/catch behavior for non-protected ids. Ensure the check references deleteInterface and update loadInterfaces so delete buttons are not rendered for protected ids.moci/js/modules/netify.js-810-825 (1)
810-825:⚠️ Potential issue | 🟠 MajorMake the flow-action entry point keyboard reachable.
These rows are mouse-only
<tr>click targets. They are not focusable and there is no Enter/Space handler, so keyboard users cannot open the action modal at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/netify.js` around lines 810 - 825, The generated row elements (tbody.innerHTML mapping creating <tr class="netify-flow-row" data-flow-index="...">) are not keyboard focusable or actionable; update the row markup to include tabindex="0" and ensure keyboard activation is handled by adding a keydown listener that triggers the same action as the click (handle Enter and Space) — either inline by wiring the same handler used for click events on elements with class "netify-flow-row" or by updating the event delegation logic that listens for clicks to also listen for keydown and check for Enter/Space on elements with data-flow-index. Ensure you reference the "netify-flow-row" class and "data-flow-index" attribute so keyboard activation mirrors the existing click behavior.moci/app.css-1624-1660 (1)
1624-1660:⚠️ Potential issue | 🟠 MajorThe new breakpoints still don't handle the expanded top navigation.
These responsive rules only adjust monitoring/dashboard layouts. After adding more primary routes in
moci/index.htmlLines 68-75,.headerand.navstill stay on a single row with fixed gaps, so the menu will overflow on narrower screens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/app.css` around lines 1624 - 1660, The header/navigation overflow is caused because .header and .nav remain single-row with fixed gaps; update the responsive CSS to make the header and nav wrap or stack at the same breakpoints you added: in the `@media` (max-width: 960px) and `@media` (max-width: 640px) blocks set .header to allow wrapping (e.g., flex-wrap: wrap) and adjust .nav to either reduce gap or switch to column layout (e.g., .nav {flex-direction: column} or enable flex-wrap) and ensure any .nav-item/.nav-toggle/.logo rules accommodate the new flow so extra primary routes no longer overflow.moci/js/modules/network.js-557-568 (1)
557-568:⚠️ Potential issue | 🟠 MajorApply firewall changes after
uciCommit().These handlers persist firewall rules but never reload the running firewall, so the UI shows a success toast while the live ruleset is unchanged until a manual restart.
moci/js/modules/netify.jsLines 1017-1021 already restart the firewall after adding Netify blocks; these save/delete paths need the same apply step.Also applies to: 574-583, 618-629, 635-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/network.js` around lines 557 - 568, Handlers call this.core.uciCommit('firewall') but never apply the updated ruleset, so after commit you must trigger a running firewall restart/apply (the same action used in moci/js/modules/netify.js) before closing the modal and showing success; update the code paths around the shown diff and the other mentioned ranges (574-583, 618-629, 635-644) to invoke the system firewall restart/apply (for example run "/etc/init.d/firewall restart" or the project's core method that executes system commands) immediately after this.core.uciCommit('firewall') and before this.core.closeModal/showToast and loadFirewall so the live rules reflect the change.moci/js/modules/netify.js-283-286 (1)
283-286:⚠️ Potential issue | 🟠 MajorLoad the total row count before computing
hasMoreFlows.
loadFlowFile(true)decides whether more history exists fromthis.totalFlowCount, but Line 285 refreshes that count only afterwards. On a cold loadknownTotalis still0, so older history gets marked exhausted after the first window and pagination cannot fetch any more rows.🔧 Minimal fix
- await this.loadFlowFile(true); - await this.loadFlowTotalCount(); + await this.loadFlowTotalCount(); + await this.loadFlowFile(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/netify.js` around lines 283 - 286, The code calls loadFlowFile(true) before updating this.totalFlowCount, causing hasMoreFlows (which relies on this.totalFlowCount) to be computed from a stale 0 value; fix by calling loadFlowTotalCount() before loadFlowFile(true) so the total row count is available when loadFlowFile computes hasMoreFlows (keep updateStatus() and refreshHostnameMap() in place).moci/js/modules/devices.js-620-624 (1)
620-624:⚠️ Potential issue | 🟠 MajorPropagate non-zero command exits out of
exec().This wrapper only checks the ubus RPC status. If
sqlite3exits non-zero,querySql()gets empty stdout and the detail panel quietly renders “no data” instead of surfacing the backend error.moci/js/modules/netify.jsLines 1101-1110 already handle this correctly.🔧 Minimal fix
async exec(command, params = [], options = {}) { const [status, result] = await this.core.ubusCall('file', 'exec', { command, params }, options); if (status !== 0) throw new Error(`${command} failed (${status})`); + if (result && Number(result.code) !== 0) { + const stderr = String(result.stderr || '').trim(); + const stdout = String(result.stdout || '').trim(); + throw new Error(stderr || stdout || `${command} exited ${result.code}`); + } return result || {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/devices.js` around lines 620 - 624, The exec() wrapper in moci/js/modules/devices.js only checks the ubus RPC status and ignores the underlying command exit code, so non-zero exits (e.g., sqlite3 errors) are swallowed; update exec(command, params, options) to inspect the returned result's exit code/exitstatus (same fields used in netify.js's handler) and when the command exit is non-zero throw an Error that includes the command, exit code and stderr/stdout details so callers like querySql() see the backend error instead of empty output.moci/js/modules/network.js-1006-1024 (1)
1006-1024:⚠️ Potential issue | 🟠 MajorThe
uci showfallback never records section types.
uci show adblock-fastemits bothadblock-fast.<section>=<type>andadblock-fast.<section>.<option>=<value>lines. This parser only handles the second form, so fallback-loaded sections never get['.type']andloadAdblock()cannot recognize anyfile_urltarget lists.🔧 Minimal fix
for (const line of lines) { const escaped = packageName.replace('-', '\\-'); + const typeMatch = line.match(new RegExp(`^${escaped}\\.([^.]+)=([^=]+)$`)); + if (typeMatch) { + const section = typeMatch[1]; + if (!cfg[section]) cfg[section] = {}; + cfg[section]['.type'] = this.stripOuterQuotes(typeMatch[2]); + continue; + } const m = line.match(new RegExp(`^${escaped}\\.([^.]+)\\.([^=]+)=(.*)$`)); if (!m) continue; const section = m[1]; const key = m[2]; const value = this.stripOuterQuotes(m[3]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/network.js` around lines 1006 - 1024, The parser parseUciShowToConfig currently ignores lines like "adblock-fast.<section>=<type>"; update it to also match and record section types by detecting lines that match the pattern `^${escaped}\.([^.=]+)=(.*)$` (i.e., section without option) and storing the parsed type into cfg[section]['.type'] before processing option lines, while keeping the existing handling for `adblock-fast.<section>.<option>=<value>` (use the same escaping via packageName.replace('-', '\\-') and the existing stripOuterQuotes for values) so loadAdblock() can read file_url targets from the '.type' entry.
🟡 Minor comments (3)
scripts/watch.js-55-62 (1)
55-62:⚠️ Potential issue | 🟡 MinorInclude the Netify collector files in the dev deploy path.
These watcher/deploy additions only cover the ping/speedtest artifacts. This PR also introduces
files/moci-netify-collector.shandfiles/netify-collector.init, so Netify backend edits never reach the target underpnpm dev.Suggested patch
-const serviceWatcher = chokidar.watch(['files/moci-ping-monitor.sh', 'files/ping-monitor.init', 'files/moci-speedtest-monitor.sh'], { - persistent: true, - ignoreInitial: true, - awaitWriteFinish: { - stabilityThreshold: 300, - pollInterval: 100 - } -}); +const serviceWatcher = chokidar.watch( + [ + 'files/moci-ping-monitor.sh', + 'files/ping-monitor.init', + 'files/moci-speedtest-monitor.sh', + 'files/moci-netify-collector.sh', + 'files/netify-collector.init' + ], + { + persistent: true, + ignoreInitial: true, + awaitWriteFinish: { + stabilityThreshold: 300, + pollInterval: 100 + } + } +); @@ function deployPingService() { try { console.log(`Deploying ping monitor service to ${targetName}...`); @@ execSync(`cat files/moci-speedtest-monitor.sh | ${SSH} "cat > /usr/bin/moci-speedtest-monitor && chmod +x /usr/bin/moci-speedtest-monitor"`, { stdio: 'pipe' }); + execSync(`cat files/moci-netify-collector.sh | ${SSH} "cat > /usr/bin/moci-netify-collector && chmod +x /usr/bin/moci-netify-collector"`, { + stdio: 'pipe' + }); + execSync(`cat files/netify-collector.init | ${SSH} "cat > /etc/init.d/netify-collector && chmod +x /etc/init.d/netify-collector"`, { + stdio: 'pipe' + }); execSync(`${SSH} "/etc/init.d/ping-monitor enable || true; /etc/init.d/ping-monitor restart"`, { stdio: 'pipe' }); + execSync(`${SSH} "/etc/init.d/netify-collector enable || true; /etc/init.d/netify-collector restart"`, { + stdio: 'pipe' + });Also applies to: 137-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/watch.js` around lines 55 - 62, The watcher currently watches only the ping/speedtest artifacts; update the chokidar.watch calls (the serviceWatcher variable and the other watcher used later around the deploy logic) to also include the new Netify files by adding 'files/moci-netify-collector.sh' and 'files/netify-collector.init' to the watched file arrays so Netify backend edits are picked up by pnpm dev and deployed the same way as the existing ping/speedtest artifacts.moci/js/modules/dashboard.js-989-993 (1)
989-993:⚠️ Potential issue | 🟡 MinorDon't blank out the new traffic meta row.
moci/index.htmlLines 163-165 add#monthly-traffic-meta, but this method clears it for every state. The result is a permanent empty strip with no interface, period, or error context even when traffic-history data loads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/dashboard.js` around lines 989 - 993, The renderMonthlyMeta function currently wipes out the `#monthly-traffic-meta` element every call; instead, update it conditionally: in renderMonthlyMeta(data, period) only clear or set metaEl.textContent when data is null/empty or an error occurred, otherwise populate metaEl with the appropriate period label and formatted summary (e.g., month range and totals) so the UI shows period/context when traffic-history is loaded; locate the renderMonthlyMeta function and replace the unconditional metaEl.textContent = '' with logic that preserves or fills content based on data and period.moci/index.html-1635-1637 (1)
1635-1637:⚠️ Potential issue | 🟡 MinorGive the icon-only toggle buttons an accessible name.
These controls only expose
▸/▾, so assistive tech gets an unlabeled button with no clear section name. Add anaria-label/aria-labelledbyand tie each button to its panel witharia-controls.Also applies to: 1721-1723, 1899-1901
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/index.html` around lines 1635 - 1637, The icon-only toggle button (id "monitoring-settings-toggle-btn" with inner span "monitoring-settings-toggle-icon") lacks an accessible name and panel association; add an aria-label or aria-labelledby that describes the button (e.g., "Expand monitoring settings") and add aria-controls pointing to the ID of the panel it toggles so assistive tech knows which region is affected. Ensure the referenced panel element has the matching id and update the button's aria-expanded state when toggled; apply the same change pattern to the other icon-only toggle buttons in this file so each has an accessible name and aria-controls reference.
🧹 Nitpick comments (6)
moci/js/modules/monitoring.js (4)
388-395: Refresh timer continues running when navigating away.The
setIntervalkeeps running even when the user leaves/monitoring. While the route check preventsrefresh()from executing, the timer itself consumes resources. Consider clearing the interval when the route changes or whencurrentRouteno longer matches.♻️ Suggested improvement
Add a method to stop the refresh loop:
stopRefreshLoop() { if (this.refreshTimer) { clearInterval(this.refreshTimer); this.refreshTimer = null; } }And call it when navigating away from the monitoring page (e.g., in a route change handler or cleanup method).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/monitoring.js` around lines 388 - 395, The refresh interval started by startRefreshLoop() is never cleared when navigating away, so add a stopRefreshLoop() method that clears this.refreshTimer and sets it to null, and ensure you call stopRefreshLoop() whenever this.core.currentRoute no longer startsWith('/monitoring') (for example from your route change handler or component cleanup) so the interval is cancelled instead of left running.
83-157: Consider extracting a reusable toggle helper.The ping and speedtest settings panel toggle/sync methods share identical logic with only element IDs differing. This could be a single parameterized helper, but it's a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/monitoring.js` around lines 83 - 157, The toggle/sync logic in toggleSettingsPanel, syncSettingsPanel, toggleSpeedtestSettingsPanel, and syncSpeedtestSettingsPanel is duplicated; extract a reusable helper (e.g., togglePanel(panelId, iconId, btnId, storageKey) and syncPanel(panelId, iconId, btnId, storageKey)) that receives the element IDs and localStorage key, then replace the four methods to call the helper with the appropriate IDs ('monitoring-settings-body', 'monitoring-settings-toggle-icon', 'monitoring-settings-toggle-btn', 'monitoring_settings_expanded') and ('monitoring-speedtest-settings-body', 'monitoring-speedtest-settings-toggle-icon', 'monitoring-speedtest-settings-toggle-btn', 'monitoring_speedtest_settings_expanded') respectively so all DOM and localStorage behavior is centralized.
328-386: Consider consolidating section resolver logic.
resolvePingSectionandresolveSpeedtestSectionare nearly identical with different type names and defaults. A single parameterized method would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/monitoring.js` around lines 328 - 386, Both resolvePingSection and resolveSpeedtestSection duplicate the same logic; extract a single parameterized helper (e.g., resolveSection or resolveMonitorSection) that accepts the UCI type string ('ping' or 'speedtest'), the default section name ('ping_monitor' or 'speedtest_monitor') and the instance cache field name (this.pingSection / this.speedtestSection). Move the shared logic that checks the cached field, calls this.core.uciGet('moci', section) and this.core.uciGet('moci') to find a section by values['.type'], and the create-if-missing logic that calls this.core.uciAdd('moci', type, defaultName) into that helper; then update resolvePingSection and resolveSpeedtestSection to simply call the new helper with the appropriate parameters and return its result. Ensure the helper preserves the same return values and error swallowing behavior as the originals.
488-495: Hardcoded 75ms threshold for 'good' status.The
getStatusFromLatencymethod uses a hardcoded75for the 'good' threshold while 'warn' and 'critical' are based onthis.thresholdMs. Consider making this configurable or deriving it from the user-configurable threshold for consistency.// Line 493: hardcoded value if (value >= 75) return 'good';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/monitoring.js` around lines 488 - 495, getStatusFromLatency uses a hardcoded 75ms for the "good" cutoff; change it to be configurable or derived from the existing this.thresholdMs so all thresholds are consistent. Update getStatusFromLatency to compute a goodThreshold (e.g., read this.goodThresholdMs if defined, otherwise derive from this.thresholdMs like a fraction or min/max) and replace the hardcoded "75" check with comparison against that goodThreshold; ensure parseFloat/NaN handling remains and the status order (critical, warn, good, ok) is preserved.moci/js/modules/system.js (2)
508-523:parseApkInfoOutputmay incorrectly split packages without version info.Using
lastIndexOf('-')to split name and version works for typical APK output likebusybox-1.35.0, but if a package name contains a hyphen and has no version suffix (unlikely but possible), it would be misinterpreted. This is a minor edge case sinceapk info -vtypically always includes versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/system.js` around lines 508 - 523, The current parseApkInfoOutput function splits on the last hyphen unconditionally, which misparses package names that contain hyphens but lack a version; update parseApkInfoOutput to detect a real version suffix before splitting by checking the substring after the last '-' against a version regex (e.g., semantic or numeric version pattern) and only treat it as version when it matches; otherwise return the whole trimmed line as name and 'N/A' as version so packages with hyphens but no version are not mis-split.
302-319: Password temporarily stored in world-readable file.The password is written to
/tmp/.passwd_inputbefore being piped topasswd. While the file is deleted in thefinallyblock, there's a brief window where another process could read it. Consider using a more secure approach.♻️ Suggested improvement using here-string
- await this.core.ubusCall('file', 'write', { - path: '/tmp/.passwd_input', - data: `${newPw}\n${newPw}\n` - }); - await this.core.ubusCall('file', 'exec', { - command: '/bin/sh', - params: ['-c', 'cat /tmp/.passwd_input | passwd root'] - }); + await this.core.ubusCall('file', 'exec', { + command: '/bin/sh', + params: ['-c', `printf '%s\\n%s\\n' '${newPw.replace(/'/g, "'\\''")}' '${newPw.replace(/'/g, "'\\''")}' | passwd root`] + });Note: The single-quote escaping handles the forbidden characters check at line 296-300.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/modules/system.js` around lines 302 - 319, The code writes the password to a temporary world-readable file via this.core.ubusCall('file','write') and then reads it with a separate this.core.ubusCall('file','exec') call; replace that pattern with a single secure exec that supplies the password on stdin (here-string or heredoc) to passwd (avoid creating /tmp/.passwd_input and the follow-up rm). Concretely, remove the this.core.ubusCall('file','write') and the cat pipe exec, and invoke this.core.ubusCall('file','exec') once to run a shell command that passes `${newPw}\n${newPw}\n` directly to passwd (using a here-string/heredoc with proper quoting/escaping consistent with the existing single-quote handling), leaving the rest of the try/catch/finally logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26fb985b-f44c-4ce2-85ef-d87b6cf6eb1a
⛔ Files ignored due to path filters (8)
screenshots/moci-adblock-fast.pngis excluded by!**/*.pngscreenshots/moci-connections.pngis excluded by!**/*.pngscreenshots/moci-dashboard.pngis excluded by!**/*.pngscreenshots/moci-devices.pngis excluded by!**/*.pngscreenshots/moci-monitoring.pngis excluded by!**/*.pngscreenshots/moci-netify.pngis excluded by!**/*.pngscreenshots/moci-network.pngis excluded by!**/*.pngscreenshots/moci-settings.pngis excluded by!**/*.png
📒 Files selected for processing (26)
MakefileQUICKREADME.BI.mdfiles/moci-netify-collector.shfiles/moci-ping-monitor.shfiles/moci-speedtest-monitor.shfiles/moci.configfiles/netify-collector.initfiles/ping-monitor.initindex.htmlmoci/app.cssmoci/app.jsmoci/index.htmlmoci/js/core.jsmoci/js/modules/dashboard.jsmoci/js/modules/devices.jsmoci/js/modules/monitoring.jsmoci/js/modules/netify.jsmoci/js/modules/network.jsmoci/js/modules/services.jsmoci/js/modules/system.jsmoci/js/modules/vpn.jsrpcd-acl.jsonscripts/build.jsscripts/setup-openwrt-router.shscripts/watch.js
|
Still fixing a few things. Will upload new files soon. |
1 participant
Added Monitoring
Added Netify
Added Devices
Added AdBlock
Fixed a few things.
Summary by CodeRabbit
New Features
Documentation