Refactor: DRY extraction and dead code removal#11
Merged
HudsonGraeme merged 5 commits intodevfrom Mar 19, 2026
Merged
Conversation
- Delete dead modules (services.js, vpn.js, app.js.bak) that were superseded by network.js CRUD implementations - Extract UCI CRUD helpers (uciEdit, uciSave, uciDeleteEntry) into core.js, replacing 8 near-identical edit/save/delete triplets in network.js with declarative field maps - Extract renderTable, filterUciSections, getFormValues/setFormValues, and spliceFileLines helpers to eliminate repeated boilerplate - Consolidate bandwidth graph drawing into drawSeries helper - Data-driven setupModals and setupHandlers replacing repetitive addEventListener blocks - Update build script to reflect removed modules
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCore UI/UCI/file helper methods were added to OpenWrtCore; dashboard, network, and system modules were refactored to use them; VPN and Services modules were removed; build/install/deploy scripts were updated to exclude the removed modules. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Module as Module (Network/System/Dashboard)
participant Core as OpenWrtCore
participant UCI as UCI/ubus/Files
User->>Module: open modal / request list / submit form
Module->>Core: getFormValues / renderTable / uciEdit / uciSave / uciDeleteEntry
Core->>UCI: ubus call / uci get/set/delete / file splice
UCI-->>Core: result / status
Core-->>Module: success/error + data
Module-->>User: update table / close modal / show toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build.js (1)
11-16:⚠️ Potential issue | 🟠 MajorUpdate dev watch sync targets to match removed modules.
Build targets now exclude
vpn.jsandservices.js, butscripts/watch.jsstill copies both (Line 85-Line 92 inscripts/watch.js). Once those files are removed, watch sync will fail withENOENT.Suggested follow-up patch (outside this file)
--- a/scripts/watch.js +++ b/scripts/watch.js @@ - execSync(`cat moci/js/modules/vpn.js | ${SSH} "cat > /www/moci/js/modules/vpn.js"`, { stdio: 'pipe' }); - execSync(`cat moci/js/modules/services.js | ${SSH} "cat > /www/moci/js/modules/services.js"`, { - stdio: 'pipe' - }); + // vpn.js and services.js removed; no sync needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build.js` around lines 11 - 16, The dev watch sync still tries to copy removed modules vpn.js and services.js while the build targets (the files array) no longer include them, causing ENOENT; update the copy logic in the watch script to match the new build targets by removing vpn.js and services.js from the watch sync list or, alternatively, guard the copy operation with an existence check (e.g., fs.existsSync) before attempting to copy; ensure the watch script's copy logic and any sync target list align with the files array defined in build.js.
🧹 Nitpick comments (1)
moci/js/core.js (1)
662-691: Await and guardreloadFnto avoid post-success UI desync.
reloadFn()is unguarded and not awaited in both save/delete helpers. Async reload failures can surface as unhandled rejections after a success toast/commit.Suggested patch
- reloadFn(); + if (typeof reloadFn === 'function') { + await reloadFn(); + } @@ - reloadFn(); + if (typeof reloadFn === 'function') { + await reloadFn(); + }Also applies to: 693-703
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@moci/js/core.js` around lines 662 - 691, The post-success reloadFn is called without checking or awaiting it, which can produce unhandled rejections; update uciSave (and the similar delete helper around lines 693-703) to first verify reloadFn is a function (typeof reloadFn === 'function'), then await its result inside the existing try block (or in a small nested try/catch) so any errors are caught and handled (e.g., log or showToast) instead of surfacing as unhandled rejections after commit/toast; reference the uciSave function and the corresponding delete helper when applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@moci/js/core.js`:
- Around line 612-625: getFormValues currently coerces and flattens input types
(checkboxes, multi-selects and list-backed UCI fields) which breaks
round-tripping; update getFormValues to preserve native types: for checkboxes
use el.checked (boolean), for multi-selects detect el.multiple and collect
Array.from(el.selectedOptions).map(o => o.value) (array), otherwise use el.value
(string), and when uciKey is an array assign a copy of the array formVal (or
wrap single values into a single-element array) instead of
serializing/flattening, ensuring values[uciKeyName] keeps the same JS type;
apply the same logic to the analogous handler referenced in the other block
(lines ~627-648) so list and boolean UCI options are preserved.
In `@moci/js/modules/dashboard.js`:
- Around line 77-85: The CPU usage calculation can divide by zero when
totalDelta is 0; update the block in the code that uses
this.lastCpuStats/current to compute idleDelta, totalDelta and usage to guard
before dividing: compute idleDelta and totalDelta as now, then if totalDelta <=
0 or not finite, set usage to "0.0" (or skip DOM updates) to avoid NaN/Infinity,
otherwise compute usage = ((1 - idleDelta / totalDelta) * 100).toFixed(1);
finally only update cpuEl.textContent and cpuBarEl.style.width when usage is a
finite number. Ensure you reference the existing symbols: this.lastCpuStats,
current, idleDelta, totalDelta, usage, cpuEl, cpuBarEl.
In `@moci/js/modules/network.js`:
- Around line 48-56: The forEach arrow callback is using an expression body
which implicitly returns a value and triggers the useIterableCallbackReturn lint
rule; change the modals.forEach callback to a block-bodied arrow function (add {
... } around the body) and call this.core.setupModal(...) inside the block
without returning anything (no implicit expression return). Specifically update
the modals.forEach(m => this.core.setupModal({...})) to modals.forEach(m => {
this.core.setupModal({ modalId: `${m.prefix}-modal`, closeBtnId:
`close-${m.prefix}-modal`, cancelBtnId: `cancel-${m.prefix}-btn`, saveBtnId:
`save-${m.prefix}-btn`, saveHandler: m.save }); }); so the callback is a
statement block and satisfies the lint rule.
In `@moci/js/modules/system.js`:
- Line 97: The forEach callback currently uses an expression-bodied arrow that
implicitly returns fn(), which triggers the lint rule
lint/suspicious/useIterableCallbackReturn; change the callback for
this.cleanups.filter(Boolean).forEach(fn => fn()); to use a block-bodied
function (e.g., forEach(fn => { fn(); })) so the callback does not return a
value; update the invocation site referencing cleanups and the forEach callback
accordingly.
---
Outside diff comments:
In `@scripts/build.js`:
- Around line 11-16: The dev watch sync still tries to copy removed modules
vpn.js and services.js while the build targets (the files array) no longer
include them, causing ENOENT; update the copy logic in the watch script to match
the new build targets by removing vpn.js and services.js from the watch sync
list or, alternatively, guard the copy operation with an existence check (e.g.,
fs.existsSync) before attempting to copy; ensure the watch script's copy logic
and any sync target list align with the files array defined in build.js.
---
Nitpick comments:
In `@moci/js/core.js`:
- Around line 662-691: The post-success reloadFn is called without checking or
awaiting it, which can produce unhandled rejections; update uciSave (and the
similar delete helper around lines 693-703) to first verify reloadFn is a
function (typeof reloadFn === 'function'), then await its result inside the
existing try block (or in a small nested try/catch) so any errors are caught and
handled (e.g., log or showToast) instead of surfacing as unhandled rejections
after commit/toast; reference the uciSave function and the corresponding delete
helper when applying this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68fce571-ae9d-4b17-a98a-dd9167456091
📒 Files selected for processing (8)
moci/app.js.bakmoci/js/core.jsmoci/js/modules/dashboard.jsmoci/js/modules/network.jsmoci/js/modules/services.jsmoci/js/modules/system.jsmoci/js/modules/vpn.jsscripts/build.js
💤 Files with no reviewable changes (2)
- moci/js/modules/vpn.js
- moci/js/modules/services.js
Comment on lines
+612
to
+625
| getFormValues(fieldMap) { | ||
| const values = {}; | ||
| for (const [elementId, uciKey] of Object.entries(fieldMap)) { | ||
| const el = document.getElementById(elementId); | ||
| if (!el) continue; | ||
| const formVal = el.type === 'checkbox' ? el.checked : el.value; | ||
| if (Array.isArray(uciKey)) { | ||
| for (const key of uciKey) values[key] = formVal; | ||
| } else { | ||
| values[uciKey] = formVal; | ||
| } | ||
| } | ||
| return values; | ||
| } |
There was a problem hiding this comment.
Fix form/UCI type coercion (booleans and lists are not round-trippable).
Current conversion mutates data types: checkbox reads use !!val (so "0" becomes checked) and array values are flattened to strings when saved back. This can corrupt list-based options (e.g., WireGuard allowed_ips) and toggle fields.
Suggested hardening patch
getFormValues(fieldMap) {
const values = {};
- for (const [elementId, uciKey] of Object.entries(fieldMap)) {
+ for (const [elementId, spec] of Object.entries(fieldMap)) {
+ const { key: uciKey, parse } =
+ typeof spec === 'string' || Array.isArray(spec) ? { key: spec } : spec;
const el = document.getElementById(elementId);
if (!el) continue;
- const formVal = el.type === 'checkbox' ? el.checked : el.value;
+ const rawVal = el.type === 'checkbox' ? (el.checked ? '1' : '0') : el.value;
+ const formVal = parse ? parse(rawVal) : rawVal;
if (Array.isArray(uciKey)) {
for (const key of uciKey) values[key] = formVal;
} else {
values[uciKey] = formVal;
}
}
return values;
}
setFormValues(fieldMap, data) {
- for (const [elementId, uciKey] of Object.entries(fieldMap)) {
+ for (const [elementId, spec] of Object.entries(fieldMap)) {
+ const { key: uciKey, format } =
+ typeof spec === 'string' || Array.isArray(spec) ? { key: spec } : spec;
const el = document.getElementById(elementId);
if (!el) continue;
let val;
@@
if (el.type === 'checkbox') {
- el.checked = !!val;
+ el.checked = val === true || val === 1 || val === '1' || val === 'true' || val === 'on';
} else {
- el.value = Array.isArray(val) ? val.join(', ') : val || '';
+ el.value = format ? format(val) : Array.isArray(val) ? val.join(' ') : (val ?? '');
}
}
}Also applies to: 627-648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@moci/js/core.js` around lines 612 - 625, getFormValues currently coerces and
flattens input types (checkboxes, multi-selects and list-backed UCI fields)
which breaks round-tripping; update getFormValues to preserve native types: for
checkboxes use el.checked (boolean), for multi-selects detect el.multiple and
collect Array.from(el.selectedOptions).map(o => o.value) (array), otherwise use
el.value (string), and when uciKey is an array assign a copy of the array
formVal (or wrap single values into a single-element array) instead of
serializing/flattening, ensuring values[uciKeyName] keeps the same JS type;
apply the same logic to the analogous handler referenced in the other block
(lines ~627-648) so list and boolean UCI options are preserved.
…emove stale file refs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
moci/js/modules/network.js (1)
627-669: DDNS fieldMap array behavior is intentional but potentially confusing.The
['lookup_host', 'domain']array in the fieldMap (lines 634, 654) causes the same form value to be written to both UCI keys. Per thegetFormValuesimplementation, this is intentional aliasing behavior. However, this might not be the intended DDNS behavior iflookup_hostanddomainshould have different values.If this is intentional (both should always match), consider adding a brief comment to clarify.
🤖 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 627 - 669, The DDNS fieldMap in editDDNS and saveDDNS currently maps the hostname form field to both UCI keys via the array ['lookup_host','domain'], which causes the same form value to be written to both lookup_host and domain; decide whether this aliasing is intentional and either (a) if intentional, add a short inline comment near the fieldMap entries in editDDNS and saveDDNS explaining that ['lookup_host','domain'] intentionally syncs both keys, or (b) if unintended, change the mapping so the form supplies separate values (e.g. map only 'lookup_host' or provide distinct form fields) so lookup_host and domain can differ; reference the fieldMap arrays in the editDDNS and saveDDNS methods and update accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@moci/js/core.js`:
- Around line 705-722: spliceFileLines currently decides whether to append a
trailing '\n' using raw.endsWith('\n'), which can be wrong after we mutate the
lines array (see spliceFileLines, lines, raw). Change the final newline decision
to inspect the modified lines array instead of only raw: after building lines
and before joining, determine if the modified lines should have a trailing
newline (e.g., if the last element of lines is '' or the original raw ended with
'\n') and then return lines.join('\n') with a trailing '\n' only when that
condition is true; update the return expression in spliceFileLines accordingly.
In `@moci/js/modules/dashboard.js`:
- Around line 35-40: The memPercent calculation in dashboard.js can divide by
zero when systemInfo.memory.total is 0; update the logic around the memPercent
computation (the block that computes memPercent and sets
memoryBarEl.style.width) to first check systemInfo.memory.total > 0 and compute
memPercent only in that case, otherwise set memPercent to 0 (and optionally
clamp between 0 and 100) before calling toFixed and assigning
memoryBarEl.style.width; keep the existing memoryEl update that uses
this.core.formatMemory(systemInfo.memory).
In `@moci/js/modules/network.js`:
- Around line 863-896: The save path currently sends edit-wg-peer-allowed-ips as
a raw string so UCI will store a single string instead of a list; modify
saveWgPeer to read the form input for 'edit-wg-peer-allowed-ips' (e.g.,
document.getElementById('edit-wg-peer-allowed-ips').value), split it into
entries on commas, whitespace or newlines, trim and filter out empty items, and
pass that resulting array as the allowed_ips value when calling
this.core.uciSave for the wireguard peer (keep using saveWgPeer and the existing
fieldMap mapping for other fields, but override or inject the processed array
for 'edit-wg-peer-allowed-ips' so UCI receives an array/list rather than a
single string).
---
Nitpick comments:
In `@moci/js/modules/network.js`:
- Around line 627-669: The DDNS fieldMap in editDDNS and saveDDNS currently maps
the hostname form field to both UCI keys via the array ['lookup_host','domain'],
which causes the same form value to be written to both lookup_host and domain;
decide whether this aliasing is intentional and either (a) if intentional, add a
short inline comment near the fieldMap entries in editDDNS and saveDDNS
explaining that ['lookup_host','domain'] intentionally syncs both keys, or (b)
if unintended, change the mapping so the form supplies separate values (e.g. map
only 'lookup_host' or provide distinct form fields) so lookup_host and domain
can differ; reference the fieldMap arrays in the editDDNS and saveDDNS methods
and update accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92b00c38-f4ac-4e83-8c33-dfb0acf48669
📒 Files selected for processing (6)
Makefilemoci/js/core.jsmoci/js/modules/dashboard.jsmoci/js/modules/network.jsmoci/js/modules/system.jsscripts/watch.js
💤 Files with no reviewable changes (2)
- Makefile
- scripts/watch.js
moci/js/modules/dashboard.js
Outdated
Comment on lines
35
to
40
| const memPercent = ( | ||
| ((systemInfo.memory.total - systemInfo.memory.free) / systemInfo.memory.total) * | ||
| 100 | ||
| ).toFixed(0); | ||
| if (memoryEl) memoryEl.textContent = this.core.formatMemory(systemInfo.memory); | ||
| if (memoryBarEl) memoryBarEl.style.width = memPercent + '%'; |
There was a problem hiding this comment.
Guard against zero total memory.
While unlikely in practice, if systemInfo.memory.total is 0, this calculation produces NaN or Infinity, which would render as invalid percentage/width.
Suggested guard
- const memPercent = (
- ((systemInfo.memory.total - systemInfo.memory.free) / systemInfo.memory.total) *
- 100
- ).toFixed(0);
+ const memTotal = systemInfo.memory.total || 1;
+ const memPercent = (((memTotal - systemInfo.memory.free) / memTotal) * 100).toFixed(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const memPercent = ( | |
| ((systemInfo.memory.total - systemInfo.memory.free) / systemInfo.memory.total) * | |
| 100 | |
| ).toFixed(0); | |
| if (memoryEl) memoryEl.textContent = this.core.formatMemory(systemInfo.memory); | |
| if (memoryBarEl) memoryBarEl.style.width = memPercent + '%'; | |
| const memTotal = systemInfo.memory.total || 1; | |
| const memPercent = (((memTotal - systemInfo.memory.free) / memTotal) * 100).toFixed(0); | |
| if (memoryEl) memoryEl.textContent = this.core.formatMemory(systemInfo.memory); | |
| if (memoryBarEl) memoryBarEl.style.width = memPercent + '%'; |
🤖 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 35 - 40, The memPercent
calculation in dashboard.js can divide by zero when systemInfo.memory.total is
0; update the logic around the memPercent computation (the block that computes
memPercent and sets memoryBarEl.style.width) to first check
systemInfo.memory.total > 0 and compute memPercent only in that case, otherwise
set memPercent to 0 (and optionally clamp between 0 and 100) before calling
toFixed and assigning memoryBarEl.style.width; keep the existing memoryEl update
that uses this.core.formatMemory(systemInfo.memory).
…lowed_ips as UCI list
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
moci/js/modules/dashboard.js (1)
341-345: Minor: Consider adding guard for empty data arrays in drawSeries.While
updateBandwidthGraphalready checksdownData.length < 2, ifdrawSeriesis called directly with empty data,data.length - 1on line 302 would be-1, causing rendering issues.Optional defensive guard
drawSeries(data, max, stepX, padding, height, fillColor, strokeColor) { + if (data.length === 0) return; const ctx = this.bandwidthCtx;🤖 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 341 - 345, The drawSeries function can be called with empty or single-element arrays which makes (data.length - 1) produce -1 and break rendering; add a defensive guard at the start of drawSeries (the function named drawSeries) to return early when data.length < 2 (or otherwise clamp the denominator) and ensure any downstream math like stepX or max computation is only performed when data has at least two points; update callers if needed but primarily protect drawSeries itself so it safely no-ops on empty/single-item arrays.moci/js/modules/network.js (2)
313-328: DRY opportunity: Firewall field maps are duplicated.The same field mapping is defined in both
editForward(lines 317-324) andsaveForward(lines 336-343), and similarly foreditFirewallRule/saveFirewallRule. Consider extracting these to constants:Suggested refactor
// At class level or module scope const FORWARD_FIELD_MAP = { 'edit-forward-name': 'name', 'edit-forward-proto': 'proto', 'edit-forward-src-dport': 'src_dport', 'edit-forward-dest-ip': 'dest_ip', 'edit-forward-dest-port': 'dest_port', 'edit-forward-enabled': 'enabled' }; // Then use: editForward(id) { this.core.uciEdit('firewall', id, FORWARD_FIELD_MAP, 'forward-modal', 'edit-forward-section'); }Also applies to: 354-370
🤖 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 313 - 328, Extract the duplicated field-mapping objects used in editForward/saveForward and editFirewallRule/saveFirewallRule into module- or class-level constants (e.g., FORWARD_FIELD_MAP and FIREWALL_RULE_FIELD_MAP) and replace the inline maps in the methods (editForward, saveForward, editFirewallRule, saveFirewallRule) with references to those constants so the same mapping is reused when calling this.core.uciEdit and this.core.uciSave.
110-111: Expression-bodied forEach callback may trigger lint warning.The cleanup callback
fn => fn()implicitly returns the result offn(). While functionally correct, some linters (like Biome'suseIterableCallbackReturn) flag this pattern.Optional fix for lint compliance
- this.cleanups.filter(Boolean).forEach(fn => fn()); + this.cleanups.filter(Boolean).forEach(fn => { fn(); });🤖 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 110 - 111, The forEach callback currently uses an expression-bodied arrow (fn => fn()) which can trigger linters like Biome; update the cleanup invocation in the method that uses this.cleanups (the block with "this.cleanups.filter(Boolean).forEach(...)") to use a statement-bodied callback or an explicit loop (e.g., change to a block-style arrow like fn => { fn(); } or iterate with for (const fn of this.cleanups.filter(Boolean)) { fn(); }) and keep the subsequent this.cleanups = [] reset as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@moci/js/modules/network.js`:
- Around line 627-642: The current editDDNS mapping uses '.name' for
'edit-ddns-name' which never exists in UCI result.values, so the form field
stays empty; update editDDNS to stop mapping 'edit-ddns-name' to '.name' and
instead set the name field explicitly from the section id (the id parameter)
before/after calling this.core.uciEdit, or remove the 'edit-ddns-name' entry
from the field map so the existing 'edit-ddns-section' handling supplies the
section ID—change references in editDDNS and ensure the DOM id 'edit-ddns-name'
is assigned id when opening the modal.
---
Nitpick comments:
In `@moci/js/modules/dashboard.js`:
- Around line 341-345: The drawSeries function can be called with empty or
single-element arrays which makes (data.length - 1) produce -1 and break
rendering; add a defensive guard at the start of drawSeries (the function named
drawSeries) to return early when data.length < 2 (or otherwise clamp the
denominator) and ensure any downstream math like stepX or max computation is
only performed when data has at least two points; update callers if needed but
primarily protect drawSeries itself so it safely no-ops on empty/single-item
arrays.
In `@moci/js/modules/network.js`:
- Around line 313-328: Extract the duplicated field-mapping objects used in
editForward/saveForward and editFirewallRule/saveFirewallRule into module- or
class-level constants (e.g., FORWARD_FIELD_MAP and FIREWALL_RULE_FIELD_MAP) and
replace the inline maps in the methods (editForward, saveForward,
editFirewallRule, saveFirewallRule) with references to those constants so the
same mapping is reused when calling this.core.uciEdit and this.core.uciSave.
- Around line 110-111: The forEach callback currently uses an expression-bodied
arrow (fn => fn()) which can trigger linters like Biome; update the cleanup
invocation in the method that uses this.cleanups (the block with
"this.cleanups.filter(Boolean).forEach(...)") to use a statement-bodied callback
or an explicit loop (e.g., change to a block-style arrow like fn => { fn(); } or
iterate with for (const fn of this.cleanups.filter(Boolean)) { fn(); }) and keep
the subsequent this.cleanups = [] reset as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32fc7c82-42b1-4b31-8a4c-0c7f2a095f24
📒 Files selected for processing (3)
moci/js/core.jsmoci/js/modules/dashboard.jsmoci/js/modules/network.js
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@moci/js/modules/dashboard.js`:
- Around line 111-113: Clamp negative network deltas before computing and
recording rates: compute deltaRx = current.rx - this.lastNetStats.rx and deltaTx
= current.tx - this.lastNetStats.tx, set each to Math.max(0, delta) before
dividing to produce rxRate and txRate, and use those clamped values when
updating any networkHistory or pushing to the graph; apply the same clamp logic
to the other block referenced (lines 119-124) where similar deltas are computed
so no negative bandwidth values are rendered or stored.
- Around line 85-88: The catch block that handles CPU sampling failure sets the
CPU element's textContent to "N/A" but leaves any prior bar width/styling, so
update the catch to also reset the visual bar state: after obtaining const cpuEl
= document.getElementById('cpu') ensure you clear/reset its width/transform/aria
attributes (e.g., set inline style width to "0%" or remove the progress style
and update any aria-valuenow) so no stale utilization remains; reference the
getElementById('cpu') usage in the catch and adjust its style/attributes
accordingly.
In `@moci/js/modules/network.js`:
- Around line 551-556: Validate the provided hosts entry index before calling
spliceFileLines and before writing hosts: check that the numeric index is within
the bounds of this.hostsRaw entries (and that it's a valid integer), call
spliceFileLines only if the index is valid, and verify the returned newContent
differs from the original (or that spliceFileLines indicates a change); if the
index is invalid or no-op, abort the flow and surface an error instead of
proceeding to writeHosts and reporting success. Apply this same validation logic
around the usages of index/newContent in the block that creates newContent via
this.core.spliceFileLines and the later block (the second occurrence at the
569–574 area) so both edits/deletes fail fast and do not silently drop user
changes.
- Around line 748-756: The values object is always setting addresses to ['']
when the wg-address field is blank; change the code that builds values (around
ifaceName and values) to read the wg-address value into a local variable, trim
it, and only include an addresses property when that trimmed string is non-empty
(e.g., set addresses: [addr] or omit the addresses key entirely if empty);
reference the wg-address element and the values object to locate where to apply
this conditional assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bfab3f1-5092-4de7-864e-60591001281e
📒 Files selected for processing (2)
moci/js/modules/dashboard.jsmoci/js/modules/network.js
…ries, forEach lint
HudsonGraeme
force-pushed
the
refactor/dry-simplify
branch
from
6912bb3 to
387758b
Compare
…ex, guard empty WG address
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
services.js,vpn.js,app.js.bak) that were superseded bynetwork.jsCRUD implementationscore.js:uciEdit,uciSave,uciDeleteEntry,renderTable,filterUciSections,spliceFileLines— replacing 8 near-identical edit/save/delete triplets with declarative field mapssetupModals/setupHandlersreplacing repetitiveaddEventListenerblocksNet: −4,353 lines (840 added, 5,193 removed)
Test plan
pnpm run build)pnpm run format:check)Summary by CodeRabbit
New Features
Bug Fixes
Removed
Chores