docs: give feature-local view fragments a documented home - #1422
Merged
Conversation
styling.md named only lib/utils/ui.ts, so a fragment used by one feature had no documented location and landed in modules/<feature>/utils/ beside the pure data helpers, with nothing in the path to say it returns markup. Worse, with no feature-local home the reflex is a display-only custom element, which stops being elided as soon as a shipping island renders it, turning free SSR markup into shipped JavaScript. Documents the second tier, modules/<feature>/utils/ui/<name>.ts, across the skill, the root AGENTS.md, the docs site, and both conventions files.
The primitive-routing table is asserted one-row-per-gallery-demo (test/repo-health/skill-gallery-intent-parity.test.mjs), and the row I added reused app/examples/todo, so a demo was named twice. There is no demo for this concept and adding one is out of scope here, so the row comes out. The scaffold's CONVENTIONS.md is a deliberate thin bridge under a 2200-character ceiling (test/scaffolds/scaffold-integration.test.js) with 17 characters of headroom, so it is the wrong surface to expand. It points at AGENTS.md and the skill, which now carry the tier.
The first draft said a fragment is the default for read-only markup on cost grounds, and that a component is only for markup with behaviour. Both are too strong. A display-only component rendered by a page is elided, so there the two cost the same and the choice is taste. What survives is narrower and worth stating precisely: the fragment's cost is unconditionally zero, while a component's depends on who renders it, so it starts shipping the day an island renders it or it grows a lifecycle hook, with no edit to the file itself. That makes the fragment the safer default around islands, not the always-cheaper one.
Two corrections from reading the diff back. The cost claim was wrong. I wrote that a fragment stays free where a component ships, but an island's imports are fetched either way: the browser really does download the fragment module (confirmed by watching the network panel for a helper drawn inside a live island). Under a page both are free; under an island both ship, and what the component adds is an element class, its registration, an upgrade per instance, and the display-only components it renders, which stop being elided with it. The section says that now, and its heading no longer asserts a preference the mechanism only partly supports. The gallery demonstrates the tier instead of a new card, since this is architecture rather than a feature. modules/stream already defined its row three times (a rowCls const, a string builder, and two inlined copies in render() that used neither), so it moves to modules/stream/utils/ui/row.ts in the two shapes the demo needs. SSR output is byte-identical.
The guidance argued from elision, which is the weak reason: under a page the two tie at zero and under an island both ship. The decisive reason was missing entirely. A component is a tag in the DOM, so the question is whether the markup can carry an extra wrapper. Four places it cannot, verified in Chromium: a table foster-parents an unknown element out of the table per the HTML spec; a select stops offering wrapped options; a ul/dl keeps the element but breaks ul > li, :nth-child, and list markers; a grid or flex container makes the wrapper the item. A fifth case is output that is not DOM at all, such as a webjs-stream payload, which no component can return. The skill, AGENTS.md, the docs site, and the gallery carve-out now lead with that and treat bytes as the tiebreaker. The gallery comment in particular explains itself by the two constraints the demo actually hits rather than by shipping cost.
Reading the diff back, the guidance contradicted its own opening. The section starts with a heading, a lede, and a back link, then says to prefer a component everywhere a wrapper is harmless, which would make those three components too. They are not units, they are one element with a class list nobody wanted to type twice, and nobody wants <page-lede> as a tag. So the choice now asks that first: a repeated class bundle is always a fragment, and the wrapper question only arises for a genuine unit of markup. The gallery stream demo had no test at any layer (the e2e that mentions /stream-demo is the blog's), and the carve-out changed its component. A co-located browser test now pins the seeded rows as direct list children and asserts a streamed row carries the same classes as a seeded one, which is the invariant the fragment exists to hold. Counterfactual: drifting the string shape reds that assertion.
website/AGENTS.md documented lib/ui/ for the site's composed page fragments, which read as a third spelling beside lib/utils/ui.ts and modules/<feature>/utils/ui/. It is not a third convention. It is the app-wide tier after the same barrel-to-directory move module-structure.md describes, and the website already made it (four fragments, one file each). The skill now says so and names all three sizes as one convention, website/AGENTS.md points back at it, and the layout block and docs page carry the growth path in a clause.
…g the examples Two problems reading the diff back a third time. streamRowHTML built raw HTML by interpolation, which the html tag's own escaping does not cover. The demo only passes its own literals, so nothing was exploitable, but this is a gallery helper people copy, and renderStream puts its output straight in the document. It now runs escapeAttr on the id and escapeText on the label, with a browser test asserting both holes are closed. Counterfactual: dropping either escape reds that test. The lib/ui growth rule I added last round said to split once app-wide fragments reach three or more, which made the gallery (three helpers) and the blog (nine) non-compliant with the skill describing them. The threshold was borrowed from the website's rule about subsystem FILES and does not transfer to helper count inside one file. It is a judgment call now, keyed on a fragment composing others or the file no longer being scannable, and it says outright that nine small helpers in one file are fine.
The topic table pointed at styling.md for Tailwind, tokens, headers, and layout, so an agent classifying a task as 'where does this markup helper go' was never routed there. The routing table earlier in the file cannot carry it, since each of its rows is anchored to a gallery demo and this is architecture rather than a primitive.
I wrote that a wrapped <option> stops being offered by the control. Measured in Chromium it is still in select.options and still selectable; what breaks is that it is no longer select > option, so selector-based CSS and DOM code miss it, and the content model is invalid. Second unverified mechanism claim in this PR, so the row now states what was measured. That correction exposed the table's framing. It presented four cases as ones where the fragment is the only shape that works, but only a table child is a hard failure (the parser moves the element out of the table). A list, select, grid, or flex child renders fine and is wrong in ways that surface later. They are now two tables, impossible versus lands-wrong, and the summary says so. Also folded the three-spellings paragraph into the one before it (it restated its neighbours) and fixed 'both tiers', stale since the third tier landed.
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
1 participant
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.
Closes #1421
references/styling.mdnamed exactly one home for a repeatedhtml-fragment helper,lib/utils/ui.ts. That is right for an app-wide chunk, but it left a feature-scoped one with no documented location, so it lands inmodules/<feature>/utils/beside the pure data helpers with nothing in the path to say it returns markup. This documents the feature tier,modules/<feature>/utils/ui/<name>.ts, states the split in one line (components/is custom elements,utils/ui/returns aTemplateResult, the rest ofutils/returns data), and nameslib/ui/as the same convention at subsystem size, since the website already made that move.It also answers the question that comes with the folder: when is a fragment the right shape and when is a display-only component? The honest answer is narrower than the first draft claimed. Bytes rarely decide it: under a page both are free (the page is inert, the component elides), and under a shipping island both ship, since an island's imports are fetched either way. What decides it is structure. A repeated class bundle (a heading, a lede) is always a fragment. For a real unit, a
<table>child and string output (a<webjs-stream>payload) rule an element out entirely, a list / select / grid / flex child renders but lands wrong, and everywhere else the component is the better default. Every mechanism claim in that section was measured in Chromium rather than reasoned, after two earlier drafts turned out to be wrong on inspection.What changed
.agents/skills/webjs/references/styling.md, the tier table, the repeats table split by scope, the three sizes of the app-wide tier, and a "Fragment or display-only component?" section (class bundle vs unit, then impossible vs lands-wrong, then cost as the tiebreaker)..agents/skills/webjs/SKILL.md, the layout block namesutils/ui/and labels each sibling by what it returns; the topic table routes "where a markup helper lives" tostyling.md.AGENTS.md,website/app/docs/styling/page.ts,website/app/docs/conventions/page.ts,examples/blog/CONVENTIONS.md,website/AGENTS.md, kept in agreement.modules/streamdefined its row three times (arowClsconst, a string builder, and two inlined copies inrender()that used neither). It now lives once ingallery/modules/stream/utils/ui/row.ts, in the two shapes the demo needs, and the string shape escapes its holes. Both file comments say why this is a fragment (a direct<ul> > <li>child, and a string payload) and end with "where a wrapper is harmless, prefer the component." SSR output is byte-identical.<stream-demo>(it had none at any layer): seeded rows are direct list children, a streamed row carries the same classes as a seeded one, replace / reset behave, and the string shape escapes both holes. Counterfactuals: drifting the string shape reds the parity assertion; dropping either escape reds the escaping one.Deliberately excluded
No new gallery card. The convention is architecture, so it is demonstrated by carving a fragment out of an existing demo rather than by a demo about fragments. That also keeps
SKILL.md's primitive-routing table untouched, which is right, since a repo-health test anchors each of its rows to a gallery demo.No scaffold
CONVENTIONS.mdedit. It is a thin bridge under a 2200-character ceiling with 17 to spare, and it points at the skill, which now carries the rule.Existing
lib/utils/ui.tsfiles left alone. The gallery's three and the blog's nine helpers are app-wide class bundles at the correct tier; the growth guidance says outright that nine small helpers in one file are fine.Test plan
test/repo-health/site-pages-well-formed.test.mjs,test/repo-health/skill-gallery-intent-parity.test.mjs,test/docs/doc-source-consistency.test.mjs, 12 passing.test/scaffolds/scaffold-integration.test.js, 13 passing.webjs checkclean,tscclean,/features/streamserves 200 with its seeded rows and byte-identical SSR; the new browser suite, 4 passing, both counterfactuals confirmed red.createRequestHandlerin prod mode,/,/docs/styling,/docs/conventions,/ui,/ui/buttonall 200 with no broken modulepreload hints.Worth knowing for anyone working in a fresh worktree:
/ui/button500s there untilnode website/scripts/copy-registry.mjsruns, becausewebsite/modules/ui/components/is a generated, gitignored mirror. Same class as the missingnode_modules, not a defect.