feat(sdk): variable usage scan + preview-values adapter seam#2048
Conversation
c9bc8fb to
d3ab9db
Compare
a4cd1b5 to
a2085a5
Compare
d3ab9db to
ed9f222
Compare
6acf033 to
39e9b70
Compare
ed9f222 to
52b7e1d
Compare
cd67511 to
5e26843
Compare
52b7e1d to
1935108
Compare
1935108 to
3c94b40
Compare
5e26843 to
47e6506
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
LGTM with nits. The scanVariableUsage scanner is well-designed — the honest-lower-bound approach via scanIncomplete is the right architectural choice, and the two-pass acorn walk (classify calls, then track aliases) covers the patterns agents actually write without over-promising on opaque access. The __hfVariables detection and CSS usage channel are both good defensive additions.
Nit (non-blocking): The cssUsed check (cssText.includes(var(--${id})) can match prefix-extended variable ids (e.g., variable x matching CSS var(--x-extended)). This errs on the safe side (false negative on "unused" — keeps the variable alive), but a trailing delimiter check would tighten it. Low priority since the only consequence is a variable not being badged as unused when it could be.
Nit (non-blocking): undeclaredReads uses declaredIds.includes(id) which is O(n*m). Fine for typical composition sizes (single-digit variables), but a Set<string> for declaredIds would be more robust if this pattern grows.
The setPreviewVariables adapter seam is clean — optional method, boolean return, tests cover both supported and unsupported paths. The content-keyed scan cache with full replacement on each call is a smart eviction strategy.
Cross-stack note: All three PRs (#2046, #2047, #2048) form a coherent API surface with consistent naming conventions. The @hyperframes/core/variables entry point accumulates exports cleanly. No cross-PR SSOT violations detected.
— Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Stamped after Miga's stack review. GitHub state checked: mergeable, no red checks visible; #2046 base conflict still needs resolution separately.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 47e6506a. Stack context: SDK foundation (#2046-#2048) — bottom of the 12-PR template-variables stack.
Very careful design. The honest-lower-bound invariant is the load-bearing decision — a wrong "unused" badge is a footgun that invites deleting variables the render depends on — and this PR gets it right in three separate places (opaque access, __hfVariables global reads, unparseable scripts). The classifier-driven scanner pattern is the right shape for acorn analysis (I've seen it done worse many times) and the two-pass alias resolution with "unrelated same-named identifier can only make the scan MORE conservative" (variableUsage.ts L149-152) is exactly the safety property to codify.
🟠 Hold
packages/sdk/src/session.ts:getVariableUsageL237-238 — the CSS-usage detection usescssText.includes("var(--" + id + ")")(via template literalvar(--${id}). That's a partial-match — an idfoowill matchvar(--foo-header),var(--foo_bar), and any custom prop whose name starts withfoo. The direction of the bias is safe (over-attribute "used" rather than under-attribute → matches the honest-lower-bound principle forunusedDeclarations), so this isn't a correctness blocker per se. But it does mean the report will mark a variable "used via CSS" when the composition author never actually wrotevar(--foo)— the panel would then silently swallow real orphan cases. Recommend anchoring:new RegExp(\var\(--${escapeRegex(id)}[\s,)]`)or checking for boundary chars (space, comma, close-paren) after the id. Same pattern the runtime'sinjectCompositionCssVariables` collision detector uses. Not urgent, but the more accurate the report, the more Studio can trust it.
🟡 Nits / questions
variableUsage.tsL164-166 — chained aliases (const v2 = vars; use(v2.x);) flipscanIncompleterather than following the chain. Right call for v1; but ifv2.xis the only form the alias appears in,xis invisible and won't ever be marked used. A composition author writingconst vars = getVariables(); const cfg = vars; use(cfg.title);would seetitlemissing fromusedIdsAND flipped incomplete → panel degrades gracefully. Documented in the header ("chained aliases are not followed"); no action needed, just confirming the behavior.session.tsL207 — inline scripts only; external<script src="…">skipped. HyperFrames compositions don't typically use external scripts, but if a template imports a shared helper viasrc=, itsgetVariables()reads are invisible.scanIncompleteisn't flipped for that case — the presence of asrcscript isn't a signal of hidden reads. If external scripts get real, worth flippingscanIncompletewhenscript[src]is present in the doc; not needed today.session.tsL213 —if (text.includes("__hfVariables")) scanIncomplete = true;— this catches literal string matches, including comments (// __hfVariables shape…). Erring on the safe side (false-positive incomplete over false-negative). Consistent with the rest of the design.PreviewAdapter.setPreviewVariables(adapters/types.tsL84) is typedRecord<string, unknown>, no Zod at the seam. The SDK exposesvalidateVariableValues()as the callable check — but nothing enforces that a caller actually calls it beforesetPreviewVariables. That's the studio consumer's contract, not the SDK's — noting for the #2050 review.session.variableusage.test.tscovers the cross-reference well. Missing case that would be worth pinning: a composition whose only variable usage isvar(--foo)in an inline<style>block — the "CSS-only variable stays out ofunusedDeclarations" branch (session.ts L241). Add a small assertion to lock the CSS-compat channel counting as usage; otherwise the current tests pass whether or not that branch fires.
🟢 What's good
- Content-keyed scan cache (
_variableUsageScanCacheL196) mirrors_gsapLabelCache's discipline — same rationale, same eviction semantics (per-call fresh Map, so removed scripts drop out). Verified no unbounded growth. - Cross-referenced report shape (
usedIds/unusedDeclarations/undeclaredReads/scanIncomplete) is the right primitive for a UI badge system — each field has a clear owner in the panel, andscanIncompleteis the escape hatch that lets the panel show "we might be wrong" without lying to the user. - Adapter delegation via optional
setPreviewVariables?+ boolean return handles the "no adapter" AND "adapter without support" cases cleanly — tests at L95-103 pin both. - CI: all required checks green.
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2048 at 47e6506a
🟢 LGTM on the scanner design; concur with Rames-D's 🟠 Hold on the CSS var(--…) partial-match bug.
Verified independently:
scanIncompleteis an honest lower bound. Scanner conservative by design — flag flips on computed keys (values[dynamic]), rest spreads, values-object escape, unparseable scripts. Downstream (#2050 unused badges) only render "unused" affordances when!scanIncomplete, so a false-positive unused badge is structurally prevented on the scanner-visibility dimension.- Direct global reads flip
scanIncomplete.window.__hfVariablesdirect reads atsession.ts:417correctly degrade confidence. - CSS
var(--id)usage counted.session.ts:435-442— a variable consumed only via CSS is not badged unused. Right invariant to preserve (removing such a variable would break the binding). - Content-keyed caching. Script scans keyed by content at
session.ts:400, 419mirrors_gsapLabelCacheprecedent. - Preview adapter seam.
PreviewAdapter.setPreviewVariables?optional;Composition.setPreviewVariables()returns boolean. Ephemeral — never persisted. - 104 test lines across scanner cases + opaque-access degradation + adapter delegation.
- CI green.
Concurring with Miga's LGTM.
Concurring with Rames-D's 🟠 Hold:
cssText.includes("var(--" + id + ")")is a partial-match — idfoomatchesvar(--foo-header),var(--foo_bar), any custom-prop starting withfoo. Rames-D nailed the direction (over-attribute "used" is safer than under-attribute — matches the honest-lower-bound principle on the unused side), so this doesn't miscategorize a scanner-visible variable as unused. But it also means the report will mark a variable "used via CSS" that the composition author never actually referenced — Studio's unused-badge silently swallows the real orphan case for any variable whose id is a prefix of a real CSS var. Concur on the anchored regex:new RegExp(\var\(--${escapeRegex(id)}[\s,)]`)(boundary char after the id) matches the runtime'sinjectCompositionCssVariables` collision-detector convention. Not urgent, but Studio's report accuracy is only worth as much as its trust boundary — orphan-detection is exactly the use case that requires the anchoring.
No unique blockers beyond Rames-D's coverage.
R1 by Via
3c94b40 to
d55c96d
Compare
47e6506 to
4f96eef
Compare
d55c96d to
b0a0727
Compare
4f96eef to
f6b03f7
Compare
|
Addressed R1 🟠 Hold (CSS |
f6b03f7 to
dc29e00
Compare
d3df94c to
53ad171
Compare
dc29e00 to
6d6b162
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 at 6d6b162.
Prior R1 🟠 (CSS-usage detection .includes("var(--" + id + ")") — id foo matches var(--foo-header), hides orphans) resolved at session.ts:255-262: new RegExp(var\(\s*--${escapeRegExp(id)}[\s,)]).test(cssText) — matches the R1 recommendation exactly (whitespace / comma / closing-paren boundary, escapeRegExp for values that may predate the id-shape check). Comment at line 258-260 correctly names the id-escaping rationale (values read from disk may predate can()'s enforcement).
No residuals from my side. CI green.
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM concurring with Rames-D — R2 at 6d6b162.
R1 🟠 hold (cssText.includes("var(--" + id + ")") partial-match; id foo matched var(--foo-header) and silently swallowed real orphan cases) resolved via the anchored regex from Rames-D's R1 ask, with a proper escapeRegExp helper.
Verified at head:
packages/sdk/src/session.ts:255-257—new RegExp(\var\(\s*--${escapeRegExp(id)}[\s,)]`).test(cssText)— anchored on the boundary character class[\s,)], plus\s*after--to toleratevar( --foo )` whitespace.session.ts:78-80— newescapeRegExp(s: string): string { return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); }— defends against id values pre-can()-gate (belt-and-suspenders with #2047's slug regex).packages/sdk/src/session.variableusage.test.ts:73-82— explicit "does not count var(--id) as usage of a prefix-extended custom property": idaccentcorrectly appears inunusedDeclarationsdespitevar(--accent-shadow)being present incssText.
Boundary class [\s,)] correctly handles var(--foo,fallback) (comma-separator) and var(--foo) (paren-terminator). No new issues introduced.
No residuals from my side.
R2 by Via
53ad171 to
7966539
Compare
6d6b162 to
2f5d730
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Reconciliation review (post-#2098 rebase): Clean. This PR is purely additive — new scanner + usage report + preview adapter seam — and touches none of the declaration CRUD paths that #2098 overlapped. getVariableUsage() correctly delegates to the canonical getVariableDeclarations(), not to any removed #2098 helpers. No stale artifacts, no duplicated decisions.
LGTM.
— Miga
7966539 to
de6b076
Compare
2f5d730 to
11d547a
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 LGTM at R3 — 2f5d730.
R2 hold preserved through the rebase-onto-#2098. R2 verdict stands.
Verified at head:
packages/sdk/src/session.ts:80—escapeRegExphelper preserved.:286—new RegExp(\var\(\s*--${escapeRegExp(id)}[\s,)]`).test(cssText)` — anchored regex preserved (line drifted from R2 :255-257 → :286).packages/sdk/src/session.variableusage.test.ts:73— prefix-collision test "does not count var(--id) as usage of a prefix-extended custom property" still locking the fix.
Anchored CSS-usage regex + escapeRegExp defense-in-depth both survive.
R3 by Via
de6b076 to
b9cc0ce
Compare
11d547a to
193eb5c
Compare
193eb5c to
91831f3
Compare

What
Third PR of the template-variables stack: usage analysis + the preview-values adapter seam.
scanVariableUsage(scriptText)— browser-safe acorn scan forgetVariables()reads (destructuring, member access, single-alias tracking). Anything opaque (computed keys, rest spreads, escaping values object, parse errors) flipsscanIncompleteso consumers treatusedIdsas a lower bound, never a factcomp.getVariableUsage()cross-references the scan of every inline script against the declared schema →{ usedIds, unusedDeclarations, undeclaredReads, scanIncomplete }PreviewAdapter.setPreviewVariables(values | null)+comp.setPreviewVariables()delegation — the platform seam for "preview with these values" (ephemeral, never persisted)window.__hfVariablesreads now flipscanIncomplete(they're invisible to the scanner),var(--id)CSS consumers count as usage (a variable consumed only via CSS must not be badged unused), and scans are content-keyed-cached (same rationale as_gsapLabelCache)Why
Compositions read variables via JS calls — there's no DOM attribute to scan — so script analysis is the only way to answer "which variables does this composition actually use". The honest-lower-bound design (scanIncomplete) is the load-bearing decision: a wrong "unused" badge invites users to delete variables their render depends on.
Test plan
variableUsage.test.ts(9 scanner cases incl. alias tracking, escape flagging, parse errors),session.variableusage.test.ts(cross-reference report, adapter delegation)🤖 Generated with Claude Code