fix(ui): give every js declaration a reader and gate it - #331
Merged
Merged
Conversation
`ui/js/app.js` carried two module-level declarations with no reader left: `DAY_LABELS` (its only read point became a `share.day.N` lookup in #86 while the constant stayed behind, value changed) and `nowTime()` (#94 removed its five call sites -- four `D.TRANSACTIONS.unshift` and one `D.RAISE_REQUESTS.unshift` -- and left the body). Rust reports this shape via `dead_code`; JS has no such lint, so nothing was looking. Remove both declarations, correct the two `ui/README.md` sentences that still described them (`nowTime()` was documented as the way new timestamps are written; `DAY_LABELS` was listed among the language-sensitive constants), and add `src/js_gate.rs`: every top-level declaration in `ui/js/*.js` must appear at least twice, on identifier boundaries, across `ui/index.html` + `ui/js/*.js`. A companion test derives the file roster from disk so a new `ui/js/*.js` cannot slip out from under the rule. The prose (`ui/README.md`) and the stylesheet are deliberately not readers -- measured: counting the README makes the gate blind to exactly these two offenders, because the README happens to mention both names (1 -> 2). Extraction masks comments, strings and regex literals. The regex leg is load-bearing: `esc()`'s `/[&<>"']/g` otherwise reads as the start of a string and swallows the following code, which is how `nowTime()` escaped the first pass at this gate. Scope: the gate is lexical. It proves a second occurrence exists, not that the occurrence is a reachable read point, and it does not model JS syntax (`let a = 1, b = 2` second names, destructuring names and `window.X = ...` exports are out of the roster by design). Tests: cargo test 442 passed / 0 failed (was 436); cargo fmt --check and cargo clippy --all-targets -- -D warnings both clean. A/B on the pre-fix tree (upstream/main's `ui/js/app.js`) fails exactly the new axis test, naming `app.js:982 DAY_LABELS` and `app.js:3127 nowTime` at 1 occurrence each.
Owner
Author
|
Self-review (maintainer is the author of this PR; GitHub does not allow approving Checked:
One thing I deliberately did not do: gate the Local verification: |
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.
Summary
ui/js/*.jshas no equivalent of Rust'sdead_codelint, and this tree had twodeclarations that had lost their last reader:
DAY_LABELS(ui/js/app.js:982)constd31641d(#32) introduced it;68f9f70(#86) replaced its only read point with ashare.day.Nkey lookup and left the constant behind (value changed, too)nowTime()(ui/js/app.js:3127)function9a3ff3e/cfd38acintroduced it;89963f3(#94, "zero mock data") deleted its five call sites — fourD.TRANSACTIONS.unshiftand oneD.RAISE_REQUESTS.unshift— and left the bodyBoth are drift rather than a trade-off: the read point moved in a commit that
moved a "who writes this value" rule, and the carrier did not move with it.
ui/README.mdstill documentednowTime()as the way new timestamps arewritten, and still listed
DAY_LABELSamong the language-sensitive constants.Related Issue
None — found by inspection, no issue was ever filed for it.
Changes
ui/js/app.jsui/README.mdsentences that described them, and record thenew invariant + its scope
js/app.jscache-bust token (20260929-1→20260930-1)src/js_gate.rs(registered insrc/main.rsas#[cfg(test)] mod js_gate;), no new dependencies, no production codeThe invariant. Every top-level declaration in
ui/js/*.js(module-body level,i.e. indentation ≤ 2) must occur at least twice, on identifier boundaries, across
ui/index.html+ui/js/*.js. A companion test derives the file roster from disk,so adding a
ui/js/*.jsfile without registering it turns the gate red instead ofsilently leaving it unguarded.
Why the README and the stylesheet are deliberately not readers. Measured: with
ui/README.mdcounted, the gate is blind to exactly the two offenders above — theREADME happens to mention both names, taking each count from 1 to 2 (A/B readings:
html+jsreports 2 violations,html+js+css+READMEreports 0). Mentioning a namein prose is not a read point.
ui/css/style.cssis excluded for the same reason inreverse: it declares classes and never consumes JS identifiers, so counting it would
only add a false-green channel.
Scope (lexical, stated in the module doc and in
ui/README.md). The gate provesa second occurrence exists, not that the occurrence is a reachable read point —
a same-named token in a comment, a string, or other dead code still satisfies it.
It does not model JS syntax:
let a = 1, b = 2second names, destructuring names andwindow.X = ...assignment-style exports are outside the roster by design (prefer amiss over a false red). No screenshots: this changes no rendered output.
Tests
cargo test— 442 passed / 0 failed (baseline 436; the 6 new tests are the gate's own)cargo fmt --check— cleancargo clippy --all-targets -- -D warnings— clean-
the_declaration_extractor_has_teeth— declarations inside comments, strings andtemplate literals do not count; a regex literal must not swallow the code after it
-
the_judge_detects_an_injected_dead_declaration— injected dead declaration is named;a cross-file reader and a markup reader both count
-
the_identifier_counter_respects_boundaries—fmtMis not matched byfmtMega-
the_roster_covers_every_js_file_on_diskA/B. Reverting
ui/js/app.jstoupstream/mainfails exactly the new axis test, andnames both offenders:
app.js:982 DAY_LABELS (1 occurrence) · app.js:3127 nowTime (1 occurrence).One implementation note worth flagging for review: the extractor masks regex literals, and
that leg is load-bearing.
esc()'s/[&<>"']/greads as the start of a string under a naivemasker and swallows the following code — which is how
nowTime()slipped past the first passat this gate (
baseyielded 130 declarations, the fixed tree 129; the difference should havebeen 2).
Checklist
fix/)except ~15 lines of removals and documentation)