feat: a public testing seam, an icon action row, and Stop that actually stops - #393
Merged
Merged
Conversation
…ly stops Three items carried over from the last analysis, including the two I had listed as "not done" and one I found but failed to report. `insideShadow` was written twice, identically, in two packages — I measured that during the analysis and then left it out of the report. It is now `ranui/testing`, exported rather than kept as scaffolding, because it is not internal: every consumer testing against these components hits the same wall, since closed shadow roots stop `getByRole`, `getByText` and `querySelector` alike and do it *silently*. Two suites here were written against locators that could never match before anyone noticed. It ships with `settlePainted` for the same reason — a fixed `waitForTimeout` passes whether the work finished or not, which is how two baselines came to record blank swatches. Typed against the shape of a page, not against Playwright: a component library should not make consumers install a test runner to import a type. The action row under every message was two text buttons reserving 20px plus an 8px gap, whether or not anyone was looking at them — and they must reserve it, or revealing them on hover shifts the transcript under the pointer. They are icons now, which is quieter and matches the reference design. Honest accounting: this reclaims 4px, not 28. The buttons are 24px because that is the minimum pointer target (WCAG 2.5.8) and my first pass made them 20 — the row cannot get much shorter without becoming hard to hit, and the real improvement here is noise, not height. Stop only aborted the stream. A turn whose tools were mid-flight carried on: the fetch the model started kept running and its answer arrived in a conversation that had moved on. There is now one AbortController per exchange, handed to every tool and passed on to `fetch`, plus a check after the tools settle — their results belong to an exchange the reader ended, and feeding them back starts a round nobody asked for. Nothing said how long a turn had been running, which matters most in the case where nothing else moves: tools finished, answer not started. A clock sits above the composer. It is deliberately not a live region — it ticks ten times a second, and `role="status"` would announce every one of them; the semantic signal is Stop becoming enabled. The reasoning row's duration is computed from the events rather than from a clock in the view, so a reloaded conversation reports the time the reader watched. Sub-call nesting is deliberately still absent. No tool here calls another, so building the tree would be an abstraction with no consumer and no way to test it — the repository's own rule. It belongs with the first tool that needs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying ran with
|
| Latest commit: |
d5fa16f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e670c3ec.ran-4ty.pages.dev |
| Branch Preview URL: | https://feat-testing-seam-compact-ac.ran-4ty.pages.dev |
…or them
Every component built its shadow tree with the builder and then asked
`querySelector` for the elements it had just created. The builder returns each
one; the query re-derives it through a class name, so the two sides are coupled
by a string. Rename a class in `index.less` and only one side changes: the query
returns `null`, the `!` waves it through, and the failure surfaces much later as
a property read on nothing, far from the rename that caused it.
Components now capture each element with `.ref()` while building it and read it
back through `shadowPart`, which throws at construction — naming the field — when
a ref was never filled. That is not hypothetical: it caught four elements this
sweep missed, at the moment of construction rather than on first use.
`ensureShadowElement` skips its factory when the subtree already exists, which is
why the queries looked necessary. Measured in a browser, that path never runs:
`attachShadow` clears a declarative shadow root's children, so an SSR-hydrated
component rebuilds, and every component builds in its constructor, so a reconnect
does not re-enter the factory either. Refs are always filled.
`verify:design` gains `built-then-queried` to keep it that way, at zero rather
than in the baseline. A container filled from data after the build is genuinely
different — no ref could have captured those children — so it opts out with
`// runtime children: <what puts them there>`; a marker with no reason does not
silence the rule.
Two defects fell out of the conversion. `r-select`'s `_search` was declared as
the custom `Input` element but is a native `<input>`, hidden by an unchecked
`as Input` on the old query; with the type corrected, `setAttribute('value', '')`
turns out never to have cleared the search box, because a dirty input no longer
tracks the attribute. `r-theme-switch` built its buttons inside the factory and
re-queried them on every theme change; they are now built before it, so the list
`_syncLabels` and `_reflect` drive cannot be empty.
…ents were missing Every other component has a baseline per project; the four added with the conversation work had only chromium, so a local full run wrote them and failed on first pass for anyone who had not run it before.
…tructor `ensureShadowElement(root, selector, factory)` searched for an existing tree before building one. That branch never runs, and the search is where the build-then-query pattern came from: the extraction it grew out of was written to work on both paths, so it took elements the only way that worked on both. The reuse was meant for server-rendered markup, and it cannot happen here. Rendering does emit a declarative shadow root, but every component attaches a closed one, so `host.shadowRoot` is null, `ensureShadowRoot` reaches `attachShadow`, and attaching to an element that already has a declarative shadow root removes that root's children. Measured, not assumed: an open declarative root keeps its children across `attachShadow` and a closed one is left empty. The server-rendered tree paints the first frame and is then replaced by an identical client-built one. So the helper is now `mountShadowTree(root, factory)` — build and append, no selector, no dead branch. What made the removed idempotence unnecessary is that every component builds in its constructor, which ran on luck rather than on a rule; `verify:design` gains `tree-built-outside-constructor` to make it a guarantee, since `mountShadowTree` appends unconditionally and a second call would mount a second copy and rebind the refs the component is already driving. The unit test for the reuse branch asserted the branch worked, having hand-appended the element it then found. Nothing asserted the branch was reachable, which is how it stayed. It now describes what the helper does: the factory runs even when the root already holds a matching element.
The constructor built the header and the body inside one factory, appended both to the shadow root itself, and returned the header. The mount helper then appended what it was given — and appending a node that is already a child moves it — so the header landed after the body. `:host` is `display: block`, so document order is paint order: every `r-section` ever rendered put its title under its content, with the header's 24px bottom margin separating it from nothing. It survived because `r-section` had no end-to-end test. The unit tests assert the heading and subtitle elements exist and read back their text, which stayed true the whole time; nothing asserted where they were. The header and body are now mounted as two trees, in order, the way `r-conversation` already mounts its scrollport and footer. The new spec asserts child order and painted position, and both fail when the old constructor is put back.
…in between `mountShadowTree(root, factory)` was `appendChild` with a closure around it. The closure existed because the version it replaced might not have called it; with nothing to decide, it was ceremony — and not harmless ceremony. It made building and mounting look like one step, which is how `r-section` came to append two elements inside its own factory and return one of them, so the helper's append moved that one to the end and the heading rendered below the body (fixed in the previous commit). Components now build with the builder and append the result. Call sites lose a nesting level, and the block-bodied factories lose their refs outright: with no closure, the intermediate elements are already in scope, so `r-scratch` and `r-select-option` use them directly instead of capturing and reading back. The design rule moves with it, from `mountShadowTree` to appends on the shadow root. That anchor is strictly better: it also sees mounts the old one could not, and it found four. Two are real and now say why — `r-input` creates its validation message only when there is one, and `ensurePlayerView` is the r-player constructor's own work extracted into a function with exactly one caller. Two were dead: `r-player` and `r-scratch` each re-appended a tree "if missing", and nothing in either component ever removes it. `r-player`'s sat after `this.innerHTML = ''`, which clears light DOM and never touched the shadow tree it was guarding. `r-scratch`'s unit test kept that branch alive by removing the container itself and asserting the component put it back — the same shape as the reuse test deleted last commit: it proved the branch worked, never that anything could reach it. It now asserts what has to hold, that the container mounted at construction stays mounted. `r-markdown` had hand-rolled the same ref-or-look-it-up fallback for the same disproved reason; it uses the ref.
…t per instance
`ensureShadowRoot` built `` `${cssText}\n${REDUCED_MOTION_CSS}` `` on every
construction. The stylesheet cache is keyed on that text, and a template literal
is a new string each time, so every instance made the cache hash the component's
entire CSS again — 6KB for `r-button` — before finding the entry that was
already there.
Keying a small cache on `cssText` instead looks up through the module-level
string the component imported, whose hash the engine keeps, and hands the
stylesheet cache the same joined string every time.
Measured in Chrome, median of nine runs of 2000 constructions each, in µs per
instance:
| component | before | after |
| ---------------- | ----------- | --------- |
| `r-button` | 12.15–12.9 | 8.8–9.8 |
| `r-state-dot` | 4.75–5.0 | 3.85–4.1 |
| `r-card` | 11.4–12.0 | 10.1–10.6 |
| `r-progress` | 18.9–20.1 | 17.5–18.6 |
Adopting two sheets instead — the component's and a shared reduced-motion one —
was tried first and recovered about half as much: the second
`adoptedStyleSheets` assignment costs roughly what the hashing did. It also
needed a second marker, because the `<style>` fallback keeps one marked style
per root and would have dropped the second.
Behaviour is unchanged: one sheet, overrides last, and a browser check under
`prefers-reduced-motion: reduce` still reports `transition-duration: 1e-05s`.
`base.less` is pulled into most components' stylesheets for its `@font-family` and `@box-shadow` variables, which LESS only emits where they are used. The class rule beside them is not a variable: it was copied verbatim into every stylesheet that imported the file, 702 times across the build, and nothing in the library, the demo or the docs ever applies it. Removing it takes 48KB out of `dist`.
Every component holds `_events = new EventManager()`, which allocated an AbortController in the field initializer, and `abort()` allocated a replacement on every disconnect. Nothing needed one that early: of the 27 components with an EventManager, none registers a listener in its constructor — they all wait for `connectedCallback` — and some managers never register at all unless an interaction happens, like the drag-scoped one in `r-progress`. Creating it on first read of `signal`, and dropping it in `abort()`, costs nothing on a component that is constructed and left connected. It shows on components that mount and unmount repeatedly, which is what a paged transcript does. Median of nine runs, 20 mount/unmount cycles over 200 elements, in µs per cycle: | component | before | after | | ------------ | ----------- | --------- | | `r-progress` | 11.25–11.68 | 7.88–8.57 | | `r-button` | 6.25–6.45 | 6.22–6.5 | | `r-card` | 1.35–1.38 | 1.28–1.42 | `r-progress` gains because it carries two managers and aborts both on disconnect, one of which had never been used. The new tests cover the two edges laziness introduces: aborting before anything was registered, and registering again afterwards. The second fails silently under a plausible mistake — leaving the aborted controller in place makes every later listener dead on arrival with nothing thrown — so both were checked against that implementation and against a `signal` getter that hands out a fresh controller.
`prettier --check .` covers the whole repository; the two files it flagged were written against the ranui package's own gates, which do not include it. A quote style in `verify-design-rules.ts` and one call that fits on a line in `section.spec.ts`. No behaviour change.
Thirteen warnings, none of which failed a build, which is how they accumulated. Six are residue from the shadow-tree sweep: `math` and `mermaid` kept refs for elements whose queries were restored because their children arrive at runtime, and `scratch`, `select` and `select/option` kept imports for helpers they no longer call. `select` also still imported the `Input` type, unused since `_search` was corrected to the native `HTMLInputElement` it actually is. `verify-design-rules` had a `COMPONENTS_DIR` no rule reads. Three are in the im client. `compactedFrom` is not a dropped wire: `compactIfNeeded` computes the same value inline because it needs the whole compaction, not just its index, so the extracted helper was left with no caller. `KEEP_RECENT` and `TokenUsage` are used where they belong, in `budget.ts` and `sessions.ts`. Four are the private-address checks in the fetch tool's SSRF guard, where `startsWith` says plainly what a caret regex said obliquely. `172.16`–`172.31` stays a regex because it is a range rather than a prefix, and now says so. Splitting `/^f[cd]/` into its two prefixes left one of them untested, so the unique-local case gained `fc00::1` beside the `fd00::1` already there; dropping either branch now fails that test.
…efreshed
Every spec runs under three projects, so each screenshot has three baselines.
Three separate changes refreshed `chromium` and left the other two holding a
picture of the old component — one of them says so in its own commit message
("refreshes the two player visual-regression baselines"), having touched only the
chromium pair. Nothing reported it: CI ignores snapshots, and locally the eleven
failures read as background noise, to the point that a genuinely flaky
`progress variants` had to be told apart from them by hand.
Two of the three were not drift. The colorpicker baselines held **blank swatches**
on pickers that declare colours — the bug already fixed for chromium by waiting on
the swatch instead of a timeout, but never re-recorded elsewhere. The tab baseline
held a strip with **no active-tab indicator**, and that one was still live: the
spec waited for the host to be visible, which says nothing about a line whose width
`setTabLine` only writes after measuring the headers. Sampled twelve times, the
indicator was unplaced at that moment twelve times out of twelve — chromium
recorded a line only by scheduling luck. `tab.spec.ts` now waits for the width, and
`tab-second-active` drops its `waitForTimeout(100)` for the same condition; eight of
the nine tab screenshots still matched afterwards, so the wait added a guarantee
without moving anything.
The player difference is real and intended: the play overlay lost its circular
backdrop, and chromium already records the current look.
Each re-recorded PNG was opened and checked against what the component should look
like, rather than accepted because the run went green.
Both were found by re-reading what this branch changed rather than by a failure.
`r-select` clears its search box by property now, because the field is a native
`<input>` whose value goes dirty the moment a user types and stops following the
attribute — the old `setAttribute('value', '')` left the text there for the next
open. Putting the attribute write back fails the new test.
`r-player` lost the `if (!contains) append` that `updatePlayer` ran after
`this.innerHTML = ''`, which clears light DOM and never reached the shadow tree it
appeared to guard. The tests around it read `_video`, which answers the same
whether or not the tree is still mounted, so nothing held the property the guard
was there for. Detaching the tree now fails three of them.
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.
The three items I listed as outstanding — including one I found during the analysis and then left out of the report.
1 ·
insideShadowwas written twice, identicallyI measured this and did not report it. It is now
ranui/testing, and exported rather than kept as scaffolding, because it is not internal to this repo: closed shadow roots stopgetByRole,getByTextandquerySelectoralike, and do it silently — two suites here were written against locators that could never match before anyone noticed. Every consumer testing against these components hits the same wall.It ships
settlePaintedwith it for the same reason: a fixedwaitForTimeoutpasses whether the work finished or not, which is exactly how two baselines came to record blank swatches on a picker that declares a colour.Typed against the shape of a page rather than against Playwright — a component library that made consumers install a test runner to import a type would be charging them for our convenience.
2 · The action row is icons now — and it reclaims 4px, not 28
Honest accounting, because I proposed this as "28px of empty space under every message".
Two text buttons reserved 20px plus an 8px gap. They have to reserve something: revealing them on hover otherwise shifts the whole transcript under the pointer. As icons they are 24×24 — and 24 is the floor, not a preference: it is the minimum pointer target size (WCAG 2.5.8), and my first pass made them 20px, which would have been an accessibility regression traded for four more pixels.
So the row goes 28px → 24px. What actually improved is the noise — an icon row instead of two text labels under every message — not the height. The height was mostly not reclaimable.
3 · Stop now stops the whole exchange
It only aborted the stream. A turn whose tools were mid-flight carried straight on: the fetch the model started kept running, and its answer arrived in a conversation that had already moved on.
One
AbortControllerper exchange, handed to every tool and passed through tofetch, plus a check after the tools settle — their results belong to an exchange the reader ended, and feeding them back starts a round nobody asked for.And a clock above the composer. Nothing said how long a turn had been running, which matters most in the case where nothing else moves: tools finished, answer not started. It is deliberately not a live region — it ticks ten times a second and
role="status"would announce every one of them; the semantic signal is Stop becoming enabled.The reasoning row's duration ("思考过程 · 4.2s") is computed from the events, not from a clock in the view, so a reloaded conversation reports the time the reader actually watched rather than the time since the page opened.
Deliberately not done
Sub-call nesting. No tool here calls another, so the tree would be an abstraction with no consumer and no way to test it — the repository's own rule. It belongs with the first tool that needs one.
Verification
tsc, 2,775 unit tests, 99 ranui e2e + 17 im e2e,verify:design(7 rules),verify:docs,verify-md-links, prettier — all green.Verified in a browser: the clock runs (0.3s → 1.9s sampled) and clears when the turn ends, and the icon row carries
pencil/refreshat 24×24 with the accessible names intact.