Skip to content

fix(ui): stop registry modules doing work at module scope - #1360

Merged
vivek7405 merged 5 commits into
mainfrom
fix/ui-registry-module-scope-work
Aug 9, 2026
Merged

fix(ui): stop registry modules doing work at module scope#1360
vivek7405 merged 5 commits into
mainfrom
fix/ui-registry-module-scope-work

Conversation

@vivek7405

Copy link
Copy Markdown
Collaborator

Closes #1320

Two @webjsdev/ui registry modules ran work at module scope, which the elision analyser correctly reads as client work, so every page or layout reaching them on a component-free path shipped whole instead of being elided. cn sits under essentially every kit helper and every scaffolded app runs webjsui init, so this silently cost page elision in every app using the kit.

What changed

lib/utils.ts: the GROUPS conflict table spread ...borderGroups() at module scope, a real top-level call. The table is now built on first use behind let _groups; function GROUPS() { return (_groups ??= [...]) }. The literal is untouched, only re-indented into the function body.

native-select.ts: it injected its <option> / <optgroup> colour rule from module scope behind a typeof document guard, which made the module both a top-level call and a browser-global reference. The rule moved into the theme block's @layer base, and installNativeSelectStyles is removed. That also fixes a progressive-enhancement regression on its own terms: the rule exists to stop dark-mode options going invisible, and injecting it from JavaScript left the bug visible with JS off and for one frame before hydration.

examples/blog/lib/utils/cn.ts carries the same memoisation. It is the second of the two hand-synced sources, and app/ui-demo/page.ts shipped whole for exactly this reason.

website/public/input.css gets the <option> rule by hand, beside the other kit rule the site already reproduces, because it writes its own stylesheet instead of running webjs ui init.

packages/server/src/component-elision.js: hasModuleScopeSideEffect is exported. One word, no behaviour change, so the purity guard calls the real predicate instead of re-implementing its depth-0 scan.

Consequences worth knowing

An app that already ran init does NOT receive the <option> rule. ensureTheme keys the whole block on its marker and returns early, and init --overwrite's flag reaches only writeLibUtils, so no command rewrites an existing theme block. Such an app keeps the browser-default <option> colours until it re-runs init or adds the rule by hand. Documented in packages/ui/AGENTS.md and the skill reference.

That asymmetry is why checkbox.ts and radio-group.ts keep their injections. Their CSS is the ONLY source of the checkmark and the radio dot, so moving it would silently break the checked state in every already-initialised app (a WCAG 1.4.1 failure, not a degraded read). They are pinned in the purity test's allowlist with the reason written down.

