Skip to content

luci-mod-network: switch: fix port status missing <br> on first render - #8937

Draft
jjm2473 wants to merge 1 commit into
openwrt:masterfrom
jjm2473:pr-master/switch-view-fix-port-br-missing
Draft

luci-mod-network: switch: fix port status missing <br> on first render#8937
jjm2473 wants to merge 1 commit into
openwrt:masterfrom
jjm2473:pr-master/switch-view-fix-port-br-missing

Conversation

@jjm2473

@jjm2473 jjm2473 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Pull request details

Description

There was no line break between icon and text during the initial render,
but the subsequent polling of port status added one, causing the LuCI interface to jitter and shift position.

Screenshot or video of changes (if applicable)

Maintainer (preferred)

@systemcrash @jow-


Tested on

OpenWrt version: iStoreOS 25.12.5 2026081215
LuCI version: LuCI istoreos-25.12 branch 26.195.42598~4a1ef8c
Web browser(s): Chrome (151.0.7922.76)


Checklist

@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Aug 12, 2026
There was no line break between icon and text during the initial render,
but the subsequent polling of port status added one,
causing the LuCI interface to jitter and shift position.

Signed-off-by: Liangbin Lian <jjm2473@gmail.com>
@jjm2473
jjm2473 force-pushed the pr-master/switch-view-fix-port-br-missing branch from c2adefd to a2900e3 Compare August 12, 2026 15:10
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 12, 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. The commit message accurately describes the change, and the fix does address the reported first-render/poll mismatch. Two notes inline: a hard merge-ordering dependency on #8936, and a question about fixing the destructive stripTags() at the source instead.


Generated by Claude Code

'data-switch': switch_name,
'data-port': portspec.num
}), portstate);
}), portstate).outerHTML;

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.

This cannot be merged before #8936, or the Switch page stops rendering at all.

Turning description into a string routes it through the dom.parse('<div>' + s + '</div>') branch of stripTags() (form.js:305-316). There the <br> sits inside the <small>, i.e. it is not a direct child of the parsed wrapper <div>, so x.replaceChild(document.createTextNode('\n'), br) throws NotFoundError. That call is reached on every row via data-description in renderFrame() (form.js:4430), so m.render() rejects and the whole page dies — the exact stack trace in #8936.

The dependency is ticked in the checklist, but given that merging out of order is a hard breakage rather than a cosmetic regression, it would help to spell it out in the description text too.


Generated by Claude Code

@@ -358,7 +358,7 @@ return view.extend({
o.description = render_port_status(E('small', {

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.

For the record, the root cause is that stripTags() mutates the node it is handed rather than a copy. With a DOM-node description, dom.elem(s) is true so x === <small>, the <br>s are direct children, and the <br>'\n' replacement (form.js:311-313) rips them out of the live node that renderHeaderRows() (form.js:3065) had already appended to the description <th>. The next update_port_status() poll calls dom.content() on that same node and restores them — hence the jitter.

Note that #8936 does not fix this half: br.parentElement is still the <small>, so the live node keeps getting stripped. Would making stripTags() non-destructive at the source be preferable, e.g. const x = dom.elem(s) ? s.cloneNode(true) : dom.parse(...)? That removes the need for the .outerHTML workaround here (this is the only in-tree view assigning a node to description, but the trap is easy to fall into again).


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would making stripTags() non-destructive at the source be preferable, e.g. const x = dom.elem(s) ? s.cloneNode(true) : dom.parse(...)? That removes the need for the .outerHTML workaround here (this is the only in-tree view assigning a node to description, but the trap is easy to fall into again).

Yes, that's better.

@jjm2473
jjm2473 marked this pull request as draft August 13, 2026 02:19
jjm2473 added a commit to jjm2473/luci that referenced this pull request Aug 13, 2026
`stripTags` modifies the source node, causing switch port status missing `<br>` on first render.

Clone source node as AI suggests: openwrt#8937 (comment)

Signed-off-by: Liangbin Lian <jjm2473@gmail.com>
@jjm2473

jjm2473 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Fix by commit "luci-base: form.js: fix stripTags modify source node" in #8936

this PR should be close if that commit merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants