feat(core): expose pretext text measurement on window.__hyperframes - #3302
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Read this one at 58c20ae. The change is right and the reasoning in the body holds up where I checked it. Commenting rather than approving for two reasons that aren't the merits: three required checks (Test, Render on windows-latest, Tests on windows-latest) still hadn't reported when I looked, and you asked for a review rather than a stamp. Say the word once Test is green and I'm happy to approve.
One finding worth acting on before merge, and it's about the guard rather than the code.
Nothing would catch this regressing again
The bug being fixed is "the docs name window.__hyperframes.pretext, the runtime doesn't attach it." The fix is one line in entry.ts. Delete that line again and the whole suite stays green.
pretext.test.ts asserts the shape of the pretext module export — that prepare, layout and the three width helpers are functions, and that clearCache/setLocale are absent. All five assertions pass whether or not entry.ts ever attaches the object to the window. The thing that actually broke is the attachment, and it's the one thing not asserted.
There is an obvious-looking home for the guard, and I want to flag that the obvious fix there does not work:
packages/core/scripts/test-hyperframe-runtime-contract.ts exists for exactly this class — a requiredSnippets list checked against buildHyperframesRuntimeScript(), already carrying "__hyperframes" and "fitTextFontSize". It's a required context. Adding "pretext" to that list would look like it closes the gap and would not, because fitTextFontSize.ts line 1 is import { prepare, layout } from "@chenglou/pretext"; — the substring is already in the built runtime source through a path that has nothing to do with your attachment. The snippet check would pass on a reverted entry.ts, so the guard would read as green while asserting nothing.
Two ways to get a guard that discriminates:
- Cheapest that works in that file: assert on the attachment rather than the identifier — something keyed to the assignment block (
__hyperframesandpretextco-occurring in the object literal), not a bareincludes("pretext"). - What I'd actually suggest: you already did the real verification by hand — rendering a composition that calls the documented API and getting
lines=2 height=216 naturalWidth=1785, self-consistent against the 1600 container. That's the assertion. Promoting it into the browser check inpackages/clithat your test file's own comment points at turns a one-off manual result into the thing that fails when the attachment goes away. It also coversprepareneeding a canvas, which is precisely why the unit test can't.
Not blocking — the code is correct today. But the PR body lists two more doc/runtime mismatches queued up behind this one, so the class is live rather than hypothetical, and this API just demonstrated it can silently not exist.
What I checked, and what held
- "Sub-compositions inherit it for free" — verified at source, and it's the claim I most wanted to be true.
compositionScoping.ts:583-592builds the scoped variant asObject.assign({}, __hfBaseHyperframes, { getVariables }), so anything on the base object is carried through by construction. The proxy at:487additionally routes thewindow.__hyperframesspelling inside a sub-comp to that same scoped variant, so both the bare script param and the documented form getpretext. Nothing to do here; noting it because a reader could reasonably assume the scoped object was an explicit literal that would need a matching edit. @chenglou/pretextis already a declared dependency ofpackages/core(^0.0.5), so no manifest change is missing. Worth knowing that a caret on a0.0.xversion resolves to that exact patch, so widening the exposed surface doesn't widen version drift.- The include/exclude split is principled, not just cautious.
preparedoes write to the shared measurement cache, so "exposes nothing stateful" wouldn't have been true — but a memoization write keyed by text and font can't change what any other call returns, whereasclearCacheandsetLocalechange results for everyone. That's the right line, and the test pinning the two omissions with"clearCache" in pretextis a good guard against a later "just spread the whole module" refactor. - The doc correction is the honest kind. Replacing "pure arithmetic, ~0.0002 ms per call" with the split between
prepare(canvas, browser-only) and everything downstream of a prepared string is a correction against the author's own earlier text, and it's the half that decides where you can call these. window.d.tsneeded no edit — it never declares__hyperframesat all; that global is typed structurally per call site (entry.ts,variableScope.ts), each covering only what it uses. Checked because a second stale declaration of the same global is the usual trap here, and there isn't one.
One open question I did not resolve
clearCache is withheld for good reasons, and the docs now advertise prepare as safe to run per frame. Those two together mean a composition can add cache entries at frame rate — a per-frame counter or typewriter effect produces a distinct prepared string every frame — with no exposed way to release them. Whether that matters depends on whether the upstream cache is bounded, which I did not verify; I only read the package's declared version, not its source. Flagging the shape rather than claiming a leak. If it's an unbounded map it's worth knowing before the docs encourage the pattern, and fitTextFontSize already populates it today so it wouldn't be new, just newly easy to hit.
Cross-PR note
This shares base 67edb01 with #3299, and both touch skills-manifest.json. They edit disjoint keys — #3299 rewrites the hyperframes-cli and hyperframes-registry hashes, this one rewrites hyperframes-core — so I don't expect a conflict or a stale-manifest check in either order. Mentioning it only because a shared generated artifact between two in-flight PRs is usually where that goes wrong, and here it doesn't.
Scope: I read pretext.ts, pretext.test.ts, the entry.ts and text/index.ts changes, the determinism-rules.md diff, compositionScoping.ts around the scoping shim, test-hyperframe-runtime-contract.ts in full, and window.d.ts. I did not build the bundle, so the +4,903 byte figure is taken as reported.
— Rames Jusso
|
Both points addressed in 6895d32. The guard. You were right that nothing failed on a reverted Instead the contract check now evaluates the built runtime in a jsdom window and reads Verified by deleting the attachment line rather than assuming: and green again once restored. It also covers Two notes on the implementation. I kept it in The cache question. Good catch to name the shape rather than claim a leak. I read the package source and it does not bite. The cache is keyed by Your read on Full core suite still green: 903 passed across 46 files. Will ping when the three Windows checks report. |
The agent-facing skill reference documents
`window.__hyperframes.pretext.prepare()` / `.layout()` for measuring text
without forcing a DOM reflow, but the runtime only ever exposed
`fitTextFontSize` and `getVariables`. Any composition written against that
documented API threw at runtime.
Expose it for real rather than deleting the docs: reflow-free measurement is
the right tool for sizing text per frame, and `fitTextFontSize` is already
built on it.
Also exposes `prepareWithSegments`, `measureLineStats` and
`measureNaturalWidth`. `layout()` returns only `{ lineCount, height }`, so the
documented "shrinkwrap containers" use case is not achievable without a width
function. `clearCache` and `setLocale` are deliberately withheld: both mutate
state shared across compositions, which would make a render depend on what ran
before it.
Corrects the reference doc's "pure arithmetic" claim. `prepare` measures fonts
through a canvas and throws outside a browser; only the steps after a prepared
string are arithmetic.
Runtime bundle grows 4,903 bytes (+1.30%), which ships inline in every
composition.
Review correctly pointed out that nothing here fails if the attachment is removed again. pretext.test.ts asserts the shape of the module export, which passes whether or not entry.ts ever attaches it, and a snippet check over the bundle would be decorative: fitTextFontSize.ts imports prepare/layout from @chenglou/pretext directly, so the name is already in the output through an unrelated path. Evaluate the built runtime in a jsdom window and read window.__hyperframes back instead. That is the assertion that actually breaks. Verified by deleting the attachment line: the check exits 1 with "window.__hyperframes.pretext is not attached", and passes again once restored. Covers fitTextFontSize and getVariables the same way, and asserts clearCache and setLocale stay off the surface so the determinism choice is enforced rather than just documented. requestAnimationFrame is stubbed rather than enabling jsdom's pretendToBeVisual, which starts a frame loop that keeps the process alive. Also answers the open question raised in review about the withheld clearCache. The upstream cache is keyed by (segment, font), with segments coming from Intl.Segmenter at word granularity, so it grows with the number of distinct words a composition renders, not with frames. A per-frame counter or typewriter reuses existing entries; only new words allocate. Recorded next to the omission.
6895d32 to
dd1f583
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at dd1f5839. This closes the loop on my earlier comment — the one thing I asked for is here, and it's the version that actually guards.
The new check is real, not decorative. The trap I flagged was that requiredSnippets looks like the right home but asserts nothing, because fitTextFontSize.ts already imports prepare/layout from @chenglou/pretext, so the substring is in the bundle through a path unrelated to the attachment. You left that list untouched and instead evaluate the built runtime in jsdom and read window.__hyperframes back. That inverts the dependency the right way: with the entry.ts attachment deleted, hyperframes.pretext is undefined and the assert at line 69 fires. The comment at 30-34 records why, which is worth keeping — it's the reason the next person won't "simplify" it back into a grep.
What I checked on the mechanism rather than taking on trust:
loadHyperframeRuntimeSource()returnsbuildHyperframesRuntimeScript(), so the eval sees a built bundle, not TypeScript that would throw onimport.test:hyperframe-runtime-cirunsbuild:hyperframes-runtimebeforetest:hyperframe-runtime-contract, so the bundle under assertion is current. The manifesttry/catchat 92-99 has an empty catch, but every load-bearing assert sits above it and can't be swallowed.jsdom(^29) and@types/jsdomare declared devDeps of the package, so the required check isn't relying on a hoisted transitive.runScripts: "outside-only"still permitsdom.window.eval, and stubbingrequestAnimationFrameinstead of enablingpretendToBeVisualavoids the frame loop holding the process open — with the stub never firing, which is correct since you're asserting attachment-time state, not animation.- The forbidden-key loop uses
inrather than atypeofprobe, so it also catches an explicitly-undefinedkey. That turns theclearCache/setLocaleomission from a documented intention into an enforced one, which is the part I'd have expected to rot first.
To be exact about one thing: I read the mechanism and it supports your reported mutation result, but I did not run the deletion myself. The mechanism is what I'm approving on.
On the cache question I left open — you answered it in the commit message: keyed by (segment, font) with segments from Intl.Segmenter at word granularity, so it grows with distinct words rather than frames, and a per-frame counter or typewriter reuses entries. That's the right shape of answer and it resolves the concern as I framed it. I did not read @chenglou/pretext's source to confirm the key, so I'm taking that from you rather than restating it as verified.
Gate state at this head: all 8 required contexts green, exact-matched against the branch ruleset rather than by substring (Build, Render on windows-latest, Semantic PR title, Test, Test: runtime contract, Tests on windows-latest, Typecheck, regression). Check-runs pulled paginated, 58 rows against total_count 58. behind_by: 0 against current main, so the base is current and the manifest change applies on top of the sibling that landed earlier. blocked was the review gate alone — no live changes-requested anywhere on the PR.
Scope: I read entry.ts, the full contract check, the package manifest and its script chain, and the diff's 7 files. I did not run the suite locally, did not execute the mutation, and did not read the upstream text package's source.
— Rames Jusso
What
Exposes the
pretexttext-measurement API onwindow.__hyperframes, so the API our agent-facing docs already describe actually exists.Adds
pretext.prepare,.layout,.prepareWithSegments,.measureLineStats,.measureNaturalWidth.Why
skills/hyperframes-core/references/determinism-rules.mdis required reading for any agent authoring a composition. Line 59 tells them to callwindow.__hyperframes.pretext.prepare(text, font)thenpretext.layout(prepared, maxWidth, lineHeight)for text measurement without a DOM reflow.That object did not exist. The runtime exposed exactly
fitTextFontSizeandgetVariables. Any composition following the documented recipe threw at runtime.Deleting the doc line was the smaller change, but reflow-free measurement is genuinely the right tool for sizing text per frame, and
fitTextFontSizeis already built on it. Making the docs true is the better fix.How
packages/core/src/text/pretext.tsassembles the exposed surface in one place, with the include/exclude rationale next to it.entry.tsattaches it alongside the existing helpers. Sub-compositions inherit it for free: the scoping shim builds its scoped variant withObject.assign({}, base, { getVariables }), so anything added to the base object is carried through.Two deliberate decisions:
Wider than the doc named.
layout()returns only{ lineCount, height }. The doc's own "shrinkwrap containers" use case needs a width, which is impossible with justprepare+layout.measureNaturalWidthandmeasureLineStatsmake that claim achievable;prepareWithSegmentsis their required input.clearCacheandsetLocalewithheld. Both mutate state shared across compositions. Exposing them would let one composition change how a later one measures, making a render depend on what ran before it.Doc correction. The reference called this "pure arithmetic, ~0.0002 ms per call". Not quite:
preparemeasures fonts through a canvas and throws outside a browser. Only the steps after a prepared string are arithmetic. Reworded, and documented the width helpers and the omissions.Trade-off
The runtime bundle grows 4,903 bytes (+1.30%), from 377,865 to 382,768. That ships inline in every composition. Measured by building the artifact with and without the change.
Test plan
packages/core/src/text/pretext.test.tsguards the shape of the published surface: the two documented names exist, the width helpers exist, and the two stateful functions are absent. Behaviour is deliberately not asserted there.prepareneeds a canvas, and mocking it (asfitTextFontSize.test.tsmust) would assert nothing real.Real behaviour was verified by rendering a composition that calls the documented API:
Self-consistent: natural width 1785 exceeds the 1600 container so it wraps to 2 lines, and 2 x 108 line-height is exactly the reported 216. The frame was inspected visually.
Also run:
packages/corefull suite from the package root: 903 passed, 46 filestsc --noEmitandtsc --noEmit -p tsconfig.runtime.json: cleanoxlint/oxfmt: cleanFollow-ups (not in this PR)
An audit of the wider attribute surface found several more doc/runtime mismatches, including
data-gpu-modedocumented as an HTML attribute when it is a config field, anddata-no-timelinebeing real, load-bearing, and absent from the table agents read. Those are separate changes.