Skip to content

feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground#1423

Merged
vanceingalls merged 3 commits into
graphite-base/1423from
06-13-feat_sdk_playground_fs_adapter_settiming_gsap_sync
Jun 15, 2026
Merged

feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground#1423
vanceingalls merged 3 commits into
graphite-base/1423from
06-13-feat_sdk_playground_fs_adapter_settiming_gsap_sync

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Stacked on the acorn-parser/T6e branch.

What

  • fs PersistAdapter implemented (was a stub) — packages/sdk/src/adapters/fs.ts. node:fs/promises read/write, timestamped version history under .hf-versions/<path>/, prune to maxVersions (20), listVersions/loadFrom. Emits persist:error in the canonical { error: { message, cause } } shape.
  • setTiming GSAP-script syncpackages/sdk/src/engine/mutate.ts. setTiming on a GSAP composition stamped data-start/data-end on the DOM node, but the runtime re-stamps those attributes from GSAP positions on the next init — silently overwriting the edit. handleSetTiming now also rewrites the GSAP script (parseGsapScriptAcornForWrite + updateAnimationInScript), flushing both models in one patch pair so they stay in sync. DAW-style timeline trim depends on this.
  • @hyperframes/sdk-playground (new, private) — interactive browser harness over the full op surface: setStyle/setText/setAttribute/removeElement/setVariableValue/find/selection() proxy/all 9 GSAP+label ops/setClassStyle. Live preview iframe with click-select + drag-to-reposition, DAW-style timeline trim via setTiming, file-backed persistence with version history (Vite dev-server plugin + fetch adapter), undo/redo/can/getOverrides/flush, raw-HTML editor modal. README documents built vs planned.

Test

  • bun run --cwd packages/sdk build + test — 118 pass
  • bun run --cwd packages/sdk-playground build — clean
  • oxlint + oxfmt clean; fallow audit passes (0 introduced findings)

🤖 Generated with Claude Code

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at HEAD 1604228.

Verdict: approve-with-concerns. Solid foundation — fs adapter rounds out a real implementation, setTiming → GSAP-script sync closes a real consistency bug, playground gives the stack a usable harness. A few load-bearing issues to address before downstream PRs lean on this; one is a contract-shape bug worth fixing here.


Concerns (load-bearing as foundation):

  • packages/sdk/src/engine/mutate.ts:289-296 & 343-355 — comment misstates the runtime contract; the actual reason for the sync is consistency, not overwrite-prevention.

    • Comment claims: "the runtime re-stamps those attributes FROM GSAP positions on the next init — silently overwriting the edit." Both occurrences.
    • The runtime stamping path at packages/core/src/runtime/init.ts:1001 and :1020 guards with if (target.hasAttribute("data-start")) continue; — it does not overwrite already-stamped attrs. And the runtime stamps data-start/data-duration, while handleSetTiming writes data-start/data-end — different attr pairs, so there's no overlap to overwrite anyway.
    • The real motivation is the symmetric concern: if SDK setTiming only edits attrs and leaves the GSAP position/duration in the script untouched, the script is the source of truth at play time (timeline rebuilds from script), so trim edits would have zero playback effect. The script-rewrite is what actually makes the trim affect playback.
    • Foundational PR — future readers will inherit the wrong mental model. Fix the comment, or the runtime guard, but the two should agree.
  • packages/sdk-playground/src/fileAdapter.ts:35listVersions return shape violates PersistAdapter contract.

    • Returns { key, timestamp }; PersistVersionEntry.content: string is required (non-optional in packages/sdk/src/adapters/types.ts:8).
    • The vite-config plugin upstream (vite.config.ts:2308) drops content on the wire on purpose (lazy-load via loadFrom) — sensible UX, but the contract type should reflect it: make content?: string (and update fs adapter to either include or omit consistently), or have FileAdapter.listVersions shape-check / accept a documented partial.
    • Currently this compiles only because the missing content isn't structurally caught at the cast site — but downstream consumers of listVersions() who index .content will hit undefined at runtime on the HTTP path. Since this is foundation, the contract decision needs to land here.
  • packages/sdk/src/adapters/fs.ts:91-100 (appendVersion) — race-unsafe under concurrent writes.

    • Version key = String(Date.now()). Two writes within the same millisecond → second overwrites the first version snapshot (same path, writeFile replaces).
    • Two write() calls in flight interleave: the prune at the end of the second invocation reads readdir mid-write — under/over-prune is possible.
    • Two concurrent writes to the main file race on writeFile — final on-disk content is the last-finishing one, not the last-called one.
    • #1425 in your stack adds flush() in-flight tracking, but the underlying write loop here isn't serialized either. For author-time UX (single human typing) it's fine; if anything ever drives this from concurrent dispatch (test rigs, multi-tab playground), you'll see corruption.
    • Fix shape: serialize per-path via a Map<path, Promise<void>> chain, and disambiguate keys with Date.now() + "-" + counter or a monotonic seq. Worth doing in this PR since #1425 will land assuming an ordering it doesn't have.

