luci-app-librespeed: add new application - #8951
Conversation
ffc4f6d to
4475d33
Compare
| '<text x="%d" y="%d" font-size="10" fill="currentColor" fill-opacity="0.6">%s</text>' | ||
| .format(L, H - 6, s1) + | ||
| midLabel + | ||
| '<text x="%d" y="%d" font-size="10" fill="currentColor" fill-opacity="0.6" text-anchor="end">%s (%s)</text>' | ||
| .format(W - R, H - 6, s3, unit) + |
There was a problem hiding this comment.
s1/s3 are raw backend strings assembled into innerHTML. chartStampShort() returns e.timestamp verbatim whenever it has no T or does not parse as a date, so a history entry whose timestamp contains markup is injected into the page unescaped. %s in LuCI's String.format does no escaping — case 's': subst = param; in cbi.js:772 — but %h runs esc(param, html_esc).
The same applies to s2 in the midLabel template on line 276.
| '<text x="%d" y="%d" font-size="10" fill="currentColor" fill-opacity="0.6">%s</text>' | |
| .format(L, H - 6, s1) + | |
| midLabel + | |
| '<text x="%d" y="%d" font-size="10" fill="currentColor" fill-opacity="0.6" text-anchor="end">%s (%s)</text>' | |
| .format(W - R, H - 6, s3, unit) + | |
| '<text x="%d" y="%d" font-size="10" fill="currentColor" fill-opacity="0.6">%h</text>' | |
| .format(L, H - 6, s1) + | |
| midLabel + | |
| '<text x="%d" y="%d" font-size="10" fill="currentColor" fill-opacity="0.6" text-anchor="end">%h (%s)</text>' | |
| .format(W - R, H - 6, s3, unit) + |
Generated by Claude Code
| let row = '%s,%s,%s,%s,%s,"%s",'.format( | ||
| esc(e.timestamp), agg ? '1d' : 'raw', esc(e.interface), | ||
| esc(e.family), esc(e.proto), esc(e.server && e.server.name)) + |
There was a problem hiding this comment.
Only the server field is wrapped in quotes, but esc() (line 30) doubles " on every field it is applied to. For the unquoted timestamp/interface/family/proto columns that doubling is meaningless, and any value containing a comma, a quote or a newline silently shifts the remaining columns of that row. interface and proto come straight from the history file, so this is not purely hypothetical.
Quoting all six text fields makes esc()'s doubling correct everywhere:
| let row = '%s,%s,%s,%s,%s,"%s",'.format( | |
| esc(e.timestamp), agg ? '1d' : 'raw', esc(e.interface), | |
| esc(e.family), esc(e.proto), esc(e.server && e.server.name)) + | |
| let row = '"%s","%s","%s","%s","%s","%s",'.format( | |
| esc(e.timestamp), agg ? '1d' : 'raw', esc(e.interface), | |
| esc(e.family), esc(e.proto), esc(e.server && e.server.name)) + |
Generated by Claude Code
| 'click': ui.createHandlerFn(view, function() { | ||
| return onpick.call(view, it[0]); | ||
| }) | ||
| }, [ _(it[1] != null ? it[1] : it[0]) ]))); |
There was a problem hiding this comment.
_() is called on a runtime value, so xgettext never sees these strings and they can never be translated. Confirmed against the generated template: msgid "24h", "7d", "30d" and "1y" are all absent from po/templates/librespeed.pot, even though RANGES (lines 22–28) is fed through here by history.js:253.
It also translates twice: history.js:251 passes _('Speed')/_('Latency'), i.e. already-localised text, which is then looked up again here.
Either translate at the call sites and drop _() here, or keep _() here and wrap the RANGES labels in literal _() calls where the array is declared so the extractor can find them.
Generated by Claude Code
| 'class': 'librespeed-step' + | ||
| (i == phaseIdx ? ' librespeed-step-active' : | ||
| (phaseIdx >= 0 && i < phaseIdx ? ' librespeed-step-done' : '')) | ||
| }, [ _(p[0]) ])))); |
There was a problem hiding this comment.
Same runtime-_() problem as common.js:178: p[0] is 'ping', 'download' or 'upload' from PHASES (lines 90–94), and none of those msgids exist in po/templates/librespeed.pot. The three phase names are prominent user-visible labels in the progress strip, so they will stay English in every locale.
Give PHASES a fourth, literal, translatable label element (_('Ping'), _('Download'), _('Upload') — the last two are already extracted from common.js) and render that instead of _(p[0]).
Generated by Claude Code
| .librespeed-stroke-0 { stroke: var(--primary-color, #5bc0de); } | ||
| .librespeed-stroke-1 { stroke: var(--primary-color, #5bc0de); } | ||
| polyline.librespeed-stroke-1 { stroke-dasharray: 6 3; } | ||
| .librespeed-avgline.librespeed-stroke-1 { stroke-dasharray: 2 4; } | ||
| .librespeed-fill-0 { fill: var(--primary-color, #5bc0de); } | ||
| .librespeed-fill-1 { fill: var(--primary-color, #5bc0de); } | ||
| circle.librespeed-fill-1 { fill-opacity: .55; } | ||
| .librespeed-bg-0 { background: var(--primary-color, #5bc0de); } |
There was a problem hiding this comment.
--primary-color is not defined by any in-tree theme, so every one of these declarations always resolves to the hardcoded #5bc0de fallback. That defeats the stated intent here ("One theme color for every series") and in common.js:53–54 ("the first of its group wears the theme's primary color").
The only variables the themes actually publish are --primary-color-high/-medium/-low in luci-theme-bootstrap (with a dark-mode override);`` material, openwrt and openwrt-2020 define none, so those will keep the fallback regardless.
| .librespeed-stroke-0 { stroke: var(--primary-color, #5bc0de); } | |
| .librespeed-stroke-1 { stroke: var(--primary-color, #5bc0de); } | |
| polyline.librespeed-stroke-1 { stroke-dasharray: 6 3; } | |
| .librespeed-avgline.librespeed-stroke-1 { stroke-dasharray: 2 4; } | |
| .librespeed-fill-0 { fill: var(--primary-color, #5bc0de); } | |
| .librespeed-fill-1 { fill: var(--primary-color, #5bc0de); } | |
| circle.librespeed-fill-1 { fill-opacity: .55; } | |
| .librespeed-bg-0 { background: var(--primary-color, #5bc0de); } | |
| .librespeed-stroke-0 { stroke: var(--primary-color-medium, #5bc0de); } | |
| .librespeed-stroke-1 { stroke: var(--primary-color-medium, #5bc0de); } | |
| polyline.librespeed-stroke-1 { stroke-dasharray: 6 3; } | |
| .librespeed-avgline.librespeed-stroke-1 { stroke-dasharray: 2 4; } | |
| .librespeed-fill-0 { fill: var(--primary-color-medium, #5bc0de); } | |
| .librespeed-fill-1 { fill: var(--primary-color-medium, #5bc0de); } | |
| circle.librespeed-fill-1 { fill-opacity: .55; } | |
| .librespeed-bg-0 { background: var(--primary-color-medium, #5bc0de); } |
Same substitution applies to the gradient on line 232 and to .librespeed-fg-0/-1 (269–270) and .librespeed-accent-0/-1 (293–294).
Generated by Claude Code
|
|
||
| /* Only offered when the backend actually returned one -- telemetry may | ||
| * be off, in which case there is nothing to link to. */ | ||
| if (result.share) { |
There was a problem hiding this comment.
result.share goes straight into an href with no scheme check. It originates from the LibreSpeed telemetry server's reply (relayed by the rpcd backend), i.e. from outside the router, so a hostile or compromised telemetry endpoint can return javascript:… and get it executed in the LuCI origin — with the session already authenticated — as soon as the user clicks "View result".
Restricting it to http(s) at the guard costs nothing:
| if (result.share) { | |
| if (result.share && /^https?:\/\//i.test(result.share)) { |
Generated by Claude Code
| msgid "" | ||
| msgstr "Content-Type: text/plain; charset=UTF-8" | ||
|
|
||
| #: applications/luci-app-librespeed/htdocs/luci-static/resources/view/librespeed/test.js:216 |
There was a problem hiding this comment.
nit: the template is stale — every reference into the three view files is off by exactly 4 lines, which is the length of the /* The alias comes from the require above… */ + /* global lscommon */ block that was added to each of them afterwards.
%d measurements is cited as test.js:216 but lives at test.js:220; All as history.js:71 but lives at history.js:75; 1 year as settings.js:125 but lives at settings.js:129. References into common.js (e.g. avg at common.js:322) are correct, since that file has no such block.
Please regenerate the template from the current sources.
Generated by Claude Code
| /* The dial range starts from the last known result -- a familiar | ||
| * line gets a familiar dial from the first tick -- and only grows | ||
| * when this run's peak outruns it. */ | ||
| /* The dial range is frozen at the start of the run -- a familiar | ||
| * line gets a familiar dial -- and only ever expands, once the | ||
| * rate actually exceeds it. Recomputing it downwards mid-run | ||
| * would pull the needle back while the speed grows. */ |
There was a problem hiding this comment.
nit: two comment blocks describing the same three lines of code, and they contradict each other — the first says the dial range "starts from the last known result … and only grows", the second says it "is frozen at the start of the run". The code below matches the second. Looks like a leftover from a rewrite.
| /* The dial range starts from the last known result -- a familiar | |
| * line gets a familiar dial from the first tick -- and only grows | |
| * when this run's peak outruns it. */ | |
| /* The dial range is frozen at the start of the run -- a familiar | |
| * line gets a familiar dial -- and only ever expands, once the | |
| * rate actually exceeds it. Recomputing it downwards mid-run | |
| * would pull the needle back while the speed grows. */ | |
| /* The dial range is frozen at the start of the run -- a familiar | |
| * line gets a familiar dial -- and only ever expands, once the | |
| * rate actually exceeds it. Recomputing it downwards mid-run | |
| * would pull the needle back while the speed grows. */ |
Generated by Claude Code
| 'require librespeed.common as lscommon'; | ||
|
|
||
| /* The alias comes from the require above; the repo eslint config only | ||
| * knows the stock module names. */ | ||
| /* global lscommon */ |
There was a problem hiding this comment.
nit: lscommon is never referenced in this file — unlike test.js and history.js, settings.js uses neither lscommon.cssLink() nor any of the chart helpers. The require and the accompanying /* global */ pragma can both go.
Generated by Claude Code
5d5e160 to
1d1d942
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (4475d33). The XSS fix on the chart labels, the CSV quoting, the https?: scheme guard on the share link, the --primary-color-medium switch, the PHASES label table, the stale duplicate comment and the unused librespeed.common require in settings.js are all addressed, and the .pot line references now match the sources. Three follow-ups posted inline; CI is green on 1d1d942.
Generated by Claude Code
| /* Callers hand in display-ready labels: a runtime _() here | ||
| * would both miss the extractor and translate twice. */ | ||
| }, [ it[1] != null ? it[1] : it[0] ]))); |
There was a problem hiding this comment.
The new comment block landed at the wrong indent depth: the opening /* sits at three tabs, its continuation line and the closing } at four. Everything here closes the object literal opened by E('button', { at three tabs, so all three lines belong at three tabs.
| /* Callers hand in display-ready labels: a runtime _() here | |
| * would both miss the extractor and translate twice. */ | |
| }, [ it[1] != null ? it[1] : it[0] ]))); | |
| /* Callers hand in display-ready labels: a runtime _() here | |
| * would both miss the extractor and translate twice. */ | |
| }, [ it[1] != null ? it[1] : it[0] ]))); |
Generated by Claude Code
| [ '24h', 86400 ], | ||
| [ '7d', 604800 ], | ||
| [ '30d', 2592000 ], | ||
| [ '1y', 31536000 ], | ||
| [ 'All', 0 ] |
There was a problem hiding this comment.
Dropping the runtime _() from switcher() fixed the extractor problem for the group buttons, but only one of the two call sites was updated to compensate. history.js:271 now passes `_('Speed')`/`_('Latency')`, but [history.js:274](https://github.com/openwrt/luci/blob/1d1d9420561ce3908f96350683d4970e222fcb42/applications/luci-app-librespeed/htdocs/luci-static/resources/view/librespeed/history.js#L274)`` feeds r[0] from RANGES through unchanged, so the five range buttons are now hard-wired English.
All is a visible regression rather than just a missed opportunity: the msgid exists in the template (extracted from _('All') in filterSelect, history.js:75),`` so before this change the range button did get translated at runtime and now no longer does — while the filter dropdown right next to it still does.
r[0] is only ever used as a display label (switcher's key is String(r[1])), so wrapping it here is safe, and it matches what the PHASES table in test.js:90-94 now does:
| [ '24h', 86400 ], | |
| [ '7d', 604800 ], | |
| [ '30d', 2592000 ], | |
| [ '1y', 31536000 ], | |
| [ 'All', 0 ] | |
| [ _('24h'), 86400 ], | |
| [ _('7d'), 604800 ], | |
| [ _('30d'), 2592000 ], | |
| [ _('1y'), 31536000 ], | |
| [ _('All'), 0 ] |
Generated by Claude Code
| this.table = new ui.Table([ | ||
| _('Time'), _('Server'), _('Interface'), _('Protocol'), | ||
| _('Download [Mbps]'), _('Upload [Mbps]'), _('Ping [ms]'), _('Jitter [ms]') | ||
| ], { id: 'librespeed-history' }, E('em', {}, [ _('No measurements yet.') ])); |
There was a problem hiding this comment.
nit: this ui.Table placeholder is unreachable now that the empty state exists. The table lives inside dataNode, and redraw() returns at line 184-192 with dataNode hidden whenever entries.length is 0, so the table is never rendered without rows.
It also leaves translators with two near-identical msgids differing only by the trailing period — "No measurements yet." here and "No measurements yet" from the new empty-state heading at line 320. Dropping the placeholder argument removes both problems.
Generated by Claude Code
1d1d942 to
ca267d5
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (1d1d942). All three earlier follow-ups are addressed: the switcher() comment indentation is fixed, RANGES now carries literal _() so the range buttons are extractable and translated again, and the unreachable ui.Table placeholder is gone. I re-validated the template mechanically — all 157 source references resolve to the right line, there are no missing msgids, and no orphans. CI is green on ca267d5.
Two follow-ups posted inline, both about the same relabelling pass.
Generated by Claude Code
|
|
||
| if (!drew) { | ||
| this.recentNode.appendChild(E('p', { 'class': 'librespeed-muted' }, | ||
| [ _('No measurements yet') ])); |
There was a problem hiding this comment.
The history page just got this exact branch relabelled (No data for the selected metric., history.js:211) with the reasoning that reaching !drew does not mean "no measurements" — but the identical branch here kept the old wording, and here it is actually contradicted on screen.
renderChart returns false whenever chartScale finds no numeric values for the requested series, not only when `entries` is empty. This block requests only `download_mbps`, so a 24-hour window holding measurements without a download figure lands here while [`this.recentCount` two lines up still reads "%d measurements"](https://github.com/openwrt/luci/blob/ca267d5ff028f8732ade61fb0800af74fe305a17/applications/luci-app-librespeed/htdocs/luci-static/resources/view/librespeed/test.js#L219-L220)`` and the section subtitle says Download · last 24 hours. Older measurements outside the window produce the same claim.
Naming the window also restores the sentence period that was dropped only to share a msgid with the History page's <h?>-style empty-state heading (history.js:322), where no period is correct.
| [ _('No measurements yet') ])); | |
| [ _('No download data in the last 24 hours.') ])); |
The template needs regenerating afterwards so this msgid and its reference replace the shared No measurements yet entry.
Generated by Claude Code
| [ 'download_mbps', 'Download', 'Mbps' ], | ||
| [ 'upload_mbps', 'Upload', 'Mbps' ], | ||
| [ 'ping_ms', 'Ping', 'ms' ], | ||
| [ 'jitter_ms', 'Jitter', 'ms' ] |
There was a problem hiding this comment.
nit: RANGES (line 24) and PHASES in test.js now carry literal _() so the extractor sees them, but METRICS is the one label table left holding bare literals that are fed through a runtime _() — at common.js:324, [431](https://github.com/openwrt/luci/blob/ca267d5ff028f8732ade61fb0800af74fe305a17/applications/luci-app-librespeed/htdocs/luci-static/resources/librespeed/common.js#L431),`` 437, [history.js:143](https://github.com/openwrt/luci/blob/ca267d5ff028f8732ade61fb0800af74fe305a17/applications/luci-app-librespeed/htdocs/luci-static/resources/view/librespeed/history.js#L143)`` and history.js:157.``
These four happen to translate today only because the same words are extracted from unrelated places in test.js — the template credits Ping to test.js:91/481 and Jitter to test.js:482 alone, nothing in common.js. Rewording the dial captions or the phase table would silently turn every metric label in the charts, tooltips and legend cards back to English.
| [ 'download_mbps', 'Download', 'Mbps' ], | |
| [ 'upload_mbps', 'Upload', 'Mbps' ], | |
| [ 'ping_ms', 'Ping', 'ms' ], | |
| [ 'jitter_ms', 'Jitter', 'ms' ] | |
| [ 'download_mbps', _('Download'), 'Mbps' ], | |
| [ 'upload_mbps', _('Upload'), 'Mbps' ], | |
| [ 'ping_ms', _('Ping'), 'ms' ], | |
| [ 'jitter_ms', _('Jitter'), 'ms' ] |
The five _(m[1]) call sites above should then drop their _() and pass m[1] straight through, exactly as switcher() was changed to do, and the template regenerated.
Generated by Claude Code
cc401bb to
ed953ce
Compare
|
Hey @1715173329, since you're maintaining the librespeed-go backend in the openwrt/packages repository, what do you think about adding the backend configuration here? I'd like to do that in an upcoming version once this is merged, to avoid making more changes right now. For the time being, I'd like to keep this PR focused on the client. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (ca267d5). Both earlier follow-ups are addressed: METRICS now carries literal _() and the five _(m[1]) call sites pass the label straight through, and the Test page's empty-chart branch was reworded — the metric switcher makes the metric-neutral "No data in the last 24 hours." the right wording there. The new work in this push is the recent-history metric switcher, the ui.Table swap on the Test page, History pagination, and the tooltip-bearing configuration panel. CI is green on ed953ce.
Four follow-ups posted inline: one behavioural regression from the new paging, one stale template, and two smaller ones.
Generated by Claude Code
|
|
||
| this.renderPager(rows.length, pages); | ||
|
|
||
| this.table.update(rows.slice(this.page * pageSize, (this.page + 1) * pageSize).map(e => [ |
There was a problem hiding this comment.
Paging the rows before handing them to ui.Table turns the sortable headers into a per-page sort. update() stores exactly what it is given — this.data = data; at ui.js:3892 — and a header click re-sorts only that stored array: this.update(this.data, this.placeholder); at ui.js:4137.
So on a 30-day range with ~3000 measurements, clicking Download [Mbps] descending gives the fastest of the 50 newest rows, not the fastest of the range, and the pager label right below still says 3000 measurements. The comment at line 306-309 above the table promises the opposite ("The table is where sorting belongs"), and before this change it held, because the whole set was passed in.
Either sort rows yourself with the table's active sort state before slicing (this.table.getActiveSortState() plus deriveSortKey give you the same ordering update() would apply), or construct the table with sortable: false so the headers stop advertising an ordering they cannot deliver.
Generated by Claude Code
| msgid "Leave empty to run every day." | ||
| msgstr "" | ||
|
|
||
| #: applications/luci-app-librespeed/htdocs/luci-static/resources/view/librespeed/test.js:684 |
There was a problem hiding this comment.
nit: the template was not regenerated after the last edit to test.js. Everything up to Schedule status (test.js:657) still matches, but the four references past the point where the three-line /* Heading and count on one line… */ comment was inserted are now off by two or three lines:
| msgid | template says | actual |
|---|---|---|
LibreSpeed |
test.js:684 | test.js:687 |
Recent history |
test.js:673 (line 363) | test.js:675 |
Router speed test |
test.js:685 (line 371) | test.js:688 |
Measures the internet connection from this router. |
test.js:687 (line 267) | test.js:690 |
No msgids are missing and there are no orphans, so it is only the source references — please regenerate the template from the current sources so they point at the right lines again.
Generated by Claude Code
| rows.map(r => E('tr', { 'class': 'tr' }, [ | ||
| E('td', { 'class': 'td librespeed-muted', 'style': 'width:45%' }, [ r[0] ]), | ||
| E('td', { 'class': 'td' }, [ | ||
| r[2] ? E('span', { 'class': 'librespeed-hint', 'data-tooltip': r[2] }, [ r[1] ]) : r[1] |
There was a problem hiding this comment.
The comment above promises "on hover or focus", but a bare <span> is not focusable, so the focus half never fires. LuCI installs both listeners globally — document.addEventListener('focus', this.showTooltip.bind(this), true) at ui.js:4178 — but the element has to be reachable by the tab order for the event to ever reach it. Today the only explanation of HTTP (forced) / HTTPS (forced) is mouse-only.
| r[2] ? E('span', { 'class': 'librespeed-hint', 'data-tooltip': r[2] }, [ r[1] ]) : r[1] | |
| r[2] ? E('span', { 'class': 'librespeed-hint', 'tabindex': '0', 'data-tooltip': r[2] }, [ r[1] ]) : r[1] |
Generated by Claude Code
| this.recentSwitch = lscommon.switcher(this, | ||
| lscommon.GROUPS.speed.map(k => { | ||
| const m = lscommon.METRICS.find(x => x[0] == k); | ||
| return [ k, m[1] ]; | ||
| }), this.recentMetric, | ||
| function(v) { | ||
| this.recentMetric = v; | ||
| this.renderRecent(this.recentData); | ||
| }); |
There was a problem hiding this comment.
nit: this switcher is discarded before it is ever seen. renderRecent() runs eight lines down at line 586 and its first act on the node is this.recentSwitch.replaceChildren(...lscommon.switcher(...).childNodes) (lines 227-235), rebuilding the identical button row from the identical GROUPS.speed.map(...) expression. The wrapper <div> carries no class or style, so nothing distinguishes the two copies — they just both have to be kept in sync by hand.
An empty container is enough; renderRecent() fills it on every call, including the first.
| this.recentSwitch = lscommon.switcher(this, | |
| lscommon.GROUPS.speed.map(k => { | |
| const m = lscommon.METRICS.find(x => x[0] == k); | |
| return [ k, m[1] ]; | |
| }), this.recentMetric, | |
| function(v) { | |
| this.recentMetric = v; | |
| this.renderRecent(this.recentData); | |
| }); | |
| this.recentSwitch = E('div', {}); |
Generated by Claude Code
e65af09 to
fbeecb7
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (ed953ce). All four earlier follow-ups are addressed:
- the paging/sorting regression is fixed —
redraw()now sorts the whole range with the table's own key derivation before slicing, and the new header-click listener ontableNoderesets to page 1 and re-sorts the range. The listener ordering works out:handleSortis bound on thetr.table-titleschild andcreateHandlerFnapplies it synchronously, so the bubbled listener on the<table>already sees the new sort state; - the template is regenerated. I re-validated it mechanically against the current sources — all 169 source references resolve to the right line, no msgids are missing, and the only entries without a JS literal are the three that come from the ACL and menu JSON;
- the configuration-panel hints carry
tabindex, so the focus half of the tooltip fires; - the duplicate
recentSwitchis gone, and the plainE('div', {})matches whatswitcher()returns anyway (its wrapper carries no class), so nothing is lost by lettingrenderRecent()fill it.
CI is green on fbeecb7. Two follow-ups posted inline, one on the numeric sort keys the new range-wide sort now relies on, one nit.
Generated by Claude Code
| [ num(e.upload_mbps), fmtNum(e.upload_mbps, 2) ], | ||
| [ num(e.ping_ms), fmtNum(e.ping_ms, 1) ], | ||
| [ num(e.jitter_ms), fmtNum(e.jitter_ms, 1) ] |
There was a problem hiding this comment.
Now that sorting drives the whole range rather than one page, it is worth noting that the four numeric columns order wrongly whenever two values share an integer part.
deriveSortKey is handed the entire [raw, display] cell — this.deriveSortKey(row[sorting[0]], sorting[0]) at ui.js:3869, mirrored by the block just below — and no branch in it matches an array, so it ends at return String(value) at ui.js:4052-4057, producing "94.4,94.40". L.naturalCompare is new Intl.Collator(undefined, { numeric: true }).compare at luci.js:2246, which compares each digit run on its own, so the fraction 4 loses to 35:
-1,– 0.5,0.50 0.45,0.45 8.07,8.07 8.7,8.70 9.5,9.50 94.4,94.40 94.35,94.35 120,120.00
Ascending Download puts 94.4 before 94.35 and 0.5 before 0.45; Ping and Jitter do the same with one decimal (8.7 before 8.07). The manual sort here and ui.Table.update() agree with each other — they just agree on the wrong order, so the range-wide sort inherits it.
The raw half only needs the same fixed decimal count as the display half; equal-width fraction runs then compare correctly. num (history.js:223) has no other caller:
const num = (v, d) => (typeof v == 'number') ? v.toFixed(d) : '';with the cells becoming [ num(e.download_mbps, 2), fmtNum(e.download_mbps, 2) ] and so on — line 252 needs it too, not just the three anchored here. That yields ,– 0.45 0.50 8.07 8.70 9.50 94.35 94.40 120.00.
Generated by Claude Code
| this.table.update(rows.slice(this.page * pageSize, (this.page + 1) * pageSize)); | ||
|
|
There was a problem hiding this comment.
nit: stray blank line before the closing brace — left over from moving the pager and update() call down here. It is the only one in the package; every other method in the four JS files closes straight after its last statement.
| this.table.update(rows.slice(this.page * pageSize, (this.page + 1) * pageSize)); | |
| this.table.update(rows.slice(this.page * pageSize, (this.page + 1) * pageSize)); |
Generated by Claude Code
7e2ea54 to
44c91d3
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (fbeecb7). Both earlier follow-ups are addressed:
- the numeric sort keys now carry the same fixed decimal count as their display half, so
deriveSortKey'sString(value)fallback produces equal-width digit runs andL.naturalCompareorders94.35before94.40again; the display halves also became<span>nodes, which closes thedom.append→innerHTMLpath for the server name; - the stray blank line before
redraw()'s closing brace is gone.
New work in this push: the schedule-interval table shared between Settings and the Test page, resolution-aware seriesStats() extremes, the --librespeed-accent indirection, the stopped/elapsed status handling, N_() plurals, the ARIA pass (live region, aria-pressed, aria-current), the filter reconciliation in renderControls(), the history-off empty state, the widened ACL, and moving the schedule preview out of the Map root — that last one is correct, renderContents() does dom.content(mapEl, null) on every save()/reset(), so the old node.appendChild(box) really did lose the preview.
I re-validated the template mechanically against the current sources: all 139 entries' source references resolve to the right line, no msgids are missing, and no orphans beyond the five that come from the ACL and menu JSON. CI is green on 44c91d3.
Five follow-ups posted inline: one missed resolution argument, one dark-mode media query that does not match how LuCI signals dark, one config-encoding question, and two nits.
Generated by Claude Code
| /* The stability story of the last day, told in one sentence and four | ||
| * figures: what is normal, how much it wobbles, where it is now. */ | ||
| const metric = this.recentMetric || 'download_mbps'; | ||
| const st = lscommon.seriesStats(entries, metric); |
There was a problem hiding this comment.
seriesStats() grew a third resolution parameter in this push, and every other call site was updated to pass it — history.js:122-123 and [common.js:350](https://github.com/openwrt/luci/blob/44c91d30083fb9c33264e4dbc04e2f3383f76ac3/applications/luci-app-librespeed/htdocs/luci-static/resources/librespeed/common.js#L350)`` — but this one still calls it with two arguments, so resolution is undefined and the '1d' branch never runs.
The Test page does treat 1d as reachable for this window: the chart just below is given resolution: data && data.resolution (line 270) and when() passes it to the date-only guard (line 304). When the backend answers the 24-hour window with daily aggregates, st.min/st.max here fall back to the min/max of the daily means, so Variable — ranged from %d to %d Mbps. and the best/worst figures at lines 295-296 understate the spread against the min/max band the chart directly above them draws — the exact contradiction the new comment on seriesStats, common.js:154-158`` says the parameter exists to prevent.
| const st = lscommon.seriesStats(entries, metric); | |
| const st = lscommon.seriesStats(entries, metric, data && data.resolution); |
Generated by Claude Code
| /* The theme's accent where the theme publishes one: --primary-color-medium | ||
| * exists only in luci-theme-bootstrap. Every other in-tree theme sees the | ||
| * fallback, which must clear 3:1 against a light page -- the arc and the | ||
| * history line are the only graphical encoding of the data. */ | ||
| :root { | ||
| --librespeed-accent: var(--primary-color-medium, #3c8dbc); | ||
| } | ||
|
|
||
| @media (prefers-color-scheme: dark) { | ||
| :root { | ||
| --librespeed-accent: var(--primary-color-medium, #6ab0de); | ||
| } | ||
| } |
There was a problem hiding this comment.
prefers-color-scheme is not how a LuCI page signals that it is dark, so this override fires at the wrong times.
luci-theme-bootstrap keys its dark palette off an attribute — :root[data-darkmode="true"] at cascade.css:123 — and only the auto variant ties that attribute to the OS. header.ut:12 pins darkpref to 'true'/'false' for bootstrap-dark/bootstrap-light, and the matchMedia block at header.ut:22-31 is then not emitted at all. luci-theme-material, -openwrt and -openwrt-2020 have no dark mode in any form.
Those are precisely the themes that reach the fallback. On material/openwrt/openwrt-2020 with the OS set to dark, this block swaps in #6ab0de on a permanently white page — the low-contrast case the comment above says the fallback exists to avoid — and bootstrap-light on a dark OS does the same. Conversely bootstrap-dark on a light OS keeps #3c8dbc.
Keying on the attribute covers every case, the auto variant included, since its script sets data-darkmode from that same media query:
| /* The theme's accent where the theme publishes one: --primary-color-medium | |
| * exists only in luci-theme-bootstrap. Every other in-tree theme sees the | |
| * fallback, which must clear 3:1 against a light page -- the arc and the | |
| * history line are the only graphical encoding of the data. */ | |
| :root { | |
| --librespeed-accent: var(--primary-color-medium, #3c8dbc); | |
| } | |
| @media (prefers-color-scheme: dark) { | |
| :root { | |
| --librespeed-accent: var(--primary-color-medium, #6ab0de); | |
| } | |
| } | |
| /* The theme's accent where the theme publishes one: --primary-color-medium | |
| * is defined by luci-theme-bootstrap (and luci-theme-footstrap on current | |
| * master); the other in-tree themes see the fallback, which must clear 3:1 | |
| * against the page -- the arc and the history line are the only graphical | |
| * encoding of the data. Dark mode is the data-darkmode attribute, not the | |
| * OS preference: bootstrap-dark/-light pin it regardless of | |
| * prefers-color-scheme, and material/openwrt/openwrt-2020 are light-only. */ | |
| :root { | |
| --librespeed-accent: var(--primary-color-medium, #3c8dbc); | |
| } | |
| :root[data-darkmode="true"] { | |
| --librespeed-accent: var(--primary-color-medium, #6ab0de); | |
| } |
Side note on the comment's premise: the branch is based on a master that predates luci-theme-footstrap, which also defines the variable — footstrap cascade.css:12 on this PR's base commit — and it uses data-darkmode as well.
Generated by Claude Code
| const histOff = noneAtAll && this.config && | ||
| this.config.history && this.config.history.enabled === false; |
There was a problem hiding this comment.
This requires enabled to be a real JavaScript false, but the other reader of the same config payload in this PR assumes something weaker: sched.enabled ? _('Yes') : _('No') at test.js:666`` is plain truthiness. The two disagree on the encoding, and at most one of them survives whatever the backend actually emits:
- backend emits a boolean — both are right;
- backend emits
0/1— test.js is right, this branch never fires and the empty state keeps offeringStart teston a router that will never record one, which is the whole point of the newcallConfig()inload(); - backend passes UCI's raw
'0'/'1'through — this branch never fires and test.js reports a disabled schedule asYes.
librespeed-common is not in the packages feed yet (openwrt/packages#30294), so I could not check which shape config returns. Which is it, and would it be worth making both sites agree — e.g. a small isOff(v) helper in librespeed.common that treats false, 0 and '0' alike — so a later change to the backend's serialisation cannot silently disarm one of them?
Generated by Claude Code
| /* Plain figures of one series: what is normal, how much it wobbles, and | ||
| * where it stands now. The story sentences are built from these. On | ||
| * daily aggregates the extremes come from the _min/_max columns the | ||
| * chart also draws -- min/max of the daily means would understate the | ||
| * spread and contradict the band directly above the sentence. */ | ||
| /* One token->label table for the schedule intervals: Settings builds | ||
| * its choices from it and the Test page looks the status label up, so | ||
| * the two cannot drift. The init script accepts more shapes than these | ||
| * six, so readers must fall back to the raw token. */ | ||
| INTERVALS: { | ||
| '15m': _('Every 15 minutes'), | ||
| '30m': _('Every 30 minutes'), | ||
| '1h': _('Hourly'), | ||
| '6h': _('Every 6 hours'), | ||
| '12h': _('Every 12 hours'), | ||
| '1d': _('Daily') | ||
| }, | ||
|
|
||
| /* Exported alongside the chart: every timestamp a page prints should | ||
| * come through here, or the date-only guard gets lost in a copy. */ | ||
| chartStampFull: chartStampFull, | ||
|
|
||
| seriesStats(entries, metric, resolution) { |
There was a problem hiding this comment.
nit: INTERVALS and chartStampFull were inserted between the seriesStats doc comment and seriesStats itself, so the comment now reads as documentation for the interval table — and the _min/_max paragraph it just gained describes code twenty lines further down.
| /* Plain figures of one series: what is normal, how much it wobbles, and | |
| * where it stands now. The story sentences are built from these. On | |
| * daily aggregates the extremes come from the _min/_max columns the | |
| * chart also draws -- min/max of the daily means would understate the | |
| * spread and contradict the band directly above the sentence. */ | |
| /* One token->label table for the schedule intervals: Settings builds | |
| * its choices from it and the Test page looks the status label up, so | |
| * the two cannot drift. The init script accepts more shapes than these | |
| * six, so readers must fall back to the raw token. */ | |
| INTERVALS: { | |
| '15m': _('Every 15 minutes'), | |
| '30m': _('Every 30 minutes'), | |
| '1h': _('Hourly'), | |
| '6h': _('Every 6 hours'), | |
| '12h': _('Every 12 hours'), | |
| '1d': _('Daily') | |
| }, | |
| /* Exported alongside the chart: every timestamp a page prints should | |
| * come through here, or the date-only guard gets lost in a copy. */ | |
| chartStampFull: chartStampFull, | |
| seriesStats(entries, metric, resolution) { | |
| /* One token->label table for the schedule intervals: Settings builds | |
| * its choices from it and the Test page looks the status label up, so | |
| * the two cannot drift. The init script accepts more shapes than these | |
| * six, so readers must fall back to the raw token. */ | |
| INTERVALS: { | |
| '15m': _('Every 15 minutes'), | |
| '30m': _('Every 30 minutes'), | |
| '1h': _('Hourly'), | |
| '6h': _('Every 6 hours'), | |
| '12h': _('Every 12 hours'), | |
| '1d': _('Daily') | |
| }, | |
| /* Exported alongside the chart: every timestamp a page prints should | |
| * come through here, or the date-only guard gets lost in a copy. */ | |
| chartStampFull: chartStampFull, | |
| /* Plain figures of one series: what is normal, how much it wobbles, and | |
| * where it stands now. The story sentences are built from these. On | |
| * daily aggregates the extremes come from the _min/_max columns the | |
| * chart also draws -- min/max of the daily means would understate the | |
| * spread and contradict the band directly above the sentence. */ | |
| seriesStats(entries, metric, resolution) { |
This shifts the six INTERVALS labels up by five lines, so the template's references into common.js need regenerating afterwards. (I re-validated the current template mechanically: all 139 entries' source references resolve to the right line, no msgids are missing, and the only ids without a JS literal are the five that come from the ACL and menu JSON — so it is correct as it stands today.)
Generated by Claude Code
| rows.push(E('p', { 'class': 'spinning', 'style': 'margin-top:.75em' }, | ||
| [ _('Connecting to the test server…') ])); | ||
| else { | ||
| const holder = E('div', { 'aria-hidden': 'true' }); |
There was a problem hiding this comment.
nit: the reason given for the live region above ("announcing the rebuilt gauge SVG would read the whole dial out several times a run") does not apply to aria-hidden — statusNode is not a live region, so nothing in it was ever announced automatically; only the new liveNode is. What aria-hidden changes is browse mode, and it removes the running rate along with the tick labels: the %.2f readout at line 178-179 is the only place the current Mbps figure appears while a test runs, and liveNode carries just the phase name (Ping/Download/Upload). The overall percent and the elapsed time below stay readable, the speed no longer is.
Hiding the ten tick labels is clearly right; if the intent is to keep the whole SVG out of the way, folding the rate into the announced string (or leaving the readout outside the aria-hidden holder) would keep the figure reachable. Was the readout meant to go with the dial?
Generated by Claude Code
dd7f981 to
84decc2
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (44c91d3). All five earlier follow-ups are addressed:
renderRecent()now passesdata && data.resolutionintoseriesStats(), so the'1d'branch reaches the Test page's stability sentence and itsbest/worstfigures agree with the min/max band the chart above them draws;- the dark-mode override is keyed on
:root[data-darkmode="true"]instead ofprefers-color-scheme, which is what bootstrap'sheader.utactually stamps — the light-only themes that reach the fallback no longer darken on an OS preference, andbootstrap-darkon a light OS now gets the dark accent; - both readers of
configare strict aboutenabledand say so in the source, so the encoding question is settled from the author's side; - the
seriesStatsdoc comment sits aboveseriesStatsagain rather than aboveINTERVALS; - the gauge's rate is repeated as visually-hidden text, so hiding the dial no longer takes the running Mbps figure with it.
Also new in this push: CSS_VERSION is gone and the stylesheet is linked plainly. That checks out — uhttpd emits ETag and Last-Modified on every static response and honours If-None-Match, and plain L.resource() is what luci-app-nlbwmon and luci-app-banip do.
CI is green on 84decc2. Two follow-ups posted inline, one on the template, one nit.
Generated by Claude Code
| msgid "Every 12 hours" | ||
| msgstr "" | ||
|
|
||
| #: applications/luci-app-librespeed/htdocs/luci-static/resources/librespeed/common.js:159 |
There was a problem hiding this comment.
The common.js references were shifted by hand rather than regenerated, so all eight of them are now off by exactly three lines. They moved by −5 — the distance the seriesStats doc comment travelled when it was relocated below chartStampFull — but this push also removed the four-line CSS_VERSION block above metricOf() and grew the cssLink() comment from two lines to three. Net −8, template says −5.
| msgid | template says | actual |
|---|---|---|
Every 15 minutes |
common.js:159 | common.js:156 |
Every 30 minutes |
common.js:160 | common.js:157 |
Hourly |
common.js:161 | common.js:158 |
Every 6 hours |
common.js:162 | common.js:159 |
Every 12 hours |
common.js:163 | common.js:160 |
Daily |
common.js:164 | common.js:161 |
avg |
common.js:290 | common.js:287 |
avg |
common.js:361 | common.js:358 |
common.js:161 and :164 are the amusing ones — they now point at _('Hourly')'s and _('Daily')'s neighbours, so a translator following the reference lands on the wrong interval.
Everything else in the template is fine: I re-validated it mechanically against the current sources — the other 131 entries' references all resolve to the right line, no msgids are missing, and there are no orphans beyond the five that come from the ACL and menu JSON. Please regenerate it from the current sources so these eight point at the right lines again.
Generated by Claude Code
| rows.push(E('span', { 'class': 'librespeed-visually-hidden' }, | ||
| [ '%.2f Mbps'.format(status.mbps) ])); |
There was a problem hiding this comment.
nit: .librespeed-visually-hidden has a second user now, but its definition still claims to have only one — /* Off-screen but spoken: the live region announcing the running phase. */ at librespeed.css:335.`` This span is deliberately not in a live region (statusNode is a plain sibling of `liveNode`, both appended at line 714-715), so a reader who takes the CSS comment at its word would conclude the rate is announced on every poll, which is the opposite of what the comment four lines up here says.
Something like "Off-screen but reachable: the live region, and the gauge figures repeated for browse mode" would cover both callers.
Generated by Claude Code
Web UI for librespeed-cli measurements: a Test page with live progress and a quick 24-hour chart, a History page with filters, a large chart and CSV/JSON export, and Settings for scheduling and history retention. Uses the librespeed-common rpcd backend. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
84decc2 to
8522919
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed the force-pushed commit against the previous head (84decc2). Both earlier follow-ups are addressed, and nothing else changed in this push:
- the template is regenerated — the six
common.jsINTERVALSreferences shifted from 159–164 to 156–161 and the two_('avg')references from 290/361 to 287/358, which is where those literals now sit. I re-validated the whole.potmechanically against the current sources: all 178 source references resolve to the right line, none of the 175_()/N_()literals in the JS is missing a msgid, and the only three entries without a JS literal areTest,SettingsandGrant access to LibreSpeed measurements, which come from the menu and ACL JSON; - the
.librespeed-visually-hiddencomment now covers both users of the class — therole="status"live region and the gauge rate repeated as plain text — rather than just the live region.
CI is green on 8522919, and the commit subject still matches the diff. Reviewed 1 new commit; no new issues found.
Generated by Claude Code
This LuCI (as a client) looks really lovely to me, and it's already quite complete. Adding server/backend configuration would make it feel a bit bloated/overdesigned to me. Of course, that's up to you. |
Pull request details
Description
Web UI for librespeed-cli measurements: a Test page with a live speedometer
and 24-hour chart, a History page with speed/latency series, filters and
CSV/JSON export, and Settings for scheduling and history retention.
Talks to the librespeed-common rpcd backend over ubus. Live progress uses the
client's
--json-streamwhen available, with an interface-counter fallback.Screenshot or video of changes (if applicable)
Maintainer (preferred)
@BKPepe
Tested on
OpenWrt version: OpenWrt SNAPSHOT (r0+35801-c0ffca4c63)
LuCI version: LuCI master branch (26.228.64482~c26820a)
Checklist