Skip to content

luci-app-51sdwan: add web interface - #8913

Open
21hkcloud wants to merge 1 commit into
openwrt:masterfrom
21hkcloud:add-luci-app-51sdwan
Open

luci-app-51sdwan: add web interface#8913
21hkcloud wants to merge 1 commit into
openwrt:masterfrom
21hkcloud:add-luci-app-51sdwan

Conversation

@21hkcloud

Copy link
Copy Markdown

This adds a JavaScript LuCI view for the 51SDWAN OpenWrt edge agent.
It provides account enrollment, connection state, routing mode,
traffic statistics, connect/disconnect and logout actions with scoped
rpcd ACL permissions.

The router-side agent is proposed in:
openwrt/packages#30176

Runtime-tested together with the agent on OpenWrt 25.12.5 x86_64.

@openwrt openwrt Bot added add package Introduces a new package Makefile build script not following guidelines Pull request does not follow formatting guidelines labels Aug 6, 2026
@21hkcloud
21hkcloud force-pushed the add-luci-app-51sdwan branch 2 times, most recently from 645f9a7 to 827340f Compare August 6, 2026 16:55
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 6, 2026

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 new commit (827340f). The commit message matches the diff.

The two things most likely to block merge are the complete absence of translation support (all UI strings are hardcoded Chinese, no _(), no po/) and agent-supplied status fields being rendered through innerHTML; details inline.

One note on the cross-repo dependency: the 51sdwan package that backs the 51sdwan ubus object does not exist in the packages feed yet, so LUCI_DEPENDS:=+51sdwan is unsatisfiable until openwrt/packages#30176 lands. The PR description already calls this out, so this is just a reminder about merge ordering, not a change request.


Generated by Claude Code

}

function formatTime(epoch) {
if (!epoch) return '尚未握手';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every user-facing string in this view is a hardcoded Chinese literal that is never passed through _(), and the package ships no po/ directory. All 101 other applications/luci-app-* packages have a po/ tree, and no in-tree LuCI resource file contains CJK UI text — LuCI strings are authored in English and translated via gettext (luci.mk picks up po/ automatically). As written this page cannot be translated and is unreadable outside a Chinese locale.

The same change is needed for the literals at lines 57, 61, 62, 79, 95, 101, 109, 118, 121, 123, 128, 129, 132, 133, 135, 146, 149, 150, 155, 157, 162, 163, 165, 172, 176, 182, 186, 187, 194, 196, 197, 200, 201, 202, 203, 204 and 218, plus the B/KB/MB/GB unit suffixes in bytes() at lines 40-43.

Suggested change
if (!epoch) return '尚未握手';
if (!epoch) return _('No handshake yet');

Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. UI source strings are now English and wrapped with _(); the package includes a scanner-generated POT plus a complete zh_Hans translation catalog.

]), action
])
]),
data.error ? E('div', { 'class': 'alert-message warning' }, data.error) : '',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data.error comes straight from the 51sdwan ubus object and is handed to E() as a bare string child. dom.append() at luci.js:1394-1396 assigns a non-array, non-node child via node.innerHTML, so any markup in the agent's reply is parsed as HTML rather than shown as text. Wrapping the value in an array takes the createTextNode() branch instead.

Suggested change
data.error ? E('div', { 'class': 'alert-message warning' }, data.error) : '',
data.error ? E('div', { 'class': 'alert-message warning' }, [ data.error ]) : '',