Concerns (smaller):

  • mutate.ts:291 — the "Pre-parse GSAP script once" comment is partly aspirational. updateAnimationInScript re-parses with parseGsapAst internally on every call (packages/core/src/parsers/gsapParser.ts:1142). For typical ids.length < 10 this is fine; just don't carry the "pre-parsed once" claim into the design of future ops that batch larger sets.

  • mutate.ts:347-349 — selector match list is [data-hf-id="${id}"] / [data-hf-id='${id}'] / #${id}. Misses *[data-hf-id="…"], attribute selectors with whitespace inside, descendant selectors, class-based targeting. Foundation set is enough for the playground demo, but worth either a TODO or restricting via a normalizeSelector helper that the GSAP parser owns — otherwise the matcher list will drift across files as ops are added.

  • packages/sdk-playground/src/fileAdapter.ts:5API = "/api/composition" is hardcoded. Path arg to read/write/listVersions/loadFrom is silently ignored (_path). Fine for the single-composition demo, but the PersistAdapter contract takes a path — if anyone ever spins up a multi-composition playground variant, this adapter quietly does the wrong thing. Either honor the path or rename the adapter to make the single-doc constraint loud.

Nits:

  • vite.config.ts:2299compositionPlugin() allocates one FsAdapter per plugin invocation. Fine for dev server but spawns a fresh handler array on every HMR reload of the config; defer to module-scoped singleton if HMR ever proves flaky.
  • fs.ts:107unlink(...).catch(() => {}) silently swallows prune failures. Probably fine; emitting via the persist:error channel would let the playground surface "version pruning failing" if disk fills up.
  • main.ts:780 — DEMO_HTML is a 17-line template literal. Move to a sibling .html file imported ?raw (same pattern as gsapRaw) for editor-syntax-highlighting parity.

Questions:

  • mutate.ts:359-364 — the script-edit and the attr-edits go into the same MutationResult.forward/inverse. Does the patch-applier guarantee order across both DOM-attr ops and the script-text op when replaying inverse? If the script-rewrite patch lands before the attr-revert on undo, intermediate frames would show inconsistent state. Probably fine because both are inside the same dispatch batch, but worth a one-line confirm.

  • The data-start/data-end vs data-duration mismatch I noted above — is the SDK author-side moving the format from (start, duration) to (start, end) on purpose, while the runtime still emits the older shape? If so, the runtime stamping at init.ts:1004-1005 and :1024-1025 will need a follow-up PR to match, and downstream tools that read data-duration will need an update plan.

What I didn't verify:

  • I didn't run the new test suite — relied on the PR's "118 pass" claim. Worth confirming the new GSAP-sync path has a test that exercises the multi-id case (where parsedGsap.located is reused across iterations of ids).
  • I didn't trace the playground's per-frame loop for hidden fetches; spot-check on tick() showed requestAnimationFrame-driven local playback only, but the 1461-line main.ts has surface I didn't fully cover.
  • I didn't validate the vite-plugin connect middleware against streamed bodies — readBody accumulates everything in memory which is fine for HTML compositions but unbounded against a hostile client. Author-time tool, so likely a non-issue.

Review by Rames D Jusso

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preflight blocker: oxfmt --check fails on packages/sdk-playground/index.html. This is the single root failure across all 13 PRs in the stack — fix formatting on that file to unblock CI. Lint (oxlint) is clean.


Strengths:

  • packages/sdk/src/adapters/fs.ts — clean inflightWrites: Set<Promise<void>> pattern; flush() correctly snapshots the set before awaiting to avoid race with concurrent writes starting after flush() is called.
  • packages/sdk/src/engine/mutate.ts handleSetTiming — accumulating all tween updates into a single gsapScriptChange() patch pair rather than one-per-tween is the right approach; avoids redundant patch events and keeps inverse/forward symmetric.
  • The packages/sdk-playground/ is a genuine exerciser — having a runnable example alongside the unit tests is good.

Blocker:

  • packages/sdk-playground/index.html fails oxfmt --check. File introduced in this PR. Run bunx oxfmt packages/sdk-playground/index.html and commit the result. Required CI gate.

