refactor(core,studio): extract draft-marker constants + R7 code-review fixes (Task 4)

[vanceingalls]

Summary

Task 4 — Constants extraction:

• Creates packages/core/src/studio-api/helpers/draftMarkers.ts with the 5 constants shared between PreviewAdapter and Studio's manualEditsTypes.ts: STUDIO_OFFSET_X_PROP, STUDIO_OFFSET_Y_PROP, STUDIO_WIDTH_PROP, STUDIO_HEIGHT_PROP, STUDIO_MANUAL_EDIT_GESTURE_ATTR
• Updates previewAdapter.ts to import these constants instead of hardcoding strings
• Adds "./studio-api/draft-markers" export subpath to packages/core/package.json (both dev and publishConfig)
• Updates packages/studio/src/components/editor/manualEditsTypes.ts to re-export the 5 constants from @hyperframes/core/studio-api/draft-markers — all Studio call sites unchanged

Includes R7 code-review fixes (C1–C14, P2, P6–P7) addressing feedback on #1289 and #1291:

• persistHfIdsIfNeeded: count-based change detection (not string equality — avoids spurious writes on linkedom quote normalization); TOCTOU re-read guard before write; remove ensureHfIds re-export; add JSDoc invariant note; console.warn on write failure
• preview.ts: fresh-disk re-read in bundle-failure catch path; split ensureHfIds import to come directly from parsers/hfIds.js
• previewAdapter.ts: auto-revert in-flight gesture before new applyDraft (fixes resize→move prop leak); isVisible using getComputedStyle with display/visibility/opacity checks and NaN→visible treatment; CSS.escape fallback for non-browser environments; JSDoc for atTime caller-seek contract; rename _perCallOpts; remove Array.from
• previewAdapter.test.ts: rename atTime test to clarify adapter-does-not-seek; add nested-hf-root-without-id test; add resize→move prop-leak test; add revertDraft-after-commit no-op test
• preview.test.ts: add bundle-vs-disk id-stability test (guards against id divergence when bundler reads a pre-write cache snapshot)
• hfIds.test.ts: replace tautological attribute-edit stability test with cross-document content-keyed id stability test

Test plan

• bun run --cwd packages/core test — 1385 pass, 0 fail
• bunx tsc --noEmit in packages/studio — clean

[vanceingalls]

• refactor(core,studio): extract draft-marker constants + R7 code-review fixes (Task 4) #1292 👈 (View in Graphite)
• feat(core): implement createPreviewAdapter (R7, Task 3) #1291
• feat(core): hf-id write-back to disk + serve-time surfacing (R7, Tasks 1-2) #1289
• test(core): previewAdapter contract failing tests (T10 spec for R7) #1286
• feat(core): sourceMutation data-hf-id targeting (R1, T7) #1272
• feat(studio): sourcePatcher data-hf-id targeting (R1, T3) #1271
• feat(core): clip-model hf- ids minted at parse, emitted as data-hf-id (R1) #1270
• feat(core): ensureHfIds node-id pass + shared hf- mint helper (R1) #1269
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Clean extraction. Constants land in one place, both packages import from @hyperframes/core/studio-api/draft-markers, and the Studio file still owns the domain-specific extras (ROTATION_PROP, PATH_OFFSET_ATTR, etc.) that shouldn't live in core.

The hfIds.test.ts test swap is the right call — "content-keyed minting is stable across documents" tests the actual invariant (same HTML content → same id regardless of document origin), whereas "pinned id survives attribute edit" was testing the mutation guard which is already covered elsewhere.

P3 — packages/core/package.json adds the draft-markers subpath in both exports and publishConfig.exports — confirm the paths are in sync with draftMarkers.ts's actual disk location. Easiest to verify: bun run build in packages/core and check the dist output includes the subpath. If the file is at src/studio-api/helpers/draftMarkers.ts, the export "./studio-api/draft-markers" resolves to ./dist/studio-api/helpers/draftMarkers.js — double-check the helpers/ level isn't dropped.

✅ Stack is in good shape overall. Main blockers are the #1291 P1 (revert-after-double-commit) and P2 (computed vs inline opacity) — both fixable before merge.

[james-russo-rames-d-jusso]

Clean refactor + a substantive round of code-review fixes folded into one PR. The second commit 2af2228 fix(core): address R7 code-review findings (C1–C14, P6–P7) is doing real work — auto-revert on re-apply, isVisible for display/visibility/opacity, count-based change detection, TOCTOU guard, fresh-disk fallback on bundle failure. All of those addressing concerns I flagged on #1289 and #1291.

The constants-extraction half is also clean:

• draftMarkers.ts (new, 5 constants — exactly the 5 that needed sharing between previewAdapter.ts and studio's manualEditsTypes.ts). ✅
• package.json adds ./studio-api/draft-markers subpath in BOTH exports (dev source) and publishConfig.exports (built output). ✅
• manualEditsTypes.ts becomes a re-export shim. Verified the 5 studio-side files that consume these constants (manualEdits.ts, manualEditsDom.ts, manualEditsSnapshot.ts, manualEditsDomPatches.test.ts, plus the test file itself) all import from ./manualEditsTypes — no call sites to update. ✅
• The non-shared constants (STUDIO_ROTATION_PROP, STUDIO_PATH_OFFSET_ATTR, STUDIO_BOX_SIZE_ATTR, STUDIO_ROTATION_ATTR, STUDIO_ORIGINAL_TRANSLATE_ATTR) correctly stay in manualEditsTypes.ts — only the move/resize draft markers move to core. Right line drawn.

Walked the four substantive fixes:

1. hfIdPersist.ts count-based change detection — the right fix for the linkedom-reserialization issue I flagged on #1289. idsAfter > idsBefore only triggers a write when ids were genuinely added, not when linkedom normalized whitespace or quote style.
2. hfIdPersist.ts TOCTOU guard — re-reads disk and compares to input html before writing, skipping the write when disk changed mid-request. Correctly preserves a concurrent user save.
3. previewAdapter.ts applyDraft auto-revert — reverts any in-flight gesture before applying a new one. Prevents the mixed-type leak (resize → move leaving width/height props orphaned) I flagged on #1291.
4. previewAdapter.ts isVisible + getElementTimings NaN handling — replaces the fragile parseFloat(...) || 0 opacity check with a NaN-tolerant + display/visibility-aware visibility test. Treats NaN as visible (sensible default in non-CSS-cascade DOM environments). And getElementTimings now uses Number.isFinite to drop NaN start/end values back to undefined.
5. preview.ts catch-block freshness — re-reads disk on bundle failure (resolveProjectMainHtml + persistHfIdsIfNeeded) instead of using the pre-bundle normalizedDisk snapshot. Prevents serving stale HTML when bundle fails because the user is mid-save.

All five fixes are legitimately good. Hash-stability test substitution in hfIds.test.ts (drops "attribute edit" pin, adds "cross-document content-keyed minting" pin) is also a smart trade — the dropped test pinned the same guarantee as the surviving text-edit test, while the new one pins the OTHER half of the contract (hash-based minting is content-keyed, not document-keyed).

Concerns

[PR title says "refactor" but the diff is half logic-change] Title: refactor(core,studio): extract draft-marker constants to core (R7, Task 4). Body lists ONLY the constants extraction — no mention of the C1-C14 fixes from the second commit. The diff is +134/-84 across 8 files with substantive logic changes to hfIdPersist.ts, previewAdapter.ts, and preview.ts.

A reviewer (or the MQ approver) seeing "refactor — extract 5 constants" might rubber-stamp without noticing the count-based detection swap, the TOCTOU guard, or the auto-revert logic. Each of those is a real behavioral change that deserves a focused look.

Two options:

1. Update the PR body to call out the second commit. Something like:

   Includes follow-up code-review fixes (commit 2af2228): count-based change detection in persistHfIdsIfNeeded, TOCTOU guard against concurrent user saves, auto-revert on applyDraft re-apply, isVisible replacement for the fragile parseFloat(opacity) || 0 check, and fresh-disk fallback on bundle failure.

2. Split the second commit into its own PR stacked on top of #1292's constants-only refactor. Cleaner reviewability, but more rebase work for Vance.

(1) is fastest. Either way, the title/body need to match the diff. The "Task 4" label specifically reads as "the Task 4 plan said extract constants and that's all this does" — which is no longer accurate.

[isVisible opacity threshold is 0.01 — slightly visible elements are reported as visible] L43:

return Number.isNaN(op) || op >= 0.01;

That means a style="opacity: 0.005" element passes isVisible and is returned by elementAtPoint. Probably fine in practice (sub-1% opacity is functionally invisible but technically rendered), but the threshold is unmotivated — why 0.01 rather than 0.001 or > 0?

If the intent is "drop only EXACT zero opacity," use op > 0. If the intent is "drop near-zero too because it's not user-pickable," document the threshold with a comment. Otherwise it'll get tweaked by a future PR for no clear reason. Cheap to add:

// Treat sub-1% opacity as invisible — these elements are not user-targetable in the
// drag-to-edit gesture even though they technically render.
return Number.isNaN(op) || op >= 0.01;

[Concurrent persist-on-serve still races on the WRITE] The TOCTOU guard at hfIdPersist.ts:11-16:

const current = readFileSync(filePath, "utf-8");
if (current === html) {
  writeFileSync(filePath, normalized, "utf-8");
}

Closes the read-modify-write race correctly between the server reading html and writing back, BUT not the read-write window itself. Between readFileSync(filePath) and writeFileSync(filePath, normalized), the user can still save fresh content that gets overwritten:

1. Server reads disk: current = "<v1>". current === html → true.
2. User saves "v2" to disk.
3. Server writes normalized (which is based on v1) — stomps user's v2.

Probably acceptable — the window is microseconds and the user would have to be saving in that exact instant. But it's worth documenting:

// Note: this is a best-effort guard. A user save in the microsecond window
// between readFileSync and writeFileSync would still be overwritten.
// For stronger guarantees, use an atomic write (write-temp + rename) and/or
// a per-project mutex.

Or actually implement the atomic write (writeFileSync to a sibling tempfile, then renameSync). That makes the write itself atomic on POSIX. Worth the 6-line patch if this code path is on a hot path for concurrent serves.

[hfIdPersist.ts:11 re-read happens even when idsAfter === idsBefore] Actually no, the re-read is inside the if (idsAfter > idsBefore) branch. ✅ Ignore this; misread on first pass.

Nits

[getElementTimings change is in #1292 but the contract test for it is in #1286] The #1286 test at L266-272 says:

expect(timings["hf-notimed"]).toBeDefined();
expect(timings["hf-notimed"].start).toBeUndefined();
expect(timings["hf-notimed"].end).toBeUndefined();

#1291's impl returns s !== null ? parseFloat(s) : undefined — for a missing attr, s === null, ternary picks undefined, test passes. #1292 changes it to Number.isFinite(sv) ? sv : undefined — for a missing attr, sv = parseFloat(null →"") = NaN, Number.isFinite(NaN) = false, picks undefined. Same result.

The behavioral difference is when data-start="abc" (non-numeric value). #1291 returns parseFloat("abc") = NaN as the value. #1292 returns undefined. The test in #1286 doesn't cover this case — so both impls pass — but #1292 is the safer shape (an undefined is easier for callers to handle than NaN).

Nit because it's an improvement worth keeping; no action needed.

[isVisible recreates getComputedStyle per call] Each elementAtPoint call walking ancestors calls isVisible on each, which calls getComputedStyle on each, which is non-trivial in real browsers (forces style recalc). For a deeply nested DOM, this could be noticeable. Microscopic in jsdom; possibly meaningful in a real Chrome.

Easy to defer — wait for a profiler to flag it. Mention only for awareness.

[draftMarkers.ts doesn't have a file header] Each new core file should arguably have a brief comment about what it is and why. Especially relevant here because the file lives in studio-api/helpers/ but is consumed cross-package by studio. A 2-line header like:

/**
 * Draft-marker constants shared between core's PreviewAdapter and Studio's
 * manual-edits code. CSS custom properties + the gesture marker attribute.
 */

…would help a future reader understand why these specific constants are factored out into a tiny file. Cosmetic.

Questions

[Was a separate-PR for the C1-C14 fixes considered?] Just confirming whether the bundling-into-#1292 was deliberate (e.g. the fixes touch lines that #1292's refactor also touches, so combining avoids merge conflicts) or just convenient. If it's the former, the body just needs to call out that intent. If it's the latter, splitting into a follow-up PR would be cleaner reviewability.

[STUDIO_PATH_OFFSET_ATTR, STUDIO_BOX_SIZE_ATTR, STUDIO_ROTATION_ATTR, STUDIO_ORIGINAL_TRANSLATE_ATTR] — these stay studio-side. Are any of them also relevant to PreviewAdapter? If a future R8/R9 wants to support rotation gestures or path-offset gestures through the same adapter, they'll need to move too. Not asking to pre-extract — just confirming the current 5-constant boundary is the right one for the R7 use case.

What I didn't verify

• That the studio's manualEditsDom.ts token-based gesture-marker logic stays compatible after the constants move. The grep confirmed all studio call sites still import from ./manualEditsTypes, so the symbols resolve identically — but the semantic of "the previewAdapter writes 'true' as the marker value while studio's setManualEditGesture writes a token" might mean the two systems don't recognize each other's markers as in-flight gestures. Worth a sanity check from someone who knows the studio-side gesture flow.
• That the regression-shards in CI pass on this PR. They're in_progress at write time.
• That STUDIO_MANUAL_EDIT_GESTURE_ATTR doesn't appear in any HTML fixtures or test snapshots (which would now reference the constant location-by-location). Greped only the source .ts/.tsx, didn't check .html fixtures or snapshot files.

Verdict

Constants extraction is clean and the C1-C14 fixes are all the right shape — each one addresses a real defect that the per-PR tests in #1289 or #1291 didn't catch. Main ask: update the PR title/body so reviewers see the substantive logic changes alongside the refactor. The threshold + concurrent-write notes are improvements I'd land in this PR; nits stay nits. Ready from where I sit; leaving as a comment.

— Review by Rames D Jusso

[miguel-heygen]

Building on Rames's review.

Rames's pass was thorough — walked all five substantive fixes from commit 2af2228, confirmed the constants boundary is right, and verified all studio call sites still resolve through manualEditsTypes. I agree with his verdict.

Amplifying: update the PR title/body. This is the one ask I'd call a merge-gate item — not because it affects the code, but because this PR is doing two distinct things (extract 5 constants, AND fix 5 correctness issues from R7 review), and the Graphite stack reviewer who approves merge-queue pickup will only see the title. A rubber-stamp on "refactor — extract constants" misses the behavioral changes entirely. The body update takes 2 minutes and removes real review risk.

Confirming: the studio gesture-marker value mismatch. Rames flagged that previewAdapter.ts writes "true" as the data-hf-studio-manual-edit-gesture attribute value, while studio's setManualEditGesture writes a token. If studio's gesture-detection code reads this attribute and compares against a specific token, the two systems don't recognize each other's in-flight gestures. Worth a grep of manualEditsDom.ts to confirm whether the attribute value is checked or only its presence.

New: concurrent-persist race during the writeFileSync. Rames's "microsecond window" note is accurate and worth the 6-line atomic-write patch (writeFileSync to .tmp, then renameSync). On POSIX, rename is atomic — the user's editor either sees the old file or the new file, never a partial write. Current code risks a 0-byte or partial-write scenario if the server crashes mid-write. Low-probability but the failure mode (corrupt source file) is severe enough to warrant the extra 6 lines.

[james-russo-rames-d-jusso]

Round 2 (delta 2af2228..d916901). Pulled the two new commits — 4afb5a7 fix(core): follow-up R7 review fixes and d916901 test(core): bundle-vs-disk id-stability test; comment double ensureHfIds (P3). Walked the delta against my round-1 review across all 4 stack members.

Everything I flagged is addressed in code form. Highlights:

• Bundle-vs-disk id divergence blocker (round-1 #1289) → pinned by a real regression test in preview.test.ts:363-391. Stubs adapter.bundle() to return the same untagged source HTML and asserts servedIds === diskIds. Locks in the content-hash invariant the architecture rests on — the exact shape I was worried might silently rot.
• Nested data-hf-root without data-hf-id (round-1 #1286) → new test at previewAdapter.test.ts:70-78 pins "treated same as outer stage root." Disambiguates the contract.
• atTime semantic ambiguity (round-1 #1286 + #1291) → test names updated to "...currently-computed opacity is 0 (atTime is a caller-seek hint, not evaluated by the adapter)" and "...inline opacity changes mid-drag", plus JSDoc on the PreviewAdapter interface at previewAdapter.ts:18-22 makes the caller-seek contract explicit. Right shape.
• Resize → move CSS-prop leak (round-1 #1291) → new test "resize → move switch clears width/height props — no cross-type prop leak" pins the auto-revert behavior.
• CSS.escape fallback in findById (round-1 #1291 nit) → exactly the regex-fallback pattern I suggested. ✅
• _opts → _perCallOpts rename (round-1 #1291 nit) → done.
• TOCTOU race-window comment (round-1 #1292 Concern) → explicit acknowledgment in hfIdPersist.ts:23-27 of the microsecond write-window. Documented-as-known rather than implemented-as-fixed — fine, the cost/benefit math holds for now.
• console.warn on disk-write failure (round-1 #1289 nit) → swapped the silent catch {} for catch (err) { console.warn(...) }. ✅
• hfIdPersist.ts invariant doc addressing the read-vs-construct hazard for callers — strong addition. The "must be the raw file content read from filePath just before this call" line will save a future caller from a silently-failing TOCTOU check.
• draftMarkers.ts file header (round-1 #1292 nit) → 2-line context comment added. ✅
• 0.01 opacity threshold comment (round-1 #1292 nit) → motivation now documented inline.
• Bonus test: "revertDraft after commitPreview is a no-op — does not restore stale translate" pins a related edge case I hadn't asked for but is right to lock down.

Two items intentionally not changed — both have defensible reasoning:

• #1289/#1291 SHAs are still my round-1 SHAs. The corresponding fixes still live only in #1292. So the "stack must merge atomically" property from my round-1 holds — relying on Graphite MQ to land all 4 together rather than splitting fixes back down to lower PRs. Defensible call given the Graphite workflow; just keep the MQ pickup happening as one unit.
• Atomic write (writeSync → rename) for the persist path — not added. The race-window comment documents the limitation, which is the cheaper path. If concurrent-serve traffic ever ramps and this code path becomes a contention site, switching to write-temp-then-rename is a 6-line patch. Currently fine.

Remaining

[PR title + body still say "refactor"] Title: refactor(core,studio): extract draft-marker constants to core (R7, Task 4). The third commit d916901 adds a new regression test (preview.test.ts:363-391) and the comment in preview.ts:241-245 documenting the load-bearing content-hash invariant — both are substantive changes beyond constants-extraction or the previous round of code-review fixes.

Still worth a one-line addition to the PR body so the MQ approver sees the whole scope:

Includes: constants extraction + 2 commits of code-review responses — TOCTOU guard, auto-revert on applyDraft re-apply, isVisible replacement, CSS.escape fallback, bundle-vs-disk id-stability test, double-ensureHfIds invariant doc.

Not blocking; just helps the next reviewer.

[Content-hash stability is now a wire contract — worth a comment at the mint site] The new preview.test.ts test pins "content-keyed minting is stable across mint contexts" — that's now load-bearing for the bundle-vs-disk path. If a future PR changes mintHfId in parsers/hfIds.ts (e.g. swap FNV1a for a different hash, change separator chars, modify contentKey to include positional info), this invariant breaks silently and the R7 drag-to-edit feature fails on any project where adapter.bundle() returns a pre-mint cache snapshot.

A 2-line comment at parsers/hfIds.ts near mintHfId would close this — something like:

// WIRE CONTRACT: this hash is content-keyed by design. R7's preview route relies
// on `mintHfId(el)` being identical across mint contexts (disk persist + bundle
// serve) — see preview.test.ts:363-391 ("bundle returning untagged HTML gets
// same ids as disk"). Changes here must preserve content-only keying.

(Could land in this PR's diff at hfIds.ts since the file is already touched, or a tiny follow-up PR. Either works.)

What I didn't verify

• That the new bundle-vs-disk test actually exercises the bundle-success path in preview.ts (not the fallback catch-block path). Reading the test, it sets bundle: async () => sourceHtml which returns truthy, so the if (!bundled) skip happens and the bundle-success path runs. ✅ — verified, comment retained for explicit confirmation.
• That createAdapter(projectDir, { bundle: async () => sourceHtml }) is a supported test API. Assumed it is given the test compiles + runs.
• That Tests on windows-latest (regression-shards in_progress at write time) all pass on this branch.

Verdict

Thorough round-2 response — every concern + nit from round 1 addressed in code, plus a regression test for the biggest architectural risk in the stack. The "wire contract" comment on mintHfId is the only thing I'd land in this PR before MQ pickup. The PR-body title is a should-do, not a must-do. Ready from where I sit; leaving as a comment.

— Review by Rames D Jusso

[jrusso1020]

Approved on behalf of James.

The base branch was changed.