Test plan

  • Unit: packages/ui/test/utils-purity.test.js gains a pinned-set EQUALITY over every .ts under registry/lib and registry/components, calling the framework's own predicate. Equality, not subset, so a new offender fails immediately and an entry can only be removed deliberately. Plus two named assertions that utils.ts and native-select.ts are clean and that the injector is gone.
  • Unit: packages/ui/test/cn-helper.test.js gains a table of the order-dependent conflict cases, since the table is now built on first call.
  • Unit: packages/ui/test/base-colors.test.js asserts the rule ships in all seven base colours and sits INSIDE @layer base (sliced to the block's closing brace, since placement is what a plain substring match cannot see).
  • Browser: new packages/ui/test/components/browser/ui-native-select.test.js. Importing the module injects no <style>, and the rule paints options in light and dark, wrapped and bare. Bare is the case the original wrapper-scoped selector missed.
  • Integration: test/scaffolds/scaffold-ui-integration.test.js scaffolds an app, adds button card input native-select, writes a page using all four, and asserts on the elision REPORT (never on byte size, which moves with unrelated kit changes).
  • Cross-copy: test/ui/cn-copies-in-sync.test.mjs passes unchanged, which is the guard that the blog mirror was done right.
  • Output invariance: the registry cn before and after were loaded side by side and compared over the sync test's 110-token battery. 12,100 pairs and 242,000 triples, zero mismatches.

Counterfactuals and the dogfood results are posted on the PR.

Docs

  • packages/ui/AGENTS.md: the no-module-scope-work rule, the copy-on-add caveat including the theme-block gap, the native-select inventory row, and the a11y form-controls bullets.
  • .agents/skills/webjs/references/ui-kit.md: both facts an agent needs, under Idioms.
  • website/public/input.css carries a comment beside the rule saying why the site needs its own copy.
  • N/A: the docs site under website/app/docs/** documents framework surfaces, not kit internals, and no page mentions installNativeSelectStyles or the <option> rule. README.md, no headline capability changed. packages/cli/templates/, which has no references/ directory and carries themes/index.css verbatim through create.js.
  • No hand-written changelog: it is generated from the conventional-commit subject on a version bump.

Layers that do not apply

  • Bun parity: nothing here touches a runtime-divergent surface. The only packages/*/src edit adds the export keyword to a static source-scanning predicate, which matches none of the patterns require-bun-parity-with-runtime-src.sh gates on, and the registry .ts files are copied text rather than framework runtime.
  • E2E: there is no JavaScript-disabled harness in the repo and building one for this is scope creep. The progressive-enhancement property is discharged structurally instead: native-select.ts now contains no document token at all (asserted), so the rule cannot be JavaScript-dependent.
  • Smoke: examples/blog renders no <select>, so the only blog-visible change is the elision verdict, covered at the right layer.

Merge order

This must merge BEFORE #1338, which edits the same packages/ui/packages/registry/lib/utils.ts and examples/blog/lib/utils/cn.ts. The regions do not overlap (this one is the table opener, closer, and the single read site; #1338 is hintedGroup and variantPrefix), so they resolve cleanly in that order.

The GROUPS table spread `...borderGroups()` at module scope, which is a real
top-level call, so the elision analyser read lib/utils.ts as client-effecting.
Every page or layout reaching `cn` on a component-free path then shipped whole
instead of being elided, and `cn` is reached by essentially every kit helper,
so this cost page elision in every app that runs `webjsui init`.

Memoising the table behind a function keeps the same literal and the same
output (12,100 pairs and 242,000 triples over the sync test's 110-token
battery, zero mismatches) while leaving nothing to run at module load.

The blog copy carries the same change: it is the second of the two hand-synced
sources, and examples/blog/app/ui-demo/page.ts shipped whole for this reason.
native-select.ts injected its <option> / <optgroup> colour rule from module
scope, guarded by a `typeof document` check. That made the module both a
top-level call and a browser-global reference, so the elision analyser pinned
every page rendering a <select>, and it was a progressive-enhancement
regression on its own terms: the rule exists to stop dark-mode options going
invisible, and injecting it from JavaScript leaves the bug visible with
JavaScript off and for one frame before hydration.

The CSS is four static, selector-only lines, so it belongs in the stylesheet
the kit already manages. It goes in the theme block's `@layer base`, outside
the `:root` and `.dark` blocks `mergeThemeCss` rewrites, so all seven base
colours carry it with no per-colour edit, and being layered makes it strictly
more overridable than the unlayered injected <style> was.

`installNativeSelectStyles` is removed rather than left as a no-op. It had no
callers, and the kit is copy-on-add, so an existing app keeps its own copy and
a re-adder takes the new file whole.

An app that already ran `init` does NOT get the rule: `ensureTheme` returns
early on its marker, so no command rewrites an existing theme block. Its
<option> colours revert to the browser default until it re-runs `init` or adds
the rule by hand. That gap is why checkbox and radio-group keep their
injections, since theirs are the only source of the checkmark and the dot.

The marketing site gets the rule by hand in its own input.css, beside the other
kit rule it reproduces, because it never runs `webjs ui init`.

`hasModuleScopeSideEffect` is exported so the purity guard calls the real
predicate rather than re-implementing its depth-0 scan, which would drift the
moment the analyser changes.
@vivek7405 vivek7405 self-assigned this Aug 9, 2026
@vivek7405

Copy link
Copy Markdown
Collaborator Author

Design rationale: why the option CSS moved to the theme block, and what that costs an existing app

The obvious smaller fix for native-select.ts was to keep the injection and make it lazy, called from nativeSelectClass() instead of module scope. That does not work: nativeSelectClass() runs during SSR, where document is undefined, so the styles would never install at all. Once lazy is off the table, the choice is between keeping a module-scope injection (the defect) and moving the CSS somewhere static, and there is only one static home the kit already manages.

Putting it in @layer base rather than leaving it unlayered is a deliberate second call. It is where themes/index.css already puts element-level global defaults, and it is where shadcn puts the equivalent. The selector and its specificity are preserved exactly, two element selectors at 0,0,2, so the class-based override the old comment promised still works. The layer additionally means a Tailwind utility on an <option> now wins, which it did not before, because the injected <style> was unlayered and beat every layered rule regardless of specificity. That is a strict improvement in the direction the comment already wanted.

installNativeSelectStyles is removed outright rather than left as a deprecated no-op. It had no callers anywhere outside its own invocation, and the kit is copy-on-add, so an existing app holds its own copy of the file and nothing in it breaks. A shim would only ever be read by someone re-adding the component, and a re-adder takes the new file whole. npx @webjsdev/ui diff is the discovery channel for the drift either way.

What this genuinely costs: an app that already ran init does NOT get the rule. ensureTheme keys the whole block on its marker and returns early when it is present, and init --overwrite's flag reaches only writeLibUtils, so no command rewrites an existing theme block. Such an app's <option> colours revert to the browser default until it re-runs init after deleting the marker line, or adds the rule by hand. That is a worse read, not a broken control, which is exactly why checkbox.ts and radio-group.ts keep their injections in this PR: their CSS is the only source of the checkmark and the radio dot, so the same move would leave an already-initialised app announcing its checked state by colour alone. Moving those two needs a theme-block upgrade path in ensureTheme first, and that changes the contract of a command users run against their own edited stylesheet, so it is not something to fold in here.

On cn, I kept the name GROUPS on the memoised function rather than renaming to groups() or getGroups(). Three comments in the file refer to "the GROUPS table above" and there is exactly one read site, so keeping the name means one changed character at the call site and no comment churn. ??= was checked through the framework's own stripper before committing to it: the output carries it verbatim, it is ES2021 rather than TypeScript syntax, so erasable-syntax invariant 10 is untouched.

The skill reference promised that importing any Tier-1 helper never pins a
page, which the PR's own pinned-set test contradicts: checkbox, radio-group,
pagination and progress are Tier-1 and all six flagged modules still ship
their importing page. Name them instead, and say the analyser-precision four
cost the same elision as the two real injections.

The AGENTS.md line calling checkbox and radio-group the last JavaScript-injected
stylesheets was wrong too: dialog and alert-dialog inject one from a lifecycle
hook. They are Tier-2 and need JavaScript regardless, which is the distinction
the sentence was reaching for.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The mechanical change is right and I checked it rather than trusting it: both files come back clean from the framework's own predicate, the pinned set matches what the analyser actually reports, the memoised table gives byte-identical output over the sync test's whole token battery, and the theme rule survives all seven base-colour merges inside @layer base. Moving the CSS into the theme block instead of keeping a lazy injection is the right call, since nativeSelectClass() runs at SSR where there is no document, so lazy could never have installed anything.

Where it falls down is the docs, which is the part of this change an agent actually reads before writing a page. Two claims in there are just false, and the PR's own test proves one of them false. Comments inline.

Comment thread .agents/skills/webjs/references/ui-kit.md Outdated
Comment thread packages/ui/AGENTS.md Outdated
@vivek7405
vivek7405 marked this pull request as ready for review August 9, 2026 08:32
@vivek7405
vivek7405 merged commit 276ca9e into main Aug 9, 2026
10 checks passed
@vivek7405
vivek7405 deleted the fix/ui-registry-module-scope-work branch August 9, 2026 10:21
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.

dogfood: ui kit cn.ts and native-select.ts do module-scope work, pinning every page

1 participant