Stop stubbing DOM elements the page no longer has - #43
Merged
Conversation
test-ui-radio-loading.mjs built its fake document from a hand-maintained selector list, and four entries — #serial-transaction, #tx-hex, #rx-bytes, #rx-timeout — outlived the debug panel removed in ff5607a. The suite was asserting against a page shape production cannot have. Nothing failed, because FakeDocument auto-vivifies an unregistered selector, so the stubs were inert. That is exactly why it went unnoticed: the list can only ever drift in the direction of claiming too much, and nothing read it back against the UI's own element contract. Removes the four dead entries, hoists the list to a module-level STUBBED_SELECTORS, and adds a test asserting every stubbed selector is one web/js/ui/dom.js actually declares. A deleted id now fails here as well as in test-dom-selectors.mjs, which only checks the other direction (that every required element exists in index.html).
jasiek
force-pushed
the
chore/remove-dead-serial-txrx
branch
from
July 30, 2026 19:54
3a63b37 to
4885f07
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The changes are confined to test scaffolding and add a concrete assertion that prevents future silent DOM-stub drift without affecting production code paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR tightens the headless UI test harness (scripts/test-ui-radio-loading.mjs) so it can’t silently drift from the real UI’s DOM contract declared in web/js/ui/dom.js, preventing stale element stubs from masking removed UI elements.
Changes:
- Removes four obsolete DOM stubs that no longer exist in the production page.
- Hoists the stubbed selector list into a module-level
STUBBED_SELECTORSconstant. - Adds a guard test ensuring all stubbed selectors are declared by
dom.js(REQUIRED_ELEMENTS ∪ ELEMENT_COLLECTIONS).
File summaries
| File | Description |
|---|---|
| scripts/test-ui-radio-loading.mjs | Removes dead stubs, centralizes the stub list, and adds a drift-check test to keep stubs aligned with web/js/ui/dom.js. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
jasiek
added a commit
that referenced
this pull request
Aug 2, 2026
Covers everything merged since the 2026-07-30 section: home-screen install (#52), the RSGB ETCC repeater query (#47), variant/alias image metadata matching (#46), the test-gated deploy and retention hostname fix (#45, #42), the DOM stub cleanup (#43), and the boot-time empty-state notice. FINDINGS: silent-dom-guards-hid-dead-code's closing claim that the stale test stubs were still there is retired — #43 removed them and pinned the stub list to dom.js — and a new markup-defaults-are-the-boot-state entry records why index.html has to ship in the pre-runtime state. Screenshots regenerated at CHIRP b7ae1b6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
scripts/test-ui-radio-loading.mjsbuilds a fake document from a hand-maintained selector list. Four entries —#serial-transaction,#tx-hex,#rx-bytes,#rx-timeout— outlived the debug panel thatff5607aremoved fromindex.html. The suite was asserting against a page shape production cannot have.Nothing failed, because
FakeDocument.querySelectorauto-vivifies an unregistered selector, so the stale stubs were inert. That is precisely why it went unnoticed: the list can only drift in the direction of claiming too much, and nothing ever read it back against the UI's element contract.This is the failure mode
FINDINGS.md→ silent-dom-guards-hid-dead-code was written about, surviving in the tests after being fixed in the app.Change
STUBBED_SELECTORS.web/js/ui/dom.jsdeclares (REQUIRED_ELEMENTS∪ELEMENT_COLLECTIONS).The two directions are now both covered:
test-dom-selectors.mjschecks every required element exists inindex.html; this checks the test's own stubs stay inside that set.npm run test:channelsunaffected otherwise — 6/6 in this file.Dependencies
None added.
Follow-up, deliberately not in this PR
The RPC behind that deleted panel is still present and unreachable:
serialTxRx(web/js/runtime-rpc.js) is registered inRUNTIME_METHODSwith no caller anywhere, callinghandleSerialTxRx→webserial_txrx_hex(web/python/runtime_bridge.py). It is either dead code to delete or a UI to restore — a hex write/read against a loopback jumper is a genuinely useful way to exercise a serial adapter without a radio, so I did not want to delete it unasked.🤖 Generated with Claude Code