feat(sdk): stage 7 step 2 — setSelection API#1442
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verified at HEAD 88f067ef.
Stage 7 step 2 — setSelection API (+96/-0). Thin state-setter plus selectionchange dispatch, well-tested. Verdict: approve-with-concerns.
What's good
- Defensive copy on both input (
[...ids]) and output (h([...this.currentSelection])) — listeners can't mutate internal state. Good. - Explicit tests that
setSelectiondoes NOT push to undo stack (line 269) and does NOT emit apatchevent (line 281). Exactly the determinism / replay-safety properties the stack needs for #1450's shadow-parity to work. selection()proxy snapshots ids at call time (line 261) — no stale-ref hazard.
Concerns
-
No equality check before firing
selectionchange(packages/sdk/src/session.ts:224-227).setSelection(["hf-title"])followed bysetSelection(["hf-title"])firesselectionchangetwice. Wired into Studio via #1443'suseSdkSelectionSync, this becomes a real perf risk: any React render that recreates thedomEditGroupSelectionsarray (even with identical contents) re-fires the effect → re-firessetSelection→ re-firesselectionchange→ potentially re-renders subscribers. Cheap fix: shallow-compare before assigning and skip the dispatch if unchanged. Worth doing now before #1450 starts measuring shadow-parity dispatch counts. -
No de-dup of input ids.
setSelection(["hf-title", "hf-title"])stores duplicates verbatim. Probably fine, but #1426'sCanResultdispatch-boundary checks iterate selection — duplicate ids = duplicate work. Worth aArray.from(new Set(ids))on the way in, or an explicit "callers are responsible for de-dup" note in the jsdoc. -
No validation that ids exist in the composition.
setSelection(["nonexistent-id"])succeeds silently. Tolerant API choice (matchesgetSelectionreturning whatever was set), but means a typo from an agent caller silently selects nothing. Not blocking — tolerant-by-default is consistent with the rest of the SDK surface.
Nits
- jsdoc on
setSelection(types.ts:256) says "Pass [] to clear" — true, butsetSelection(["only-hf-id"])also implicitly clears any prior selection. The "replace" semantic is in the first half of the sentence; consider tightening to "Replaces the current selection (use [] to clear)."
What I didn't verify
- Whether #1450's shadow-parity comparison includes
selectionchangeevent counts — if it does, the no-equality-check concern is a likely false-positive source for shadow-parity drift.
Review by Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
CI blocked by the root oxfmt failure in #1423. setSelection implementation is clean.
Strengths:
packages/sdk/src/session.tssetSelection— correctly fires a fresh copy of the array to each listener ([...this.currentSelection]), so a listener that mutates its argument doesn't corrupt the stored selection. The snapshot test verifies this.- Tests cover: empty-array clear, multi-id set, event payload copy, unsubscribe stops firing, undo-stack isolation, patch-event isolation. Comprehensive for a 5-line implementation.
Important:
packages/sdk/src/session.tssetSelectiondoes not validate that the ids exist in the document. Passing ids for elements that don't exist silently sets the selection. For a debug/dev experience, aconsole.warnorCanResult-style validation when an id resolves to null would help authors catch typos. Not a blocker — callers may intentionally set selection before the document is populated.
Nit:
packages/sdk/src/types.ts—setSelectionis documented "Replace the current selection; fires selectionchange." Missing: "Pass[]to clear." (already on the impl comment but not the type doc — copy it there for IDE hover).
Verdict: APPROVE (pending #1423 oxfmt fix)
Reasoning: Correct, minimal, and well-tested. Selection does not affect undo stack or patch events — correct isolation.
— magi
…rops onStart, onUpdate, onRepeat were missing from the assertion; only onComplete was checked. All four are listed in DROPPED_VAR_KEYS so all four belong here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- gsapParserAcorn: top-level variable targets now resolved via program-scope
null-key fallback in lookupBindingFromAncestors (const el = querySelector...)
- gsapParserAcorn: fromTo guard requires args.length >= 3, preventing undefined
args[2]/args[3] access when fewer args supplied
- gsapWriterAcorn: remove fuzzing fallback in removeAnimationFromScript that
silently deleted the wrong animation (from→to ID conversion)
- gsapWriterAcorn: valueToCode guards NaN → "0" to avoid broken tween props;
safeKey regex aligned to ASCII-only (matching gsapSerialize)
- mutate: handleSetGsapTween now includes stagger in extras (was in addGsapTween
but missing from setGsapTween)
- apply-patches: script case now mirrors stylesheet — op=remove calls
setGsapScript("") instead of silently ignoring the patch
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
88f067e to
5f335b3
Compare
db5032f to
757ec4c
Compare
a32b07b to
02bd36c
Compare
|
Thanks @james-russo-rames-d-jusso and @miguel-heygen. Fixed:
Not changed:
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
- setGsapScript: remove element when newScript="" (fixes undo/redo duplicate-script bug)
- parseDeclarations: track quotes so ; inside CSS values (data URIs) doesn't split
- handleRemoveGsapKeyframe: guard against duplicate-percentage ambiguity (return EMPTY)
- resolveKeyframe: return kfs so callers can check uniqueness
- handleSetClassStyle: emit op:"add" (not "replace") when no prior <style> element
- FsAdapter listVersions: Number(f.split("_")[0]) — was NaN due to underscore in key
- FsAdapter doWrite: split try/catch so appendVersion failure doesn't fire error handlers
- FileAdapter playground: add content:"" field to satisfy PersistVersionEntry contract
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… result.code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
…rride-set cleanup
… scoped ids Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
Expose the concrete adapter factories so consumers no longer reach into deep adapter paths: - createHeadlessAdapter — no-op PreviewAdapter for agents/CI/SSR (no browser) - createMemoryAdapter — in-memory PersistAdapter for tests/headless open - createFsAdapter (+ FsAdapterOptions) — node fs PersistAdapter for local dev Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds fully-qualified scoped ids for addressing elements inside inlined sub-compositions, so callers can target "hf-HOST/hf-LEAF" unambiguously even when bare hf-ids collide across sub-composition boundaries. Changes: - model.ts: resolveScoped() traverses id segments through nested subtrees; isNewHostBoundary() detects host boundaries (dcf ≠ parent dcf handles outerHTML innerRoot edge case) - types.ts: HyperFramesElement gains scopedId field - document.ts: buildElement carries scopePrefix, propagates childPrefix at host boundaries; buildRoots starts with "" - patches.ts: RFC 6902 escapeIdForPath / decodePathSegment for scoped ids containing "/"; all path builders and pathToKey/keyToPath updated - session.ts: getElement() matches by scopedId; find() returns scopedIds; orphan cleanup decodes RFC 6902 before key comparison, preserves removal markers, purges property sub-keys for both bare and scoped ids - mutate.ts: all element handlers use resolveScoped instead of findById; handleRemoveElement collects full subtree hf-ids before removal for complete GSAP animation cascade (Q3 fix); validateOp uses resolveScoped 20 new contract tests in session.subcomp.test.ts covering resolveScoped, scopedId propagation, dispatch to scoped targets, RFC 6902 patch encoding, override-set key format, orphan purge, and serialize stability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes the last headless-testable Stage 6 gap (F9 workstream C).
`find({ composition: "hf-host" })` returns all scopedIds whose prefix
matches the given host id — i.e. every element mounted inside that
sub-composition, at any depth. Combinable with other FindQuery fields
(tag, text, name, track). 3 new contract tests in session.subcomp.test.ts.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Browser-fetch-based PersistAdapter for Studio REST file API. Uses fetch-only (no node:fs) so it is safe to bundle in Vite. - packages/sdk/src/adapters/http.ts — HttpAdapter class + createHttpAdapter factory - packages/sdk/src/adapters/http.test.ts — 14 unit tests (fetch mocked via vi.stubGlobal) - packages/sdk/src/index.ts — re-export createHttpAdapter + HttpAdapterOptions - packages/sdk/package.json — ./adapters/http subpath export (dev + publishConfig) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add per-path promise queue so rapid successive writes to the same composition file cannot race at the server. Different paths still write concurrently. Two new tests (RED→GREEN verified). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lush-drains-two test - Add optional headers?: HeadersInit | (() => HeadersInit) to HttpAdapterOptions for cross-origin / CLI / auth injection (function form refreshes on each PUT) - Document listVersions/loadFrom as intentional no-ops (server versioning not exposed) - Add flush-drains-two-concurrent-writes test to mirror fs adapter T13 coverage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
Adds setSelection(ids: string[]) to Composition interface and CompositionImpl. Fires selectionchange; does not touch undo stack or patch stream. 11 contract tests: get/set/clear, event firing, copy semantics, no undo/patch side-effects. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Skip event dispatch when ids are identical (same length, same order) to prevent double-firing selectionchange from callers that call setSelection with the same list. Two new tests (RED→GREEN verified). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
Reviewer noted it is dead surface in this stack — no caller uses it. Add comment explaining it is wired up in stage 8 when the preview host pushes selection events up to the SDK session. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
02bd36c to
c13f332
Compare
0f6f62b to
ae58fa2
Compare
|
@james-russo-rames-d-jusso — all concerns addressed in post-review commits
|
ae58fa2 to
9e58fd1
Compare

What
Brief description of the change.
Why
Why is this change needed?
How
How was this implemented? Any notable design decisions?
Test plan
How was this tested?