luci-app-usteer: per-SSID settings tabs and known-station tracking - #8869
luci-app-usteer: per-SSID settings tabs and known-station tracking#8869grzegorz914 wants to merge 2 commits into
Conversation
279269a to
ff4374e
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the two new commits (per-SSID settings UI + translation refresh). The backend dependency (openwrt/usteer#22) is disclosed in the PR body, so the new UCI options / delete_known ubus call are not flagged as frontend-only. Both commit messages match their diffs. Two inline questions about the per-SSID section handling below.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the two new commits since ff4374e (aab5f29 lazy per-SSID section creation + collision-free ids, 18f43f1 re-nest SSID tabs after every render). Both previous inline findings are addressed. The lazy-write approach checks out against form.js/uci.js: uci.set() is indeed a no-op on a section that doesn't exist yet, so creating the section inside the write() wrapper rather than at render time is the right hook, and AbstractValue.write() honours ucisection, so the wrapper lands in the correct section.
One follow-on issue with the collision fix and two nits inline.
Commit checks
- 18f43f1 "luci-app-usteer: re-nest SSID tabs after every render, not just the first" — the body's claim "Pre-existing bug, not introduced by the per-SSID tabs commit itself (reproduced on an older build too)" doesn't hold against this tree:
master(96a255d) has no tab-relocation code inusteer.jsat all — nodata-tabDOM manipulation, noknowndevicestab, norelocateSsidTabsequivalent. All of it is added by cd58ab1 in this same PR, so this is fixing a bug introduced two commits earlier in the series, not a pre-existing one. Worth rewording (or dropping the sentence) so the history doesn't mislead someone bisecting later.
Generated by Claude Code
18f43f1 to
cae4bcd
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the new work since 18f43f1. The branch was force-pushed: e1488b2 squashes the previously reviewed cd58ab1 + aab5f29 + 18f43f1 together with this round's ssidSectionId() fixes, cae4bcd carries the i18n, and 2a5c682 + 1443c8e are genuinely new. All four commit messages match their diffs, so no Commit checks section.
The ssidSectionId() rework addresses the previous round's findings correctly — ids are now resolved from each existing usteer_ssid section's own ssid option before any allocation, usedSsidSectionIds is seeded from the existing section names, and both maps are Object.create(null). uci.load('usteer') is part of load(), so uci.sections() has real data by the time render() calls ssidSectionId(). The known_stations_timeout o.default = '0' removal is right: renderWidget() seeds the input with this.default when cfgvalue is null, which is what was pre-filling a literal 0 over the placeholder.
The band steering toggle itself behaves as the commit message describes — with default = '1' and optional, Flag.parse() removes the option when checked and writes '0' when unchecked, so the daemon's implicit on-by-default is preserved and no section is created just by leaving the box ticked. One issue with how the four dependent fields are wired to it, plus one series nit, inline.
Generated by Claude Code
1443c8e to
3d84344
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-pushed series (now b3f29ea + 3d84344, replacing e1488b2/cae4bcd/2a5c682/1443c8e). The i18n squash asked for last round is done: the only non-reference change in the po files since 1443c8e is one re-wrapped Polish line in po/pl, and the .pot references line up with the final usteer.js (e.g. Band steering → usteer.js:667). Both commit messages match their diffs — b3f29ea touches only usteer.js, 3d84344 only the po files — so no Commit checks section.
The hasBandSteeringToggle hoist resolves the previous round's finding correctly: all four depends(sid + '_band_steering_enabled', '1') sites are now behind the same Initscript.includes() check that guards the toggle itself, so on a usteer build without band_steering_enabled the four parameter fields stay visible and parse() no longer strips their stored values.
New this round: the delete_known plumbing added by b3f29ea looks like it cannot work as shipped — the method is missing from the app's rpcd ACL, and the RPC declaration lacks reject: true, which turns any failure into a silent fake success. Details plus one question about remote nodes inline.
Generated by Claude Code
3d84344 to
17fead0
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-pushed series (now 2516ae3 + 17fead0, replacing b3f29ea/3d84344). The delta since 3d84344 is small: reject: true on the delete_known declaration, the catch() now surfacing err.message via ui.addNotification(), the isLocal flag on collectKnownStaEntries(), and the ACL grant. Both commit messages match their diffs — 2516ae3 touches only usteer.js and acl.d/luci-app-usteer.json, 17fead0 only the po files — so no Commit checks section.
All three findings from the previous round are resolved correctly:
reject: trueis set, sohandleCallReply()now raises on a non-zero ubus status instead of resolving with the status code, and thecatch()branch is reachable.L.raise('RPCError', …)produces a realError, soerr.messageis populated."usteer": [ "delete_known" ]landed underwrite.ubus, which is the right block for a state-mutating call.- The remote-node question is answered by dropping the button on
Remoteinforows rather than shipping one that silently undoes itself on the nextremote_update_interval; both call sites (lines 200-201 and 802-803) pass the flag consistently.
The i18n commit is now purely reference-line churn relative to 3d84344 — no msgid or msgstr changed — and the regenerated references match the final file (e.g. Delete → usteer.js:171). FormalityCheck is green on 17fead0.
One nit inline.
Generated by Claude Code
17fead0 to
ced26b7
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-pushed series (now 9a7e04e + ced26b7, replacing 2516ae3/17fead0). The delta since 17fead0 is one line of usteer.js plus the i18n propagation of that single msgid. Both commit messages match their diffs — 9a7e04e touches only usteer.js and acl.d/luci-app-usteer.json, ced26b7 only the po files — so no Commit checks section.
The previous round's nit is addressed: the Known-stations help text now explains why remote rows carry no Delete button, and it gained the trailing period the two preceding sentences already had. The .pot reference still resolves to the right place (usteer.js:184), the new msgid is propagated to all 31 language files plus the template, and the Polish msgstr is a complete translation of the new wording. All three FormalityCheck runs are green on ced26b7.
Two i18n nits inline, both confined to po/pl/usteer.po; nothing blocking.
Generated by Claude Code
…g UI
Add a settings tab per configured SSID (nested under Settings), each
bound to its own usteer_ssid UCI section, mirroring the daemon's
per-SSID configuration overrides. The SSID list determines which
tabs are shown live, without a page reload.
Section creation is lazy: opening the page or saving an unrelated
global field never materializes a usteer_ssid section for an SSID
the admin hasn't touched - the section is only created the first
time a value is actually written for it (uci.set() on a section that
doesn't exist yet is a silent no-op, so this has to happen inside
each field's write(), not at render time). ssidSectionId() resolves
an SSID's section id from any existing usteer_ssid section's own
ssid option first, and only allocates a fresh id, disambiguated with
a numeric suffix on collision, for SSIDs that have none yet - so the
mapping survives SSID renames/reordering across page loads instead
of depending on allocation order within the current one, and two
SSIDs that sanitize to the same id (e.g. "my wifi" / "my-wifi") each
still get their own section.
Add a "Known stations" tab showing the daemon's persisted
best-signal-ever-seen data per node, with a Delete button per local
entry (new delete_known ubus call, granted in the app's rpcd ACL)
and a description of the cold-start "exploratory" placeholder
semantics. Remote nodes' entries don't get a Delete button: removing
one only clears this AP's synced copy, not the owning peer's own
record, so it would just reappear on the next remote update.
Add live auto-refresh for newly seen MAC addresses on the Hearing
map, removing the previous "refresh the page" notice.
Add a "Band steering" toggle to the per-SSID tab (mirroring the
daemon's band_steering_enabled option), defaulting to checked to
match the daemon's own default - band steering was implicitly always
on before this toggle existed, so the widget must default to on too,
or it would misleadingly show off while the daemon still steers. The
four band-steering parameter fields depend on it and hide when
disabled; that dependency is itself conditional on the toggle having
actually been added (Initscript.includes('band_steering_enabled')),
since depending on an option a given usteer build never created
would permanently hide those fields and strip their saved values on
the next Save (an option with unmet deps is inactive, and parse()
removes rather than writes inactive options).
Also fixes the Settings/SSID/Known-stations tab menu rendering four
times over when usteer_ssid UCI sections are present (missing
section-type argument to form.TypedSection), re-nests the SSID tabs
after every render rather than only the first (Map.prototype.save()
re-renders the map's content in place via the same renderContents()
used for the initial render, which otherwise undid the relocation on
every Save), and improves labels, descriptions, placeholders and
default-value documentation across the existing global settings
fields (units on time/signal fields, dual SNR/dBm convention
explained where used).
Signed-off-by: Grzegorz Kaczor <grzegorz914@icloud.com>
ced26b7 to
c763e7d
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-pushed series (now 8ae5020 + c763e7d, replacing 9a7e04e/ced26b7). 8ae5020 is byte-identical to the previously reviewed 9a7e04e — same tree, same message — so the entire delta is the po/pl/usteer.po rebuild plus the paragraph documenting it in c763e7d.
Both nits from the previous round are resolved, and the rebuild's claims check out against the tree:
- Obsolete entries in
po/pl/usteer.podrop from 52 to 31, and the resulting set is exactly the setpo/de/usteer.po(and the other translated languages) gains from this PR — I compared the two files entry by entry, the msgid sets match with nothing extra on either side. The 21 PR-internal leftovers are gone. - All 165 live entries are still present, none went empty or fuzzy, and the
#:reference lines and msgid ordering are identical topo/de/usteer.po, i.e. the file really was merged against this PR's final.pot(166 entries incl. header, matchingpo/templates/usteer.pot). - The Weblate header block (
Project-Id-Version,PO-Revision-Date: 2026-03-29,Last-Translator,Plural-Forms,X-Generator) is preserved from master's copy rather than replaced by the template's, so the rebuild won't look like a translator change to Weblate. - The
Dostępne→Wypełnianerevert landed and the Polish sentence now tracks the EnglishOnly populated while …again.
All three FormalityCheck runs are green on c763e7d. One wording issue on the new commit message below; nothing blocking.
Commit checks
- c763e7d "luci-app-usteer: update translation template and Polish translation" — the new paragraph ends with "was diffed msgid-by-msgid against the carried-forward copy to confirm all 165 live translations are unchanged", but the commit does change one live translation:
po/pl/usteer.po:587-590restoresDostępne tylko gdy→Wypełniane tylko gdyin the usteer.js:184 description. That revert is correct and was the point of the previous round's second nit — but it is the only substantive msgstr change in the whole commit, and the message currently asserts the opposite. Everything else really is obsolete-entry churn, so a reader trusting the message would skip the one hunk that isn't. Suggest amending the last sentence to something like "…to confirm all 165 live translations are unchanged apart from theOnly populated while …description, whose opening verb is restored to Wypełniane."
Generated by Claude Code
Regenerate po/templates/usteer.pot for the per-SSID settings tabs, known-station tracking UI and band steering toggle, propagate the new/changed msgids to all language po files, and fully translate them into Polish (po/pl/usteer.po). po/pl/usteer.po is rebuilt from master's copy merged against this PR's final .pot, instead of being carried forward through each force-push of this series: the working copy had accumulated 21 obsolete entries left over from intermediate, since-squashed wordings that never existed in any merged tree. The rebuilt file keeps the same 31 legitimate obsolete entries every other language gains from this PR's actual wording changes, and was diffed msgid-by-msgid against the carried-forward copy to confirm all 165 live translations are unchanged apart from the "Only populated while ..." description, whose opening verb is restored to "Wypełniane" (see below). Also fixes the "Dostępne" -> "Wypełniane" wording in that same description: "Wypełniane" (populated/filled in) matches the English "Only populated while ...", while "Dostępne" (available) reads as if the whole Known stations tab were gated on the setting rather than just its rows. Signed-off-by: Grzegorz Kaczor <grzegorz914@icloud.com>
c763e7d to
51a1bf4
Compare
|
Fixed in 51a1bf4 (amend, no file changes): reworded the commit message's last sentence to call out the Dostępne→Wypełniane revert as the one substantive msgstr change, instead of implying nothing changed. |
Summary
Companion LuCI frontend for openwrt/usteer#22 (per-SSID configuration overrides and known-station tracking with cold-start exploratory candidates).
usteer_ssidUCI section, mirroring the daemon's new per-SSID configuration overrides. The SSID list determines which tabs are shown live, without a page reload.delete_knownubus call, granted in the app's rpcd ACL) and a description of the cold-start "exploratory" placeholder semantics. Remote nodes' entries have no Delete button, since removing one only clears the local synced copy, not the owning peer's own record.band_steering_enabledoption), gating the four band-steering parameter fields.Test plan
usteer_ssidUCI section, and fields fall back to the global setting when left blank