Same pattern applies to the other agent-supplied values: line 66 (error.message, which carries result.error from the agent), line 187 (data.gateway / data.tunnel_ip), line 196 (data.account), line 197 (data.version), and lines 201-202 (data.gateway / data.tunnel_ip again).


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. Agent-supplied strings are rendered only through text nodes/textContent; no dynamic value is assigned through innerHTML.

},
"write": {
"ubus": {
"51sdwan": [ "login", "login_start", "connect", "disconnect", "set_mode", "logout" ]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

login is granted write access but the view never declares or calls it — the only login entry points used are login_start (declared at lines 14-19 of 51sdwan.js) and login_status. The PR description says the ACL is "scoped", so this extra grant looks like a leftover; dropping it keeps the ACL matched to what the frontend actually invokes.

Suggested change
"51sdwan": [ "login", "login_start", "connect", "disconnect", "set_mode", "logout" ]
"51sdwan": [ "login_start", "connect", "disconnect", "set_mode", "logout" ]

Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. The unused login permission was removed and the ACL now grants only the ubus methods invoked by the view.

render: function(data) {
var page = E('div', { 'class': 'cbi-map sdwan-page' }, [
E('style', {}, '\
.sdwan-page{max-width:1120px}.sdwan-hero{margin-bottom:18px;padding:24px 28px;border-radius:18px;background:linear-gradient(125deg,#062f43,#087f84);color:#fff;display:flex;align-items:center;gap:16px}.sdwan-logo{width:52px;height:52px;border-radius:14px;background:linear-gradient(135deg,#18d7c0,#3b9dff);display:grid;place-items:center;font-size:21px;font-weight:800}.sdwan-hero h1{margin:0 0 4px;color:#fff}.sdwan-hero p{margin:0;color:#b5dce1}.sdwan-eyebrow{font-size:11px;letter-spacing:2px;color:#13bda9}.sdwan-login-card{display:grid;grid-template-columns:1fr 1fr;gap:32px;padding:28px;border:1px solid #dce8eb;border-radius:16px;background:#fff}.sdwan-login-copy{padding:18px}.sdwan-login-copy h2{font-size:26px;margin:10px 0}.sdwan-login-copy p{color:#647d87;line-height:1.7}.sdwan-login-form{display:grid;gap:9px;padding:22px;border-radius:14px;background:#f5f9fa}.sdwan-login-form label{font-weight:600}.sdwan-input{box-sizing:border-box;width:100%;min-height:44px}.sdwan-primary{margin-top:8px;min-height:44px}.sdwan-login-form small{color:#758c95;text-align:center}.sdwan-status-card{padding:22px 26px;border-radius:18px;background:linear-gradient(125deg,#082f43,#0a6570);color:#fff;box-shadow:0 8px 24px rgba(4,45,64,.16)}.sdwan-status-card.is-connected{background:linear-gradient(125deg,#06364b,#078b83)}.sdwan-status-top{display:flex;align-items:center;gap:8px;color:#bcebe5;font-size:13px}.sdwan-state-dot{width:9px;height:9px;border-radius:50%;background:#ffbd50}.is-connected .sdwan-state-dot{background:#18d7c0;box-shadow:0 0 0 5px rgba(24,215,192,.15)}.sdwan-status-body{display:flex;align-items:center;justify-content:space-between;gap:20px;margin-top:18px}.sdwan-status-body h2{margin:0 0 6px;color:#fff;font-size:27px}.sdwan-status-body p{margin:0;color:#b9dadd}.sdwan-connect{min-width:150px;min-height:44px}.sdwan-grid{display:grid;grid-template-columns:1fr 1fr;gap:16px;margin-top:18px}.sdwan-panel{margin:0!important;border:1px solid #dfe9ec;border-radius:14px;box-shadow:none}.sdwan-panel h3{margin-top:0}.sdwan-stat-row{display:flex;justify-content:space-between;gap:16px;padding:10px 0;border-bottom:1px solid #e8eef0}.sdwan-stat-row:last-child{border:0}.sdwan-stat-row span{color:#6e858e}.sdwan-stat-row b{text-align:right}.sdwan-footer-actions{display:flex;justify-content:flex-end;margin-top:12px}.sdwan-logout{color:#70858d}@media(max-width:700px){.sdwan-login-card,.sdwan-grid{grid-template-columns:1fr}.sdwan-login-copy{padding:4px}.sdwan-status-body{align-items:stretch;flex-direction:column}.sdwan-connect{width:100%}}\

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The injected stylesheet hardcodes a light-only palette (background:#fff on .sdwan-login-card, background:#f5f9fa on .sdwan-login-form, border:1px solid #dce8eb, color:#647d87, color:#758c95, border-bottom:1px solid #e8eef0). LuCI themes support dark mode via :root[data-darkmode="true"] and expose CSS custom properties for exactly this purpose — see the token block at bootstrap-light/cascade.css:23-121 and its dark override at [line 123](https://github.com/openwrt/luci/blob/827340fe60f3707b7262107d3832569b4fa36faa/themes/luci-theme-bootstrap/htdocs/luci-static/bootstrap-light/cascade.css#L123).`` With these literals the login card and both panels render as white boxes with light-grey text when the user has dark mode on. Using var(--background-color-high), var(--text-color-low), var(--border-color-medium) and friends makes the page follow whichever theme is active.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. The view now uses LuCI theme variables for backgrounds, borders, and text, including dark-mode themes.

Comment on lines +223 to +228
poll.add(function() {
return callStatus().then(function(next) {
if (!!next.connected !== !!data.connected || !!next.configured !== !!data.configured || next.error !== data.error)
window.location.reload();
});
}, 5);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The poll callback only ever triggers a full window.location.reload(), and only when connected, configured or error flips. The tunnel figures the PR description advertises — latest_handshake, rx_bytes, tx_bytes, gateway, tunnel_ip — are rendered once from the load() result at lines 201-204 and then never touched, so on a stable connection the traffic counters and handshake time stay frozen at their page-load values for as long as the page is open.

Keeping references to the stat <b> nodes and rewriting their text from next inside this callback (the usual LuCI live-view pattern) would make the polling actually useful and avoid discarding the whole DOM on every state change.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. The polling callback keeps references to the status nodes and updates handshake, traffic, gateway, tunnel IP, account, version, and errors in place.

E('h3', {}, '连接设置'),
E('div', { 'class': 'cbi-value' }, [ E('label', { 'class': 'cbi-value-title' }, '加速模式'), E('div', { 'class': 'cbi-value-field' }, modeSelect) ]),
E('div', { 'class': 'cbi-value' }, [ E('label', { 'class': 'cbi-value-title' }, '会员账号'), E('div', { 'class': 'cbi-value-field' }, data.account || '-') ]),
E('div', { 'class': 'cbi-value' }, [ E('label', { 'class': 'cbi-value-title' }, '客户端版本'), E('div', { 'class': 'cbi-value-field' }, 'v' + (data.version || '0.1.0')) ])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the field is labelled "client version" and data.version is reported by the agent, but the fallback '0.1.0' is this LuCI package's PKG_VERSION (Makefile line 12). When the agent omits version the UI confidently displays v0.1.0 as though it were a real agent version, and it silently goes stale the first time either version is bumped independently. The neighbouring unknown fields (lines 196, 201, 202) fall back to '-'; doing the same here avoids inventing a version number.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. An absent agent version now displays -, rather than inventing a package version.

@@ -0,0 +1,13 @@
{
"admin/services/51sdwan": {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this file and root/usr/share/rpcd/acl.d/luci-app-51sdwan.json use 2-space indentation. In-tree, 93 of 99 menu.d/*.json and 98 of 102 acl.d/*.json files are tab-indented — worth matching so the new package lines up with the rest of the tree.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. Both JSON files now follow the repository tab-indentation convention.

"order": 35,
"action": {
"type": "view",
"path": "services/51sdwan"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this puts the view at htdocs/luci-static/resources/view/services/51sdwan.js, but no in-tree app ships a view/services/ directory. All 87 app-owned view subdirectories are named after the package itself (view/cloudflared/, view/banip/, view/ddns/, …); the few shared names in use (view/system/, view/network/, view/firewall/) belong to core modules. Moving the file to view/51sdwan/51sdwan.js and setting "path": "51sdwan/51sdwan" keeps the package inside its own namespace.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 91fbfd8. The view moved to view/51sdwan/51sdwan.js and the menu path now uses the package namespace.

@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Aug 7, 2026
@21hkcloud
21hkcloud force-pushed the add-luci-app-51sdwan branch from 33c4866 to 91fbfd8 Compare August 7, 2026 14:37
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 7, 2026
@21hkcloud

Copy link
Copy Markdown
Author

The LuCI review findings have been addressed in 91fbfd874d.

  • all UI source strings are English and wrapped in _();
  • a scanner-generated POT and complete zh_Hans catalog are included;
  • dynamic agent values are rendered as text nodes (no dynamic innerHTML path);
  • the ACL is limited to the methods used by the view;
  • colors use LuCI theme variables and work in dark mode;
  • polling updates live status/counters in place;
  • unknown versions display -;
  • JSON formatting follows the tree convention;
  • the view is namespaced under view/51sdwan/.

Validation performed:

  • official i18n scanner output is current and msgcmp reports the Chinese catalog complete;
  • JavaScript syntax and both JSON files validate;
  • the 24.10 SDK builds the application and translation packages;
  • the packages load against the updated ubus agent on OpenWrt 25.12.5;
  • all current formality checks pass.

The core dependency remains tracked by openwrt/packages#30176. A re-review would be appreciated when convenient.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 new commit (91fbfd8) since my last review at 827340f. All eight points from that review are addressed: the view moved to view/51sdwan/51sdwan.js, every user-facing string is wrapped in _() with a scanner-generated POT plus a zh_Hans catalog, agent-supplied values now go through safeText() into array children (text nodes, not innerHTML), the unused login ACL entry is gone, both JSON files are tab-indented, the poll callback updates the stat nodes in place, and the version fallback is -. Verified the POT covers all 50 source msgids and the zh_Hans catalog has no empty msgstrs; the ACL now matches exactly the seven ubus methods the view declares.

One thing the theme-variable rework introduced: the tokens it adopted are bootstrap-only, so the panels lose their background and borders on the other three in-tree themes. Details inline, along with three nits.

Commit checks

  • 91fbfd8 "luci-app-51sdwan: address interface review feedback" — the message enumerates five changes (namespace move, translation of every string, zh_Hans catalogs, text rendering + ACL removal, theme variables + poll refresh). The diff also rewrites LUCI_URL from https://www.51sdwan.com/ to https://www.51sdwan.com/en/openwrt and bumps PKG_VERSION from 0.1.0 to 0.2.0; neither hunk is described. Both go away if the commit is squashed into 827340f (see the Makefile comment).

Generated by Claude Code

this.lastStatus = data || {};
var page = E('div', { 'class': 'cbi-map sdwan-page' }, [
E('style', {}, [ '\
.sdwan-page{max-width:1120px;color-scheme:light dark}.sdwan-hero{margin-bottom:18px;padding:24px 28px;border-radius:18px;background:linear-gradient(125deg,#062f43,#087f84);color:#fff;display:flex;align-items:center;gap:16px}.sdwan-logo{width:52px;height:52px;border-radius:14px;background:linear-gradient(135deg,#18d7c0,#3b9dff);display:grid;place-items:center;font-size:21px;font-weight:800}.sdwan-hero h1{margin:0 0 4px;color:#fff}.sdwan-hero p{margin:0;color:#b5dce1}.sdwan-eyebrow{font-size:11px;letter-spacing:2px;color:#13bda9}.sdwan-login-card{display:grid;grid-template-columns:1fr 1fr;gap:32px;padding:28px;border:1px solid var(--border-color-medium);border-radius:16px;background:var(--background-color-high);color:var(--text-color-highest)}.sdwan-login-copy{padding:18px}.sdwan-login-copy h2{font-size:26px;margin:10px 0}.sdwan-login-copy p{color:var(--text-color-medium);line-height:1.7}.sdwan-login-form{display:grid;gap:9px;padding:22px;border-radius:14px;background:var(--background-color-medium)}.sdwan-login-form label{font-weight:600}.sdwan-input{box-sizing:border-box;width:100%;min-height:44px}.sdwan-primary{margin-top:8px;min-height:44px}.sdwan-login-form small{color:var(--text-color-medium);text-align:center}.sdwan-status-card{padding:22px 26px;border-radius:18px;background:linear-gradient(125deg,#082f43,#0a6570);color:#fff;box-shadow:0 8px 24px rgba(4,45,64,.16)}.sdwan-status-card.is-connected{background:linear-gradient(125deg,#06364b,#078b83)}.sdwan-status-top{display:flex;align-items:center;gap:8px;color:#bcebe5;font-size:13px}.sdwan-state-dot{width:9px;height:9px;border-radius:50%;background:#ffbd50}.is-connected .sdwan-state-dot{background:#18d7c0;box-shadow:0 0 0 5px rgba(24,215,192,.15)}.sdwan-status-body{display:flex;align-items:center;justify-content:space-between;gap:20px;margin-top:18px}.sdwan-status-body h2{margin:0 0 6px;color:#fff;font-size:27px}.sdwan-status-body p{margin:0;color:#b9dadd}.sdwan-connect{min-width:150px;min-height:44px}.sdwan-grid{display:grid;grid-template-columns:1fr 1fr;gap:16px;margin-top:18px}.sdwan-panel{margin:0!important;border:1px solid var(--border-color-medium);border-radius:14px;background:var(--background-color-high);box-shadow:none}.sdwan-panel h3{margin-top:0}.sdwan-stat-row{display:flex;justify-content:space-between;gap:16px;padding:10px 0;border-bottom:1px solid var(--border-color-low)}.sdwan-stat-row:last-child{border:0}.sdwan-stat-row span{color:var(--text-color-medium)}.sdwan-stat-row b{text-align:right}.sdwan-footer-actions{display:flex;justify-content:flex-end;margin-top:12px}.sdwan-logout{color:var(--text-color-medium)}@media(max-width:700px){.sdwan-login-card,.sdwan-grid{grid-template-columns:1fr}.sdwan-login-copy{padding:4px}.sdwan-status-body{align-items:stretch;flex-direction:column}.sdwan-connect{width:100%}}\

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The six theme tokens this stylesheet now consumes — --background-color-high, --background-color-medium, --text-color-highest, --text-color-medium, --border-color-medium, --border-color-low — are declared only by luci-theme-bootstrap (bootstrap/cascade.css:23-91).`` grep -rl -- "--background-color-high" themes/ matches only `luci-theme-bootstrap`; luci-theme-material, luci-theme-openwrt and luci-theme-openwrt-2020 declare no CSS custom properties at all.

An unresolved var() with no fallback makes the declaration invalid at computed-value time, so the whole declaration is dropped. On those three themes .sdwan-login-card, .sdwan-login-form and .sdwan-panel end up with no background and no border, and .sdwan-stat-row loses its separator — i.e. the panels lose their framing entirely rather than degrading to the previous hardcoded palette.

Both existing in-tree consumers of these tokens always supply a fallback — switch-vlan.css:62 uses `background-color: var(--background-color-high, #fff);` and [`dashboard/custom.css:19`](https://github.com/openwrt/luci/blob/91fbfd874dcf1b6203783a970a7dc7bb88e40b5b/modules/luci-mod-dashboard/htdocs/luci-static/resources/view/dashboard/css/custom.css#L19)`` uses border-top: 1px solid var(--border-color-medium, rgba(0, 0, 0, 0.1));. Doing the same here (values matching the bootstrap light theme) keeps non-bootstrap themes rendering:

  • var(--background-color-high)var(--background-color-high, #fff) (2 sites: .sdwan-login-card, .sdwan-panel)
  • var(--background-color-medium)var(--background-color-medium, #f9f9f9) (.sdwan-login-form)
  • var(--text-color-highest)var(--text-color-highest, #000) (.sdwan-login-card)
  • var(--text-color-medium)var(--text-color-medium, #808080) (4 sites: .sdwan-login-copy p, .sdwan-login-form small, .sdwan-stat-row span, .sdwan-logout)
  • var(--border-color-medium)var(--border-color-medium, #ddd) (2 sites: .sdwan-login-card, .sdwan-panel)
  • var(--border-color-low)var(--border-color-low, #eee) (.sdwan-stat-row)

Not suggesting a patch inline because the whole stylesheet is one 2.9 KB line and a hand-edited replacement is too easy to corrupt.


Generated by Claude Code

@21hkcloud 21hkcloud Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7290079. Every LuCI theme token now has the suggested light-theme fallback, so bootstrap still uses its variables while material, openwrt and openwrt-2020 retain panel backgrounds, borders and readable text.

Comment on lines +44 to +53
function bytes(value) {
var number = Number(value || 0);
if (number < 1024)
return _('%s B').format(number);
if (number < 1024 * 1024)
return _('%s KiB').format((number / 1024).toFixed(1));
if (number < 1024 * 1024 * 1024)
return _('%s MiB').format((number / 1024 / 1024).toFixed(1));
return _('%s GiB').format((number / 1024 / 1024 / 1024).toFixed(2));
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: LuCI already ships this formatter — the %m conversion in String.prototype.format at cbi.js:819 does exactly the same 1024-based scaling and appends the i infix, so '%1024mB'.format(n) yields 512 B, 1.50 KiB, 3.21 MiB, 1.05 GiB. 100 files in the tree use it.

The bigger cost of the private helper is that it routes the unit suffixes through gettext: %s B, %s KiB, %s MiB and %s GiB are now four msgids in po/templates/51sdwan.pot, and their zh_Hans msgstrs in po/zh_Hans/51sdwan.po are byte-identical to the msgids because SI/IEC unit symbols are not translated.

Suggested change
function bytes(value) {
var number = Number(value || 0);
if (number < 1024)
return _('%s B').format(number);
if (number < 1024 * 1024)
return _('%s KiB').format((number / 1024).toFixed(1));
if (number < 1024 * 1024 * 1024)
return _('%s MiB').format((number / 1024 / 1024).toFixed(1));
return _('%s GiB').format((number / 1024 / 1024 / 1024).toFixed(2));
}
function bytes(value) {
return '%1024mB'.format(Number(value || 0));
}

The four unit msgids should then disappear from both catalogs on the next regeneration.


Generated by Claude Code

@21hkcloud 21hkcloud Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7290079. The private byte-unit formatter was replaced with LuCI's built-in %1024mB formatter, and the four unit-only msgids were removed from both catalogs.

"Language: zh_Hans\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
"Plural-Forms: nplurals=2; plural=(n != 1);\n"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Chinese has no plural forms. 92 of the 94 applications/*/po/zh_Hans/*.po files in the tree carry nplurals=1; plural=0;; only two (e.g. luci-app-acme) carry the nplurals=2 form used here, and those look like Weblate artefacts rather than intent. With nplurals=2 a future ngettext/N_() string in this package would need a second, never-used msgstr and would pick the wrong index.

Suggested change
"Plural-Forms: nplurals=2; plural=(n != 1);\n"
"Plural-Forms: nplurals=1; plural=0;\n"

Also worth noting: unlike every other in-tree catalog this file carries no #: source references (grep -c '^#:' → 0). Regenerating it from po/templates/51sdwan.pot with the normal msgmerge step keeps it in the shape Weblate expects when it takes the package over.


Generated by Claude Code

@21hkcloud 21hkcloud Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7290079. zh_Hans now declares nplurals=1; plural=0;, and the catalog was regenerated with source references. The POT and PO contain the same 48 messages with no empty Chinese translations.

Comment thread applications/luci-app-51sdwan/Makefile Outdated
LUCI_DESCRIPTION:=Web interface for the 51SDWAN OpenWrt edge agent.
LUCI_DEPENDS:=+luci-base +51sdwan
LUCI_PKGARCH:=all
PKG_VERSION:=0.2.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: luci-app-51sdwan does not exist in the tree yet — it is created by 827340f, the first commit of this same unmerged series. Bumping PKG_VERSION to 0.2.0 (and rewriting LUCI_URL on line 8) inside 91fbfd8 records a version history for a package that has never shipped a 0.1.0.

Squashing 91fbfd8 into 827340f makes both hunks disappear and leaves a single "add package" commit, which is the usual shape for a new LuCI app. Note that 87 of the 101 applications/luci-app-* Makefiles set no PKG_VERSION at all and let luci.mk:165 derive it from git, so dropping the line entirely is also an option.


Generated by Claude Code

@21hkcloud 21hkcloud Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7290079. PKG_VERSION was dropped and the complete application history was squashed into one add-package commit rebased on current upstream master. The final commit message describes the complete application rather than an unpublished version transition.

@21hkcloud
21hkcloud force-pushed the add-luci-app-51sdwan branch from 91fbfd8 to 955a65b Compare August 7, 2026 17:02
@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Aug 7, 2026
@21hkcloud

21hkcloud commented Aug 7, 2026

Copy link
Copy Markdown
Author

Follow-up review items are addressed in 7290079ee4 and the PR is now a single add-package commit rebased on current master. Theme variables have cross-theme fallbacks, byte formatting uses LuCI %1024mB, the zh_Hans plural rule/source references are corrected, unit-only translations are removed, and PKG_VERSION is no longer set. Local JavaScript syntax, JSON parsing, catalog parity/completeness, source-reference and git diff --check validations pass.

@21hkcloud
21hkcloud force-pushed the add-luci-app-51sdwan branch from 955a65b to 7290079 Compare August 7, 2026 17:03
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 7, 2026

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the squashed head 7290079 against my last review at 91fbfd8. The series is now a single add-package commit rebased on master, and its message matches the diff.

All four items from the previous round are fixed, and I verified each: every one of the six theme tokens now carries the light-theme fallback (--background-color-high,#fff, --background-color-medium,#f9f9f9, --text-color-highest,#000, --text-color-medium,#808080, --border-color-medium,#ddd, --border-color-low,#eee), so material/openwrt/openwrt-2020 keep their panel framing; bytes() is now '%1024mB'.format(...) and the four unit-only msgids are gone from both catalogs; zh_Hans declares nplurals=1; plural=0; and carries #: references whose line numbers match the current 51sdwan.js; POT and PO hold the same 49 msgids in the same order with no empty Chinese translations; PKG_VERSION is gone. FormalityCheck is green on 7290079.

Nothing blocking left — three nits inline (POT line wrapping, a now-dead PKG_RELEASE, and the per-package LICENSE).

The 51sdwan package backing the ubus object is still absent from the packages feed (packages@0aba856), so LUCI_DEPENDS:=+51sdwan stays unsatisfiable until openwrt/packages#30176 lands — merge-ordering note only, already covered in the PR description.


Generated by Claude Code

msgstr ""

#: applications/luci-app-51sdwan/htdocs/luci-static/resources/view/51sdwan/51sdwan.js:172
msgid "Logging out stops acceleration but does not delete the device record from the member portal."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the three long msgids lost their 80-column wrapping in this revision. build/i18n-scan.pl passes --no-wrap to xgettext but then pipes everything through msguniq -s at i18n-scan.pl:236, which rewraps — so canonical scanner output is wrapped. That matches what 91fbfd8 had here, and no in-tree POT has an unwrapped long msgid (grep -hE '^msgid ' applications/*/po/templates/*.pot | awk 'length>80' matches nothing across all 101 apps). As it stands, regenerating this catalog with the in-tree scanner produces a diff.

Suggested change
msgid "Logging out stops acceleration but does not delete the device record from the member portal."
msgid ""
"Logging out stops acceleration but does not delete the device record from "
"the member portal."

Same applies to lines 178 and 194 of this file, and to lines 100, 182 and 198 of po/zh_Hans/51sdwan.po.


Generated by Claude Code

Comment thread applications/luci-app-51sdwan/Makefile Outdated
Comment on lines +11 to +12
LUCI_PKGARCH:=all
PKG_RELEASE:=1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: dropping PKG_VERSION left this line dead. luci.mk:165 only folds PKG_RELEASE into VERSION when PKG_VERSION is set — with it unset the package version comes from PKG_SRC_VERSION, and luci.mk:115 already defaults PKG_RELEASE to 1 regardless. 86 of the 87 applications/luci-app-* Makefiles that omit PKG_VERSION also omit PKG_RELEASE.

Suggested change
LUCI_PKGARCH:=all
PKG_RELEASE:=1
LUCI_PKGARCH:=all

Generated by Claude Code

Comment thread applications/luci-app-51sdwan/Makefile Outdated
LUCI_PKGARCH:=all
PKG_RELEASE:=1
PKG_LICENSE:=GPL-2.0-or-later
PKG_LICENSE_FILES:=LICENSE

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: none of the 101 applications/luci-app-* packages sets PKG_LICENSE_FILES, and none ships its own LICENSE file — the repo-wide LICENSE covers the tree, and PKG_LICENSE:=GPL-2.0-or-later on the line above already records the per-package license for the buildroot. Dropping this line together with applications/luci-app-51sdwan/LICENSE would put the new package in line with every other app; is the extra copy deliberate?


Generated by Claude Code

Add a JavaScript LuCI view for account enrollment and management of the
51SDWAN router agent.

Provide scoped rpcd ACL permissions, English source strings with a complete
Simplified Chinese catalog, safe text rendering, live status polling and
cross-theme styling.

Signed-off-by: Jia Liu <support@51sdwan.com>
@21hkcloud
21hkcloud force-pushed the add-luci-app-51sdwan branch from 7290079 to 2a2cbdd Compare August 8, 2026 05:44
@21hkcloud

Copy link
Copy Markdown
Author

Addressed the three remaining review nits in 2a2cbdd226:

  • restored canonical gettext wrapping for the long POT/PO msgids;
  • removed the redundant PKG_RELEASE assignment;
  • removed the per-package PKG_LICENSE_FILES entry and duplicate LICENSE file.

Local git diff --check, JavaScript syntax, JSON parsing and long-msgid wrapping checks pass.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 new commit; no new issues found.

Diffed 7290079..2a2cbdd — the amended head touches only four files and fixes exactly the three nits from my last review:

  • the three long msgids in po/templates/51sdwan.pot and po/zh_Hans/51sdwan.po are now wrapped in canonical gettext form (continuation lines under 78 columns, trailing space kept on the split), matching how the rest of the tree's catalogs wrap long strings;
  • PKG_RELEASE:=1 is gone — luci.mk already defaults it via PKG_RELEASE?=1;
  • PKG_LICENSE_FILES:=LICENSE and the per-package LICENSE file are removed; no other applications/luci-app-*/Makefile sets PKG_LICENSE_FILES, and PKG_LICENSE:=GPL-2.0-or-later plus the Makefile SPDX header still carry the licensing.

Re-verified after the change: POT and PO still hold the same 49 msgids in the same order, no empty msgstr, and the #: line references are untouched (the view file did not change in this delta). The commit message matches the squashed add-package diff. All three FormalityCheck runs are green on 2a2cbdd.

The 51sdwan package backing the ubus object is still absent from the packages feed (packages@112a7e4), so LUCI_DEPENDS:=+51sdwan remains unsatisfiable until openwrt/packages#30176 lands — merge-ordering note only, already covered in the PR description.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add package Introduces a new package Makefile build script

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants