mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-10 22:20:14 +00:00
refactor(core,studio): extract draft-marker constants + R7 code-review fixes (Task 4) (#1292)
* refactor(core,studio): extract draft-marker constants to core (R7, Task 4) Create draftMarkers.ts in core with 5 shared CSS custom property names and the gesture DOM attribute. PreviewAdapter imports from draftMarkers.ts instead of hardcoding strings. Adds @hyperframes/core/studio-api/draft-markers export subpath. Studio's manualEditsTypes.ts re-exports the shared constants from core so all existing call sites are unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): address R7 code-review findings (C1–C14, P6–P7) - previewAdapter: auto-revert previous gesture in applyDraft (C3); clearDraftProps on commitPreview not just revertDraft (C4); isVisible NaN→visible for JSDOM (P7); remove redundant GestureState.hfId field (C12); remove Array.from (C14); extract clearDraftProps/revertGesture helpers (C5/C6) - hfIdPersist: replace string-equality change detection with attribute count to avoid false-positive writes on single-quoted HTML (C1); re-read disk before write for TOCTOU guard (C7); remove normalizeHfIds wrapper (C11) - preview.ts: remove dead null-check on normalizedDisk after diskMain guard (C9); catch path re-reads disk fresh instead of using stale pre-request snapshot (C8) - hfIds.test.ts: replace tautological second stability test with cross-document content-keyed id stability test (P6) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): follow-up R7 review fixes — CSS.escape fallback, invariant docs, new edge-case tests - hfIdPersist: remove ensureHfIds re-export (P2); add JSDoc invariant note; improve TOCTOU comment; pass err to console.warn - preview.ts: split import — ensureHfIds from parsers/hfIds.js (not re-export) - previewAdapter: CSS.escape + inline fallback for non-browser environments; add JSDoc for atTime caller-seek contract; add 0.01 opacity-threshold comment - previewAdapter.test: 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 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(core): bundle-vs-disk id-stability test; comment double ensureHfIds (P3) - preview.test: add "bundle returning untagged HTML gets same ids as disk" test — guards against id divergence when bundler reads a pre-write cache snapshot; content-keyed FNV1a minting ensures served ids == disk ids for same source HTML - preview.ts: comment the second ensureHfIds call explaining it's intentional for adapter-injected elements and idempotent on the no-bundle path (P3 from miguel) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(core): wire-contract comment on mintHfId + fallow suppressions (R7) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Vance Ingalls
co-authored by
Claude Sonnet 4.6
Claude Sonnet 4.6
parent
1a23938bef
commit
3d7d7c0291
@@ -0,0 +1,10 @@
|
||||
/**
|
||||
* Draft-marker constants shared between core's PreviewAdapter and Studio's
|
||||
* manual-edits code. CSS custom properties written during a drag gesture, plus
|
||||
* the gesture marker attribute. Exported from @hyperframes/core/studio-api/draft-markers.
|
||||
*/
|
||||
export const STUDIO_OFFSET_X_PROP = "--hf-studio-offset-x";
|
||||
export const STUDIO_OFFSET_Y_PROP = "--hf-studio-offset-y";
|
||||
export const STUDIO_WIDTH_PROP = "--hf-studio-width";
|
||||
export const STUDIO_HEIGHT_PROP = "--hf-studio-height";
|
||||
export const STUDIO_MANUAL_EDIT_GESTURE_ATTR = "data-hf-studio-manual-edit-gesture";
|
||||
@@ -2,26 +2,7 @@ import { describe, it, expect, afterEach } from "vitest";
|
||||
import { mkdtempSync, writeFileSync, readFileSync, rmSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { normalizeHfIds, persistHfIdsIfNeeded } from "./hfIdPersist.js";
|
||||
|
||||
describe("normalizeHfIds", () => {
|
||||
it("marks changed=true and adds data-hf-id to all body elements when untagged", () => {
|
||||
const raw = `<!doctype html><html><body><div><p>hello</p></div></body></html>`;
|
||||
const { html, changed } = normalizeHfIds(raw);
|
||||
expect(changed).toBe(true);
|
||||
expect(html).toContain('data-hf-id="hf-');
|
||||
const matches = html.match(/data-hf-id="hf-[a-z0-9]{4}"/g);
|
||||
expect(matches?.length).toBeGreaterThanOrEqual(2);
|
||||
});
|
||||
|
||||
it("marks changed=false for already-normalized HTML (idempotent round-trip)", () => {
|
||||
const raw = `<!doctype html><html><body><div><p>hello</p></div></body></html>`;
|
||||
const first = normalizeHfIds(raw).html;
|
||||
const { html, changed } = normalizeHfIds(first);
|
||||
expect(changed).toBe(false);
|
||||
expect(html).toBe(first);
|
||||
});
|
||||
});
|
||||
import { persistHfIdsIfNeeded } from "./hfIdPersist.js";
|
||||
|
||||
describe("persistHfIdsIfNeeded", () => {
|
||||
const tmpDirs: string[] = [];
|
||||
@@ -59,6 +40,15 @@ describe("persistHfIdsIfNeeded", () => {
|
||||
expect(readFileSync(file, "utf-8")).toBe(diskAfterFirst);
|
||||
});
|
||||
|
||||
it("does not rewrite when source is already tagged with non-standard HTML formatting", () => {
|
||||
// Single-quoted attrs would cause a false-positive write under string-equality
|
||||
// change detection; count-based detection handles this correctly.
|
||||
const alreadyTagged = `<!doctype html><html><body><div data-hf-id='hf-ab12'>hello</div></body></html>`;
|
||||
const file = tmpFile(alreadyTagged);
|
||||
persistHfIdsIfNeeded(file, alreadyTagged);
|
||||
expect(readFileSync(file, "utf-8")).toBe(alreadyTagged);
|
||||
});
|
||||
|
||||
it("returned id matches id written to disk (serve-time == persist-time invariant)", () => {
|
||||
const raw = `<!doctype html><html><body><span>text</span></body></html>`;
|
||||
const file = tmpFile(raw);
|
||||
@@ -66,4 +56,16 @@ describe("persistHfIdsIfNeeded", () => {
|
||||
const onDisk = readFileSync(file, "utf-8");
|
||||
expect(result).toBe(onDisk);
|
||||
});
|
||||
|
||||
it("skips write if file was modified concurrently (TOCTOU guard)", () => {
|
||||
const old = `<!doctype html><html><body><div>original</div></body></html>`;
|
||||
const newer = `<!doctype html><html><body><div>modified by user</div></body></html>`;
|
||||
// Disk has newer content — simulates a concurrent save after the server read old.
|
||||
const file = tmpFile(newer);
|
||||
const returned = persistHfIdsIfNeeded(file, old);
|
||||
// Serve-time HTML gets ids based on what we read.
|
||||
expect(returned).toContain('data-hf-id="hf-');
|
||||
// Disk must not be overwritten — user's concurrent save is preserved.
|
||||
expect(readFileSync(file, "utf-8")).toBe(newer);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,20 +1,37 @@
|
||||
import { ensureHfIds } from "../../parsers/hfIds.js";
|
||||
import { writeFileSync } from "node:fs";
|
||||
|
||||
export { ensureHfIds };
|
||||
|
||||
export function normalizeHfIds(html: string): { html: string; changed: boolean } {
|
||||
const normalized = ensureHfIds(html);
|
||||
return { html: normalized, changed: normalized !== html };
|
||||
}
|
||||
import { readFileSync, writeFileSync } from "node:fs";
|
||||
|
||||
/**
|
||||
* Ensure `html` has `data-hf-id` attributes minted, and write the result back
|
||||
* to `filePath` if new ids were added.
|
||||
*
|
||||
* **Invariant:** `html` must be the raw file content read from `filePath` just
|
||||
* before this call. If `html` is constructed or transformed HTML the TOCTOU
|
||||
* guard (`current === html`) will never match and writes will silently be
|
||||
* skipped — no ids will reach disk.
|
||||
*/
|
||||
export function persistHfIdsIfNeeded(filePath: string, html: string): string {
|
||||
const { html: normalized, changed } = normalizeHfIds(html);
|
||||
if (changed) {
|
||||
const normalized = ensureHfIds(html);
|
||||
// Use attribute count instead of string equality: linkedom serialization may
|
||||
// normalize quote style and whitespace even when no ids were actually minted,
|
||||
// which would cause spurious writes on every request.
|
||||
const idsBefore = (html.match(/\bdata-hf-id=/g) ?? []).length;
|
||||
const idsAfter = (normalized.match(/\bdata-hf-id=/g) ?? []).length;
|
||||
if (idsAfter > idsBefore) {
|
||||
try {
|
||||
writeFileSync(filePath, normalized, "utf-8");
|
||||
} catch {
|
||||
// non-fatal — serve with ids even if persist fails
|
||||
// Re-read before writing to guard against concurrent user saves. If the
|
||||
// file changed since we read it, skip the write — serving with ids is
|
||||
// still correct; the next request will re-persist. Best-effort only: a
|
||||
// user save landing between readFileSync and writeFileSync below can
|
||||
// still be overwritten (microsecond window).
|
||||
const current = readFileSync(filePath, "utf-8");
|
||||
if (current === html) {
|
||||
writeFileSync(filePath, normalized, "utf-8");
|
||||
}
|
||||
} catch (err) {
|
||||
// Non-fatal — serve with ids even if the disk write fails (e.g. read-only
|
||||
// filesystem, sandboxed environment). Log so the failure is diagnosable.
|
||||
console.warn("[hyperframes] persistHfIdsIfNeeded: failed to write ids to disk:", err);
|
||||
}
|
||||
}
|
||||
return normalized;
|
||||
|
||||
@@ -64,11 +64,21 @@ describe("T10 — PreviewAdapter contract (spec for R7)", () => {
|
||||
expect(adapter.elementAtPoint(0, 0)).toBeNull();
|
||||
});
|
||||
|
||||
it("skips elements whose computed opacity is 0 at the given playhead time", () => {
|
||||
it("skips elements whose currently-computed opacity is 0 (atTime is a caller-seek hint, not evaluated by the adapter)", () => {
|
||||
const elem = make("div", { "data-hf-id": "hf-zzzz" }, { opacity: "0" });
|
||||
const adapter = adapterWith(() => elem);
|
||||
expect(adapter.elementAtPoint(0, 0, { atTime: 1.0 })).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null for nested data-hf-root without data-hf-id (treated same as outer stage root)", () => {
|
||||
const outerRoot = make("div", { "data-hf-root": "true" });
|
||||
const innerRoot = document.createElement("div");
|
||||
innerRoot.setAttribute("data-hf-root", "true");
|
||||
// no data-hf-id — no explicit id means no draggable target
|
||||
outerRoot.appendChild(innerRoot);
|
||||
const adapter = adapterWith(() => innerRoot);
|
||||
expect(adapter.elementAtPoint(0, 0)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
// ── applyDraft / revertDraft ───────────────────────────────────────────
|
||||
@@ -130,19 +140,44 @@ describe("T10 — PreviewAdapter contract (spec for R7)", () => {
|
||||
expect(target.style.getPropertyValue("--hf-studio-offset-y")).toBe("15px");
|
||||
});
|
||||
|
||||
it("resize → move switch clears width/height props — no cross-type prop leak", () => {
|
||||
const target = make("div", { "data-hf-id": "hf-aaaa" });
|
||||
const adapter = adapterWith(() => null);
|
||||
adapter.applyDraft({ type: "resize", hfId: "hf-aaaa", w: 200, h: 100 });
|
||||
adapter.applyDraft({ type: "move", hfId: "hf-aaaa", dx: 10, dy: 5 });
|
||||
// move props set
|
||||
expect(target.style.getPropertyValue("--hf-studio-offset-x")).toBe("10px");
|
||||
expect(target.style.getPropertyValue("--hf-studio-offset-y")).toBe("5px");
|
||||
// resize props cleared by the auto-revert before re-apply
|
||||
expect(target.style.getPropertyValue("--hf-studio-width")).toBe("");
|
||||
expect(target.style.getPropertyValue("--hf-studio-height")).toBe("");
|
||||
});
|
||||
|
||||
it("revertDraft after commitPreview is a no-op — does not restore stale translate", () => {
|
||||
const target = make("div", { "data-hf-id": "hf-aaaa" });
|
||||
target.style.setProperty("translate", "50px 0px");
|
||||
const adapter = adapterWith(() => null);
|
||||
adapter.applyDraft({ type: "move", hfId: "hf-aaaa", dx: 10, dy: 0 });
|
||||
adapter.commitPreview();
|
||||
// simulate caller applying translate after commit
|
||||
target.style.setProperty("translate", "10px 0px");
|
||||
adapter.revertDraft(); // no gesture in flight — should be no-op
|
||||
expect(target.style.getPropertyValue("translate")).toBe("10px 0px");
|
||||
});
|
||||
|
||||
it("revertDraft is safe to call when no gesture is in progress (idempotent / no-op on empty marker)", () => {
|
||||
const adapter = adapterWith(() => null);
|
||||
expect(() => adapter.revertDraft()).not.toThrow();
|
||||
expect(() => adapter.revertDraft()).not.toThrow();
|
||||
});
|
||||
|
||||
it("elementAtPoint filtering is stable when playhead changes mid-drag — opacity re-evaluated per call", () => {
|
||||
it("elementAtPoint filtering is stable when inline opacity changes mid-drag — computed style re-evaluated per call", () => {
|
||||
const elem = make("div", { "data-hf-id": "hf-zzzz" });
|
||||
const adapter = adapterWith(() => elem);
|
||||
expect(adapter.elementAtPoint(0, 0, { atTime: 0 })).toBe(elem);
|
||||
expect(adapter.elementAtPoint(0, 0)).toBe(elem);
|
||||
// simulates GSAP seeking to a time where the element is hidden
|
||||
elem.style.setProperty("opacity", "0");
|
||||
expect(adapter.elementAtPoint(0, 0, { atTime: 1.0 })).toBeNull();
|
||||
expect(adapter.elementAtPoint(0, 0)).toBeNull();
|
||||
});
|
||||
|
||||
it("stage-root exclusion applies only to the outermost data-hf-root; nested sub-composition roots count as targets", () => {
|
||||
|
||||
@@ -1,3 +1,11 @@
|
||||
import {
|
||||
STUDIO_OFFSET_X_PROP,
|
||||
STUDIO_OFFSET_Y_PROP,
|
||||
STUDIO_WIDTH_PROP,
|
||||
STUDIO_HEIGHT_PROP,
|
||||
STUDIO_MANUAL_EDIT_GESTURE_ATTR,
|
||||
} from "./draftMarkers.js";
|
||||
|
||||
export type DraftPayload =
|
||||
| { type: "move"; hfId: string; dx: number; dy: number }
|
||||
| { type: "resize"; hfId: string; w: number; h: number };
|
||||
@@ -7,6 +15,11 @@ export type CommitPatch =
|
||||
| { type: "resize"; hfId: string; width: number; height: number };
|
||||
|
||||
export interface PreviewAdapter {
|
||||
/**
|
||||
* @param atTime - Caller hint only. The adapter reads current computed styles;
|
||||
* the caller must seek the GSAP timeline to `atTime` before invoking so that
|
||||
* GSAP-driven inline styles reflect the desired playhead position.
|
||||
*/
|
||||
elementAtPoint(x: number, y: number, opts?: { atTime?: number }): Element | null;
|
||||
applyDraft(payload: DraftPayload): void;
|
||||
revertDraft(): void;
|
||||
@@ -15,7 +28,6 @@ export interface PreviewAdapter {
|
||||
}
|
||||
|
||||
interface GestureState {
|
||||
hfId: string;
|
||||
payload: DraftPayload;
|
||||
originalTranslate: string | undefined;
|
||||
}
|
||||
@@ -27,24 +39,50 @@ export function createPreviewAdapter(
|
||||
let gesture: GestureState | null = null;
|
||||
|
||||
function findById(hfId: string): HTMLElement | null {
|
||||
return doc.querySelector(`[data-hf-id="${hfId}"]`) as HTMLElement | null;
|
||||
// CSS.escape is available in browsers; hf-ids are always hf-[a-z0-9]+ so
|
||||
// no escaping is strictly needed, but be safe in non-browser environments.
|
||||
const escaped =
|
||||
typeof CSS !== "undefined" && typeof CSS.escape === "function"
|
||||
? CSS.escape(hfId)
|
||||
: hfId.replace(/([^\w-])/g, "\\$1");
|
||||
return doc.querySelector(`[data-hf-id="${escaped}"]`) as HTMLElement | null;
|
||||
}
|
||||
|
||||
function opacity(el: Element): number {
|
||||
function isVisible(el: Element): boolean {
|
||||
const view = doc.defaultView;
|
||||
if (!view) return 1;
|
||||
return parseFloat(view.getComputedStyle(el).opacity) || 0;
|
||||
if (!view) return true;
|
||||
const style = view.getComputedStyle(el);
|
||||
if (style.display === "none" || style.visibility === "hidden") return false;
|
||||
const op = parseFloat(style.opacity);
|
||||
// NaN (empty string from environments with no CSS cascade) → treat as visible.
|
||||
// 0.01 threshold: sub-1% opacity is not user-targetable in drag gestures.
|
||||
return Number.isNaN(op) || op >= 0.01;
|
||||
}
|
||||
|
||||
function clearDraftProps(target: HTMLElement): void {
|
||||
target.style.removeProperty(STUDIO_OFFSET_X_PROP);
|
||||
target.style.removeProperty(STUDIO_OFFSET_Y_PROP);
|
||||
target.style.removeProperty(STUDIO_WIDTH_PROP);
|
||||
target.style.removeProperty(STUDIO_HEIGHT_PROP);
|
||||
target.removeAttribute(STUDIO_MANUAL_EDIT_GESTURE_ATTR);
|
||||
}
|
||||
|
||||
function revertGesture(target: HTMLElement, state: GestureState): void {
|
||||
clearDraftProps(target);
|
||||
if (state.originalTranslate !== undefined) {
|
||||
target.style.setProperty("translate", state.originalTranslate);
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
elementAtPoint(x, y, _opts) {
|
||||
elementAtPoint(x, y, _perCallOpts) {
|
||||
const hit = opts?.resolvePoint?.(x, y) ?? null;
|
||||
if (!hit) return null;
|
||||
|
||||
let el: Element | null = hit;
|
||||
while (el && el !== doc.body) {
|
||||
if (el.hasAttribute("data-hf-id")) {
|
||||
return opacity(el) === 0 ? null : (el as HTMLElement);
|
||||
return isVisible(el) ? (el as HTMLElement) : null;
|
||||
}
|
||||
// data-hf-root without data-hf-id = outermost stage root — stop
|
||||
if (el.hasAttribute("data-hf-root")) return null;
|
||||
@@ -54,64 +92,63 @@ export function createPreviewAdapter(
|
||||
},
|
||||
|
||||
applyDraft(payload) {
|
||||
// Auto-revert any in-flight gesture before starting a new one so no
|
||||
// element is left with orphaned draft CSS props or the gesture marker.
|
||||
if (gesture) {
|
||||
const prev = findById(gesture.payload.hfId);
|
||||
if (prev) revertGesture(prev, gesture);
|
||||
gesture = null;
|
||||
}
|
||||
|
||||
const target = findById(payload.hfId);
|
||||
if (!target) return;
|
||||
|
||||
const originalTranslate = target.style.getPropertyValue("translate") || undefined;
|
||||
gesture = { hfId: payload.hfId, payload, originalTranslate };
|
||||
target.setAttribute("data-hf-studio-manual-edit-gesture", "true");
|
||||
gesture = { payload, originalTranslate };
|
||||
target.setAttribute(STUDIO_MANUAL_EDIT_GESTURE_ATTR, "true");
|
||||
|
||||
if (payload.type === "move") {
|
||||
target.style.setProperty("--hf-studio-offset-x", `${payload.dx}px`);
|
||||
target.style.setProperty("--hf-studio-offset-y", `${payload.dy}px`);
|
||||
target.style.setProperty(STUDIO_OFFSET_X_PROP, `${payload.dx}px`);
|
||||
target.style.setProperty(STUDIO_OFFSET_Y_PROP, `${payload.dy}px`);
|
||||
} else {
|
||||
target.style.setProperty("--hf-studio-width", `${payload.w}px`);
|
||||
target.style.setProperty("--hf-studio-height", `${payload.h}px`);
|
||||
target.style.setProperty(STUDIO_WIDTH_PROP, `${payload.w}px`);
|
||||
target.style.setProperty(STUDIO_HEIGHT_PROP, `${payload.h}px`);
|
||||
}
|
||||
},
|
||||
|
||||
revertDraft() {
|
||||
if (!gesture) return;
|
||||
const target = findById(gesture.hfId);
|
||||
if (target) {
|
||||
target.style.removeProperty("--hf-studio-offset-x");
|
||||
target.style.removeProperty("--hf-studio-offset-y");
|
||||
target.style.removeProperty("--hf-studio-width");
|
||||
target.style.removeProperty("--hf-studio-height");
|
||||
target.removeAttribute("data-hf-studio-manual-edit-gesture");
|
||||
if (gesture.originalTranslate !== undefined) {
|
||||
target.style.setProperty("translate", gesture.originalTranslate);
|
||||
}
|
||||
}
|
||||
const target = findById(gesture.payload.hfId);
|
||||
if (target) revertGesture(target, gesture);
|
||||
gesture = null;
|
||||
},
|
||||
|
||||
commitPreview() {
|
||||
if (!gesture) return null;
|
||||
const { hfId, payload } = gesture;
|
||||
const { payload } = gesture;
|
||||
|
||||
const target = findById(hfId);
|
||||
if (target) {
|
||||
target.removeAttribute("data-hf-studio-manual-edit-gesture");
|
||||
}
|
||||
const target = findById(payload.hfId);
|
||||
if (target) clearDraftProps(target);
|
||||
gesture = null;
|
||||
|
||||
if (payload.type === "move") {
|
||||
return { type: "moveElement", hfId, dx: payload.dx, dy: payload.dy };
|
||||
return { type: "moveElement", hfId: payload.hfId, dx: payload.dx, dy: payload.dy };
|
||||
}
|
||||
return { type: "resize", hfId, width: payload.w, height: payload.h };
|
||||
return { type: "resize", hfId: payload.hfId, width: payload.w, height: payload.h };
|
||||
},
|
||||
|
||||
getElementTimings() {
|
||||
const result: Record<string, { start?: number; end?: number }> = {};
|
||||
for (const el of Array.from(doc.querySelectorAll("[data-hf-id]"))) {
|
||||
for (const el of doc.querySelectorAll("[data-hf-id]")) {
|
||||
const hfId = el.getAttribute("data-hf-id");
|
||||
if (!hfId) continue;
|
||||
const s = el.getAttribute("data-start");
|
||||
const e = el.getAttribute("data-end");
|
||||
const sv = s !== null ? parseFloat(s) : NaN;
|
||||
const ev = e !== null ? parseFloat(e) : NaN;
|
||||
result[hfId] = {
|
||||
start: s !== null ? parseFloat(s) : undefined,
|
||||
end: e !== null ? parseFloat(e) : undefined,
|
||||
start: Number.isFinite(sv) ? sv : undefined,
|
||||
end: Number.isFinite(ev) ? ev : undefined,
|
||||
};
|
||||
}
|
||||
return result;
|
||||
|
||||
Reference in New Issue
Block a user