feat: proxy-bridge directive + touch drag recovery (prereview --external)

[adnaan]

What

Client-side support for prereview's --external live-site annotation mode (livetemplate/prereview#36). Adds a cross-origin proxy bridge directive and makes the shared drag spine touch-resilient.

Changes

• lvt-fx:proxy-bridge (new directive) — on the preview stage wrapping a cross-origin proxied iframe. It relays the page's beacon:
  • nav → dispatches setProxyURL so the server swaps the visible annotation set;
  • scroll → sizes the [data-pin-layer] to the document and translates it by -scroll (live re-pin, no server hop). Re-applied on every render so morphdom can't wipe the imperative styles.
  • Locate: posts a focus message into the iframe (allowed cross-origin) so the beacon navigates to the annotation's page + scrolls it into view.
  • Origin-validated against the iframe's src.
• region-select data-surface="page" — regionRectFromBox converts a drawn box (overlay-rect fractions) to document fractions using the beacon's scroll/doc metrics (a live cross-origin page has no source-line markers, so it's a rect, not a line range).
• attachBoxDragSelect touch recovery — iOS cancels long/fast touch-drags (re-reads them as a swipe); for pointerType==='touch' we now finalize the region on pointercancel/lostpointercapture instead of discarding it (mouse/pen unchanged; the area threshold still rejects stray tiny boxes).

Tests

New jest: proxy-bridge (nav dispatch, dedup, pin-layer transform + morphdom-wipe recovery, origin reject, focus relay on token change), regionRectFromBox conversion + clamping, and touch cancel/lost-capture recovery + tiny-cancel still dropped. Full suite green (732 passing).

🤖 Generated with Claude Code

[claude]

Review

Overall the approach is clean and the touch-recovery logic is correct. Two security issues and one test nit to address.

Security

1. Origin filtering silently falls through when iframe has no src (medium)

In attachProxyBridge (dom/directives.ts):

let expectedOrigin = "";
try {
  if (iframe && iframe.src) expectedOrigin = new URL(iframe.src).origin;
} catch { expectedOrigin = ""; }
...
if (expectedOrigin && e.origin !== expectedOrigin) return;  // skipped when empty

If the iframe's src is missing, blank, or a relative path that fails new URL() at attach-time, expectedOrigin stays "" and the guard short-circuits — any window can fire a __prereview message and call send({ action, data: { url } }). If the server fetches that URL to proxy it, this is a SSRF path.

Fix: fail closed — if the origin can't be resolved, log a warning and skip attaching the listener.

if (!expectedOrigin) {
  console.warn('lvt-fx:proxy-bridge: could not resolve iframe origin, skipping attach');
  return;
}

2. postMessage target falls back to "*" (low)

win.postMessage({ __prereviewFocus: true, url, y }, expectedOrigin || "*");

Same empty-origin condition — focus messages (annotation URL + y-fraction) get broadcast to all origins. The data is not highly sensitive, but it contradicts the stated "read-only, origin-validated" contract. Fixing the early-return above eliminates this path too.

Test nit

dispatchTouch is defined twice in the diff — once inside the handleAreaSelectDirectives describe block and again later. Hoist it to module scope to avoid drift.

The rest looks good

• Touch-recovery logic in onPointerCancel / onLostCapture is correct, and the area-threshold guard preventing stray cancels from creating tiny boxes is the right safety net.
• regionRectFromBox conversion math (scroll + viewport fraction → document fraction) and clamping are correct.
• Idempotency sweep and teardown mirror the existing area/region-select pattern faithfully.
• Tests cover the important paths: dedup, wrong-origin reject, morphdom-wipe recovery, focus-token dedup.

[adnaan]

Addressed the origin findings — the proxy-bridge now fails closed: an unresolvable iframe origin rejects all inbound messages and never broadcasts focus (no "*" fallback), with a one-time warning. Added a test for it.

On the test nit: dispatchTouch is defined only once (tests/directives.test.ts:1349). I think it was conflated with the pre-existing dispatchPointer helper — no duplicate to hoist.

[claude]

Review

Touch drag recovery looks correct. The three new tests (cancel-keeps, lostcapture-keeps, tiny-cancel-still-dropped) cover the important cases. One edge case: if lostpointercapture fires before any pointermove, lastMove holds the pointerdown event (set at pointerdown), so finalize(lastMove, true) is called with a zero-area box. The area threshold correctly rejects it, but an explicit test or comment on this path would make the intent clear.

Proxy bridge: architecture is sound and origin validation is properly fail-closed. Two issues worth addressing:

1. Module-level pageBridgeMetrics singleton has no runtime guard. handleProxyBridgeDirectives iterates all matching elements and can arm multiple bridges (e.g. during a morphdom diff that transiently renders two [lvt-fx:proxy-bridge] elements). Both write to the same pageBridgeMetrics, and the first cleanup() call nulls it, leaving the second bridge blind until the next beacon. The comment acknowledges exactly-one but there is no enforcement. A console.error + early-return in attachProxyBridge when proxyBridgeArmed.size > 0 would catch this in development.

2. regionRectFromBox is exported as if it were a pure conversion utility but silently reads module-level pageBridgeMetrics. The JSDoc explains it is exported for testability, which is fine. The tests correctly arm metrics via the production beacon path so coverage is genuine, just something to be aware of.

Nothing blocking. The security model, cleanup lifecycle, and test coverage are solid.

[adnaan]

Thanks — addressed both non-blocking points: added a single-bridge guard (warn + ignore a second lvt-fx:proxy-bridge since pageBridgeMetrics is shared), and an explicit test that a lostpointercapture before any pointermove yields a zero-area box the threshold rejects. Re-vendored into prereview#36.