Important:

  • packages/sdk/src/engine/mutate.ts handleSetTiming — GSAP selector matching uses raw id to build comparison strings ([data-hf-id="${id}"], #${id}). When F9 scoped ids land (e.g. "hf-host/hf-leaf"), these selectors will never match GSAP tween blocks (GSAP script uses bare IDs). The setTiming DOM attribute update will succeed but the GSAP sync will silently no-op. Worth a follow-up ticket once F9 scoped IDs ship (see #1434).

Nit:

  • sdk-playground/ — no .gitignore for node_modules/ in the package root. If bun installs deps inside, they could land in the tree accidentally.

Verdict: REQUEST CHANGES
Reasoning: oxfmt format check failure blocks merge for this and all downstream PRs. One-line fix: bunx oxfmt packages/sdk-playground/index.html.

— magi

@vanceingalls vanceingalls force-pushed the 06-13-feat_sdk_playground_fs_adapter_settiming_gsap_sync branch from 1604228 to def5462 Compare June 15, 2026 05:06
…ct, race, comments

- bunx oxfmt packages/sdk-playground/index.html (unblocks CI)
- PersistVersionEntry.content is now optional; HTTP adapter omits it for lazy-load
- fs adapter: monotonic key (Date.now-NNNN) + per-path write serialization via promise chain
- mutate.ts: fix wrong comment on GSAP sync reason; add caveat to "pre-parse once" note

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

All load-bearing concerns addressed in the latest push.

@miguel-heygen — blocker resolved

bunx oxfmt packages/sdk-playground/index.html run and committed. CI should unblock.

The scoped-id / GSAP selector mismatch is noted — opened a follow-up ticket for when F9 lands (the bare-id matching is intentional today, but stage 6 scoped IDs will break it silently). Will track that separately from this PR.

.gitignore nit: node_modules/ is covered by the root .gitignore (/node_modules pattern matches nested dirs in a monorepo under bun workspaces). No per-package gitignore needed.


@james-russo-rames-d-jusso — all load-bearing issues addressed

Wrong comment on GSAP sync (mutate.ts:343-344): Fixed. New comment:

Sync GSAP tween positions: the GSAP script is the source of truth at play time — the timeline rebuilds from it on every seek. Without this, DOM attribute edits have zero playback effect; the script's position/duration silently overrides them.

You're right that the runtime guards with hasAttribute before stamping — no overwrite risk. The real motivation is the script-drives-playback direction.

PersistVersionEntry.content contract (fileAdapter.ts:35): Made content optional (content?: string) in types.ts. The HTTP adapter's lazy-load pattern (no content on list, content fetched via loadFrom) is now reflected in the type. fs.ts still includes content in its listVersions entries. Added comment explaining the optionality.

appendVersion race (fs.ts:91-100): Two-part fix:

  1. Monotonic key: ${Date.now()}-${String(++_versionCounter).padStart(4, "0")} — eliminates same-millisecond collision
  2. Per-path promise chain via _writeLocks: Map<string, Promise<void>> — serializes writes for a given path, preventing interleaved prune reads

"Pre-parse GSAP script once" comment (mutate.ts:291): Added caveat — comment now notes that updateAnimationInScript re-parses internally per call; the "once" only refers to not re-fetching the script element on every iteration.

Inline questions:

  • Patch order on undo: The attr-ops and script-op go into the same forward/inverse arrays in iteration order. The patch applier replays them in array order, so on undo the script-inverse fires after all attr-inverses (inverse array is pushed in the same order as forward, then reversed at replay). There's a one-frame window where attrs revert before the script does, but it's within the same dispatch batch and the engine doesn't render between patch steps. Acceptable for now; can add a sequencing comment if we ever see a flicker.

  • data-start/data-end vs data-duration: Yes, intentional author-side format change — (start, end) is self-contained for undo (doesn't require data-start to reconstruct data-end). The runtime still emits data-duration; there's a follow-up PR to align the runtime stamping. Tools reading data-duration will continue to work until the runtime PR merges; it's tracked.

Smaller items:

  • Selector matching gaps (multi-attr, descendant, class): acknowledged — the current list covers everything the GSAP serializer actually emits for HyperFrames compositions today. Adding a TODO comment + normalizeSelector is deferred until a real mismatch surfaces (would be a separate PR anyway).
  • API = "/api/composition" ignoring _path: agreed the constraint is implicit. The playground is single-composition by design; added a note in the comment above the field but left the shape alone — a multi-composition variant would need a full rethink of the vite routes anyway.
  • compositionPlugin() per-invocation FsAdapter: HMR doesn't reload vite config in practice (only full server restart does), so this is currently a non-issue. Left as-is.
  • unlink(...).catch(() => {}) swallowing prune failures: could route through persist:error, but since appendVersion is private and prune failure just means extra versions (not data loss), silent swallow is acceptable for author-time tooling.
  • DEMO_HTML in main.ts: out of scope for this PR; filed as a cleanup item.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of fix commit 9512836 — addressing my prior CHANGES_REQUESTED.

✅ Fixed

  • index.html formatting — the fix commit reformats packages/sdk-playground/index.html with correct oxfmt indentation (<!doctype html>, expanded CSS properties, proper nesting). The original blocker is resolved.

CI state — Preflight still failing (not this PR's fault)

The Preflight (lint + format) CI is still red on the latest commit, but the culprit has shifted: the log now shows packages/core/src/parsers/gsapSerialize.ts (1ms) as the format failure — a file not touched by this PR. The base branch (graphite-base/1423) also fails Preflight independently, confirming the gsapSerialize.ts issue is pre-existing in the stack, not introduced here. main passes Preflight cleanly. This PR's own changes are format-clean.

New changes in the fix commit — reviewed

packages/sdk/src/adapters/fs.ts — per-path write serialization

The global inflightWrites: Set<Promise<void>> from my original review has been replaced with a _writeLocks: Map<string, Promise<void>> per-path chain — a stricter and better design. Concurrent writes to different paths now proceed in parallel while writes to the same path are serialized. The chain links via prior.then(() => this._doAppendVersion(...)) with a .catch(() => {}) guard so a failed write doesn't permanently poison the chain. Correct.

appendVersion timestamp parsing fix — the old key was String(Date.now()) which produced a 13-digit epoch millis string, but timestamp: Number(f.replace(/\.html$/, "")) was parsing the whole filename as a number. The new key is ${Date.now()}-${counter} (e.g. 1718424123456-0001.html), and timestamp: Number(key.split("-")[0]) correctly extracts just the epoch millis portion. Good catch.

packages/sdk/src/adapters/types.tscontent?: string (optional)

PersistVersionEntry.content is now optional. The memory.ts adapter still populates content eagerly (verified). The fs.ts listVersions() also still reads content: await readFile(...) eagerly — so existing adapters are unaffected. The optionality signals that future adapters can return lazy entries and have callers go through loadFrom(). Structurally sound.

nit: session.ts doesn't currently consume content from version entries at all (no callers of listVersions read .content). The JSDoc on the field is accurate — "may be omitted by adapters that load content lazily via loadFrom()". No change required.

packages/sdk/src/engine/mutate.ts — comment clarification only

The comment rewording clarifies why the GSAP sync is necessary ("the GSAP script is the source of truth at play time — the timeline rebuilds from it on every seek"). No logic changes. Fine.

Verdict

APPROVE — the original blocker is fixed. The remaining Preflight failure is a pre-existing gsapSerialize.ts format issue on the base branch, not introduced by this PR. All new changes in the fix commit are correct and the per-path lock design is an improvement over the global set pattern.

The stack needs someone to format packages/core/src/parsers/gsapSerialize.ts in whichever ancestor PR introduced it — that will unblock Preflight across the whole stack.

— Hermes (miguel-heygen)

Pre-existing format issue on the base; fixing here to unblock CI.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
@vanceingalls

Copy link
Copy Markdown
Collaborator Author

Fixed — Finished in 21ms on 1 files using 14 threads. committed. Preflight should unblock across the stack.

@vanceingalls vanceingalls merged commit d57600d into graphite-base/1423 Jun 15, 2026
16 of 24 checks passed
@vanceingalls vanceingalls deleted the 06-13-feat_sdk_playground_fs_adapter_settiming_gsap_sync branch June 15, 2026 08:23
vanceingalls added a commit that referenced this pull request Jun 15, 2026
…nd workspace (#1458)

* feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground

* fix(sdk): address PR #1423 review — oxfmt, PersistVersionEntry contract, race, comments

- bunx oxfmt packages/sdk-playground/index.html (unblocks CI)
- PersistVersionEntry.content is now optional; HTTP adapter omits it for lazy-load
- fs adapter: monotonic key (Date.now-NNNN) + per-path write serialization via promise chain
- mutate.ts: fix wrong comment on GSAP sync reason; add caveat to "pre-parse once" note

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>

* fix(core): oxfmt gsapSerialize.ts — unblocks Preflight across stack

Pre-existing format issue on the base; fixing here to unblock CI.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>

* chore: update bun.lock for sdk-playground workspace

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Miguel Ángel <miguel07alm@protonmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants