diff --git a/.fork/customizations.yaml b/.fork/customizations.yaml index 56333fc30ec5..e2c6fec606e1 100644 --- a/.fork/customizations.yaml +++ b/.fork/customizations.yaml @@ -1738,7 +1738,10 @@ through the window.__T3_DESIGN_MODE__ command handle (designModeBridge.ts, fire-and-forget except buildSend; the scrub-driven writes applyDraft and setInset coalesce host-side to one IPC crossing per - animation frame, and every other command flushes them first so order holds). + animation frame, every other command flushes them first so order holds, and + hoverElement dedupes against the last id sent — mouseover bubbles, so the + layers rail fires it several times per row crossed and hover carries no new + information on a repeat). Panel section order and field chrome follow the fork's own Figma spec (file fZRyRTZJtLKwuq8rj2SLg4, page V2, node 193:9686): a Position section (the six align verbs over an X/Y pair, absolute-position toggle in the header), a @@ -1831,12 +1834,28 @@ and ChatView's fenced send path appends the full deterministic token-aware change-request markdown to the outgoing message inside blocks — cleared only when the turn start - succeeds (T3 threads ARE the delivery surface — the Forge's own chat + succeeds, and then only the attachments that actually rode the message: + the ids are captured before the start is awaited, so a Send made from + the panel during that round trip is not dropped unsent (T3 threads ARE + the delivery surface — the Forge's own chat feed, embedded sessions, MCP queue, watcher, verifier and every /__the-forge/* endpoint are deliberately not ported, and the engine must stay free of network calls). The host re-injects on dom-ready while the mode is on, so navigations and dev-server reloads keep the - engine and its drafts. Element→source mapping works WITHOUT any + engine and its drafts — and it reconciles on every bridge attach too + (probe the guest's protocol version, re-inject when it is absent or + skewed, otherwise just re-activate), because this component unmounts + whenever the right panel shows a terminal or a diff and a reload in + that window would otherwise leave the mode "on" over a page with no + engine, every command swallowed and Send answering "no changes" over + intact drafts. Restored drafts carry their ORIGINAL on the wire rather + than re-deriving it: the toggle destroys the engine but leaves the + previews painted, so the store's default prior oracle (the element's + live inline style) would capture each draft's own value as the page's, + making Discard restore the draft over itself and the send builder + measure before === after. Closing a preview tab is the one moment the + per-tab state (store entry, undo history, bridge hover memo) is dropped + — a mere unmount must survive. Element→source mapping works WITHOUT any project setup: project-authored forge-mode JSX tags (data-dc-source) remain the most precise source and always win per element, but on untagged React pages the desktop preload's react-grab-backed resolver @@ -1995,6 +2014,8 @@ - apps/web/src/custom/designMode/panel/LayerTypeIcon.tsx - apps/web/src/custom/designMode/designChangeTranscript.ts - apps/web/src/custom/designMode/layersDrag.ts + - apps/web/src/custom/designMode/layersDrag.test.ts + - apps/web/src/custom/designMode/designModeTabLifetime.ts - apps/web/src/custom/designMode/panel/ForkDesignPanel.tsx - apps/web/src/custom/designMode/panel/CanvasControls.tsx - apps/web/src/custom/designMode/panel/canvasResolutions.ts @@ -2056,6 +2077,11 @@ # under ElectronBrowserHost, outside the preview panel's subtree, so no # descendant selector rooted at the panel can reach it. - apps/web/src/browser/HostedBrowserWebview.tsx + # Fenced per-tab teardown in the lease's close path. This is the only place + # that knows a preview tab is CLOSED rather than merely unmounted, so it is + # where the tab's design-mode store entry, undo history and bridge memo go — + # everything else in the feature sees an unmount it must survive. + - apps/web/src/browser/desktopTabLifetime.ts # Fenced mock adjustment: trailingActions is a fragment (toggle + menu) # under the fork, so the mock finds the menu's props on a fragment child. - apps/web/src/components/preview/PreviewView.test.tsx diff --git a/apps/web/src/__fork_guards__/forkDesignMode.test.ts b/apps/web/src/__fork_guards__/forkDesignMode.test.ts index f38272b28148..d7c182088b03 100644 --- a/apps/web/src/__fork_guards__/forkDesignMode.test.ts +++ b/apps/web/src/__fork_guards__/forkDesignMode.test.ts @@ -76,6 +76,40 @@ describe("fork guard: design mode", () => { expect(previewView).not.toContain("ForkLayersTree"); }); + it("reconciles the guest on every bridge attach, and forgets a closed tab", () => { + // Injection used to happen on the toggle and on `dom-ready` only. This component unmounts + // whenever the right panel shows a terminal or a diff, or the user switches threads — and + // a page reload in that window (a non-HMR-able agent edit: the feature's own loop) wiped + // the guest with nobody listening, leaving Design mode "on" over a page with no engine. + const toggle = read("src/custom/designMode/ForkPreviewDesignMode.tsx"); + expect(toggle).toContain("reconcileEngine"); + expect(toggle).toContain("engineIsCurrent"); + expect(toggle).toContain("if (enabledRef.current) void reconcileEngine(runtimeTabId)"); + + // A re-injection invalidates every host memo keyed on guest ids, so injection owns those + // clears rather than each call site remembering them. + expect(toggle).toContain("designUndoHistory.clear(tabId)"); + expect(toggle).toContain("designModeBridge.forgetHover(tabId)"); + // ...and the reconcile re-checks the toggle after its probe's round trip, or a toggle-off + // inside that window would be undone by the injection that follows. + expect(toggle).toContain("if (!enabledRef.current) return;"); + + // The counterpart: the one place that knows a preview tab is CLOSED rather than merely + // unmounted. Without it `designModeStore.remove` had no call site at all. The lease makes + // ONE call — what gets released is the feature's own business, so the next per-tab memo + // does not grow another line in an upstream file. + const tabLifetime = read("src/browser/desktopTabLifetime.ts"); + expect(tabLifetime).toContain("fork:begin fork-design-mode"); + expect(tabLifetime).toContain("disposeDesignModeTab(tabId)"); + for (const internal of ["useDesignModeStore", "designUndoHistory", "designModeBridge"]) { + expect(tabLifetime).not.toContain(internal); + } + const disposal = read("src/custom/designMode/designModeTabLifetime.ts"); + expect(disposal).toContain("useDesignModeStore.getState().remove(runtimeTabId)"); + expect(disposal).toContain("designUndoHistory.clear(runtimeTabId)"); + expect(disposal).toContain("designModeBridge.forgetTab(runtimeTabId)"); + }); + it("commits the screen's real width and derives a height that fills the pane", () => { // The whole point: the guest's CSS viewport width IS the screen's, so a page that hides // content below a breakpoint sees the screen and not however wide the pane happens to @@ -260,13 +294,18 @@ describe("fork guard: design mode", () => { 'import { forkDesignChanges } from "~/custom/designMode/designChangeDraftStore"', ); expect(chatView).toContain("forkDesignChanges.count({ environmentId, threadId:"); - expect(chatView).toContain("forkDesignChanges.appendToPrompt("); + // ONE read: the outgoing text and the entries that went into it come back together, so + // "what rode the message" is not an invariant ChatView holds by hand across the await. expect(chatView).toContain( - "messageTextForSendWithDesignChanges || IMAGE_ONLY_BOOTSTRAP_PROMPT", + "forkDesignChanges.takeForSend(forkDesignChangeRef, messageTextForSend)", ); + expect(chatView).toContain("forkDesignSend.text || IMAGE_ONLY_BOOTSTRAP_PROMPT"); + // Cleared by ENTRY, not by id — a re-send during the awaited turn start replaces the pill + // in place under the same id, so only identity distinguishes it from what was sent. expect(chatView).toContain( - "if (turnStartSucceeded) forkDesignChanges.clear(forkDesignChangeRef)", + "if (turnStartSucceeded) forkDesignChanges.clear(forkDesignChangeRef, forkDesignSend.sent)", ); + expect(chatView).not.toContain("pendingIds"); }); it("renders sent design changes as transcript chips, not raw markdown", () => { @@ -417,6 +456,102 @@ describe("fork guard: design mode", () => { expect(budget).toEqual({ left: 7, truncated: true }); }); + it("restores a persisted draft's ORIGINAL rather than re-deriving it", async () => { + // The engine's own restore contract, and the one place it can be wrong invisibly. + // + // Toggling Design mode off destroys the engine but deliberately leaves the draft previews + // painted as inline styles (they come back from sessionStorage). So when the next + // injection re-applies them into the SAME document, DraftStore's default prior oracle — + // which for a css draft reads the element's live inline style — answers with the previous + // engine's own preview. Every restored draft would then record its drafted value as the + // page's original: Discard restores the draft over itself, and the send builder measures + // before === after and drops the change, so the panel counts N changes while Send says + // there is nothing to send. The persisted third tuple slot is what closes it. + const result = await build({ + stdin: { + contents: [ + 'export { DraftStore } from "./src/custom/designMode/engine/vendor/drafts";', + 'export { loadLifecycle } from "./src/custom/designMode/engine/vendor/lifecycle-store";', + ].join("\n"), + resolveDir: webRoot, + sourcefile: "design-mode-drafts-guard.ts", + loader: "ts", + }, + bundle: true, + format: "esm", + platform: "node", + target: "es2022", + write: false, + logLevel: "silent", + }); + const code = result.outputFiles[0]?.text ?? ""; + const moduleUrl = `data:text/javascript;base64,${NodeBuffer.Buffer.from(code).toString("base64")}`; + const engine = (await import(moduleUrl)) as { + DraftStore: new () => { + apply: (el: unknown, prop: string, value: string, knownOriginal?: string) => void; + discard: (el: unknown, props?: string[]) => void; + entries: () => Map>; + }; + loadLifecycle: (storage: unknown) => { drafts: unknown[] } | null; + }; + + // Just enough element for the css half of the store: it only ever reads and writes + // inline style declarations. + const element = () => { + const inline = new Map(); + return { + inline, + style: { + setProperty: (key: string, value: string) => inline.set(key, value), + removeProperty: (key: string) => inline.delete(key), + getPropertyValue: (key: string) => inline.get(key) ?? "", + getPropertyPriority: () => "", + }, + }; + }; + + // A restore into a document still showing the previous engine's preview. + const restored = element(); + restored.style.setProperty("padding-top", "32px"); + const store = new engine.DraftStore(); + store.apply(restored, "padding-top", "32px", ""); + expect(store.entries().get(restored)?.get("padding-top")?.original).toBe(""); + store.discard(restored, ["padding-top"]); + expect(restored.inline.has("padding-top")).toBe(false); + + // The default oracle on the same shape — the behaviour the persisted original exists to + // avoid, pinned here so nobody "simplifies" the parameter away. + const rederived = element(); + rederived.style.setProperty("padding-top", "32px"); + const naive = new engine.DraftStore(); + naive.apply(rederived, "padding-top", "32px"); + naive.discard(rederived, ["padding-top"]); + expect(rederived.inline.get("padding-top")).toBe("32px"); + + // A first-time draft is unaffected: no inline style, so the original is empty either way. + const fresh = element(); + const first = new engine.DraftStore(); + first.apply(fresh, "padding-top", "32px"); + expect(first.entries().get(fresh)?.get("padding-top")?.original).toBe(""); + + // The wire shape: triples load, pre-upgrade 2-tuples still load (a session in flight must + // not be thrown away), and a non-string original is rejected — it would be handed + // straight to setProperty on discard. + const stored = (drafts: unknown) => ({ + getItem: () => JSON.stringify({ v: 1, designModeOn: true, selection: [], drafts, sent: [] }), + }); + const entry = (props: unknown) => ({ dcSource: "App.tsx:1:1", index: 0, props }); + expect( + engine.loadLifecycle(stored([entry([["padding-top", "32px", "24px"]])]))?.drafts, + ).toEqual([entry([["padding-top", "32px", "24px"]])]); + expect(engine.loadLifecycle(stored([entry([["padding-top", "32px"]])]))?.drafts).toHaveLength( + 1, + ); + expect(engine.loadLifecycle(stored([entry([["padding-top", "32px", 24]])]))?.drafts).toEqual( + [], + ); + }); + it("keeps the native source bridge contract aligned across preload and engine", () => { // The desktop preload installs the resolver global; the engine consumes it by the // same name. A drifted literal on either side silently degrades every untagged React diff --git a/apps/web/src/browser/desktopTabLifetime.ts b/apps/web/src/browser/desktopTabLifetime.ts index 98dffda0ea0e..f9022ee96ebe 100644 --- a/apps/web/src/browser/desktopTabLifetime.ts +++ b/apps/web/src/browser/desktopTabLifetime.ts @@ -1,5 +1,9 @@ import { previewBridge } from "~/components/preview/previewBridge"; +/* fork:begin fork-design-mode — see .fork/customizations.yaml#fork-design-mode */ +import { disposeDesignModeTab } from "~/custom/designMode/designModeTabLifetime"; +/* fork:end fork-design-mode */ + import { stopBrowserRecording } from "./browserRecording"; interface DesktopTabLease { @@ -59,6 +63,13 @@ export function acquireDesktopTab(tabId: string): AcquiredDesktopTab { const latest = leases.get(tabId); if (!latest || latest.references > 0) return; leases.delete(tabId); + /* fork:begin fork-design-mode — see .fork/customizations.yaml#fork-design-mode + The tab's webview is about to be destroyed, taking the guest engine, its drafts and + its id registry with it. This is the only place that knows a preview tab is CLOSED + rather than merely unmounted, which is why the call hangs here — WHAT gets released + is the feature's own business (designModeTabLifetime.ts). */ + disposeDesignModeTab(tabId); + /* fork:end fork-design-mode */ void enqueueDesktopTabOperation(tabId, async () => { await stopBrowserRecording(tabId).catch(() => null); await previewBridge?.closeTab(tabId); diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index f9466c30c80c..84b816ed2f00 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -4782,10 +4782,7 @@ function ChatViewContent(props: ChatViewProps) { Design-change attachments append their full change-request markdown here (the composer only ever showed the pill); cleared below once the turn start succeeds. */ const forkDesignChangeRef = { environmentId, threadId: threadIdForSend }; - const messageTextForSendWithDesignChanges = forkDesignChanges.appendToPrompt( - forkDesignChangeRef, - messageTextForSend, - ); + const forkDesignSend = forkDesignChanges.takeForSend(forkDesignChangeRef, messageTextForSend); /* fork:end fork-design-mode */ const messageIdForSend = newMessageId(); const messageCreatedAt = new Date().toISOString(); @@ -4795,7 +4792,7 @@ function ChatViewContent(props: ChatViewProps) { models: ctxSelectedProviderModels, effort: ctxSelectedPromptEffort, /* fork:begin fork-design-mode — see .fork/customizations.yaml#fork-design-mode */ - text: messageTextForSendWithDesignChanges || IMAGE_ONLY_BOOTSTRAP_PROMPT, + text: forkDesignSend.text || IMAGE_ONLY_BOOTSTRAP_PROMPT, /* fork:end fork-design-mode */ }); const turnAttachmentsPromise = Promise.all( @@ -5033,8 +5030,11 @@ function ChatViewContent(props: ChatViewProps) { } /* fork:begin fork-design-mode — see .fork/customizations.yaml#fork-design-mode The design-change attachments rode the sent message; a failed send keeps the - pills so nothing is lost. */ - if (turnStartSucceeded) forkDesignChanges.clear(forkDesignChangeRef); + pills so nothing is lost. Clears exactly the entries takeForSend returned, not the + thread: the turn start is awaited above, and a Send from the design panel during + that window replaces a pill IN PLACE under the same id — so only entry identity + tells the two apart. */ + if (turnStartSucceeded) forkDesignChanges.clear(forkDesignChangeRef, forkDesignSend.sent); /* fork:end fork-design-mode */ }; diff --git a/apps/web/src/custom/designMode/ForkPreviewDesignMode.tsx b/apps/web/src/custom/designMode/ForkPreviewDesignMode.tsx index 30c36933b7a7..4a153fdbd69e 100644 --- a/apps/web/src/custom/designMode/ForkPreviewDesignMode.tsx +++ b/apps/web/src/custom/designMode/ForkPreviewDesignMode.tsx @@ -25,17 +25,60 @@ interface Props { * `.fork/customizations.yaml#fork-design-mode`. */ export function ForkPreviewDesignMode({ runtimeTabId, disabled }: Props) { - const tabState = useDesignModeStore((state) => selectDesignModeTab(state.byTabId, runtimeTabId)); + // Only `enabled` — this button re-rendered on every selection, layers and canvas message + // otherwise (up to ~4Hz while an agent edits the previewed page) to draw the same icon. + const enabled = useDesignModeStore( + (state) => selectDesignModeTab(state.byTabId, runtimeTabId).enabled, + ); const enabledRef = useRef(false); - enabledRef.current = tabState.enabled; + enabledRef.current = enabled; const injectEngine = useCallback(async (tabId: string) => { const webview = findPreviewWebview(tabId); if (!webview) throw new Error("Preview webview not found"); + // The incoming engine mints a fresh id registry (ids restart at 1) and paints no outline, + // so every host-side memo keyed on the old ids is about to name something else: the undo + // history's entries, and the bridge's last-hover dedupe — which would otherwise swallow the + // hover of whichever row draws the id it last sent (PR #74 review). Cleared here rather + // than at each call site so a future injection path cannot forget either. + designUndoHistory.clear(tabId); + designModeBridge.forgetHover(tabId); const { default: engineCode } = await import("virtual:fork-design-mode-engine"); await webview.executeJavaScript(engineCode, false); }, []); + /** + * Brings the guest back in line with what this tab's store says. + * + * The bridge attaches on mount, but injection only ever happened on the toggle and on + * `dom-ready` — and this component unmounts whenever the right panel shows a terminal or a + * diff, or the user switches threads. A full page reload in that window (exactly what a + * non-HMR-able agent edit causes, i.e. the feature's own loop) wiped the guest's globals + * with nobody listening for `dom-ready`, and the panel came back reporting Design mode on + * over a page with no engine: commands vanished into `fire`'s catch and Send answered "no + * changes" while the drafts sat untouched in the guest's sessionStorage. + * + * A live same-version engine is left alone apart from `setActive(true)`, which is the + * engine's own re-emit path (headlessMode's idempotent re-activation clears the selection + * gate and pushes a fresh snapshot), so the panel re-syncs without transferring the bundle. + */ + const reconcileEngine = useCallback( + async (tabId: string) => { + const current = await designModeBridge.engineIsCurrent(tabId); + // Re-checked after the probe's round trip: a toggle-off landing inside that window has + // already destroyed the engine and cleared the store, so reconciling on the stale answer + // would inject, boot would emit `state { active: true }`, and Design mode would turn + // itself back on under the user (PR #74 review). + if (!enabledRef.current) return; + if (current) { + designModeBridge.setActive(tabId, true); + return; + } + await injectEngine(tabId); + }, + [injectEngine], + ); + // Bridge + re-injection listeners live on the webview element itself (it outlives this // component's mounts). Attached only while the chrome row is mounted for this tab. useEffect(() => { @@ -87,8 +130,10 @@ export function ForkPreviewDesignMode({ runtimeTabId, disabled }: Props) { }; // A navigation (or dev-server full reload) wipes the guest's globals — put the engine - // back whenever design mode is meant to be on for this tab. The undo history dies - // with the old document: its entries name ids from the previous injection's registry. + // back whenever design mode is meant to be on for this tab. The undo history dies with + // the old document whether or not we re-inject (its entries name ids from the previous + // injection's registry), which is why the clear here is unconditional and not left to + // injectEngine's own. const onDomReady = () => { designUndoHistory.clear(runtimeTabId); if (!enabledRef.current) return; @@ -114,6 +159,8 @@ export function ForkPreviewDesignMode({ runtimeTabId, disabled }: Props) { } webview.addEventListener("console-message", onConsoleMessage); webview.addEventListener("dom-ready", onDomReady); + // The listeners alone do not make the guest agree with us — see reconcileEngine. + if (enabledRef.current) void reconcileEngine(runtimeTabId).catch(() => undefined); }; attach(); return () => { @@ -123,14 +170,15 @@ export function ForkPreviewDesignMode({ runtimeTabId, disabled }: Props) { webview.removeEventListener("dom-ready", onDomReady); } }; - }, [injectEngine, runtimeTabId]); + }, [injectEngine, reconcileEngine, runtimeTabId]); const handleToggle = useCallback(() => { if (!runtimeTabId) return; const store = useDesignModeStore.getState(); if (enabledRef.current) { - // Destroy (not just deactivate): drafts persist in the guest's sessionStorage and - // are restored on the next injection, so tearing the overlay down loses nothing. + // Destroy (not just deactivate): drafts persist in the guest's sessionStorage — with + // their originals, so the next injection restores them faithfully rather than + // capturing the previews this teardown leaves painted (lifecycle-store.ts's `props`). // The undo history does NOT survive the id registry it names — clear it. designUndoHistory.clear(runtimeTabId); designModeBridge.destroy(runtimeTabId); @@ -154,23 +202,23 @@ export function ForkPreviewDesignMode({ runtimeTabId, disabled }: Props) { } > - + {disabled ? "Design mode needs a loaded page" - : tabState.enabled + : enabled ? "Exit design mode" : "Design mode — click elements to edit, send changes to the agent"} diff --git a/apps/web/src/custom/designMode/designChangeDraftStore.test.ts b/apps/web/src/custom/designMode/designChangeDraftStore.test.ts index bbcfe947855c..f2c3afb78030 100644 --- a/apps/web/src/custom/designMode/designChangeDraftStore.test.ts +++ b/apps/web/src/custom/designMode/designChangeDraftStore.test.ts @@ -161,13 +161,71 @@ describe("designChangeDraftStore", () => { expect(pendingFor(OTHER_THREAD)).toHaveLength(1); }); + it("takeForSend returns the text and the entries in one read", () => { + const { add } = useDesignChangeDraftStore.getState(); + add(THREAD, "tab-a", payload({ markdown: "# one" })); + add(THREAD, "tab-b", payload({ markdown: "# two" })); + + const taken = forkDesignChanges.takeForSend(THREAD, "make it pop"); + expect(taken.sent).toHaveLength(2); + expect(extractTrailingDesignChanges(taken.text).blocks).toEqual(["# one", "# two"]); + // Reading is not taking — the pills survive until the turn start succeeds. + expect(pendingFor(THREAD)).toHaveLength(2); + }); + + it("leaves the text untouched when nothing is pending", () => { + const taken = forkDesignChanges.takeForSend(THREAD, "just a message"); + expect(taken.text).toBe("just a message"); + expect(taken.sent).toHaveLength(0); + }); + + it("clears only what the send carried, so a Send from another tab mid-flight survives", () => { + const { add } = useDesignChangeDraftStore.getState(); + add(THREAD, "tab-a", payload({ markdown: "rode along" })); + const taken = forkDesignChanges.takeForSend(THREAD, ""); + + add(THREAD, "tab-b", payload({ markdown: "arrived mid-flight" })); + forkDesignChanges.clear(THREAD, taken.sent); + + const pending = pendingFor(THREAD); + expect(pending).toHaveLength(1); + expect(pending[0]?.markdown).toBe("arrived mid-flight"); + }); + + it("survives a mid-flight RE-SEND, which reuses the id it replaces", () => { + // The common case, and the one clearing by id could never protect: `add` reuses the + // superseded entry's id for the same tab and document, so the replacement minted during + // the awaited turn start carries the very id the send captured. Only entry identity tells + // them apart (PR #74 review). + const { add } = useDesignChangeDraftStore.getState(); + add(THREAD, "tab-a", payload({ markdown: "rode along" })); + const taken = forkDesignChanges.takeForSend(THREAD, ""); + const sentId = taken.sent[0]!.id; + + add(THREAD, "tab-a", payload({ markdown: "re-sent mid-flight" })); + expect(pendingFor(THREAD)[0]?.id).toBe(sentId); // same id, different payload + forkDesignChanges.clear(THREAD, taken.sent); + + const pending = pendingFor(THREAD); + expect(pending).toHaveLength(1); + expect(pending[0]?.markdown).toBe("re-sent mid-flight"); + }); + + it("drops the thread's whole entry once a targeted clear empties it", () => { + const { add } = useDesignChangeDraftStore.getState(); + add(THREAD, "tab-a", payload()); + forkDesignChanges.clear(THREAD, forkDesignChanges.takeForSend(THREAD, "").sent); + expect(scopedThreadKey(THREAD) in useDesignChangeDraftStore.getState().byThreadKey).toBe(false); + }); + it("round-trips every pending block through the transcript extractor", () => { const { add } = useDesignChangeDraftStore.getState(); add(THREAD, "tab-a", payload({ markdown: "# one", pageUrl: "http://localhost:5173/" })); add(THREAD, "tab-b", payload({ markdown: "# two", pageUrl: "http://localhost:5173/" })); - const sent = forkDesignChanges.appendToPrompt(THREAD, "make it pop"); - const extracted = extractTrailingDesignChanges(sent); + const extracted = extractTrailingDesignChanges( + forkDesignChanges.takeForSend(THREAD, "make it pop").text, + ); expect(extracted.promptText).toBe("make it pop"); expect(extracted.blocks).toEqual(["# one", "# two"]); }); diff --git a/apps/web/src/custom/designMode/designChangeDraftStore.ts b/apps/web/src/custom/designMode/designChangeDraftStore.ts index 63d3f70f4e21..ce08b816d281 100644 --- a/apps/web/src/custom/designMode/designChangeDraftStore.ts +++ b/apps/web/src/custom/designMode/designChangeDraftStore.ts @@ -33,7 +33,19 @@ interface DesignChangeDraftStoreState { payload: DesignChangeRequestPayload, ) => void; readonly remove: (threadRef: ScopedThreadRef, id: string) => void; - readonly clear: (threadRef: ScopedThreadRef) => void; + /** + * Drops exactly `sent` — matched by ENTRY IDENTITY, not by id — or every pending attachment + * when `sent` is omitted. + * + * Identity rather than id because `add` deliberately reuses a superseded entry's id for the + * same tab and document (the composer chip derives its React key and its fill from it, so a + * re-send must update in place). That makes the id stable across replacement and therefore + * useless as a freshness token: clearing by id after an awaited turn start would delete a + * payload the panel produced DURING the round trip, which is the common re-send case and + * exactly the loss this targeting exists to prevent (PR #74 review). `add` always builds a + * fresh object, so reference identity is the thing that actually moves. + */ + readonly clear: (threadRef: ScopedThreadRef, sent?: readonly PendingDesignChange[]) => void; } let nextId = 1; @@ -97,12 +109,24 @@ export const useDesignChangeDraftStore = create()(( else byThreadKey[key] = next; return { byThreadKey }; }), - clear: (threadRef) => + clear: (threadRef, sent) => set((state) => { const key = scopedThreadKey(threadRef); if (!(key in state.byThreadKey)) return state; - const { [key]: _removed, ...rest } = state.byThreadKey; - return { byThreadKey: rest }; + if (sent === undefined) { + const { [key]: _removed, ...rest } = state.byThreadKey; + return { byThreadKey: rest }; + } + // Targeted: a send clears exactly the entries it carried. A replacement minted during + // the awaited turn start is a different object under the same id, so it survives. + const dropped = new Set(sent); + const pending = state.byThreadKey[key] ?? []; + const next = pending.filter((entry) => !dropped.has(entry)); + if (next.length === pending.length) return state; + const byThreadKey = { ...state.byThreadKey }; + if (next.length === 0) delete byThreadKey[key]; + else byThreadKey[key] = next; + return { byThreadKey }; }), })); @@ -156,21 +180,35 @@ export const forkDesignChanges = { return selectPendingDesignChanges(useDesignChangeDraftStore.getState().byThreadKey, threadRef) .length; }, - /** Appends every pending change request to the outgoing message text, each wrapped in a - * `` block (mirrors the `` idiom so a transcript - * renderer can extract it later). Returns `text` untouched when nothing is pending. */ - appendToPrompt(threadRef: ScopedThreadRef, text: string): string { - const pending = selectPendingDesignChanges( + /** + * ONE read of the pending set, producing both halves of a send: the outgoing message text + * with every request appended, and the entries that went into it — handed back verbatim so + * the caller can clear exactly those once the turn start succeeds. + * + * One call rather than two because "what rode the message" is otherwise an invariant ChatView + * has to hold by convention across an await, and the module's own rule is that each of these + * helpers is a single call so the fences stay one line (Cursor review, PR #74). Reading the + * store twice also left a window where the text and the clear list could disagree. + * + * Each request is wrapped in a `` block, mirroring the + * `` idiom so a transcript renderer can extract it later. `text` comes back + * untouched when nothing is pending. + */ + takeForSend( + threadRef: ScopedThreadRef, + text: string, + ): { readonly text: string; readonly sent: readonly PendingDesignChange[] } { + const sent = selectPendingDesignChanges( useDesignChangeDraftStore.getState().byThreadKey, threadRef, ); - if (pending.length === 0) return text; - const blocks = pending + if (sent.length === 0) return { text, sent }; + const blocks = sent .map((entry) => `\n${entry.markdown}\n`) .join("\n\n"); - return text.trim().length > 0 ? `${text}\n\n${blocks}` : blocks; + return { text: text.trim().length > 0 ? `${text}\n\n${blocks}` : blocks, sent }; }, - clear(threadRef: ScopedThreadRef): void { - useDesignChangeDraftStore.getState().clear(threadRef); + clear(threadRef: ScopedThreadRef, sent?: readonly PendingDesignChange[]): void { + useDesignChangeDraftStore.getState().clear(threadRef, sent); }, }; diff --git a/apps/web/src/custom/designMode/designModeBridge.test.ts b/apps/web/src/custom/designMode/designModeBridge.test.ts index 8945139d76ed..ff6a2a602e29 100644 --- a/apps/web/src/custom/designMode/designModeBridge.test.ts +++ b/apps/web/src/custom/designMode/designModeBridge.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it } from "vite-plus/test"; import { designModeBridge } from "./designModeBridge"; -import type { DesignModeWritableKey } from "./protocol"; +import { DESIGN_MODE_PROTOCOL_VERSION, type DesignModeWritableKey } from "./protocol"; /** * The bridge's scrub coalescing contract, which nothing else can hold: scrub-driven @@ -17,13 +17,15 @@ const PADDING: DesignModeWritableKey = "padding-top" as DesignModeWritableKey; let calls: string[] = []; let frames: Array<() => void> = []; +/** What the stub webview resolves executeJavaScript with — only the liveness probe reads it. */ +let evaluateResult: unknown = null; const webview = { isConnected: true, getAttribute: (name: string) => (name === "data-preview-tab" ? TAB : null), executeJavaScript: (code: string) => { calls.push(code); - return Promise.resolve(null); + return Promise.resolve(evaluateResult); }, }; @@ -36,6 +38,7 @@ const runFrame = () => { beforeEach(() => { frames = []; + evaluateResult = null; (globalThis as { requestAnimationFrame?: unknown }).requestAnimationFrame = ( callback: () => void, ) => { @@ -48,8 +51,10 @@ beforeEach(() => { (globalThis as { document?: unknown }).document = { querySelectorAll: () => [webview], }; - // Drain coalesced state a prior test may have left: discardAll flushes pending first. + // Drain coalesced state a prior test may have left: discardAll flushes pending first, and + // setActive drops the hover memo so a dedupe test never inherits a neighbour's last hover. designModeBridge.discardAll(TAB); + designModeBridge.setActive(TAB, true); calls = []; }); @@ -103,3 +108,57 @@ describe("designModeBridge scrub coalescing", () => { expect(calls[0]).toContain("[1,2]"); }); }); + +/** + * Hover is idempotent, and `mouseover` bubbles — the layers rail's delegated handler fires + * several times per row crossed (row, caret, glyph, label). Every repeat used to be its own + * executeJavaScript crossing plus a getBoundingClientRect in the guest. + */ +describe("designModeBridge hover deduping", () => { + it("sends one crossing per distinct hover target", () => { + designModeBridge.hoverElement(TAB, 4); + designModeBridge.hoverElement(TAB, 4); + designModeBridge.hoverElement(TAB, 4); + expect(calls).toHaveLength(1); + expect(calls[0]).toContain("hoverElement"); + }); + + it("still sends every change, including the clear on leaving the rail", () => { + designModeBridge.hoverElement(TAB, 4); + designModeBridge.hoverElement(TAB, 5); + designModeBridge.hoverElement(TAB, null); + designModeBridge.hoverElement(TAB, null); + expect(calls).toHaveLength(3); + expect(calls[2]).toContain("null"); + }); + + it("re-sends the same target after re-entering the row", () => { + designModeBridge.hoverElement(TAB, 4); + designModeBridge.hoverElement(TAB, null); + designModeBridge.hoverElement(TAB, 4); + expect(calls).toHaveLength(3); + }); + + it("forgets the memo when the engine is rebuilt, which starts with no outline", () => { + designModeBridge.hoverElement(TAB, 4); + designModeBridge.destroy(TAB); + calls = []; + designModeBridge.hoverElement(TAB, 4); + expect(calls).toHaveLength(1); + }); +}); + +/** The remount reconcile's probe — see ForkPreviewDesignMode's reconcileEngine. */ +describe("designModeBridge engine liveness", () => { + it("reports an engine speaking this host's protocol version as current", async () => { + evaluateResult = DESIGN_MODE_PROTOCOL_VERSION; + expect(await designModeBridge.engineIsCurrent(TAB)).toBe(true); + }); + + it("reports no engine, and a version-skewed one, as not current", async () => { + evaluateResult = null; + expect(await designModeBridge.engineIsCurrent(TAB)).toBe(false); + evaluateResult = DESIGN_MODE_PROTOCOL_VERSION - 1; + expect(await designModeBridge.engineIsCurrent(TAB)).toBe(false); + }); +}); diff --git a/apps/web/src/custom/designMode/designModeBridge.ts b/apps/web/src/custom/designMode/designModeBridge.ts index cc71cea65bf8..276a12bfabbf 100644 --- a/apps/web/src/custom/designMode/designModeBridge.ts +++ b/apps/web/src/custom/designMode/designModeBridge.ts @@ -1,5 +1,6 @@ import { DESIGN_MODE_GLOBAL, + DESIGN_MODE_PROTOCOL_VERSION, parseDesignChangeRequestPayload, type DesignChangeRequestPayload, type DesignModeAlignAxis, @@ -41,6 +42,12 @@ export const findPreviewWebview = (runtimeTabId: string): DesignModeWebview | nu return found; }; +/** Last hover id pushed to each tab's guest, so identical repeats never cross the boundary + * (see `hoverElement`). Reset by the engine-lifecycle verbs below, since a rebuilt engine + * starts with no hover outline and no memory of one; the ordinary staleness path is already + * closed by the rail sending `null` on mouseleave. */ +const lastHoverByTabId = new Map(); + /** Builds the executeJavaScript expression for one guest-handle call. Arguments are * JSON-encoded — the whole command surface is JSON-serializable by contract * (protocol.ts `DesignModeGuestHandle`). */ @@ -113,6 +120,7 @@ const fire = (runtimeTabId: string, member: string, args: readonly unknown[]): v */ export const designModeBridge = { setActive(runtimeTabId: string, on: boolean): void { + lastHoverByTabId.delete(runtimeTabId); fire(runtimeTabId, "setActive", [on]); }, applyDraft( @@ -175,10 +183,34 @@ export const designModeBridge = { if (result == null) return null; return parseDesignChangeRequestPayload(result) ?? "stale-engine"; }, + /** Whether a live guest engine speaking THIS host's protocol version is installed on the + * page — the remount reconcile's probe (ForkPreviewDesignMode). Cheap enough to ask on + * every attach: one property read across the boundary, no bundle transfer. Null covers both + * "no engine" and an unreadable answer; the caller re-injects either way, which is + * idempotent by boot()'s contract. */ + async engineVersion(runtimeTabId: string): Promise { + const webview = findPreviewWebview(runtimeTabId); + if (!webview) return null; + const result = await webview + .executeJavaScript(`globalThis.${DESIGN_MODE_GLOBAL}?.version ?? null`, false) + .catch(() => null); + return typeof result === "number" ? result : null; + }, + /** True when the engine on the page is one this host can drive without re-injecting. */ + async engineIsCurrent(runtimeTabId: string): Promise { + return (await this.engineVersion(runtimeTabId)) === DESIGN_MODE_PROTOCOL_VERSION; + }, selectElement(runtimeTabId: string, id: number, mode: DesignModeSelectMode = "replace"): void { fire(runtimeTabId, "selectElement", [id, mode]); }, + /** Deduped against the last hover sent for this tab. `mouseover` bubbles, so the layers + * rail's delegated handler fires two to four times per row crossed (row → caret → glyph → + * label) and every one of those used to be its own executeJavaScript crossing plus a + * getBoundingClientRect in the guest — a pointer sweep down the rail cost a hundred round + * trips to paint one outline. Hover is idempotent, so repeats carry no information. */ hoverElement(runtimeTabId: string, id: number | null): void { + if (lastHoverByTabId.get(runtimeTabId) === id) return; + lastHoverByTabId.set(runtimeTabId, id); fire(runtimeTabId, "hoverElement", [id]); }, reorderElement(runtimeTabId: string, id: number, beforeId: number | null): void { @@ -191,6 +223,19 @@ export const designModeBridge = { fire(runtimeTabId, "canvasCommand", [action]); }, destroy(runtimeTabId: string): void { + lastHoverByTabId.delete(runtimeTabId); fire(runtimeTabId, "destroy", []); }, + /** Drops the hover memo alone. A re-injected engine starts with a fresh id registry and no + * outline painted, so the memo describes nothing — and would suppress the hover of whichever + * row happens to draw the id it last sent (ForkPreviewDesignMode's injectEngine). */ + forgetHover(runtimeTabId: string): void { + lastHoverByTabId.delete(runtimeTabId); + }, + /** Drops every host-side memo for a tab that is GONE (its webview closed), including the + * cached element itself — see designModeTabLifetime's disposeDesignModeTab. */ + forgetTab(runtimeTabId: string): void { + lastHoverByTabId.delete(runtimeTabId); + webviewByTabId.delete(runtimeTabId); + }, }; diff --git a/apps/web/src/custom/designMode/designModeTabLifetime.ts b/apps/web/src/custom/designMode/designModeTabLifetime.ts new file mode 100644 index 000000000000..937466568acd --- /dev/null +++ b/apps/web/src/custom/designMode/designModeTabLifetime.ts @@ -0,0 +1,22 @@ +import { designModeBridge } from "./designModeBridge"; +import { useDesignModeStore } from "./designModeStore"; +import { designUndoHistory } from "./designUndoHistory"; + +/** + * Everything design mode keeps for one preview tab, released in one call. + * + * The only caller is `browser/desktopTabLifetime.ts`'s close path — the one place that knows a + * preview tab is CLOSED rather than merely unmounted, which every other part of this feature is + * built to survive. That insight belongs in the lease; the *list* of what design mode holds does + * not. Keeping the list here means the fenced hunk in that shared file stays one call and does + * not have to learn this feature's memory layout, and the next per-tab memo is added here rather + * than as another line in an upstream file (Cursor review, PR #74). + * + * Everything released is host-side and in-memory. The guest engine and its drafts die with the + * webview on their own; nothing here touches the page, and nothing here can fail. + */ +export function disposeDesignModeTab(runtimeTabId: string): void { + useDesignModeStore.getState().remove(runtimeTabId); + designUndoHistory.clear(runtimeTabId); + designModeBridge.forgetTab(runtimeTabId); +} diff --git a/apps/web/src/custom/designMode/engine/headlessMode.ts b/apps/web/src/custom/designMode/engine/headlessMode.ts index 42530d41c285..0cec29da202d 100644 --- a/apps/web/src/custom/designMode/engine/headlessMode.ts +++ b/apps/web/src/custom/designMode/engine/headlessMode.ts @@ -525,7 +525,9 @@ export class HeadlessDesignMode { remainingDrafts.push(d); continue; } - for (const [prop, value] of d.props) this.drafts.apply(el, prop, value); + // The persisted original rides along: re-deriving it here would read the inline style a + // destroyed engine left painted on this very element (lifecycle-store.ts's `props`). + for (const [prop, value, original] of d.props) this.drafts.apply(el, prop, value, original); } pending.drafts = remainingDrafts; @@ -630,7 +632,11 @@ export class HeadlessDesignMode { liveKeys.add(persistKey(address)); drafts.push({ ...address, - props: [...props.entries()].map(([p, d]) => [p, d.value] as [string, string]), + // Value AND original — the original is not re-derivable at restore time (see + // lifecycle-store.ts's `props` and DraftStore.apply's `knownOriginal`). + props: [...props.entries()].map( + ([p, d]) => [p, d.value, d.original] as [string, string, string], + ), }); } // Merge in still-unresolved restore work so a reload mid-retry-window doesn't lose it. diff --git a/apps/web/src/custom/designMode/engine/vendor/README.md b/apps/web/src/custom/designMode/engine/vendor/README.md index 7f8bae181783..df2ec0c6d96f 100644 --- a/apps/web/src/custom/designMode/engine/vendor/README.md +++ b/apps/web/src/custom/designMode/engine/vendor/README.md @@ -34,6 +34,14 @@ Local edits are marked with `t3-fork:` comments. The load-bearing ones: - `./shared/` import paths (were `../shared/` upstream). - A handful of mechanical lint fixes (snapshot spreads → `Array.from`, `toReversed()`, `Set#has`, two unused imports) — style-only, no behavior change. +- Restore fidelity (2026-08-07): `drafts.ts` — `apply()` takes an optional `knownOriginal`, + and `lifecycle-store.ts` — a persisted draft's `props` tuple carries that original in a + third slot (2-tuples still load). Upstream re-derives the original through `pagePrior` on + restore, which for a css draft reads the element's live INLINE style; T3 destroys and + re-injects the engine into the SAME document on every Design-mode toggle, leaving the + previous previews painted, so re-derivation captured each draft's own value as the page's + original — Discard restored the draft over itself and Send reported nothing to send. + Do not drop the parameter when re-syncing: upstream never rebuilds in-place this way. - Native-source mode (2026-08-04, `engine/nativeSource.ts` is the fork-owned core): - `source.ts` — `findSelectableElement`: a tagged ancestor still wins, but untagged elements are selectable themselves (svg internals climb to the outermost ``). diff --git a/apps/web/src/custom/designMode/engine/vendor/drafts.ts b/apps/web/src/custom/designMode/engine/vendor/drafts.ts index f1424700edd8..12e2adec22fe 100644 --- a/apps/web/src/custom/designMode/engine/vendor/drafts.ts +++ b/apps/web/src/custom/designMode/engine/vendor/drafts.ts @@ -48,7 +48,14 @@ export class DraftStore { * legitimately touch both halves. */ private readonly structural = new StructuralDraftStore(this.host) - apply(el: TaggedElement, prop: string, value: string): void { + /* t3-fork: `knownOriginal` is the restore path's answer to "what did the page have here?". + * The default oracle (`pagePrior`) reads the element's live inline style for a css draft, + * which is the previous engine's own preview whenever this session is being rebuilt into the + * SAME document (the Design-mode off/on toggle destroys the engine but leaves the previews + * painted). A restore already knows the real original — it was persisted with the draft — + * so it passes it rather than letting the oracle re-derive a lie. See + * lifecycle-store.ts's PersistedLifecycle.drafts. */ + apply(el: TaggedElement, prop: string, value: string, knownOriginal?: string): void { // Same tombstone guard as applyText: Compare un-hides a delete-drafted element // (writeAll 'original' restores its display), which makes it selectable and scrubbable // again — a css draft minted there would ride the same request as the delete, telling @@ -74,7 +81,7 @@ export class DraftStore { // THIS node), and capturing that as "original" would make a discard un-restorable — same rule, // same oracle, as every structural capture (review findings 1-3). if (existing) existing.value = value - else props.set(prop, { original: this.structural.pagePrior(el, prop), value }) + else props.set(prop, { original: knownOriginal ?? this.structural.pagePrior(el, prop), value }) if (this.showingOriginal.has(el)) { // auto-exit compare so the user sees the edit they just made diff --git a/apps/web/src/custom/designMode/engine/vendor/lifecycle-store.ts b/apps/web/src/custom/designMode/engine/vendor/lifecycle-store.ts index 7fb4a1ace825..6bfb88108793 100644 --- a/apps/web/src/custom/designMode/engine/vendor/lifecycle-store.ts +++ b/apps/web/src/custom/designMode/engine/vendor/lifecycle-store.ts @@ -32,7 +32,22 @@ export interface PersistedLifecycle { drafts: Array<{ dcSource: string index: number - props: Array<[prop: string, value: string]> + /* t3-fork: the third slot is the draft's ORIGINAL — the page's own value for the + * property, captured when the draft was first minted. + * + * It has to travel. Re-deriving it on restore reads `DraftStore.pagePrior`, which for a + * css draft answers with the element's live INLINE style — and the inline style is where + * the previous engine left the draft. That is fine after a real reload (fresh DOM, no + * inline styles) and WRONG after a same-document re-injection, which is exactly what + * toggling Design mode off and on does: `destroy()` leaves the previews painted, so every + * restored draft would capture its own drafted value as the "original". Discard then + * restores the draft over itself and the send builder measures before === after, so the + * panel counts N changes while Send answers "nothing to send". + * + * Optional, and 2-tuples still load: a session persisted by an older engine must not be + * thrown away on upgrade — it restores the way it always did (re-derived) for the rest of + * that session, and the next persist writes the triple. */ + props: Array<[prop: string, value: string, original?: string]> selector?: string }> sent: Array<{ id: string; elements: PersistedSentElement[] }> @@ -100,8 +115,16 @@ function isValidDraftEntry(v: unknown): v is PersistedLifecycle['drafts'][number if (typeof v.dcSource !== 'string' || typeof v.index !== 'number') return false if (!isValidSelectorField(v)) return false if (!Array.isArray(v.props)) return false + /* t3-fork: 2- and 3-tuples both valid — see PersistedLifecycle.drafts. The optional third + * slot is the original, and it is validated as strictly as the other two: a non-string there + * would be handed to `style.setProperty` on discard. */ return v.props.every( - (p) => Array.isArray(p) && p.length === 2 && typeof p[0] === 'string' && typeof p[1] === 'string' + (p) => + Array.isArray(p) && + (p.length === 2 || p.length === 3) && + typeof p[0] === 'string' && + typeof p[1] === 'string' && + (p.length === 2 || typeof p[2] === 'string') ) } diff --git a/apps/web/src/custom/designMode/layersDrag.test.ts b/apps/web/src/custom/designMode/layersDrag.test.ts new file mode 100644 index 000000000000..2759316b618d --- /dev/null +++ b/apps/web/src/custom/designMode/layersDrag.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { resolveDropBeforeId } from "./layersDrag"; +import type { LayerRow } from "./layersTreeModel"; + +/** + * The rail's drop arithmetic, which is the half of the gesture that can be wrong silently: + * an unusable `beforeId` reaches the guest, `reorderById` refuses it because the two elements + * do not share a DOM parent, and the drag simply does nothing with no feedback anywhere. + * + * `siblingGroup` is what makes this non-obvious — the curated walk hoists tagged descendants + * through untagged wrappers, so rows that are siblings in the TREE routinely are not siblings + * in the DOM, and `nextSiblingId` is a tree fact. + */ + +const row = ( + id: number, + siblingGroup: number, + nextSiblingId: number | null, + reorderable = true, +): LayerRow => ({ + node: { id, tag: "div", label: `#${id}`, reorderable, siblingGroup, children: [] }, + depth: 1, + parentId: 0, + nextSiblingId, + expanded: false, + hasChildren: false, +}); + +const index = (rows: readonly LayerRow[]) => new Map(rows.map((r) => [r.node.id, r])); + +describe("resolveDropBeforeId", () => { + it("drops before the hovered row", () => { + const a = row(1, 7, 2); + const b = row(2, 7, null); + expect(resolveDropBeforeId(b, a, "before", index([a, b]))).toEqual({ beforeId: 1 }); + }); + + it("drops after a row by naming its next sibling", () => { + const a = row(1, 7, 2); + const b = row(2, 7, 3); + const c = row(3, 7, null); + expect(resolveDropBeforeId(c, a, "after", index([a, b, c]))).toEqual({ beforeId: 2 }); + }); + + it("drops after the last sibling as 'to the end', not as a dangling reference", () => { + const a = row(1, 7, 2); + const b = row(2, 7, null); + expect(resolveDropBeforeId(a, b, "after", index([a, b]))).toEqual({ beforeId: null }); + }); + + it("treats a trailing hoisted sibling as the end of this group", () => { + // The original regression: `b` is last under ITS dom parent, but the tree gives it a next + // sibling hoisted out of an untagged wrapper. Shipping `3` made the guest refuse the whole + // reorder — sibling-relative moves are only meaningful within one DOM parent. + const a = row(1, 7, 2); + const b = row(2, 7, 3); + const hoisted = row(3, 9, null); + expect(resolveDropBeforeId(a, b, "after", index([a, b, hoisted]))).toEqual({ beforeId: null }); + }); + + it("scans PAST a hoisted sibling to the next real one", () => { + // `P = [a, wrapper(b), c, d]`. Dropping `c` after `a` must not read "the next row is + // hoisted" as "this is the end of the parent" — a one-step lookahead shipped `null` here + // and landed the row past `d`, while the rail drew its line under `a`. Worse than the + // refusal it replaced: a silent no-op became a visibly wrong landing (PR #74 review). + const a = row(1, 7, 2); + const hoisted = row(2, 9, 3); + const c = row(3, 7, 4); + const d = row(4, 7, null); + const rows = index([a, hoisted, c, d]); + expect(resolveDropBeforeId(d, a, "after", rows)).toEqual({ beforeId: 3 }); + // And the scan keeps going across several hoisted rows in a run, not just one. + const hoisted2 = row(5, 9, 2); + const a2 = row(1, 7, 5); + expect(resolveDropBeforeId(d, a2, "after", index([a2, hoisted2, hoisted, c, d]))).toEqual({ + beforeId: 3, + }); + }); + + it("still reports the end when every remaining sibling is hoisted", () => { + const a = row(1, 7, 2); + const hoistedA = row(2, 9, 3); + const hoistedB = row(3, 9, null); + expect(resolveDropBeforeId(a, a, "after", index([a, hoistedA, hoistedB]))).toEqual({ + beforeId: null, + }); + }); + + it("ends the scan on a cyclic sibling chain instead of spinning", () => { + // Only reachable from a malformed payload, but the walk is over host-supplied ids. + const a = row(1, 7, 2); + const b = row(2, 9, 1); + expect(resolveDropBeforeId(a, a, "after", index([a, b]))).toEqual({ beforeId: null }); + }); + + it("refuses a drop onto a row in another DOM group", () => { + const a = row(1, 7, null); + const foreign = row(2, 9, null); + expect(resolveDropBeforeId(a, foreign, "before", index([a, foreign]))).toBeNull(); + expect(resolveDropBeforeId(a, foreign, "after", index([a, foreign]))).toBeNull(); + }); + + it("treats a next sibling missing from the rendered rows as the end", () => { + // A collapsed or re-emitted tree can leave `nextSiblingId` naming a row that is no longer + // drawn; "to the end" is the honest answer, never a reference the guest cannot resolve. + const a = row(1, 7, 2); + const b = row(2, 7, 99); + expect(resolveDropBeforeId(a, b, "after", index([a, b]))).toEqual({ beforeId: null }); + }); +}); diff --git a/apps/web/src/custom/designMode/layersDrag.ts b/apps/web/src/custom/designMode/layersDrag.ts index 00f51210c10c..8a9c3cd717e9 100644 --- a/apps/web/src/custom/designMode/layersDrag.ts +++ b/apps/web/src/custom/designMode/layersDrag.ts @@ -40,6 +40,58 @@ function rowIdFromEvent(event: DragEvent): number | null { return Number.isFinite(id) ? id : null; } +/** + * The `beforeId` a drop resolves to — `{ beforeId: null }` meaning "to the end of the DOM + * sibling list" — or null when the gesture must be refused. + * + * The subtlety is `nextSiblingId`: it is the next TREE sibling, and tree siblings are not + * always DOM siblings (the curated walk hoists tagged descendants through untagged wrappers, + * which is the whole reason rows carry `siblingGroup`). Dropping below the last row of a DOM + * group therefore used to ship the id of a row under a DIFFERENT DOM parent, which the guest + * correctly refuses (`reorderById` requires a shared parent) — so the drag simply did nothing, + * with no feedback anywhere. + * + * The reference is found by SCANNING FORWARD for the next same-group tree sibling, not by + * looking one step ahead. A single-step lookahead reads "the very next row is hoisted" as "this + * is the end of the DOM group", which is only true when every REMAINING sibling is also + * hoisted. In `P = [a, wrapper(b), c, d]`, dropping `c` after `a` would take that shortcut and + * ship `null` — landing the row at the end of the parent, past `d`, while the rail drew its + * insertion line directly under `a`. That is worse than the refusal it replaced: a silent + * no-op became a visibly wrong landing (PR #74 review). + * + * `null` is therefore reserved for a genuine end-of-group. Where the tree and the DOM disagree + * the landing is inherently approximate — "between `a` and a row living inside a wrapper" has + * no DOM expression — but the next real sibling is much closer to what the rail promised. + * + * Pure, and exported, because it is the one piece of this gesture that is arithmetic. + */ +export function resolveDropBeforeId( + dragged: LayerRow, + over: LayerRow, + edge: DropEdge, + byId: ReadonlyMap, +): { readonly beforeId: number | null } | null { + // The same gate `onDragOver` paints with — re-asserted here because the drop is what + // actually commits, and a stale insertion line must never survive as a stale reference. + if (over.node.siblingGroup !== dragged.node.siblingGroup) return null; + if (edge === "before") return { beforeId: over.node.id }; + // Bounded by the map: every hop consumes a distinct row, and a row already visited cannot be + // reached again, so a cyclic `nextSiblingId` (only reachable from a malformed payload) ends + // the walk rather than spinning. + const seen = new Set([over.node.id]); + let cursor = over.nextSiblingId; + while (cursor !== null && !seen.has(cursor)) { + seen.add(cursor); + const candidate = byId.get(cursor); + if (!candidate) break; + if (candidate.node.siblingGroup === dragged.node.siblingGroup) { + return { beforeId: candidate.node.id }; + } + cursor = candidate.nextSiblingId; + } + return { beforeId: null }; +} + export function useLayersDrag({ rows, filtering, @@ -62,6 +114,18 @@ export function useLayersDrag({ const dragged = useRef(null); const byId = useMemo(() => new Map(rows.map((row) => [row.node.id, row])), [rows]); + // Mirrored in a ref so the handlers below can READ the live drop target without taking it + // as a memo dependency: `dragover` fires at pointer rate, and depending on the state meant + // rebuilding the whole handler object (and re-running the memo) on every edge flip. + const dropTargetRef = useRef(null); + const setDrop = useCallback((next: DropTarget | null) => { + const current = dropTargetRef.current; + if (current === next) return; + if (current && next && current.overId === next.overId && current.edge === next.edge) return; + dropTargetRef.current = next; + setDropTarget(next); + }, []); + const canDrag = useCallback((row: LayerRow) => row.node.reorderable && !filtering, [filtering]); const containerHandlers = useMemo( @@ -74,7 +138,7 @@ export function useLayersDrag({ // Firefox refuses to start a drag with no payload; the value itself is unused. event.dataTransfer.setData("text/plain", String(row.node.id)); dragged.current = row.node.id; - setDropTarget({ overId: row.node.id, edge: "before" }); + setDrop({ overId: row.node.id, edge: "before" }); }, onDragOver: (event) => { const source = dragged.current; @@ -93,6 +157,9 @@ export function useLayersDrag({ row.node.id !== source; if (!droppable) { event.dataTransfer.dropEffect = "none"; + // The line has to go with the refusal: leaving it painted on the last valid row + // while the pointer sits over an undroppable one promises a drop that cannot happen. + setDrop(null); return; } event.preventDefault(); @@ -102,36 +169,36 @@ export function useLayersDrag({ ?.getBoundingClientRect(); if (!bounds) return; const edge: DropEdge = event.clientY < bounds.top + bounds.height / 2 ? "before" : "after"; - if (dropTarget?.overId !== row.node.id || dropTarget.edge !== edge) { - setDropTarget({ overId: row.node.id, edge }); - } + setDrop({ overId: row.node.id, edge }); }, onDragLeave: (event) => { // Only when the pointer leaves the LIST, not on every row-to-row crossing. if (event.currentTarget.contains(event.relatedTarget as Node | null)) return; - setDropTarget(null); + setDrop(null); }, onDrop: (event) => { event.preventDefault(); const source = dragged.current; - const target = dropTarget; + const target = dropTargetRef.current; dragged.current = null; - setDropTarget(null); + setDrop(null); if (source === null || !target) return; const row = byId.get(target.overId); - if (!row) return; - const beforeId = target.edge === "before" ? row.node.id : row.nextSiblingId; + const from = byId.get(source); + if (!row || !from) return; + const resolved = resolveDropBeforeId(from, row, target.edge, byId); + if (!resolved) return; // Dropping a row onto its own edge is the "moved nothing" case; everything else goes // to the guest, INCLUDING a drag back to the original slot — that one drops the move // draft, which is the only way to undo a reorder from the rail. - if (beforeId !== source) onReorder(source, beforeId); + if (resolved.beforeId !== source) onReorder(source, resolved.beforeId); }, onDragEnd: () => { dragged.current = null; - setDropTarget(null); + setDrop(null); }, }), - [byId, dropTarget, filtering, onReorder], + [byId, filtering, onReorder, setDrop], ); return { dropTarget, canDrag, containerHandlers }; diff --git a/apps/web/src/custom/designMode/panel/ForkDesignPanel.tsx b/apps/web/src/custom/designMode/panel/ForkDesignPanel.tsx index 4599e240590e..eb86f923d191 100644 --- a/apps/web/src/custom/designMode/panel/ForkDesignPanel.tsx +++ b/apps/web/src/custom/designMode/panel/ForkDesignPanel.tsx @@ -1,5 +1,5 @@ import type { ScopedThreadRef } from "@t3tools/contracts"; -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { Button } from "~/components/ui/button"; import { toastManager } from "~/components/ui/toast"; @@ -69,44 +69,73 @@ export function ForkDesignPanel({ runtimeTabId, threadRef, tabId }: Props) { /** Every verb below is a no-op without a tab and an addressable selection — one gate. */ const target = runtimeTabId !== null && ids.length > 0 ? runtimeTabId : null; + // Read through a ref so the mutation gate below — and therefore every verb built on it — + // does not take the compare flag as a dependency and rebuild on each toggle. + const comparingRef = useRef(tab.comparing); + comparingRef.current = tab.comparing; + + /** + * THE mutation gate. Every write the panel makes goes through this — there is no second way + * in — so the rules a mutation owes are structural rather than a checklist each new verb has + * to remember (Cursor review, PR #74). + * + * Two rules today, and both are easy to get wrong by omission: + * + * - **Leave compare first.** The guest auto-exits compare for the element it is drafting + * (DraftStore.apply), but only that one — so editing while comparing left a multi-element + * selection rendering half "before" and half "after", under a button still labelled for the + * whole-page state. Exiting for everything keeps the page, the guest and the label agreeing; + * mirroring the guest's per-element rule host-side would need compare state on the wire per + * element to describe something nobody wants to look at. + * - **Clear the undo stack unless the verb records its own step.** Popping a step OLDER than + * an action undo cannot reverse would un-do the wrong thing, so clear-first is the default + * and only the two scrub-shaped writes pass a `record` (PR #70 review). + */ + const mutate = useCallback( + (run: (verbTarget: string) => void, record?: (verbTarget: string) => void) => { + if (!target) return; + if (runtimeTabId && comparingRef.current) { + designModeBridge.compareAll(runtimeTabId, false); + useDesignModeStore.getState().setComparing(runtimeTabId, false); + } + if (record) record(target); + else designUndoHistory.clear(target); + run(target); + }, + [runtimeTabId, target], + ); + const apply = useCallback( (property: DesignModeWritableKey, value: string) => { - if (!target) return; - // Undo bookkeeping rides the same snapshots the fields display: mid-gesture the - // selection snapshot still holds the pre-gesture value (the emit is a trailing - // debounce), so the first tick records exactly the state Cmd+Z should restore. - // Write-only shorthands (`gap`) never appear in snapshots — prev null makes undo - // discard that property's draft instead (designUndoHistory.ts). - designUndoHistory.recordDraft( - target, - property, - addressable.map((element) => ({ - id: element.id, - prev: element.drafted.includes(property) - ? ((element.styles as Partial>)[property] ?? null) - : null, - })), - value, - Date.now(), + mutate( + (verbTarget) => designModeBridge.applyDraft(verbTarget, ids, property, value), + // Undo bookkeeping rides the same snapshots the fields display: mid-gesture the + // selection snapshot still holds the pre-gesture value (the emit is a trailing + // debounce), so the first tick records exactly the state Cmd+Z should restore. + // Write-only shorthands (`gap`) never appear in snapshots — prev null makes undo + // discard that property's draft instead (designUndoHistory.ts). + (verbTarget) => + designUndoHistory.recordDraft( + verbTarget, + property, + addressable.map((element) => ({ + id: element.id, + prev: element.drafted.includes(property) + ? ((element.styles as Partial>)[property] ?? + null) + : null, + })), + value, + Date.now(), + ), ); - designModeBridge.applyDraft(target, ids, property, value); }, // ids is rebuilt per render but changes only with the selection snapshot array. - [target, tab.selection], + [mutate, tab.selection], ); - // Every mutating verb the history does NOT record goes through this: clear-first is the - // default, so a verb added later keeps Cmd+Z honest without remembering a line — popping - // a step older than an action undo cannot reverse would un-do the wrong thing. Only - // `apply` and `onInset` record instead (PR #70 review). - const unrecorded = useCallback( - (mutate: (verbTarget: string) => void) => { - if (!target) return; - designUndoHistory.clear(target); - mutate(target); - }, - [target], - ); + /** The gate's default arm, named for the verbs that read better with it. */ + const unrecorded = useCallback((run: (verbTarget: string) => void) => mutate(run), [mutate]); const setSizeMode = useCallback( (axis: "width" | "height", mode: DesignModeSizeMode) => @@ -122,17 +151,20 @@ export function ForkDesignPanel({ runtimeTabId, threadRef, tabId }: Props) { const onInset = useCallback( (axis: "x" | "y", px: number) => { - if (!target || !Number.isFinite(px)) return; - designUndoHistory.recordInset( - target, - axis, - addressable.map((element) => ({ id: element.id, prev: element.offsets[axis] })), - px, - Date.now(), + if (!Number.isFinite(px)) return; + mutate( + (verbTarget) => designModeBridge.setInset(verbTarget, ids, axis, px), + (verbTarget) => + designUndoHistory.recordInset( + verbTarget, + axis, + addressable.map((element) => ({ id: element.id, prev: element.offsets[axis] })), + px, + Date.now(), + ), ); - designModeBridge.setInset(target, ids, axis, px); }, - [target, tab.selection], + [mutate, tab.selection], ); const onAbsolute = useCallback(