[claude]

Review

Overall this is solid work — the iOS touch-drag recovery is well-reasoned and the proxy bridge is carefully designed (fails closed on unresolvable origin, explicit dedup on nav events, singleton guard for shared metrics). Tests cover the important edge cases. A few findings:

Bug (minor): pointerType not cleared in reset()

lastMove is reset to null in the cleanup path, but pointerType is left at its previous value. Right now this is harmless because every event handler that reads pointerType is guarded by e.pointerId !== pointerId (which returns early when no drag is active), so the stale value can't be observed. But the asymmetry with lastMove is fragile — a future change that reads pointerType without a pointerId guard would silently use stale state. Suggest adding pointerType = "" alongside lastMove = null in reset().

Observation: lastMove = e is set on pointerdown, not just pointermove

This means onLostCapture before any pointermove passes the pointerdown event to finalize, producing a zero-area box. The area-threshold test confirms this is intentional and rejects it. Good — just noting it since the variable name (lastMove) implies it's only set on moves.

regionRectFromBox ignores rect.left / rect.top

docLeft = m.scrollX + box.x * rect.width — this is correct only if box.x is a fraction of the overlay element (not a fraction of the viewport origin). Since box.x is the drag position relative to the overlay's own bounding rect, and the overlay covers the iframe's viewport, the overlay's own offset within the parent page doesn't factor into the within-iframe document coordinate conversion. This is correct, but the comment in regionRectFromBox could make the coordinate frame assumption explicit to avoid future confusion (e.g., "overlay-relative, not viewport-absolute").

No security concerns — origin validation is strict (expectedOrigin never falls back to "*"), the __prereview sentinel guards against ambient message listeners, and outbound postMessage targets the resolved origin.

[claude]

Good, well-structured PR. A few things worth considering:

Touch recovery (onLostCapture):

finalize(pointerType === "touch" ? m : null, pointerType === "touch" && m !== null);

The m !== null guard is unreachable when pointerType === "touch" — lastMove is set on pointerdown and only nulled inside finalize itself, which resets pointerId to -1 before that could happen. The condition simplifies to pointerType === "touch". Not a bug (the tests pass), but the extra complexity is misleading.

pageBridgeMetrics global reset on cleanup:
cleanup() sets pageBridgeMetrics = null. If the action attribute changes (existing bridge cleaned up, new one created in the same render pass), the new bridge starts blind until the next beacon. This is probably fine in practice since a changed action would be unusual, but worth a comment near cleanup() noting that resetting the metrics is intentional — otherwise a future reader might move the null-assignment out to "simplify" and introduce a subtle stale-metrics bug.

Origin check in onMessage:

if (!expectedOrigin || e.origin !== expectedOrigin) return;

Correct behavior, but !expectedOrigin is a fail-closed guard that's already warned about in attachProxyBridge. No issue here, just confirming the intent is right.

regionRectFromBox when box.w or box.h is negative: The function doesn't guard against negative box dimensions. The clamp in clampUnit would turn a negative w into 0, which could silently collapse the box instead of rejecting it. Probably fine given the drag spine ensures positive dimensions, but worth a note if regionRectFromBox is ever called from outside the drag path (it's exported).

Tests: Comprehensive and cover the key edge cases (zero-area cancel, tiny cancel still dropped, dedup, foreign-origin rejection, morphdom-wipe recovery). The dispatchTouch helper using MouseEvent + Object.defineProperty is a reasonable jsdom workaround.

Overall this is solid — the security posture (origin validation, fail-closed on unresolved origin, singleton guard with a clear diagnostic warning) is good. The touch recovery rationale is well-documented.

[adnaan]

Folded in the final non-blocking nits: simplified the onLostCapture condition (dropped the redundant m !== null), documented that clearing pageBridgeMetrics on cleanup is intentional, and made the exported regionRectFromBox reject a non-positive box. Thanks for the thorough review.

[claude]

Review: feat: proxy-bridge directive + touch drag recovery

Overall solid. Security posture is good and the logic holds up.

Touch recovery (attachBoxDragSelect)

• lastMove is set on pointerdown (not just pointermove), so it's guaranteed non-null for any active drag when onLostCapture fires — the comment is accurate and the zero-area test confirms the area threshold still guards stray cancels. Correct.

Proxy bridge security

• Origin validation is fail-closed: empty expectedOrigin rejects all inbound messages and blocks outbound postMessage. Good.
• postMessage into the iframe uses expectedOrigin (not "*"). Good.
• d.__prereview !== true guard is a lightweight discriminator — fine since the only sender is the trusted proxied page.
• Data coercion (Number(d.scrollX) || 0, typeof d.url === "string" ? d.url : "") safely handles adversarial payloads.

One subtle note — pageBridgeMetrics cleanup order
In handleProxyBridgeDirectives, the sweep loop calls cleanup() (which nulls pageBridgeMetrics), then the loop immediately re-arms a new entry. If a render triggers this teardown+re-arm in the same call, pageBridgeMetrics is null until the next beacon. regionRectFromBox already returns null in that case (harmless no-op drag), and the next scroll beacon repopulates it. Just worth being aware of in case the "locate" focus relay is also expected to work immediately after a re-arm — it doesn't depend on metrics so it's fine.

Minor

• The singleton guard logs a console.warn for a second bridge but silently continues — correct behavior since transient double-renders are the expected trigger.
• Tests cover all the important paths including the "morphdom-wipe recovery" and the "fails closed with no src" cases.

LGTM.