feat(cli): expose Studio context through preview#1777
Conversation
miguel-heygen
force-pushed
the
magi/studio-selection-cli
branch
from
ac2ee0a to
27f0258
Compare
miguel-heygen
changed the title
|
Really nice to see this land so fast — the push-then-read snapshot, the 1. The published selection goes stale after an external edit — i.e. the agent flow this feature targetsThe publisher effect in When an agent edits the composition file on disk, the flow is: file-change →
This is exactly the "select → agent edits → re-query" loop the feature is for, so it's worth closing. Options: re-run 2.
|
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: LGTM-with-nits
I have examined the entire crime scene — every file, every line — and I am pleased to report that the agent bridge into Studio is, on the whole, soundly constructed. The compact-context taxonomy is sensible, the snapshot validator is conscientiously paranoid, the failure codes are machine-readable, and the skill update genuinely teaches agents how to use the new surface without bloating their context windows. There is, however, one schema-drift footprint near the back door, a small accumulation of stale process-local dust I should like to point out, and a couple of cosmetic blemishes a fastidious reader will spot. Nothing rises to the level of blocking the merge.
Context snapshot
- HEAD:
27f0258e - Base:
main - CI: all required green (CLI smoke, Studio load smoke, runtime contract, SDK contract, skills manifest in sync, typecheck, lint, fallow audit)
- Prior reviewers (verified at post time): none yet — I am first on the scene
Direct answers to your three sanity checks
1. CLI UX shape — verdict: solid
The flag surface composes as expected: --selection --json and --context --json are the two terminal modes; both early-return before any browser/spawn machinery, so they cannot conflict with the live preview path. --context-fields is comma-separated with a tidy whitelist (server|selection|lint|capabilities) and rejects unknowns with code invalid-context-fields. --context-detail is binary (compact|full) and rejects with invalid-context-detail. Exit code is consistently 1 on failure via process.exitCode = 1 and 0 on success. (packages/cli/src/commands/preview.ts L147–154, L170–184, L283–301.)
One trifling rough edge worth noting: --context-fields "" (an empty or whitespace-only string) silently collapses to the default field set rather than rejecting (parseContextFields L170–184). Strict-mode pedants will prefer an explicit error; agents are unlikely to ever do this in practice. Filed as a 🟡 nit below.
2. Compact payload sufficiency for agents — verdict: yes, with one schema-drift footprint
The compact selection slice — schemaVersion, projectId, compositionPath, sourceFile, currentTime, target, label, tagName, boundingBox, textContent, thumbnailUrl (preview L48–61) — is exactly the set an edit-anchoring agent needs, and the heavy fields (computedStyles, inlineStyles, dataAttributes, textFields) are properly gated behind --context-detail full. The skill teaches agents to reach for target.hfId first and fall back to target.selector — the right ergonomics.
The footprint, however, is schema drift between the two surfaces:
preview --selection --jsonreturns{ ok: true, server, selection: <flat snapshot>, updatedAt }on the happy path and{ ok: false, error: {code, message} }on failure (preview L240–254, L147–154).preview --context --jsonreturns{ ok: true, server?, selection: { ok: true, value, updatedAt } | { ok: false, error }, lint: { ok, summary, findings } | { ok: false, error }, capabilities }(preview L322–381).
So the field selection is flat under --selection and discriminated-union wrapped under --context. An agent that learns one and tries to read selection.value.target.hfId against --selection (or selection.target.hfId against --context) will pull undefined and silently mis-anchor an edit. The skill doc already disambiguates this in prose — "Prefer selection.value.target.hfId" — so the contract is documented; it is only the asymmetry I am asking you to be sure you want to ship. If yes, leave it; if no, normalising both to { ok, value | error } would be cheap. Flagged as 🟡 below.
3. Selection-snapshot failure / clearing semantics — verdict: correct on the hot paths, one stale-leftover edge case
- No selection active: the server returns
{ selection: null, updatedAt: null }(selection.ts L1266–1273); the CLI's--selectionpath converts that tono-selectionand exit 1 (preview L227–233);--contextreportsselection.ok = falsewithno-selectionand continues to emitserver/lint/capabilities. Good. - Selection cleared mid-snapshot: Studio publishes
{ selection: null }(useDomEditSession L173–192). The serverDELETEs its entry (selection.ts L1289–1292). If the CLI request lands in between, it sees the old or the new state cleanly — no torn read, because the route handler returns a single point-in-timec.json. - Studio not running:
findPreviewServerForProjectscans active servers; if none matches the project dir, the CLI returnspreview-not-runningand exits 1. No hang. Good. - Multiple selections: Studio's
useDomEditSessionwrites a singledomEditSelection; group selection is a separate state not piped through this channel. The single-element contract holds. - Stale DOM node: the snapshot is built from
DomEditSelection(no liveHTMLElementis serialised — thestudioSelectionSnapshot.test.tseven assertsJSON.stringify(snapshot)doesn't containHTMLElement). Good.
The one edge case worth a comment for V2 (🟡 nit below): the server-side selections Map is keyed by project.id and is additive — nothing cleans it on Studio tab-unmount or project switch. Process-local store, so it dies with the server (low blast radius), but a stale snapshot from project A could be served if a CLI invocation races a project switch on the same long-running server. A cleanup PUT-null in the useEffect cleanup, or a per-key TTL on the server, would close this.
Findings
🟡 Nit — Schema drift between --selection and --context payload shapes
preview --selection --json keeps selection flat; preview --context --json wraps it in { ok, value | error }. The skill doc already teaches agents to reach for .value under --context and direct fields under --selection, but two shapes for the same conceptual object is the kind of contract footprint that bites the second consumer. Consider normalising both to { ok, value | error } (or both flat with top-level ok). Decision-only — not blocking.
📍 packages/cli/src/commands/preview.ts L240–254 vs L322–381
🟡 Nit — Stale selection persists on project switch / Studio close
useDomEditSession.ts L172–192 does not publish a { selection: null } on effect cleanup; the controller is only aborted. The server's selections Map therefore retains the last-known snapshot per projectId until the preview server process dies. Within a single long-running server with multiple project sessions, this can serve a stale entry to a CLI invocation. A controller-respecting cleanup PUT-null, or a per-key TTL on the server, would shut the door.
📍 packages/studio/src/hooks/useDomEditSession.ts L172–192; packages/studio-server/src/routes/selection.ts L1262–1303
🟡 Nit — --context-fields "" silently defaults instead of erroring
parseContextFields (preview L170–184) treats --context-fields "" and --context-fields " " as "give me everything" because of the !value?.trim() early-return. Agents are unlikely to construct this by accident, but a strict-rejection-with-invalid-context-fields is more in keeping with the rest of the validator's tone.
📍 packages/cli/src/commands/preview.ts L170–184
💭 Cosmetic — selection-unavailable message is String(reason) of a thrown Error
In printCurrentContext (preview L346–351), a rejected fetch becomes { code: "selection-unavailable", message: String(selectionResult.reason) }. String(new Error("foo")) yields "Error: foo", which agents will parse fine but humans will find slightly noisy compared to the err instanceof Error ? err.message : String(err) pattern you use one screen up. Trivial.
📍 packages/cli/src/commands/preview.ts L346–351
✅ Verified clean
- Validator paranoia.
isSelectionSnapshotenforces every required field type, including bounding-box finiteness, target shape, text-field source enum (self|child|text-node), and theschemaVersion === 1literal gate. A future v2 schema cannot accidentally be accepted by v1 servers. (selection.ts L1235–1260.) - Null vs missing. The PUT route correctly distinguishes
selection === null(clear) from missing key (400) and from malformed object (400). Tests cover all three. (selection.test.ts L1117–1156.) - No live
HTMLElementleakage.buildStudioSelectionSnapshotonly copies primitive fields and uses spread for record types; the studio-side test asserts the serialised payload is HTMLElement-free (studioSelectionSnapshot.test.ts L1450–1453). - URL building.
studioApiUrlcorrectlyencodeURIComponentsprojectName(test L863–870 verifiesdemo project→demo%20project). - CLI integration test for
selection-not-found. The PR body's manual run onproduct-promoagainst no-Studio confirms thepreview-not-runningpath end-to-end. - Skill update is honest about the new flags.
skills/hyperframes-cli/SKILL.mdupdates the--jsonexception list to call outpreview --selection --jsonandpreview --context --json;references/preview-render.mddocuments all three failure codes. createStudioApi.tswires the new route alongside existing ones (createStudioApi.ts L1004–1008) — no consumer left behind.- No backwards-compat break on existing
preview. The new flags default tofalse; existingnpx hyperframes previewflow is untouched except for an internal refactor extractingattachStudioReadyHandler,printStudioSummary,linkProjectIntoStudioData,removeSymlinkOnExit,registerChildTreeShutdown,waitForChildClose— the behavioural equivalence reads correctly acrossrunDevMode,runLocalStudioMode, andrunEmbeddedMode.
Summary
Ship it after a brief consideration of the two-shape selection field — either normalise the wrappers across --selection and --context, or commit to the asymmetry and the skill doc carries the contract. Neither blocks the merge. The rest is craft.
Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: LGTM-with-followups — layering on Leopold + Via
Reviewed at 27f0258e799664256ebf72131ecf922b82d19c6d. Leopold's two findings and Via's schema-drift / stale-Map / --context-fields "" nits cover the surface area I'd otherwise have raised; this review layers on theirs rather than parallel-posts, confirms the load-bearing pieces of Leopold's diagnosis with file:line evidence, and adds one observability finding neither caught.
The shape of the bridge is good — push-then-read with a paranoid validator, clean failure taxonomy, compact-default + escape hatch, and a skill update that genuinely teaches the new surface. The two substantive open items both target exactly the agent loop this PR is built for, so worth closing before declaring victory.
🟠 Confirming Leopold's #1 — stale snapshot survives reloadPreview (the agent-edit loop) [load-bearing]
I traced this end-to-end against the PR HEAD and Leopold's diagnosis holds:
useDomEditSession.ts:172-192(new) re-PUTs only when[domEditSelection, projectId]identity changes.- The only path that re-resolves
domEditSelectionagainst a reloaded DOM isrefreshDomEditSelectionFromPreview(useDomSelection.ts:371); its callers areuseDomEditCommits/useDomEditTextCommits(Studio's own commits only) — not the external-reload path. - External file-change →
reloadPreview→setRefreshKey(k+1)(App.tsx:148).NLELayout.tsx:152-161does a lightweight iframesrcreload (refreshPlayer, not React remount) — the Player +useDomSelectionstate persist. - The only
refreshKey-dependent effect atuseDomSelection.ts:450-452clears hover only, notdomEditSelection.
So in the exact "user selects → agent edits file → agent runs preview --context" loop this PR sells:
domEditSelectionidentity is unchanged → publisher effect does NOT fire → server keeps serving the pre-edit snapshot.- If the agent removed/renamed the element,
target.hfIdnow points at nothing on disk, butGET /projects/:id/selectionreturns it verbatim — no freshness check. - For restyles the element survives but
boundingBox/computedStyles/textContentare frozen.
Leopold's suggested shapes (re-run refreshDomEditSelectionFromPreview or publish-null on refreshKey change; and/or a freshness signal in the snapshot the agent can validate) both close it. Either works. The freshness signal is the cheaper insurance and means agents don't need to know about Studio's reload mechanics — e.g. selection.staleSinceReload: true after any refreshKey bump until next user click.
🟠 Confirming Leopold's #2 — Vite-served Studio (5190) is invisible to --context / --selection [load-bearing]
Verified the mechanics on the PR worktree:
packages/studio/vite.config.ts:187→server.port = 5190.packages/cli/src/server/portUtils.ts:23→MAX_PORT_SCAN = 100; scan starts at3002(defaultstartPort), end is3101. 5190 is not in the scan window./__hyperframes_configis registered only atpackages/cli/src/server/studioServer.ts:540. I greppedpackages/studio-serverandpackages/studio— no other registration exists. The Vite plugin mountscreateStudioApiunder/api/(vite.config.ts:104-133) but never registers__hyperframes_config, so even if a Vite session were in the scan range,scanActiveServerswould still skip it.
Net: in runDevMode and runLocalStudioMode the studio DOES publish selections (Vite plugin mounts the API), but findPreviewServerForProject returns null → CLI emits preview-not-running even though Studio is running. The skill update at SKILL.md:32-33 tells agents to run preview --context unconditionally, so this fails silently for monorepo contributors and anyone with @hyperframes/studio installed locally. End-users on the embedded path are unaffected.
Either fix Leopold proposed is fine: (a) the Vite plugin also serves /__hyperframes_config with projectName + projectDir from the adapter, and scanActiveServers is widened to also try 5190 (or a STUDIO_DEV_PORT env hint); or (b) a one-line caveat in the skill prose. (a) is the better fix because the agent-flow assumption is "preview is preview"; (b) just moves the surprise to a different surface.
🟡 NEW — Silent failure on Studio → server PUT, no observability anywhere
This one neither Leopold nor Via covered:
// packages/studio/src/hooks/useDomEditSession.ts:181-189 (new)
void fetch(`/api/projects/${encodeURIComponent(projectId)}/selection`, { … })
.catch(() => {
// The selection channel is agent context only; never interrupt Studio edits.
});The "never interrupt Studio edits" intent is right. The problem is the failure mode is completely invisible to both ends of the loop:
- Studio swallows the error; no
trackStudioEvent/console.error/ dev-only log. - The CLI consumer sees
selection: null(or a stale entry, per blocker #1) and emitsno-selectionwith no signal that "Studio tried to publish and the server rejected it 30s ago."
This is the failure-path-coverage trap your own observability rubric calls out: telemetry on the success path of a feature whose value depends on the failure path being diagnosable. One trackStudioEvent("studio_selection_publish_failed", { status, projectId }) (errors only, throttled) gives an oncall / Datadog signal when validator drift, port mismatch, or the studio-server returning 400 from a future schema change happens in the wild — without spamming on every selection change. The validator's schemaVersion === 1 literal gate (routes/selection.ts:1239) is exactly the kind of contract that will produce silent 400s during a future v2 rollout, and you'll want to know.
🟡 Nit — Map<string, StoredSelection> has no cleanup AND no eviction (compounds with Via's stale-leftover nit)
Via flagged the project-switch stale-leftover case at the Studio-side (no PUT-null on effect cleanup). Worth pairing with the server-side: routes/selection.ts:1263 allocates const selections = new Map<string, StoredSelection>() once per registerSelectionRoutes call, and the only deletion happens on an explicit PUT { selection: null } (line 1290). There's no TTL, no LRU, no cleanup on project unmount. In the embedded server case (one project, lifetime = preview process) this is fine. In the Vite plugin case (multi-project dev server), restart-to-clear is the only recovery if Studio ever crashes mid-publish without sending null. Cheap fix: a per-key updatedAt-based TTL (e.g. drop entries older than 1h on read) gives ambient self-healing without a lifecycle hook.
✅ Verified independently (re-confirming Via)
studioApiUrlencodeURIComponentsprojectNameend-to-end; embedded server'sresolveProjectdoes exact-match on the decodedid, sodemo project↔demo%20projectround-trips correctly.--contextshortcuts--selectionif both are set (preview.ts:178-190); precedence is fixed, undocumented. Minor — agree with Via not to block.process.exitCode = 1+ early return is the consistent failure shape acrossprintSelectionFailure; verified the same path inprintCurrentSelection,printCurrentContext(parse-fail, server-not-found, fetch-fail).
What I didn't verify
- Selection-snapshot accuracy when Studio is inside a sub-composition drill-down. The
compositionPathsource field inbuildStudioSelectionSnapshotfalls back throughselection.compositionPath → selection.sourceFile → "index.html". Sub-composition consumers ofdomEditSelectionmay already do the right thing, but I didn't trace thehandleDrillDownpath inNLELayoutto confirm the snapshot'scompositionPathmatches what an agent would feed back into a--variablesedit. Worth a sanity check during manual QA. - Vite plugin behavior under HMR-induced React fast-refresh of
useDomEditSession. If fast-refresh remounts the hook without unmounting, the cleanup PUT (if added per finding #3 / Via's Studio-side nit) might race with the next mount's PUT. Out of scope for this review; if you add the cleanup, the AbortController already wired in does the right thing on the fetch, but Studio HMR semantics for this hook are Via/Vai's lane.
Verdict reads "LGTM-with-followups" rather than blocker because Leopold's #1 + #2 hit the agent flow this PR targets but the non-agent path (user manually clicking → reading --selection) works correctly today, and a follow-up PR or a single push to this one closes both. Happy to revisit on R2 if any of the three 🟠/🟡 above land in a follow-up commit.
— Rames D Jusso
miguel-heygen
force-pushed
the
magi/studio-selection-cli
branch
from
27f0258 to
7986a3a
Compare
|
Addressed the review pass from Leopold / Vance / Rames at What changed:
Validation:
CI is rerunning on the pushed SHA now. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 verification — LGTM, all R1 items closed
Reviewed at 7986a3ab8e0fab0b75ea11e1935f888fecca2ed1 against R1 (27f0258e). Per-finding diff verification below; all four open items I flagged at R1 actually moved at HEAD, not just claimed-moved. No new blockers.
✅ Resolved — Leopold #1 stale snapshot on reloadPreview
The fix is two-layered and lands at the right place. useDomEditSession.ts no longer holds the publisher inline; the new useStudioSelectionPublisher hook (packages/studio/src/hooks/useStudioSelectionPublisher.ts) owns four effects:
- Publish on
[domEditSelection, projectId]identity change, gated ondomEditSelection.element.isConnectedso a detached node never publishes. - Clear on
refreshKeybump (useStudioSelectionPublisher.ts:60-66) — PUTselection:nullimmediately when external reload fires, before the iframe even finishes reloading. Closes the "agent reads pre-edit snapshot" window. - Re-resolve on
previewDocumentVersionbump (useStudioSelectionPublisher.ts:73-79) — wired throughuseDomEditPreviewSync.ts:89which firesrefreshPreviewDocumentVersion()insidepreviewIframe.addEventListener("load", handleLoad). So the new document is actually queryable when re-resolve runs. useDomSelection.ts:381-384nowapplyDomSelection(null)whenfindElementForSelectionreturns null, so a removed/renamed target clears the UI selection too — server cleanup follows via the publisher effect.
The setTimeout(80ms, 300ms) re-ticks of previewDocumentVersion (App.tsx:115-118) cover the case where the iframe's load event fires before its document body is paint-stable. Belt-and-suspenders, but fine.
✅ Resolved — Leopold #2 Vite Studio (5190) invisible to --context
studioSelectionClient.ts:31-39 now falls back to probing http://127.0.0.1:5190/api/projects after embedded scan, matches by realpathSync-normalized project.dir, and synthesizes an ActiveServer with version: "studio-dev". Test at studioSelectionClient.test.ts:39-58 covers the new path. The realpath fix also makes the matcher symlink-safe (relevant because runLocalStudioMode symlinks linkProjectIntoStudioData). Cheap + correct.
✅ Resolved — R1-NEW silent .catch(() => {}) on publish
reportSelectionPublishError (useStudioSelectionPublisher.ts:16-26) emits trackStudioEvent("studio_selection_publish_failed", { error_name, error_message }) and console.warn on failure — and correctly filters AbortError so React-StrictMode dev-mount unmount churn doesn't spam telemetry. Per feedback_observability_pr_failure_path_coverage: this emits on the FAILURE path of the channel whose value depends on diagnosing failures. Honest.
Caveat (page-context): this runs in Studio's React app, not the headless preview iframe — so the emit reaches the producer's telemetry sink (Datadog via trackStudioEvent), not lost in iframe console. ✅ no page-context-emit trap here.
✅ Resolved — Server-side Map cleanup paired with Studio unmount
Project-switch / unmount cleanup at useStudioSelectionPublisher.ts:48-53 PUTs selection:null in the [projectId] effect's cleanup. Pairs correctly with Via's Studio-side ask — the server Map entry is removed via the explicit DELETE path (selection.ts server case selection === null), not just abandoned. The server-side TTL Via and I both suggested as a fallback is not added, but cleanup-on-unmount makes it strictly less load-bearing; deferring to a follow-up is reasonable.
✅ Resolved — --context-fields "" returns invalid-context-fields
parseContextFields (preview.ts:316-318) now distinguishes undefined (→ defaults) from empty/whitespace (→ throws), and printCurrentContext (preview.ts:480-486) catches into invalid-context-fields with process.exitCode = 1. Sentinel string consistent + non-zero exit propagated.
✅ Resolved — Schema drift between --selection and --context
--context selection is now flat snapshot or null (preview.ts:516-519) with selectionUpdatedAt + errors.selection.{code,message} for failure detail. Same shape as --selection --json's flat selection field (preview.ts:386-397). Skill docs (SKILL.md:38, references/preview-render.md:34) updated to selection.target.hfId (was selection.value.target.hfId). Writer (Studio publisher) and reader (CLI consumer) symmetric: both serialize/deserialize the same StudioSelectionResponse shape end-to-end — no silent asymmetric drift.
✅ Verified — publisher hook extraction is real, not cosmetic
Per feedback_verify_peer_review_module_mechanics: useDomEditSession.ts removes the inline useEffect (37-line block) AND removes the useEffect / usePlayerStore / buildStudioSelectionSnapshot imports; the new hook is import { useStudioSelectionPublisher } from "./useStudioSelectionPublisher" + a single call. Real extraction, behavior moved with locals (projectId, domEditSelectionRef, refreshKey, previewDocumentVersion, refreshDomEditSelectionFromPreview) passed as params. useDomEditSession.ts drops under the 600-line gate as claimed.
🟡 Nit (non-blocking) — previewDocumentVersion polling shape
App.tsx:115-118 ticks setPreviewDocumentVersion at 0ms / 80ms / 300ms after refreshPreviewDocumentVersion() invocation. That's three publisher-effect re-runs per external reload (each is a no-op if domEditSelectionRef.current is null after the clear, but it does mean refreshDomEditSelectionFromPreview can run up to 3× per reload if the element survives). Idempotent given the publisher already gates on element.isConnected, but if the iframe load event proves reliable in practice, the timeouts are removable later. Not blocking — the conservative choice is right for first ship.
What I didn't verify
- HMR fast-refresh interaction with the new publisher hook. React-Refresh on
useStudioSelectionPublisher.tssaves will remount the effect;AbortControllerhandles the in-flight PUT but the cleanup-on-[projectId]effect will also fire a PUT-null then a re-PUT. Probably fine, Studio-internal, Via/Vai's lane. - 5190 fallback under multi-Vite-instance race — if a contributor has two
runLocalStudioModeinstances both on 5190 (impossible per port-bind, but plausible if one is queued), the discovery's first-match-wins might serve the wrong project. Low-likelihood, not in scope. - Sub-composition drill-down
compositionPathfield still uses the same fallback chain; same caveat as R1.
Nice work. Closes the agent-loop staleness window cleanly, fixes the dev-mode discoverability silently-empty trap, and the observability hook now actually signals when the channel goes dark.
— Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 Verdict: LGTM — all R1 items closed, no R2-NEW blockers
Reviewed at 7986a3ab against R1 (27f0258e — R1 review). Single force-pushed commit; per-finding verification follows. Rames also posted an R2 at this SHA and reaches the same verdict — this review layers on his with my own per-claim verification at HEAD and an audit of my R1 footprint specifically.
Per-claim verification at 7986a3ab
| # | R2 claim | Status | Evidence |
|---|---|---|---|
| 1 | Stale snapshots clear on preview reload, re-resolve after iframe load |
Fixed | useStudioSelectionPublisher.ts:60-66 PUTs null on refreshKey bump; effect at L73-79 re-resolves on previewDocumentVersion bump; previewDocumentVersion is ticked from useDomEditPreviewSync.ts:95 inside previewIframe.addEventListener("load", handleLoad) — really wired to the load event, not paint-proxy |
| 2 | Vite Studio dev sessions on 5190 discoverable through /api/projects |
Fixed | studioSelectionClient.ts:31-39 falls back to probe http://127.0.0.1:5190/api/projects after embedded scan; matches by realpathSync-normalized project.dir. Prod embedded discovery unchanged — fallback only fires when embedded scan misses |
| 3 | --context selection flat (selection | null) with selectionUpdatedAt + errors.selection |
Fixed | preview.ts:386-394: payload.selection = selection.ok ? selection.value : null; payload.selectionUpdatedAt = selection.ok ? selection.updatedAt : null; addContextError(payload, "selection", selection.error) on the not-ok branch. Union-wrap shape fully gone; both surfaces now symmetric. R1 footprint closed |
| 4 | --context-fields "" returns invalid-context-fields |
Fixed | preview.ts:176: if (!value.trim()) throw new Error("--context-fields cannot be empty"); — printCurrentContext (L296-299) catches into invalid-context-fields with process.exitCode = 1. My R1 nit |
| 5 | Project switch / unmount clears server selection | Fixed | useStudioSelectionPublisher.ts:48-53 — second useEffect keyed on [projectId] returns a cleanup that PUTs selection: null for the leaving projectId. Pairs cleanly with the server-side selection === null → Map.delete branch. My R1 nit |
| 6 | Publish failures emit telemetry + console warning | Fixed | reportSelectionPublishError (L16-26) emits trackStudioEvent("studio_selection_publish_failed", { error_name, error_message }) AND console.warn("[Studio] Failed to update agent selection context", error). Correctly filters AbortError so StrictMode dev-mount churn doesn't spam the sink |
| 7 | Extracted publisher hook, useDomEditSession.ts under the 600-line gate |
Fixed | File at HEAD: 570 lines (was 581 pre-extraction). New useStudioSelectionPublisher.ts (101 lines) is a clean extraction: only one new import (./useStudioSelectionPublisher) and one call site (L171-178); the prior inline useEffect + usePlayerStore / buildStudioSelectionSnapshot imports are removed from useDomEditSession.ts. trackStudioEvent import remains because the file still uses it for group telemetry — not orphaned |
My R1 findings — re-walked at HEAD
| R1 finding | R2 status |
|---|---|
🟡 Schema drift between --selection and --context payload shapes |
Closed by claim #3 — both surfaces now flat selection or null |
| 🟡 Stale selection persists on project switch / Studio close | Closed by claim #5 — useStudioSelectionPublisher.ts:48-53 PUT-null on [projectId] cleanup |
🟡 --context-fields "" silently defaults |
Closed by claim #4 |
💭 Cosmetic — selection-unavailable message used String(reason) of a thrown Error |
Closed — preview.ts:156 introduces errorMessage(error) helper used at L234, L359, L377; agents now see "foo", not "Error: foo" |
R1 ✅-verified items (validator paranoia, null-vs-missing PUT, no HTMLElement leakage, studioApiUrl percent-encoding, CLI integration test for selection-not-found, skill update honesty, createStudioApi wiring, no backwards-compat break on existing preview) — re-grep at HEAD: all intact, no regressions from the refactor.
R2-NEW observations
💭 Cosmetic — previewDocumentVersion triple-ticks per reload
App.tsx:115-118 schedules setPreviewDocumentVersion((v) => v + 1) immediately, at 80ms, and at 300ms after refreshPreviewDocumentVersion() is called. The publisher's re-resolve effect (useStudioSelectionPublisher.ts:73-79) is therefore re-invoked up to three times per external reload. Idempotent (the inner refreshDomEditSelectionFromPreview call short-circuits when the element resolves stably), but means three round-trips to the iframe document per reload in the happy path. Rames flagged the same pattern. Belt-and-suspenders for the case where load fires before the document body is paint-stable — conservative + removable later if iframe load proves reliable. Not blocking.
💭 Cosmetic — VITE_STUDIO_DISCOVERY_PORTS is a single-element const tuple
studioSelectionClient.ts:11 declares const VITE_STUDIO_DISCOVERY_PORTS = [5190] as const; and the helper iterates with for (const port of ...). A monorepo contributor running Vite on a non-default port (e.g. --port 5191 because 5190 was busy) is back to preview-not-running. The shape is forward-compatible (just add ports to the array) and a STUDIO_DEV_PORT env hint would be a 5-line follow-up. Reasonable to ship as-is; the single hardcoded port covers the common case.
Cross-reviewer convergence
Rames posted his R2 at 7986a3ab (2026-06-28T18:52:20Z) — same verdict, same per-finding closures. Independent verification reaches the same conclusion: the publisher extraction is real (not just cosmetic), the iframe load wiring is genuine (not paint-proxy), and the silent .catch(() => {}) observability gap that Rames raised at R1 is honestly closed with telemetry + console.warn on the failure path.
Verdict
Ship it. The agent-loop staleness window is closed at the right layer (publisher hook, not glued to useDomEditSession); the dev-mode discoverability gap is fixed with cheap probe-then-fall-back; the schema drift my R1 surfaced is gone end-to-end (writer and reader symmetric, skill docs reflect the new flat shape). Both R2-NEW notes above are cosmetic — neither is the kind of footprint that compounds.
R2 by Via
miguel-heygen
force-pushed
the
magi/studio-selection-cli
branch
from
7986a3a to
9b932d0
Compare
|
Small R3 delta pushed at
Additional validation for this delta:
CI restarted on the new SHA. |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R3 Verdict: LGTM — small, tidy delta closes the R2-NEW cosmetic
Reviewed at 9b932d0a against R2 (7986a3ab — R2 review, R1 review). One force-pushed commit; the real R2→R3 delta is ~111 lines across three files (studioSelectionClient.ts +23, preview.ts +46, useStudioSelectionPublisher.ts +1 net) — the compare diff's larger silhouette is rebase noise against the advanced base, nothing more.
Per-claim verification at 9b932d0a
| # | R3 claim | Status | Evidence |
|---|---|---|---|
| 1 | Multi-server match still resolves by normalized project dir | Fixed | studioSelectionClient.ts:35-43 normalizePath = resolve → realpathSync → forward-slash + lowercase; matcher at L54-57 compares both sides through it, so symlinked or relatively-spelt working dirs converge on the same canonical key. Test at studioSelectionClient.test.ts:40-55 ("matches by project directory when multiple projects are open") exercises the multi-server happy path |
| 2 | Ambiguous duplicate servers → ambiguous-preview-server; --port disambiguates; single-server case untouched |
Fixed | New AmbiguousPreviewServerError (studioSelectionClient.ts:21-32) thrown only when embeddedServers.length > 1 (L62) — single-server branch returns at L61. --port opt-in via hasExplicitPreviewPort(process.argv) (preview.ts:147, helper at L295-297): the default-3002 path stays undefined, so users who never pass --port never accidentally short-circuit the ambiguous error. Both printCurrentSelection (L362-377) and printCurrentContext (L478-493) catch the thrown error and emit printSelectionFailure("ambiguous-preview-server", err.message, json). Tests: ambiguous case studioSelectionClient.test.ts:57-66 (asserts name + sorted ports); preferred-port disambiguation L68-76 |
| 3 | useStudioSelectionPublisher consumes one re-resolve per refreshKey |
Fixed | The cosmetic I flagged in R2 is closed at the right layer. lastPreviewDocumentVersionRef is gone; replaced by pendingSelectionRefreshKeyRef (useStudioSelectionPublisher.ts:46). Set path at L83 (refreshKey effect): writes refreshKey when a selection exists at reload time, null otherwise. Read+clear path at L96-97 (previewDocumentVersion effect): bails when null, otherwise sets back to null before doing the one re-resolve. The follow-up 80ms + 300ms previewDocumentVersion ticks now find the latch consumed and return immediately — three round-trips collapse to one. Dispatch is balanced (both effects mutate the same ref), and the null-write on no-selection prevents a stale set from a prior reload from being mis-consumed by a future tick |
R2-LGTM items — re-walked at 9b932d0a
| R2 item | R3 status |
|---|---|
--selection / --context symmetric flat shape |
Intact (preview.ts reader/writer unchanged) |
| Project-switch / unmount clears server selection | Intact — useStudioSelectionPublisher.ts:67-72 [projectId] cleanup PUTs null, untouched by the latch refactor |
previewDocumentVersion actually wired to iframe load event |
Intact — useDomEditPreviewSync.ts not in delta |
| Publish-failure telemetry + console.warn | Intact — reportSelectionPublishError not in delta |
useDomEditSession.ts under the 600-line gate |
Intact — file not in delta |
R3-NEW
None. The delta is one targeted helper class (with sorted-ports message), one process.argv sniff for the explicit-port intent, and a one-line latch swap in the publisher hook. Each piece pulls its weight; no band-aid shapes, no decorative gates, no unreachable branches.
Verdict
Ship it. Single-server users see no behaviour change; the multi-server cohort that R2 left implicit now has an honest error + a stated escape hatch (--port); my R2-NEW triple-tick observation is closed end-to-end. Tests cover all three new branches.
R3 by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 verification — tight delta LGTM, all three claims resolved
Reviewed 9b932d0a against R2 baseline (7986a3ab — R2 review). Delta is 4 files / ~125 lines, scoped exactly to the three claimed changes; verified each against the operational goal, did not re-verify R2-resolved items.
✅ Different projects still match by normalized project dir
studioSelectionClient.ts:53-58 — embeddedServers = servers.filter(... normalizePath(server.projectDir) === normalizedProjectDir). Distinct projects normalize to distinct paths, so the filter produces ≤1 match for the unrelated-project case. New test matches by project directory when multiple projects are open (studioSelectionClient.test.ts:40-55) pins this with a 3-server scan + /tmp/third lookup. No regression vs the R2 realpath fix.
✅ Duplicate servers for the same project → ambiguous-preview-server
- Detection:
studioSelectionClient.ts:62-64—embeddedServers.length > 1throwsAmbiguousPreviewServerErrorcarrying the sorted port list; error message lists ports + tells user to pass--port <port>. - Wire shape for agents: both
printCurrentSelection(preview.ts:368-383) andprintCurrentContext(preview.ts:483-498) catch the error and emitprintSelectionFailure("ambiguous-preview-server", err.message, json). JSON sentinel is{"ok": false, "error": {"code": "ambiguous-preview-server", "message": "..."}}(preview.ts:299-304). Exit code isprocess.exitCode = 1so shells/agents can branch. --port <port>disambiguation path:studioSelectionClient.ts:59-62searchesembeddedServers(already filtered to matching project) forserver.port === options.preferredPort. If the preferred port matches one of the dups → returns it. If it doesn't match (user typo / wrong port), falls through to thelength > 1branch and re-throws ambiguous — error message lists the actual discovered ports, so the user can correct. Trust-and-fail risk is bounded to the 1-dup case where--port 9999silently uses the only discovered server, which is benign.--portgate is driven byhasExplicitPreviewPort(process.argv)(preview.ts:296-298) — only treats the port as a disambiguator when the user explicitly passed--port, not the citty default of3002. Good: a user who happens to have a server on 3002 doesn't get auto-disambiguated to that one by accident.- Inverse-op check (per my own "verify INVERSE op tolerance" rubric): CLI is stateless per invocation, so there's no cached choice to invalidate on subsequent commands. Disambiguation is transient-per-invocation, matching the operational goal — no asymmetric staleness window.
- Test coverage:
rejects ambiguous duplicate servers for the same project+uses an explicit preferred port to disambiguate(studioSelectionClient.test.ts:57-77) cover both branches.
✅ Publisher consumes one post-load re-resolve per refreshKey
useStudioSelectionPublisher.ts:46, 83, 96-97 — pendingSelectionRefreshKeyRef flips on refreshKey advance (only when a selection actually exists; otherwise explicit null), and the previewDocumentVersion effect bails on pending === null, sets it to null, then runs refreshDomEditSelectionFromPreview exactly once. The 0/80/300ms previewDocumentVersion follow-up ticks after a reload now short-circuit on the 2nd/3rd tick — verified against the comment update at useStudioSelectionPublisher.ts:84-89. Idempotency under racing refreshes: a new refreshKey advance during/after consumption simply resets pending to the new key, so a second reload mid-flight cleanly kicks off a fresh single-consume cycle. No leaked closures, no stale-version anchoring.
Quick non-blocking observations (no action requested)
- 🟢 Minor UX wart, not a bug:
--port <bad>with a single-dupembeddedServerssilently uses the discovered server instead of erroring on the bad port. Bounded scope — only matters when the user typed a port that doesn't match any running server but the project has exactly one match. Lower-priority than the ambiguous case; flag for future polish if it ever bites. - 🟢 Test-coverage gap (pre-existing):
useStudioSelectionPublisherstill has no hook-level test file underpackages/studio/src/hooks/. Out of scope for this delta — flagging as the visibility hole that would have made (c) easier to validate.
Verdict
LGTM-with-followups remains the standing verdict from R2, no R3-NEW blockers. All three R3 changes meet their operational goals; CI is still spinning at post time (Lint/Test/Build/Studio-load-smoke in progress on the latest CI run), but functional review is clean. Safe to land once required checks go green.
— Rames D Jusso
Add a small Studio selection channel so agents can ask a running preview server for the element the user selected in Studio. This keeps the UX on the existing npx hyperframes preview surface while giving agents a stable source file, target selector, timeline time, and thumbnail URL for follow-up edits.
miguel-heygen
force-pushed
the
magi/studio-selection-cli
branch
from
9b932d0 to
ca193aa
Compare
4 participants
What
Closes #1776.
Adds a native Studio context bridge to
npx hyperframes previewso agents can query a running Studio session instead of guessing what the user means by “this selected element”.New commands:
Why
Agents need a simple, low-friction way to work from the user's actual Studio state. The key UX is: user opens Studio with
preview, selects an element, and the agent can read the relevant source/target/time/lint context through the same CLI.This avoids adding MCP, a second service, or a separate workflow while making Studio more agentic and reducing back-and-forth for targeted edits.
How
preview --selection --jsonreturns the full selected-element payload for direct edit anchoring.preview --context --jsonreturns compact agent context by default: server, flatselection, lint, and capability flags.--context-fieldslets agents request only the useful slices (server,selection,lint,capabilities) and skips unrequested endpoint calls.--context-detail fullis the explicit escape hatch for heavy selection details like computed styles, inline styles, data attributes, and text-field metadata.5190.ambiguous-preview-serverunless the user disambiguates with--port.hyperframes-cliskill now teaches agents to use compact Studio context for user-directed edits.Notable design decisions:
preview-not-running,ambiguous-preview-server,no-selection,selection-unavailable, and invalid context flag errors.--context-fieldsshape.Test plan
Commands run:
bun --filter @hyperframes/cli test -- src/utils/studioSelectionClient.test.tsbun --filter @hyperframes/studio test -- src/utils/studioSelectionSnapshot.test.tsbun --filter @hyperframes/studio-server test -- src/routes/selection.test.tsbun --filter @hyperframes/cli typecheckbun --filter @hyperframes/studio typecheckbun --filter @hyperframes/studio-server typecheckbun run lintbun run format:checkbun run test:skillsbun run test:scriptsenv -u GIT_DIR -u GIT_INDEX_FILE -u GIT_WORK_TREE bunx fallow audit --base origin/main --fail-on-issuesbun --filter @hyperframes/cli buildbun --filter @hyperframes/studio buildbun --filter @hyperframes/studio-server buildgit diff --checkbun run dev -- preview ../../registry/examples/product-promo --context --json --context-fields ""(expectedinvalid-context-fieldsJSON + exit 1)bun run dev -- preview ../../registry/examples/product-promo --context --json --context-fields selection(expectedpreview-not-runningJSON + exit 1 when no Studio server is active)