From ed4f6fe06072445dbb0d43a703e7826b04e46f9c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 22:58:21 +0500 Subject: [PATCH 01/18] feat(builder): one mapping between the canvas frame and the host The canvas renders in an iframe and the editor's chrome is drawn over it in the host document, so every overlay has to answer the same question: where is this canvas rectangle in host coordinates. Two modules answering it separately agree the day they are written and drift afterwards, and the symptom is an indicator a few pixels off the gap it names - which neither module's tests catch, because each is correct about the question it asked. The functions take plain numbers rather than DOM nodes, so the mapping is exercisable without a browser and the DOM reads stay at the edge. Scale applies to size as well as position: an overlay sized from the unscaled rectangle is correct at 100% and wrong everywhere else, and 100% is where it gets looked at. A frame that cannot describe a mapping throws. Every value that could stand in is wrong in a way that looks right - zero collapses the canvas to a point, a negative mirrors it, a non-finite yields NaN coordinates - and all three place an overlay somewhere wrong while reporting nothing. A sibling guard reads the AST for getBoundingClientRect and getClientRects and allows them only in this module, so the second implementation cannot arrive quietly. It carries its own positive control, because a visitor that matches nothing reports the same clean pass as one that found nothing. --- .../builder/src/geometry-ownership.test.ts | 122 +++++++++++++++++ packages/builder/src/geometry.test.ts | 88 ++++++++++++ packages/builder/src/geometry.ts | 127 ++++++++++++++++++ 3 files changed, 337 insertions(+) create mode 100644 packages/builder/src/geometry-ownership.test.ts create mode 100644 packages/builder/src/geometry.test.ts create mode 100644 packages/builder/src/geometry.ts diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts new file mode 100644 index 0000000000..bffac7f07c --- /dev/null +++ b/packages/builder/src/geometry-ownership.test.ts @@ -0,0 +1,122 @@ +/** + * Geometry crosses the frame in ONE module, and this is what says so. + * + * The editor's chrome is drawn in the host document over a canvas that lives in + * an iframe, so anything positioning an overlay has to convert between the two + * coordinate spaces. Two modules doing that conversion separately are correct + * about their own question and disagree about the shared one — the indicator + * lands a few pixels off the gap it names, and neither module's tests fail. + * + * A convention cannot hold that, because the second implementation looks + * reasonable in isolation and arrives when someone needs a rectangle and has a + * DOM node to hand. So it is checked: `getBoundingClientRect` may be read in + * `geometry.ts` and nowhere else in this package. + * + * Read from the AST rather than by matching text, so a call written as + * `el["getBoundingClientRect"]()` is seen as the same thing — the spelling that + * would slip past a search for the dotted form. + */ +import { readFileSync, readdirSync } from "node:fs"; +import { dirname, join, relative } from "node:path"; +import { fileURLToPath } from "node:url"; + +import ts from "typescript"; +import { describe, expect, it } from "vitest"; + +const SRC_DIR = dirname(fileURLToPath(import.meta.url)); + +/** Extensions the bundler follows, and therefore the ones this guard must read. */ +const BUNDLED_MODULE = /\.(?:tsx?|jsx?|mjs|cjs)$/; + +/** The one module allowed to convert between the frame and the host. */ +const GEOMETRY_MODULE = "geometry.ts"; + +/** + * Reads that cross the frame, and are therefore the ones to own in one place. + * + * `getBoundingClientRect` is the whole of it today. `getClientRects` is included + * because it answers the same question for a wrapped element and would be the + * natural way to write the second implementation. + */ +const CROSS_FRAME_READS = new Set(["getBoundingClientRect", "getClientRects"]); + +function sourceFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = join(dir, entry.name); + if (entry.isDirectory()) out.push(...sourceFiles(full)); + else if (BUNDLED_MODULE.test(entry.name)) out.push(full); + } + return out; +} + +/** Every cross-frame read a source text performs, by the name it used. */ +function crossFrameReads(text: string, file: string): string[] { + const source = ts.createSourceFile( + file, + text, + ts.ScriptTarget.Latest, + true, + file.endsWith("x") ? ts.ScriptKind.TSX : ts.ScriptKind.TS + ); + const found: string[] = []; + + const visit = (node: ts.Node): void => { + // `el.getBoundingClientRect()` + if (ts.isPropertyAccessExpression(node)) { + if (CROSS_FRAME_READS.has(node.name.text)) found.push(node.name.text); + } + // `el["getBoundingClientRect"]()` — the same call, spelled so a text search + // for the dotted form does not see it. + if ( + ts.isElementAccessExpression(node) && + ts.isStringLiteralLike(node.argumentExpression) && + CROSS_FRAME_READS.has(node.argumentExpression.text) + ) { + found.push(node.argumentExpression.text); + } + ts.forEachChild(node, visit); + }; + + visit(source); + return found; +} + +describe("geometry crosses the frame in one place", () => { + const files = sourceFiles(SRC_DIR); + + it("has files to check", () => { + // A guard that read nothing reports the same clean pass as one that read + // everything and found nothing, and a renamed directory is all it takes. + expect(files.length).toBeGreaterThan(0); + }); + + it("finds no cross-frame read outside the geometry module", () => { + const offenders = files + .filter(file => !file.endsWith(GEOMETRY_MODULE)) + .filter(file => !file.endsWith("geometry-ownership.test.ts")) + .flatMap(file => { + const reads = crossFrameReads(readFileSync(file, "utf8"), file); + return reads.map(read => `${relative(SRC_DIR, file)} reads ${read}`); + }); + + expect(offenders).toEqual([]); + }); + + it("can see a cross-frame read when there is one", () => { + // The positive control. Without it the assertion above passes just as + // happily against a visitor that never matches anything — which is the + // failure it would take longest to notice, because it looks like success. + const dotted = crossFrameReads( + "const r = el.getBoundingClientRect();", + "probe.ts" + ); + const bracketed = crossFrameReads( + 'const r = el["getBoundingClientRect"]();', + "probe.ts" + ); + + expect(dotted).toEqual(["getBoundingClientRect"]); + expect(bracketed).toEqual(["getBoundingClientRect"]); + }); +}); diff --git a/packages/builder/src/geometry.test.ts b/packages/builder/src/geometry.test.ts new file mode 100644 index 0000000000..f8e5879e60 --- /dev/null +++ b/packages/builder/src/geometry.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it } from "vitest"; + +import { + FrameGeometryError, + pointToCanvas, + pointToHost, + rectToHost, + type FrameGeometry, +} from "./geometry"; + +/** A frame offset from the host origin and zoomed out, so neither term is 1 or 0. */ +const FRAME: FrameGeometry = { origin: { x: 120, y: 64 }, scale: 0.5 }; + +/** The unscaled, unoffset case, which must not be the only one that works. */ +const IDENTITY: FrameGeometry = { origin: { x: 0, y: 0 }, scale: 1 }; + +describe("mapping a point across the frame", () => { + it("places a canvas point where the host sees it", () => { + expect(pointToHost({ x: 40, y: 20 }, FRAME)).toEqual({ x: 140, y: 74 }); + }); + + it("places a host point where the canvas sees it", () => { + expect(pointToCanvas({ x: 140, y: 74 }, FRAME)).toEqual({ x: 40, y: 20 }); + }); + + it.each<[string, FrameGeometry]>([ + ["offset and scaled", FRAME], + ["identity", IDENTITY], + ["scaled up", { origin: { x: -30, y: 12 }, scale: 2 }], + ["fractional scale", { origin: { x: 7.5, y: 0.25 }, scale: 1.75 }], + ])("round-trips a point unchanged: %s", (_label, frame) => { + const point = { x: 37, y: 91 }; + expect(pointToCanvas(pointToHost(point, frame), frame)).toEqual(point); + }); + + it("is not satisfied by returning its input", () => { + // The control for the round trip above, which a pair of identity functions + // would pass. At any scale or offset other than the identity the mapped + // point has to differ from the one given. + expect(pointToHost({ x: 40, y: 20 }, FRAME)).not.toEqual({ x: 40, y: 20 }); + }); +}); + +describe("mapping a rectangle across the frame", () => { + it("scales the size, not only the position", () => { + // An overlay sized from the unscaled rectangle is correct at 100% and wrong + // everywhere else — and 100% is where it gets looked at. + expect(rectToHost({ x: 40, y: 20, width: 200, height: 80 }, FRAME)).toEqual( + { + x: 140, + y: 74, + width: 100, + height: 40, + } + ); + }); + + it("leaves a rectangle alone under the identity frame", () => { + const rect = { x: 10, y: 20, width: 30, height: 40 }; + expect(rectToHost(rect, IDENTITY)).toEqual(rect); + }); +}); + +describe("a frame that describes no mapping", () => { + it.each<[string, FrameGeometry]>([ + ["zero scale", { origin: { x: 0, y: 0 }, scale: 0 }], + ["negative scale", { origin: { x: 0, y: 0 }, scale: -1 }], + [ + "infinite scale", + { origin: { x: 0, y: 0 }, scale: Number.POSITIVE_INFINITY }, + ], + ["NaN scale", { origin: { x: 0, y: 0 }, scale: Number.NaN }], + ["NaN origin", { origin: { x: Number.NaN, y: 0 }, scale: 1 }], + ])("refuses rather than mapping to nowhere: %s", (_label, frame) => { + // Each of these has a plausible-looking answer — a point, a mirror image, + // `NaN` — and every one of them puts the overlay somewhere wrong without + // reporting anything. + expect(() => pointToHost({ x: 1, y: 1 }, frame)).toThrow( + FrameGeometryError + ); + expect(() => pointToCanvas({ x: 1, y: 1 }, frame)).toThrow( + FrameGeometryError + ); + expect(() => + rectToHost({ x: 1, y: 1, width: 1, height: 1 }, frame) + ).toThrow(FrameGeometryError); + }); +}); diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts new file mode 100644 index 0000000000..c07b127f36 --- /dev/null +++ b/packages/builder/src/geometry.ts @@ -0,0 +1,127 @@ +/** + * The one place geometry crosses between the canvas frame and the host page. + * + * The canvas renders inside an iframe and the editor's chrome — the insertion + * indicator, selection outlines, drag affordances — is drawn in the host + * document above it. Every one of those has to answer the same question: where + * is this canvas rectangle, in host coordinates? + * + * **Asked here and nowhere else.** Two modules computing that separately agree + * on the day they are written and drift the first time anything changes — a + * scroll offset one of them forgot, a zoom the other did not apply — and the + * symptom is an indicator drawn a few pixels off the gap it names. That class of + * bug is not caught by either module's own tests, because each is correct about + * the question it asked. A sibling test asserts that no other module in this + * package reads a rectangle across the frame. + * + * The functions are pure and take plain numbers rather than DOM nodes, so the + * mapping can be exercised without a browser and the DOM reads stay at the edge. + * + * @module geometry + */ + +/** A point in whichever coordinate space the caller is working in. */ +export interface Point { + readonly x: number; + readonly y: number; +} + +/** A rectangle, in the shape `getBoundingClientRect` reports. */ +export interface Rect { + readonly x: number; + readonly y: number; + readonly width: number; + readonly height: number; +} + +/** + * How the canvas frame sits inside the host page. + * + * `origin` is where the frame's own viewport origin lands in host coordinates — + * the frame element's position, already including any host scrolling, because + * that is what `getBoundingClientRect` reports. `scale` is the visual scale the + * host applies to the frame: a zoomed-out canvas at 50% has `scale: 0.5`. + * + * Scroll INSIDE the frame is deliberately not a field. A rectangle read from + * inside the frame is already relative to the frame's viewport, so subtracting + * its scroll would count it twice — the mistake that makes an overlay drift as + * the canvas scrolls, rather than being wrong by a constant. + */ +export interface FrameGeometry { + readonly origin: Point; + readonly scale: number; +} + +/** + * A frame geometry that cannot describe a mapping. + * + * Thrown rather than defaulted, because every value that could stand in is + * wrong in a way that looks right: a scale of zero maps the whole canvas onto a + * point, a negative one mirrors it, and a non-finite one yields `NaN` + * coordinates that place an overlay nowhere and report no error. An overlay + * silently drawn at the wrong place is the exact failure this module exists to + * prevent, so an unusable frame has to be loud. + */ +export class FrameGeometryError extends Error { + constructor(message: string) { + super(message); + this.name = "FrameGeometryError"; + } +} + +function assertUsable(frame: FrameGeometry): void { + if (!Number.isFinite(frame.scale) || frame.scale <= 0) { + throw new FrameGeometryError( + `A frame scale of ${String(frame.scale)} describes no mapping. ` + + `Scale must be finite and greater than zero.` + ); + } + if (!Number.isFinite(frame.origin.x) || !Number.isFinite(frame.origin.y)) { + throw new FrameGeometryError( + `A frame origin of (${String(frame.origin.x)}, ${String(frame.origin.y)}) ` + + `describes no mapping. Both coordinates must be finite.` + ); + } +} + +/** A point inside the canvas frame, in host coordinates. */ +export function pointToHost(point: Point, frame: FrameGeometry): Point { + assertUsable(frame); + return { + x: frame.origin.x + point.x * frame.scale, + y: frame.origin.y + point.y * frame.scale, + }; +} + +/** + * A point on the host page, in the canvas frame's coordinates. + * + * The exact inverse of {@link pointToHost}: a pointer event arrives in host + * coordinates and has to be compared against rectangles read inside the frame, + * which is the same mapping run backwards rather than a second one written to + * match. + */ +export function pointToCanvas(point: Point, frame: FrameGeometry): Point { + assertUsable(frame); + return { + x: (point.x - frame.origin.x) / frame.scale, + y: (point.y - frame.origin.y) / frame.scale, + }; +} + +/** + * A rectangle inside the canvas frame, in host coordinates. + * + * Its size scales with the frame. An overlay sized from the unscaled rectangle + * would be correct only at 100%, and would look correct there — which is how a + * zoom bug survives review. + */ +export function rectToHost(rect: Rect, frame: FrameGeometry): Rect { + const origin = pointToHost({ x: rect.x, y: rect.y }, frame); + return { + x: origin.x, + y: origin.y, + width: rect.width * frame.scale, + height: rect.height * frame.scale, + }; +} From 7852bb1902a8c68060da009d1f7b045b730b4ca6 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 22:59:25 +0500 Subject: [PATCH 02/18] chore(release): add the changeset for the frame geometry module --- .changeset/builder-frame-geometry.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .changeset/builder-frame-geometry.md diff --git a/.changeset/builder-frame-geometry.md b/.changeset/builder-frame-geometry.md new file mode 100644 index 0000000000..d873ebe6ce --- /dev/null +++ b/.changeset/builder-frame-geometry.md @@ -0,0 +1,27 @@ +--- +"nextly": patch +"create-nextly-app": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/ui": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/builder": patch +--- + +Add the builder's host-canvas coordinate mapping: one module converts between the canvas frame and the host page, with a guard that no other module reads a rectangle across the frame. From b3a17c0fb3d7c119de2226a3e8756d61772de72c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 00:03:07 +0500 Subject: [PATCH 03/18] fix(builder): match the geometry module exactly, and let e2e measure against it Two ways the one-mapping claim was wider than what it enforced. The ownership guard exempted by suffix, so overlay-geometry.ts and nested/frame-geometry.ts were allowed to read a rectangle across the frame - which are the names a second implementation would actually be given. Matched by basename equality now, with a case pinning that the three names are told apart. And the mapping already existed. e2e/tests/canvas/coordinate-mapping.ts carries the same three conversions with the same arithmetic, and both the acceptance spec and the canvas driver use it, so a module added under the heading of one mapping was the second one. The e2e helper now adapts the editor's functions rather than restating them: same arithmetic, a call shape a Playwright test can supply. A browser harness with its own copy certifies its own copy - the two agree until either is corrected, and then the suite validates a stale implementation while reporting the editor is fine. Behaviour changes for that helper in one place: a frame that describes no mapping now throws instead of yielding NaN coordinates. No caller passes such a frame; a test measuring an unrendered element gets an error naming the problem rather than an assertion about meaningless numbers. --- e2e/package.json | 1 + e2e/tests/canvas/coordinate-mapping.ts | 75 +++++++++++-------- .../builder/src/geometry-ownership.test.ts | 36 ++++++++- packages/builder/src/index.ts | 18 +++++ pnpm-lock.yaml | 3 + 5 files changed, 97 insertions(+), 36 deletions(-) diff --git a/e2e/package.json b/e2e/package.json index 3390b2fa2d..0a24c1816a 100644 --- a/e2e/package.json +++ b/e2e/package.json @@ -17,6 +17,7 @@ "@nextlyhq/adapter-drizzle": "workspace:*", "@nextlyhq/adapter-sqlite": "workspace:*", "@nextlyhq/admin": "workspace:*", + "@nextlyhq/builder": "workspace:*", "@nextlyhq/eslint-config": "workspace:*", "@nextlyhq/plugin-form-builder": "workspace:*", "@nextlyhq/plugin-page-builder": "workspace:*", diff --git a/e2e/tests/canvas/coordinate-mapping.ts b/e2e/tests/canvas/coordinate-mapping.ts index 50f26ce433..3d906af676 100644 --- a/e2e/tests/canvas/coordinate-mapping.ts +++ b/e2e/tests/canvas/coordinate-mapping.ts @@ -1,58 +1,69 @@ +/** + * The canvas↔host mapping the acceptance tests measure against. + * + * **Adapts the editor's own mapping rather than restating it.** The arithmetic + * lives once, in `@nextlyhq/builder`, and this file only changes the call shape: + * these helpers take `(value, frameOrigin, scale)` because that is how a + * Playwright test has the numbers to hand — origin from the frame element's + * box, scale read off the page — while the editor holds them together as one + * `FrameGeometry`. + * + * The reason it is an adapter and not a copy is what the tests are FOR. A + * browser harness carrying its own arithmetic certifies its own arithmetic: the + * two agree on the day they are written, and the first correction to either + * makes the acceptance suite validate a stale copy while reporting that the + * editor is fine. That failure is invisible, because both sides are + * individually self-consistent. + * + * A consequence worth knowing before it surprises someone: a frame that cannot + * describe a mapping — a zero, negative or non-finite scale — now THROWS rather + * than returning `NaN` coordinates, because that is what the editor's mapping + * does. A test measuring an unrendered element gets an error naming the problem + * instead of an assertion failure about numbers that were never meaningful. + */ +import { + pointToCanvas, + pointToHost, + rectToHost, + type FrameGeometry, +} from "@nextlyhq/builder"; + import type { Point, Rect } from "./driver"; +/** The two numbers a Playwright test has, in the shape the editor's mapping takes. */ +function frame(frameOrigin: Point, scale: number): FrameGeometry { + return { origin: frameOrigin, scale }; +} + /** * Convert a point inside the canvas frame to the host document's coordinates. * - * Kept as a pure function so it can be tested against browser-reported - * geometry rather than inferred from whether an overlay happens to look right. - * - * A frame-local point is scaled by whatever transform the frame carries and - * then offset by the frame's own position in the host. The scale term is not - * optional: a canvas offering zoom-to-fit is exactly the case dnd-kit #1706 - * covered, and omitting it puts the overlay progressively further out the - * further a point sits from the frame's transform origin. + * The scale term is not optional: a canvas offering zoom-to-fit is exactly the + * case dnd-kit #1706 covered, and omitting it puts the overlay progressively + * further out the further a point sits from the frame's transform origin. */ export function mapFramePointToHost( framePoint: Point, frameOrigin: Point, scale = 1 ): Point { - return { - x: frameOrigin.x + framePoint.x * scale, - y: frameOrigin.y + framePoint.y * scale, - }; + return pointToHost(framePoint, frame(frameOrigin, scale)); } -/** - * The inverse: a host point expressed in the canvas's own coordinates. - * - * Both directions are needed and neither is optional. Drawing an overlay in - * parent chrome maps canvas -> host; deciding which block sits under the - * pointer maps host -> canvas. A canvas that implements only one ends up - * open-coding the other at the call site, which is how the two drift apart. - */ +/** Convert a host-document point back into the canvas frame's coordinates. */ export function mapHostPointToFrame( hostPoint: Point, frameOrigin: Point, scale = 1 ): Point { - return { - x: (hostPoint.x - frameOrigin.x) / scale, - y: (hostPoint.y - frameOrigin.y) / scale, - }; + return pointToCanvas(hostPoint, frame(frameOrigin, scale)); } -/** The same mapping for a rect, so an indicator can be drawn in parent chrome. */ +/** Convert a rectangle inside the frame to the host document's coordinates. */ export function mapFrameRectToHost( frameRect: Rect, frameOrigin: Point, scale = 1 ): Rect { - const topLeft = mapFramePointToHost(frameRect, frameOrigin, scale); - return { - x: topLeft.x, - y: topLeft.y, - width: frameRect.width * scale, - height: frameRect.height * scale, - }; + return rectToHost(frameRect, frame(frameOrigin, scale)); } diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index bffac7f07c..67aced3bea 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -17,7 +17,7 @@ * would slip past a search for the dotted form. */ import { readFileSync, readdirSync } from "node:fs"; -import { dirname, join, relative } from "node:path"; +import { basename, dirname, join, relative } from "node:path"; import { fileURLToPath } from "node:url"; import ts from "typescript"; @@ -28,9 +28,25 @@ const SRC_DIR = dirname(fileURLToPath(import.meta.url)); /** Extensions the bundler follows, and therefore the ones this guard must read. */ const BUNDLED_MODULE = /\.(?:tsx?|jsx?|mjs|cjs)$/; -/** The one module allowed to convert between the frame and the host. */ +/** + * The one module allowed to convert between the frame and the host, matched by + * its exact file name. + * + * By basename EQUALITY rather than by suffix. `endsWith("geometry.ts")` also + * exempts `overlay-geometry.ts` and `nested/frame-geometry.ts` — precisely the + * names a second implementation would be given — so a guard written that way + * admits the case it exists to refuse. + */ const GEOMETRY_MODULE = "geometry.ts"; +/** This file, which necessarily names the reads it is looking for. */ +const OWN_TEST = "geometry-ownership.test.ts"; + +/** Whether a path IS the named module, rather than merely ending with its name. */ +function isModule(file: string, name: string): boolean { + return basename(file) === name; +} + /** * Reads that cross the frame, and are therefore the ones to own in one place. * @@ -93,8 +109,8 @@ describe("geometry crosses the frame in one place", () => { it("finds no cross-frame read outside the geometry module", () => { const offenders = files - .filter(file => !file.endsWith(GEOMETRY_MODULE)) - .filter(file => !file.endsWith("geometry-ownership.test.ts")) + .filter(file => !isModule(file, GEOMETRY_MODULE)) + .filter(file => !isModule(file, OWN_TEST)) .flatMap(file => { const reads = crossFrameReads(readFileSync(file, "utf8"), file); return reads.map(read => `${relative(SRC_DIR, file)} reads ${read}`); @@ -103,6 +119,18 @@ describe("geometry crosses the frame in one place", () => { expect(offenders).toEqual([]); }); + it("exempts the geometry module by name, not by suffix", () => { + // The allowance has to be exactly as wide as the thing allowed. A suffix + // test also exempts `overlay-geometry.ts` and `nested/frame-geometry.ts`, + // which are the names a second implementation would actually be given — so + // the guard would wave through the duplicate it exists to catch. + expect(isModule("/a/b/geometry.ts", GEOMETRY_MODULE)).toBe(true); + expect(isModule("/a/b/overlay-geometry.ts", GEOMETRY_MODULE)).toBe(false); + expect(isModule("/a/nested/frame-geometry.ts", GEOMETRY_MODULE)).toBe( + false + ); + }); + it("can see a cross-frame read when there is one", () => { // The positive control. Without it the assertion above passes just as // happily against a visitor that never matches anything — which is the diff --git a/packages/builder/src/index.ts b/packages/builder/src/index.ts index eed78064d0..e6f3f8b2d6 100644 --- a/packages/builder/src/index.ts +++ b/packages/builder/src/index.ts @@ -41,3 +41,21 @@ * than with a constant nothing reads yet. */ export const BUILDER_PACKAGE_NAME = "@nextlyhq/builder" as const; + +/** + * The one mapping between the canvas frame and the host page. + * + * Exported because the acceptance harness measures against the SAME arithmetic + * the editor positions with. A browser test carrying its own copy certifies its + * own stale copy, and would keep passing through exactly the correction it + * exists to catch. + */ +export { + FrameGeometryError, + pointToCanvas, + pointToHost, + rectToHost, + type FrameGeometry, + type Point, + type Rect, +} from "./geometry"; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 5fd20bbc9b..215cb335ba 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -262,6 +262,9 @@ importers: '@nextlyhq/admin': specifier: workspace:* version: link:../packages/admin + '@nextlyhq/builder': + specifier: workspace:* + version: link:../packages/builder '@nextlyhq/eslint-config': specifier: workspace:* version: link:../packages/eslint-config From 4c5645632aeb8c05fcc88ee9c0489b81d01f69a1 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 00:56:45 +0500 Subject: [PATCH 04/18] docs(builder): say what the geometry guard checks, and what review holds The guard was presented as enforcing one mapping. It enforces one place where a rectangle is READ across the frame, which is narrower: a module handed an origin and a scale can open-code the arithmetic without touching the DOM, and a duplicate implementation added beside it passes every assertion. Two numbers multiplied and added are indistinguishable from any other code, so no scan can tell a second mapping from ordinary arithmetic - the same reason the builder's draws-with-blocks-react rule is a review-time convention rather than a checked one. The test is renamed to what it checks, and both it and the module say which half review is holding. Narrowing the claim rather than the guard: the DOM read is the door this can actually hold, and a guard advertised wider than it reaches is worse than one that says where it stops. --- .../builder/src/geometry-ownership.test.ts | 18 ++++++++++++++++-- packages/builder/src/geometry.ts | 11 +++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index 67aced3bea..6c240bf13e 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -1,5 +1,19 @@ /** - * Geometry crosses the frame in ONE module, and this is what says so. + * Rectangles are READ across the frame in one module, and this is what says so. + * + * ⚠️ Read the name carefully, because it is narrower than the invariant it + * serves. This checks where a rectangle is READ. It does NOT check where the + * mapping is COMPUTED: a module handed an origin and a scale can open-code + * `origin.x + point.x * scale` without touching the DOM at all, and nothing + * here would see it. A duplicate mapping added alongside this file passes every + * assertion below. + * + * That half is a review-time convention rather than a checked one, for the same + * reason the builder's "draws with `blocks-react`" rule is: arithmetic on two + * numbers is indistinguishable from any other arithmetic, so no scan can tell a + * second implementation from ordinary code. What IS checkable is the DOM read + * that a mapping needs its inputs from, and narrowing the guard to that leaves + * the door it can actually hold. * * The editor's chrome is drawn in the host document over a canvas that lives in * an iframe, so anything positioning an overlay has to convert between the two @@ -98,7 +112,7 @@ function crossFrameReads(text: string, file: string): string[] { return found; } -describe("geometry crosses the frame in one place", () => { +describe("rectangles are read across the frame in one place", () => { const files = sourceFiles(SRC_DIR); it("has files to check", () => { diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts index c07b127f36..5d684ddd93 100644 --- a/packages/builder/src/geometry.ts +++ b/packages/builder/src/geometry.ts @@ -11,8 +11,15 @@ * scroll offset one of them forgot, a zoom the other did not apply — and the * symptom is an indicator drawn a few pixels off the gap it names. That class of * bug is not caught by either module's own tests, because each is correct about - * the question it asked. A sibling test asserts that no other module in this - * package reads a rectangle across the frame. + * the question it asked. + * + * How much of that is ENFORCED, stated plainly because the difference matters: + * a sibling test refuses a rectangle READ across the frame anywhere else in this + * package, and the e2e helper adapts these functions rather than restating them. + * Neither can stop a module that is handed an origin and a scale from + * open-coding the arithmetic — two numbers multiplied and added look like any + * other code — so that half is held at review, the same way the builder's + * "draws with `blocks-react`" rule is. * * The functions are pure and take plain numbers rather than DOM nodes, so the * mapping can be exercised without a browser and the DOM reads stay at the edge. From 41627354c30e30c7ce387a86dfb91890094b765c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 01:25:58 +0500 Subject: [PATCH 05/18] fix(builder): match the geometry module by path, and typecheck e2e without a build Two more spellings the guard let through, and a build I made e2e depend on. The ownership allowance was narrowed from suffix to basename last round, and a basename test still exempts overlays/geometry.ts - a nested module whose file name is identical, which is exactly what a second implementation would be called. Each narrowing left one more spelling open, so the allowance is now the one relative path and no choice of filename can widen it. That is the third iteration on this guard; matching a shape of name was the wrong instrument rather than the wrong pattern. And importing @nextlyhq/builder from the e2e helper broke the standalone typecheck: check-types deliberately does not build dependencies, so resolving through the package manifest needs a dist a freshly installed tree does not have. Mapped to source in e2e's tsconfig, which is what packages/admin already does for nextly. Verified by removing dist and running e2e's check-types: clean with the mapping, TS2307 without it. --- e2e/tsconfig.json | 11 +++++- .../builder/src/geometry-ownership.test.ts | 34 +++++++++++-------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/e2e/tsconfig.json b/e2e/tsconfig.json index bdf5ce9a23..7e75652bd2 100644 --- a/e2e/tsconfig.json +++ b/e2e/tsconfig.json @@ -8,7 +8,16 @@ // The base config leaves this off, and without it the `.mjs` below is // included and then skipped — which is the same silent gap the comment // under `include` exists to avoid. - "allowJs": true + "allowJs": true, + "baseUrl": ".", + // The builder is imported for its coordinate mapping, and `check-types` + // deliberately does not build dependencies first — so resolving the package + // through its manifest would need a `dist` that a freshly installed tree + // does not have. Mapped to source instead, which is what `packages/admin` + // does for `nextly`. + "paths": { + "@nextlyhq/builder": ["../packages/builder/src"] + } }, // Everything, deliberately. The two page-builder specs that rotted did so // because their package's tsconfig only included `src/**/*`, so an import of diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index 6c240bf13e..df4bf756dc 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -31,7 +31,7 @@ * would slip past a search for the dotted form. */ import { readFileSync, readdirSync } from "node:fs"; -import { basename, dirname, join, relative } from "node:path"; +import { dirname, join, relative } from "node:path"; import { fileURLToPath } from "node:url"; import ts from "typescript"; @@ -46,19 +46,20 @@ const BUNDLED_MODULE = /\.(?:tsx?|jsx?|mjs|cjs)$/; * The one module allowed to convert between the frame and the host, matched by * its exact file name. * - * By basename EQUALITY rather than by suffix. `endsWith("geometry.ts")` also - * exempts `overlay-geometry.ts` and `nested/frame-geometry.ts` — precisely the - * names a second implementation would be given — so a guard written that way - * admits the case it exists to refuse. + * By its RELATIVE PATH, not by any part of its name. A suffix test also exempts + * `overlay-geometry.ts`; a basename test still exempts `overlays/geometry.ts`. + * Both are names a second implementation would plausibly be given, and each + * narrowing let exactly one more spelling through — so the allowance is the one + * path itself, which no choice of filename can widen. */ const GEOMETRY_MODULE = "geometry.ts"; /** This file, which necessarily names the reads it is looking for. */ const OWN_TEST = "geometry-ownership.test.ts"; -/** Whether a path IS the named module, rather than merely ending with its name. */ -function isModule(file: string, name: string): boolean { - return basename(file) === name; +/** Whether a file IS the named module, by its path beneath `src`. */ +function isModule(file: string, relativePath: string): boolean { + return relative(SRC_DIR, file) === relativePath; } /** @@ -133,16 +134,21 @@ describe("rectangles are read across the frame in one place", () => { expect(offenders).toEqual([]); }); - it("exempts the geometry module by name, not by suffix", () => { + it("exempts one path, not a family of names", () => { // The allowance has to be exactly as wide as the thing allowed. A suffix // test also exempts `overlay-geometry.ts` and `nested/frame-geometry.ts`, // which are the names a second implementation would actually be given — so // the guard would wave through the duplicate it exists to catch. - expect(isModule("/a/b/geometry.ts", GEOMETRY_MODULE)).toBe(true); - expect(isModule("/a/b/overlay-geometry.ts", GEOMETRY_MODULE)).toBe(false); - expect(isModule("/a/nested/frame-geometry.ts", GEOMETRY_MODULE)).toBe( - false - ); + expect(isModule(join(SRC_DIR, "geometry.ts"), GEOMETRY_MODULE)).toBe(true); + expect( + isModule(join(SRC_DIR, "overlay-geometry.ts"), GEOMETRY_MODULE) + ).toBe(false); + // The spelling a basename test still admitted: a nested module whose file + // name is identical. Each narrowing left one more open, which is why the + // allowance is now the path rather than a shape of name. + expect( + isModule(join(SRC_DIR, "overlays", "geometry.ts"), GEOMETRY_MODULE) + ).toBe(false); }); it("can see a cross-frame read when there is one", () => { From 1771e39745a2eb5abb0033817b5388d0bd9a18ae Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 01:58:34 +0500 Subject: [PATCH 06/18] fix(builder): follow every source extension, and map from the content origin The guard's extension pattern omitted .mts and .cts, so a module written with either was invisible to it - a bypass file with that extension passed all four ownership tests. The same pattern was copy-pasted into layering.test.ts, so the hole was in both guards; the list now lives in one importless module both read, because two copies of what counts as a source module drift the moment TypeScript grows an extension. Verified by adding the .mts bypass: caught by name, and gone when the file is removed. And origin was documented as the frame's viewport but taken from getBoundingClientRect, which reports the BORDER box. Rectangles read inside the frame are relative to the content viewport, so on a frame with any border the two differ by clientLeft/clientTop and every mapped point lands a scaled couple of pixels out. A canvas that does not reset the browser's default iframe border has that from the first render, and it reads as the indicator feeling slightly off rather than as a fault. The correction belongs to whoever reads the DOM, since this module takes numbers so it can run without a browser - so origin is now documented as the CONTENT origin and the two measurement sites satisfy it: the canvas driver and the acceptance spec both add clientLeft/clientTop. The fixture sets border: none, which is why the border-box version passed; the contract is now stated where the next measurement will read it. --- e2e/tests/canvas/coordinate-mapping.spec.ts | 19 ++++++++++++++--- e2e/tests/canvas/poc-driver.ts | 19 +++++++++++++++-- .../builder/src/geometry-ownership.test.ts | 4 +--- packages/builder/src/geometry.ts | 21 +++++++++++++++---- packages/builder/src/layering.test.ts | 2 +- packages/builder/src/source-modules.ts | 18 ++++++++++++++++ 6 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 packages/builder/src/source-modules.ts diff --git a/e2e/tests/canvas/coordinate-mapping.spec.ts b/e2e/tests/canvas/coordinate-mapping.spec.ts index 0a97a643df..9d7592c0bf 100644 --- a/e2e/tests/canvas/coordinate-mapping.spec.ts +++ b/e2e/tests/canvas/coordinate-mapping.spec.ts @@ -50,10 +50,23 @@ async function mapped(page: import("@playwright/test").Page, scale: number) { }, PROBE_ID); expect(frameRect).not.toBeNull(); - const origin = await page.locator("iframe").boundingBox(); + const frameElement = page.locator("iframe"); + const origin = await frameElement.boundingBox(); expect(origin).not.toBeNull(); - - return mapFrameRectToHost(frameRect!, { x: origin!.x, y: origin!.y }, scale); + // The content origin: `boundingBox()` gives the border box, and rectangles + // read inside the frame are relative to the content viewport. The two agree + // only while the fixture keeps `border: none`, which is why measuring against + // the border box passed here and would drift on any bordered canvas. + const inset = await frameElement.evaluate(el => ({ + left: (el as HTMLIFrameElement).clientLeft, + top: (el as HTMLIFrameElement).clientTop, + })); + + return mapFrameRectToHost( + frameRect!, + { x: origin!.x + inset.left, y: origin!.y + inset.top }, + scale + ); } /** Largest absolute difference across all four rect components. */ diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index 07065cb2ca..ca8d4a5a83 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -156,9 +156,24 @@ export function createPocDriver(page: Page): CanvasDriver { }, async frameOrigin() { - const box = await page.locator("iframe").boundingBox(); + const frame = page.locator("iframe"); + const box = await frame.boundingBox(); if (!box) throw new Error("canvas iframe has no box"); - return { x: box.x, y: box.y }; + // The CONTENT origin, not the border-box corner. `boundingBox()` reports + // the border box, while every rectangle read inside the frame is relative + // to the content viewport — so on a frame with any border the two differ + // by `clientLeft`/`clientTop` and every mapped point lands a couple of + // pixels out. A canvas that does not reset the browser's default iframe + // border has that gap from the first render, and it reads as "the + // indicator feels slightly off" rather than as a fault. + const inset = await frame.evaluate< + { left: number; top: number }, + HTMLIFrameElement + >(el => ({ + left: el.clientLeft, + top: el.clientTop, + })); + return { x: box.x + inset.left, y: box.y + inset.top }; }, async readBlockBoxes() { diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index df4bf756dc..d43c1f0ddb 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -36,12 +36,10 @@ import { fileURLToPath } from "node:url"; import ts from "typescript"; import { describe, expect, it } from "vitest"; +import { BUNDLED_MODULE } from "./source-modules"; const SRC_DIR = dirname(fileURLToPath(import.meta.url)); -/** Extensions the bundler follows, and therefore the ones this guard must read. */ -const BUNDLED_MODULE = /\.(?:tsx?|jsx?|mjs|cjs)$/; - /** * The one module allowed to convert between the frame and the host, matched by * its exact file name. diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts index 5d684ddd93..4d71baf1cb 100644 --- a/packages/builder/src/geometry.ts +++ b/packages/builder/src/geometry.ts @@ -44,10 +44,23 @@ export interface Rect { /** * How the canvas frame sits inside the host page. * - * `origin` is where the frame's own viewport origin lands in host coordinates — - * the frame element's position, already including any host scrolling, because - * that is what `getBoundingClientRect` reports. `scale` is the visual scale the - * host applies to the frame: a zoomed-out canvas at 50% has `scale: 0.5`. + * `origin` is where the frame's CONTENT viewport lands in host coordinates, and + * the word content is load-bearing. `getBoundingClientRect` on an iframe reports + * its BORDER box, while every rectangle read inside the frame is relative to the + * content viewport — so on a frame with any border the two differ by + * `clientLeft`/`clientTop`, and an overlay built from the border box sits a + * couple of scaled pixels out at every point. A canvas that never sets + * `border: none` gets the browser default and the fault is present from the + * first render, which is exactly the sort of near-miss that reads as "the + * indicator feels slightly off" rather than as a bug. + * + * That correction belongs to whoever reads the DOM, not here: this module takes + * numbers so it can be exercised without a browser, and the caller that measured + * the frame is the only one holding `clientLeft`. See the e2e helper for the + * measurement that satisfies this contract. + * + * `scale` is the visual scale the host applies to the frame: a zoomed-out canvas + * at 50% has `scale: 0.5`. * * Scroll INSIDE the frame is deliberately not a field. A rectangle read from * inside the frame is already relative to the frame's viewport, so subtracting diff --git a/packages/builder/src/layering.test.ts b/packages/builder/src/layering.test.ts index e400b6a92f..d670167e06 100644 --- a/packages/builder/src/layering.test.ts +++ b/packages/builder/src/layering.test.ts @@ -4,6 +4,7 @@ import { fileURLToPath } from "node:url"; import ts from "typescript"; import { describe, expect, it } from "vitest"; +import { BUNDLED_MODULE } from "./source-modules"; /** * The package's layering contract, enforced rather than documented. @@ -103,7 +104,6 @@ const UNRESOLVABLE_SPECIFIER = ""; * bundles it; a scan restricted to TypeScript would walk past the one file free to import * anything, with the typecheck none the wiser because `allowJs` is off. */ -const BUNDLED_MODULE = /\.(?:tsx?|jsx?|mjs|cjs)$/; function sourceFiles(dir: string): string[] { const out: string[] = []; diff --git a/packages/builder/src/source-modules.ts b/packages/builder/src/source-modules.ts new file mode 100644 index 0000000000..29ef619406 --- /dev/null +++ b/packages/builder/src/source-modules.ts @@ -0,0 +1,18 @@ +/** + * Which files the bundler will follow, as one answer both guards ask. + * + * The layering guard and the geometry-ownership guard each walk `src` looking + * for files to inspect, and each was written with its own copy of this pattern. + * Two copies of "what counts as a source module" drift the moment TypeScript + * grows an extension — and a guard that walks past a file reports clean about + * code it never read, which is the failure mode both of them exist to prevent. + * + * `.mts` and `.cts` are here because TypeScript and tsup follow them. They were + * missing from both copies, so a module written with either extension was + * invisible to every check in this package. + * + * This module imports nothing on purpose. It is reached from test files, and a + * shared helper that pulled in `node:fs` would put a Node import inside `src` + * where the layering guard is entitled to refuse it. + */ +export const BUNDLED_MODULE = /\.(?:[cm]?tsx?|[cm]?jsx?)$/; From 6fe5d98c44d9b5a5a57efbe172bf93436a1b9cc6 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 02:42:36 +0500 Subject: [PATCH 07/18] fix(builder): decide test files from one list the runner also reads The layering guard relaxes its import allowlist for anything it considers a test, and the runner decided the same question separately. Discovery had been widened to .mts/.cts while both the guard's name pattern and the vitest globs still said .ts/.tsx, so a probe.test.mts was classified as a test, exempted from the allowlist, and never run. That direction is the dangerous one: a shipped module could reach any import it liked by choosing its filename. All three now derive from one extension list, so the guard and the runner cannot mean different things by "test". Listing the extensions also drops .mtsx and .ctsx, which the previous pattern admitted and no compiler follows. --- packages/builder/src/layering.test.ts | 55 +++++++++++++- .../builder/src/module-extensions.test.mts | 37 ++++++++++ packages/builder/src/source-modules.ts | 71 ++++++++++++++++--- packages/builder/vitest.config.ts | 11 ++- 4 files changed, 159 insertions(+), 15 deletions(-) create mode 100644 packages/builder/src/module-extensions.test.mts diff --git a/packages/builder/src/layering.test.ts b/packages/builder/src/layering.test.ts index d670167e06..6e7d7a1395 100644 --- a/packages/builder/src/layering.test.ts +++ b/packages/builder/src/layering.test.ts @@ -4,7 +4,7 @@ import { fileURLToPath } from "node:url"; import ts from "typescript"; import { describe, expect, it } from "vitest"; -import { BUNDLED_MODULE } from "./source-modules"; +import { BUNDLED_MODULE, TEST_MODULE } from "./source-modules"; /** * The package's layering contract, enforced rather than documented. @@ -493,7 +493,7 @@ describe("the builder's layering contract", () => { it("imports only what the contract allows", () => { const violations: string[] = []; for (const file of files) { - const inTest = /\.test\.tsx?$/.test(file); + const inTest = TEST_MODULE.test(file); for (const specifier of importsOf(file).filter(isBare)) { if (!isAllowed(specifier, inTest)) { violations.push(`${file}: ${specifier}`); @@ -503,4 +503,55 @@ describe("the builder's layering contract", () => { expect(violations).toEqual([]); }); + + it("relaxes its allowlist only for files the runner actually runs", () => { + // The test above trusts `TEST_MODULE` to say which files may import + // `vitest` and `node:fs`. That trust is only sound while vitest RUNS + // everything `TEST_MODULE` matches: a config listing narrower globs would + // leave a file classified as a test, exempt from the allowlist, and never + // executed — so a shipped module could reach anything it liked by choosing + // its filename. + // + // Both now derive from one list, and this checks the config still asks for + // it rather than restating it. The assertion is syntactic because the + // question is syntactic: whether this file derives its globs or spells them + // out again. Importing the config to compare values is not available — + // `rootDir` is `src`, and reaching outside it fails `check-types` (TS6059). + const configPath = join(SRC_DIR, "..", "vitest.config.ts"); + const config = ts.createSourceFile( + "vitest.config.ts", + readFileSync(configPath, "utf8"), + ts.ScriptTarget.ESNext, + true + ); + + let includeInitialiser: ts.Expression | undefined; + const visit = (node: ts.Node): void => { + if ( + ts.isPropertyAssignment(node) && + ts.isIdentifier(node.name) && + node.name.text === "include" + ) { + includeInitialiser = node.initializer; + } + ts.forEachChild(node, visit); + }; + visit(config); + + // Positive control: an `include` that stopped being found would make the + // assertion below vacuous, and this guard exists because a check that + // passes on nothing is the failure mode the package keeps paying for. + expect( + includeInitialiser, + "vitest.config.ts must set `include`" + ).toBeDefined(); + expect( + includeInitialiser && ts.isIdentifier(includeInitialiser) + ? includeInitialiser.text + : config.text.slice( + includeInitialiser!.getStart(config), + includeInitialiser!.getEnd() + ) + ).toBe("TEST_GLOBS"); + }); }); diff --git a/packages/builder/src/module-extensions.test.mts b/packages/builder/src/module-extensions.test.mts new file mode 100644 index 0000000000..803da8a1f2 --- /dev/null +++ b/packages/builder/src/module-extensions.test.mts @@ -0,0 +1,37 @@ +import { expect, it } from "vitest"; + +import { BUNDLED_MODULE, TEST_MODULE } from "./source-modules"; + +/** + * A positive control for the extension list, written in one of the extensions + * it exists to cover. + * + * Three things have to agree about `.mts`: the guards must WALK it, the layering + * guard must CLASSIFY it as a test so it may import `vitest`, and the runner + * must RUN it. The first two are asserted below. The third cannot be asserted + * from inside — it is proved by this file executing at all, which is why it is + * `.mts` rather than a `.ts` file with `.mts` in a string. + * + * Without it, narrowing `TEST_GLOBS` back to `.ts` would silently drop whatever + * `.mts` tests exist and the suite would still report green, because a test that + * stops being collected looks exactly like a test that passed. That failure has + * a history here, which is why the control is a file rather than a note. + */ + +it("is discovered, classified as a test, and executed under a .mts extension", () => { + const self = "module-extensions.test.mts"; + + // Executing at all is the third assertion. These two cover the other + // classifications the guards make about this same name. + expect(BUNDLED_MODULE.test(self)).toBe(true); + expect(TEST_MODULE.test(self)).toBe(true); +}); + +it("does not classify a plain module as a test", () => { + // The separating property: `TEST_MODULE` has to REJECT something, or a + // predicate that returned true for everything would satisfy the test above + // while exempting every shipped module from the import allowlist. + expect(TEST_MODULE.test("source-modules.mts")).toBe(false); + expect(TEST_MODULE.test("geometry.ts")).toBe(false); + expect(BUNDLED_MODULE.test("README.md")).toBe(false); +}); diff --git a/packages/builder/src/source-modules.ts b/packages/builder/src/source-modules.ts index 29ef619406..8aa223db2b 100644 --- a/packages/builder/src/source-modules.ts +++ b/packages/builder/src/source-modules.ts @@ -1,18 +1,69 @@ /** - * Which files the bundler will follow, as one answer both guards ask. + * Which files count as modules, and which of those are tests, as one answer + * everything in this package asks. + * + * Three separate things need to agree here, and each was written with its own + * copy of the answer: + * + * - the layering guard and the geometry-ownership guard each WALK `src` looking + * for files to inspect; + * - the layering guard also CLASSIFIES what it finds, because a test file may + * import `vitest` and `node:fs` while a shipped module may not; + * - `vitest.config.ts` decides which files actually RUN as tests. * - * The layering guard and the geometry-ownership guard each walk `src` looking - * for files to inspect, and each was written with its own copy of this pattern. * Two copies of "what counts as a source module" drift the moment TypeScript * grows an extension — and a guard that walks past a file reports clean about - * code it never read, which is the failure mode both of them exist to prevent. + * code it never read, which is the failure mode both guards exist to prevent. + * + * The classification copy fails in a nastier direction, and it is why the + * extension list is not merely shared but drives the vitest globs too. Widening + * only the guard's idea of a test name grants a file test PRIVILEGES without + * test EXECUTION: `probe.test.mts` would be allowed to import `vitest`, and + * vitest would never run it, because a glob of `src/**\/*.test.ts` does not + * match a `.mts` file. A shipped module could then reach anything it liked by + * choosing its filename. Deriving both from this list keeps the two definitions + * of "test" the same definition. * - * `.mts` and `.cts` are here because TypeScript and tsup follow them. They were - * missing from both copies, so a module written with either extension was + * The extensions are listed rather than pattern-matched. The pattern this + * replaced (`[cm]?tsx?`) also admitted `.mtsx` and `.ctsx`, which TypeScript + * does not recognise, so it was matching names no compiler would ever follow. + * + * This module imports nothing on purpose. It is reached from test files and + * from the vitest config, and a shared helper that pulled in `node:fs` would put + * a Node import inside `src` where the layering guard is entitled to refuse it. + */ + +/** + * The extensions TypeScript and tsup follow. + * + * `.mts` and `.cts` are here because both tools resolve them. They were missing + * from every copy of this list, so a module written with either extension was * invisible to every check in this package. + */ +const MODULE_EXTENSIONS = [ + "ts", + "tsx", + "mts", + "cts", + "js", + "jsx", + "mjs", + "cjs", +] as const; + +const ANY_EXTENSION = MODULE_EXTENSIONS.join("|"); + +/** A file the bundler will follow, and therefore one a guard must read. */ +export const BUNDLED_MODULE = new RegExp(`\\.(?:${ANY_EXTENSION})$`); + +/** + * A file that runs as a test, and may therefore import test-only tooling. * - * This module imports nothing on purpose. It is reached from test files, and a - * shared helper that pulled in `node:fs` would put a Node import inside `src` - * where the layering guard is entitled to refuse it. + * Matches exactly what {@link TEST_GLOBS} tells vitest to run. Keeping the two + * derived from one list is what stops the guard trusting a file the runner + * ignores. */ -export const BUNDLED_MODULE = /\.(?:[cm]?tsx?|[cm]?jsx?)$/; +export const TEST_MODULE = new RegExp(`\\.test\\.(?:${ANY_EXTENSION})$`); + +/** The same set as {@link TEST_MODULE}, in the form `vitest.config.ts` takes. */ +export const TEST_GLOBS = MODULE_EXTENSIONS.map(ext => `src/**/*.test.${ext}`); diff --git a/packages/builder/vitest.config.ts b/packages/builder/vitest.config.ts index a762c9b760..c3b7af7b85 100644 --- a/packages/builder/vitest.config.ts +++ b/packages/builder/vitest.config.ts @@ -1,5 +1,7 @@ import { defineConfig } from "vitest/config"; +import { TEST_GLOBS } from "./src/source-modules"; + /** * The suite is static analysis over source files, not rendering. * @@ -9,13 +11,16 @@ import { defineConfig } from "vitest/config"; * renderer tests arrive they will need `jsdom`, and switching then is a * deliberate change rather than an inherited default. * - * `.tsx` is included alongside `.ts` because blocks are React components and - * their tests will live beside them. + * The include list is DERIVED rather than written here. The layering guard + * relaxes its import allowlist for anything it considers a test, so the runner + * and the guard have to mean the same thing by the word: a hand-written glob + * that omitted an extension the guard accepted would let a file import `vitest` + * and never be run. Both now come from one list in `src/source-modules.ts`. */ export default defineConfig({ test: { environment: "node", - include: ["src/**/*.test.ts", "src/**/*.test.tsx"], + include: TEST_GLOBS, }, }); From c8868f8268b3b78f0138d7c7c7722ba4e8c13b59 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 02:47:46 +0500 Subject: [PATCH 08/18] fix(builder): scale the frame's border inset with the frame boundingBox() reports a post-transform corner while clientLeft stays in the frame's own untransformed pixels, so adding the inset raw misplaces the content origin by (1 - scale) * inset. Zero at 100%, which is where a canvas gets developed, and growing as it zooms out. The sum lived at two call sites because the geometry module had pushed it out to "whoever reads the DOM". That conflated the DOM read with the arithmetic after it: reading clientLeft needs a browser, converting it does not. Both callers duly wrote it themselves and both wrote it wrong. frameContentOrigin now owns it and both callers ask. Adds a bordered-and-scaled acceptance case, since every existing one runs against border: none and agrees either way. It asserts the border applied and that the raw sum fails the same tolerance, so neither half can pass vacuously. --- e2e/tests/canvas/coordinate-mapping.spec.ts | 92 ++++++++++++++++++--- e2e/tests/canvas/coordinate-mapping.ts | 12 +++ e2e/tests/canvas/poc-driver.ts | 18 ++-- packages/builder/src/geometry.test.ts | 58 +++++++++++++ packages/builder/src/geometry.ts | 56 ++++++++++++- packages/builder/src/index.ts | 11 ++- 6 files changed, 221 insertions(+), 26 deletions(-) diff --git a/e2e/tests/canvas/coordinate-mapping.spec.ts b/e2e/tests/canvas/coordinate-mapping.spec.ts index 9d7592c0bf..75f4f0ba9c 100644 --- a/e2e/tests/canvas/coordinate-mapping.spec.ts +++ b/e2e/tests/canvas/coordinate-mapping.spec.ts @@ -17,9 +17,11 @@ import { expect, test } from "@playwright/test"; import { FLAT_LIST_FIXTURE, seedPage } from "./fixtures"; import { + frameContentOrigin, mapFramePointToHost, mapFrameRectToHost, mapHostPointToFrame, + type FrameInset, } from "./coordinate-mapping"; import { createPocDriver } from "./poc-driver"; @@ -39,8 +41,22 @@ async function groundTruth(page: import("@playwright/test").Page) { return box!; } +/** + * How the frame's content origin is derived from its measured border box. + * + * `"raw"` reproduces the arithmetic this file used to carry: add `clientLeft` + * to a post-transform corner without scaling it. It exists so the bordered test + * below can show its own tolerance is load-bearing, the same way the scale test + * shows the scale term is. + */ +type InsetMode = "scaled" | "raw"; + /** Apply our mapping to the probe's frame-local rect. */ -async function mapped(page: import("@playwright/test").Page, scale: number) { +async function mapped( + page: import("@playwright/test").Page, + scale: number, + insetMode: InsetMode = "scaled" +) { const frame = page.frames().find(f => f.url() === "about:blank")!; const frameRect = await frame.evaluate(id => { const el = document.querySelector(`[data-nx-id="${id}"]`); @@ -55,18 +71,22 @@ async function mapped(page: import("@playwright/test").Page, scale: number) { expect(origin).not.toBeNull(); // The content origin: `boundingBox()` gives the border box, and rectangles // read inside the frame are relative to the content viewport. The two agree - // only while the fixture keeps `border: none`, which is why measuring against - // the border box passed here and would drift on any bordered canvas. - const inset = await frameElement.evaluate(el => ({ - left: (el as HTMLIFrameElement).clientLeft, - top: (el as HTMLIFrameElement).clientTop, - })); - - return mapFrameRectToHost( - frameRect!, - { x: origin!.x + inset.left, y: origin!.y + inset.top }, - scale + // only while a canvas keeps `border: none`, which is why measuring against + // the border box passed here and drifts on any bordered canvas. + // + // The scale is passed through rather than measured so that `mapped(page, 1)` + // stays a coherent "what a naive implementation computes" — it gets the wrong + // origin AND the wrong mapping, which is what the control below asserts. + const inset = await frameElement.evaluate( + el => ({ left: el.clientLeft, top: el.clientTop }) ); + + const contentOrigin = + insetMode === "scaled" + ? frameContentOrigin(origin!, inset, scale) + : { x: origin!.x + inset.left, y: origin!.y + inset.top }; + + return mapFrameRectToHost(frameRect!, contentOrigin, scale); } /** Largest absolute difference across all four rect components. */ @@ -187,6 +207,54 @@ test("point 5: the mapping survives scroll and scale together", async ({ expect(delta).toBeLessThanOrEqual(1); }); +test("point 5: the mapping survives a bordered frame under scale", async ({ + page, + request, +}) => { + const fixture = await seedPage(request, FLAT_LIST_FIXTURE); + const driver = createPocDriver(page); + await driver.mountTree(fixture); + + // Every other test in this file runs against a canvas with `border: none`, + // where the content origin and the border-box corner are the same point. That + // makes them all agree whether or not the inset is scaled — so none of them + // can see this, and the fault would ship on the first bordered canvas. + await page + .locator("iframe") + .evaluate( + (el: HTMLIFrameElement) => (el.style.border = "8px solid transparent") + ); + await driver.setZoom(0.5); + + // Precondition, not decoration. If the canvas stylesheet wins over the inline + // border, `clientLeft` is 0, this silently becomes the at-rest case, and it + // passes while testing nothing at all. + const inset = await page + .locator("iframe") + .evaluate((el: HTMLIFrameElement) => el.clientLeft); + expect( + inset, + "the border must actually apply for this to test anything" + ).toBe(8); + + const scaled = worstDelta(await mapped(page, 0.5), await groundTruth(page)); + const raw = worstDelta( + await mapped(page, 0.5, "raw"), + await groundTruth(page) + ); + + test.info().annotations.push({ + type: "delta-bordered-scaled", + description: `scaled=${scaled} raw=${raw} inset=${inset}`, + }); + + // Both halves, for the same reason as the scale test. The first says scaling + // the inset is right; the second says it MATTERS — an 8px border at 50% puts + // the raw sum 4px out, so a regression to it cannot pass this quietly. + expect(scaled).toBeLessThanOrEqual(1); + expect(raw).toBeGreaterThan(1); +}); + test("point 5: the two directions are exact inverses", async ({ page, request, diff --git a/e2e/tests/canvas/coordinate-mapping.ts b/e2e/tests/canvas/coordinate-mapping.ts index 3d906af676..a17d1a368e 100644 --- a/e2e/tests/canvas/coordinate-mapping.ts +++ b/e2e/tests/canvas/coordinate-mapping.ts @@ -22,10 +22,12 @@ * instead of an assertion failure about numbers that were never meaningful. */ import { + frameContentOrigin, pointToCanvas, pointToHost, rectToHost, type FrameGeometry, + type FrameInset, } from "@nextlyhq/builder"; import type { Point, Rect } from "./driver"; @@ -67,3 +69,13 @@ export function mapFrameRectToHost( ): Rect { return rectToHost(frameRect, frame(frameOrigin, scale)); } + +/** + * Where the frame's content viewport starts, from a measured border box. + * + * Re-exported rather than wrapped: this is the arithmetic every caller needs + * after reading `boundingBox()` and `clientLeft`, and the two places that + * previously did it by hand both added the inset without scaling it. The + * measurement stays in the driver; the sums stay in the editor's module. + */ +export { frameContentOrigin, type FrameInset }; diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index ca8d4a5a83..d115ed4ffc 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -7,7 +7,11 @@ import { expect, type Frame, type Page } from "@playwright/test"; import { gotoAdmin } from "../support/admin"; -import { mapFrameRectToHost } from "./coordinate-mapping"; +import { + frameContentOrigin, + mapFrameRectToHost, + type FrameInset, +} from "./coordinate-mapping"; import type { ActiveTargetReader, CanvasDriver, @@ -166,14 +170,16 @@ export function createPocDriver(page: Page): CanvasDriver { // pixels out. A canvas that does not reset the browser's default iframe // border has that gap from the first render, and it reads as "the // indicator feels slightly off" rather than as a fault. - const inset = await frame.evaluate< - { left: number; top: number }, - HTMLIFrameElement - >(el => ({ + // + // Measured here, converted there. `clientLeft` is in the frame's own + // untransformed pixels while the box is post-transform, so the two cannot + // be added without the scale, and doing that sum at the call site is what + // put the same error in two files. + const inset = await frame.evaluate(el => ({ left: el.clientLeft, top: el.clientTop, })); - return { x: box.x + inset.left, y: box.y + inset.top }; + return frameContentOrigin(box, inset, await frameScale()); }, async readBlockBoxes() { diff --git a/packages/builder/src/geometry.test.ts b/packages/builder/src/geometry.test.ts index f8e5879e60..3d61533e78 100644 --- a/packages/builder/src/geometry.test.ts +++ b/packages/builder/src/geometry.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { FrameGeometryError, + frameContentOrigin, pointToCanvas, pointToHost, rectToHost, @@ -61,6 +62,63 @@ describe("mapping a rectangle across the frame", () => { }); }); +describe("locating the frame's content viewport", () => { + it("scales the border inset with the frame", () => { + // The separating case, and the only one that distinguishes a correct + // implementation from adding the inset raw: a 4px border at 50% occupies 2 + // host pixels, so the content starts at 100 + 2 rather than 100 + 4. + expect( + frameContentOrigin({ x: 100, y: 50 }, { left: 4, top: 8 }, 0.5) + ).toEqual({ x: 102, y: 54 }); + }); + + it("agrees with adding the inset raw only at 100%", () => { + // Why the fault survived review: at scale 1 the two implementations are the + // same function, and 100% is the state a canvas is developed in. + expect( + frameContentOrigin({ x: 100, y: 50 }, { left: 4, top: 8 }, 1) + ).toEqual({ x: 104, y: 58 }); + }); + + it("leaves the origin alone when the frame has no border", () => { + // The fixture case. It passes whether or not the inset is scaled, which is + // precisely why it could not have caught this. + expect( + frameContentOrigin({ x: 100, y: 50 }, { left: 0, top: 0 }, 0.5) + ).toEqual({ x: 100, y: 50 }); + }); + + it("feeds a geometry that maps a content-relative point correctly", () => { + // The reason the correction exists at all: a rectangle read INSIDE the frame + // is relative to the content viewport, so the origin it is added to has to + // be the content corner. Composing the two here is what a caller does. + const origin = frameContentOrigin( + { x: 100, y: 50 }, + { left: 4, top: 8 }, + 0.5 + ); + expect(pointToHost({ x: 20, y: 10 }, { origin, scale: 0.5 })).toEqual({ + x: 112, + y: 59, + }); + }); + + it.each([ + ["zero scale", 0, { left: 1, top: 1 }], + ["negative scale", -1, { left: 1, top: 1 }], + ["NaN scale", Number.NaN, { left: 1, top: 1 }], + ["NaN inset", 1, { left: Number.NaN, top: 1 }], + ["infinite inset", 1, { left: 1, top: Number.POSITIVE_INFINITY }], + ])( + "refuses an unusable measurement rather than returning one: %s", + (_label, scale, inset) => { + expect(() => frameContentOrigin({ x: 0, y: 0 }, inset, scale)).toThrow( + FrameGeometryError + ); + } + ); +}); + describe("a frame that describes no mapping", () => { it.each<[string, FrameGeometry]>([ ["zero scale", { origin: { x: 0, y: 0 }, scale: 0 }], diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts index 4d71baf1cb..85e82a5467 100644 --- a/packages/builder/src/geometry.ts +++ b/packages/builder/src/geometry.ts @@ -54,10 +54,13 @@ export interface Rect { * first render, which is exactly the sort of near-miss that reads as "the * indicator feels slightly off" rather than as a bug. * - * That correction belongs to whoever reads the DOM, not here: this module takes - * numbers so it can be exercised without a browser, and the caller that measured - * the frame is the only one holding `clientLeft`. See the e2e helper for the - * measurement that satisfies this contract. + * Build one with {@link frameContentOrigin} rather than adding that correction + * at the call site. An earlier version of this note pushed the arithmetic out to + * "whoever reads the DOM", on the grounds that only the caller holds + * `clientLeft`. That reasoning conflated the DOM READ with the arithmetic that + * follows it: reading `clientLeft` needs a browser, turning it into an origin is + * three multiplications over plain numbers. Two callers duly wrote the + * correction themselves, and both wrote it the same way round and both wrong. * * `scale` is the visual scale the host applies to the frame: a zoomed-out canvas * at 50% has `scale: 0.5`. @@ -104,6 +107,51 @@ function assertUsable(frame: FrameGeometry): void { } } +/** + * How far the frame's content viewport sits inside its border box. + * + * `clientLeft` and `clientTop` exactly as the DOM reports them, which is the + * whole reason this type exists rather than the caller passing two numbers: they + * are CSS pixels in the FRAME's own untransformed space. Every other coordinate + * in this module is host space. Mixing the two is the mistake below. + */ +export interface FrameInset { + readonly left: number; + readonly top: number; +} + +/** + * Where the frame's content viewport starts, in host coordinates. + * + * `borderBox` is the corner `getBoundingClientRect` reports for the frame + * element, and `inset` is its border width. The border is laid out in the + * frame's own pixels, so a host that has scaled the frame scales the border with + * everything else: at 50% a 2px border occupies 1 host pixel. Adding the inset + * unscaled therefore misplaces the origin by `(1 - scale) * inset`, which is + * zero at 100% and grows as the canvas zooms out — a fault that is invisible in + * exactly the configuration people develop in. + * + * Both facts are needed together and neither is guessable from the other, which + * is why this is a function rather than a note telling callers to add them up. + */ +export function frameContentOrigin( + borderBox: Point, + inset: FrameInset, + scale: number +): Point { + assertUsable({ origin: borderBox, scale }); + if (!Number.isFinite(inset.left) || !Number.isFinite(inset.top)) { + throw new FrameGeometryError( + `A frame inset of (${String(inset.left)}, ${String(inset.top)}) ` + + `describes no mapping. Both edges must be finite.` + ); + } + return { + x: borderBox.x + inset.left * scale, + y: borderBox.y + inset.top * scale, + }; +} + /** A point inside the canvas frame, in host coordinates. */ export function pointToHost(point: Point, frame: FrameGeometry): Point { assertUsable(frame); diff --git a/packages/builder/src/index.ts b/packages/builder/src/index.ts index e6f3f8b2d6..73497c92ba 100644 --- a/packages/builder/src/index.ts +++ b/packages/builder/src/index.ts @@ -22,10 +22,11 @@ * predicate would not have prevented that, because sharing a predicate does not * share the decision to call it; only sharing the entry point does. * - * This entry exports no features yet. The package exists ahead of them so its - * name is claimed on npm: trusted publishing cannot perform a package's first - * publish, and the bootstrap script will not claim a name that is not already a - * workspace package. + * **Public surface so far**: {@link BUILDER_PACKAGE_NAME}, and the frame + * geometry below. The editor itself is not exported yet — the package was + * created ahead of it so its name could be claimed on npm, because trusted + * publishing cannot perform a package's first publish and the bootstrap script + * will not claim a name that is not already a workspace package. * * @module @nextlyhq/builder */ @@ -52,10 +53,12 @@ export const BUILDER_PACKAGE_NAME = "@nextlyhq/builder" as const; */ export { FrameGeometryError, + frameContentOrigin, pointToCanvas, pointToHost, rectToHost, type FrameGeometry, + type FrameInset, type Point, type Rect, } from "./geometry"; From b8ad32a18ab4c9b81a7661f9c5883cd149d2dfbc Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 03:47:58 +0500 Subject: [PATCH 09/18] fix(builder): hash the extensions the guards read, and see an aliased rect read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the second review pass. Turbo's input globs listed .ts/.tsx and not .mts/.cts, so the new positive control was absent from the builder's test, check-types and lint inputs — a change confined to it could not move the task hash and CI would replay a cached green over code nothing ran. Confirmed by dry run in both directions: ABSENT before, IN INPUTS after, for all three tasks. The ownership guard missed a rectangle read taken off the element by destructuring and called through a new name, in both the renamed and shorthand spellings. Added, with a positive control for each. It also now records what it CANNOT see. Three narrowings have each been followed by another spelling walking past, which says the design is the limit rather than the coverage: a scan over syntax has an unbounded surface. The claim is narrowed to a review aid over a bounded set of spellings, with a passing test asserting a computed name goes unseen so the limit is written down rather than discovered. The README described a package that ships nothing while the entry exports the geometry contract. --- packages/builder/README.md | 52 +++++++++++++++-- .../builder/src/geometry-ownership.test.ts | 58 +++++++++++++++++++ turbo.jsonc | 24 ++++++-- 3 files changed, 124 insertions(+), 10 deletions(-) diff --git a/packages/builder/README.md b/packages/builder/README.md index 46f9b8ea26..2d51d810b6 100644 --- a/packages/builder/README.md +++ b/packages/builder/README.md @@ -3,10 +3,12 @@ The visual page-builder editor: the shell, the canvas, and the op store that everything in it either produces or reads. -**It ships no features yet.** The package exists ahead of them so its name is -claimed on npm — trusted publishing cannot perform a package's first publish, and -the bootstrap script will not claim a name that is not already a workspace -package. There is nothing to install it for until the editor lands. +**The editor itself has not landed.** What ships today is the frame geometry — +the one mapping between the canvas frame and the host page — plus the package +name constant. See [Public surface](#public-surface). The package was created +ahead of the editor so its name could be claimed on npm: trusted publishing +cannot perform a package's first publish, and the bootstrap script will not +claim a name that is not already a workspace package. ## What this package is not @@ -73,6 +75,48 @@ which packages a host loaded. The name and not the version: a version literal in source would be stale one release after it was written, because every release bumps this package in lockstep with its siblings. +### Frame geometry + +The canvas renders inside an iframe while the editor's chrome — insertion +indicator, selection outlines, drag affordances — is drawn in the host document +above it. Every one of those asks the same question: where is this canvas +rectangle, in host coordinates? It is answered here and nowhere else, because +two modules computing it separately agree on the day they are written and drift +the first time anything changes. + +`FrameGeometry` — how the frame sits in the host: an `origin` and a `scale`. +The origin is where the frame's CONTENT viewport lands, not its border box. +Scroll inside the frame is deliberately not a field: a rectangle read from +inside is already relative to the frame's viewport, so subtracting its scroll +would count it twice. + +`frameContentOrigin(borderBox, inset, scale)` — build that origin from what the +DOM reports. `getBoundingClientRect` gives the BORDER box while the inset +(`clientLeft`/`clientTop`) is in the frame's own untransformed pixels, so the +inset has to be scaled before it is added. Getting that wrong misplaces every +overlay by `(1 - scale) * inset`, which is zero at 100% and therefore invisible +in the state a canvas is developed in. + +`pointToHost` / `pointToCanvas` — a point across the frame, in either +direction. Exact inverses rather than two mappings written to match, because +hit-testing a pointer and drawing an overlay use opposite directions of the +same question. + +`rectToHost` — a rectangle across the frame, size scaled with it. An overlay +sized from the unscaled rectangle is correct at 100% and wrong everywhere else. + +`FrameGeometryError` — thrown when a frame describes no mapping: a zero, +negative or non-finite scale, or a non-finite origin. Thrown rather than +defaulted, because every value that could stand in is wrong in a way that looks +right, and an overlay silently drawn in the wrong place is the failure this +module exists to prevent. + +The functions take plain numbers rather than DOM nodes, so the mapping can be +exercised without a browser and the DOM reads stay at the edge. The e2e +acceptance suite adapts these rather than restating them: a browser harness +carrying its own copy certifies its own copy, and would keep passing through +exactly the correction it exists to catch. + ## Development Run these from this directory (`packages/builder`), not the repository root — diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index d43c1f0ddb..d51665983f 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -104,6 +104,23 @@ function crossFrameReads(text: string, file: string): string[] { ) { found.push(node.argumentExpression.text); } + // `const { getBoundingClientRect: read } = el` — the method taken off the + // element and called later through a name of the caller's choosing. Neither + // branch above sees it: the access is a binding pattern, and the call site + // is a bare identifier that could be anything. + // + // Both spellings count. A renamed binding carries `propertyName`, a + // shorthand one carries only `name`, and the shorthand is the form someone + // reaches for first. + if (ts.isBindingElement(node)) { + const read = node.propertyName ?? node.name; + if (ts.isIdentifier(read) && CROSS_FRAME_READS.has(read.text)) { + found.push(read.text); + } + if (ts.isStringLiteralLike(read) && CROSS_FRAME_READS.has(read.text)) { + found.push(read.text); + } + } ts.forEachChild(node, visit); }; @@ -165,4 +182,45 @@ describe("rectangles are read across the frame in one place", () => { expect(dotted).toEqual(["getBoundingClientRect"]); expect(bracketed).toEqual(["getBoundingClientRect"]); }); + + it("sees the method taken off the element and called through a new name", () => { + // The spelling that walked past the two above: the access is a binding + // pattern rather than a property access, and the call site is a bare + // identifier indistinguishable from any other function. + const renamed = crossFrameReads( + "const { getBoundingClientRect: read } = el; read.call(el);", + "probe.ts" + ); + const shorthand = crossFrameReads( + "const { getBoundingClientRect } = el; getBoundingClientRect.call(el);", + "probe.ts" + ); + + expect(renamed).toEqual(["getBoundingClientRect"]); + expect(shorthand).toEqual(["getBoundingClientRect"]); + }); + + it("does not claim to see a read routed through a computed name", () => { + // Recorded as a LIMIT rather than left for the next reader to discover. + // + // Three narrowings of this guard have each been followed by a finding of + // the same shape — another spelling it did not recognise — and that is the + // signal that the design is the problem, not the coverage. A scan over + // syntax has an unbounded surface: a name assembled at runtime, a + // `Reflect.get`, a property descriptor, an `eval`. Adding a fourth case + // would move the boundary without closing it. + // + // So the claim is narrowed to what is true. This is a REVIEW AID over a + // bounded set of spellings, NOT a boundary the code cannot cross. The + // enforceable half is elsewhere: `geometry.ts` owns the arithmetic and + // every caller is expected to ask it, which review checks. This assertion + // exists so that fact is written down as a passing test rather than as a + // sentence someone may stop believing. + const computed = crossFrameReads( + 'const name = "getBounding" + "ClientRect"; const r = el[name]();', + "probe.ts" + ); + + expect(computed).toEqual([]); + }); }); diff --git a/turbo.jsonc b/turbo.jsonc index 6294498691..aadfeb259e 100644 --- a/turbo.jsonc +++ b/turbo.jsonc @@ -164,7 +164,16 @@ // Note: Uses incremental compilation (.tsbuildinfo) for speed "check-types": { "dependsOn": [], // Optimized: No build dependency for faster parallel execution - "inputs": ["src/**/*.{ts,tsx}", "*.{ts,tsx}", "tsconfig.json"], + // Every extension TypeScript follows, not only `.ts`/`.tsx`. A task input + // glob decides what turbo HASHES: a change confined to an extension + // missing here leaves the hash unmoved, so CI replays a cached green + // without ever checking the changed module. Kept in step with + // `MODULE_EXTENSIONS` in `packages/builder/src/source-modules.ts`. + "inputs": [ + "src/**/*.{ts,tsx,mts,cts}", + "*.{ts,tsx,mts,cts}", + "tsconfig.json" + ], "outputs": ["**/*.tsbuildinfo"] // TypeScript incremental cache }, @@ -208,8 +217,8 @@ "lint": { "dependsOn": [], // No build needed (lints source directly) "inputs": [ - "src/**/*.{ts,tsx,js,jsx}", - "*.{ts,tsx,js,jsx}", + "src/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}", + "*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}", "eslint.config.*", "tsconfig.json" ], @@ -283,9 +292,12 @@ "test": { "dependsOn": ["^build"], // Need built dependencies for imports "inputs": [ - "src/**/*.{ts,tsx}", - "src/**/*.test.{ts,tsx}", - "src/**/*.spec.{ts,tsx}", + // `.mts`/`.cts` included for the same reason the JavaScript line + // below exists: a file the glob misses is a file whose change cannot + // move the hash, so a cached green is replayed over code nothing ran. + "src/**/*.{ts,tsx,mts,cts}", + "src/**/*.test.{ts,tsx,mts,cts}", + "src/**/*.spec.{ts,tsx,mts,cts}", // JavaScript is a test input too. A bundler follows a side-effect import // into a `.js` sibling and ships it, and a guard that reads those files // is cached against a hash that never saw them — so the check would be From 594701ff788b19ec8616bd74d1a51b9a62c36a01 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 03:55:09 +0500 Subject: [PATCH 10/18] fix(builder): route the indicator reader through the driver's content origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readIndicatorRect built its own origin from iframe.boundingBox(), which is the BORDER box, so on a bordered canvas every indicator rectangle it reported was inset * scale out — the same fault in a third place. Three sites is the design telling us no caller should assemble this. It now asks driver.frameOrigin(), which is the one reader that measures the inset and hands it to frameContentOrigin. --- e2e/tests/canvas/poc-driver.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index d115ed4ffc..149a22772f 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -474,8 +474,12 @@ export function createPocDriver(page: Page): CanvasDriver { ); if (!inFrame) return null; - const frameOrigin = await page.locator("iframe").boundingBox(); - if (!frameOrigin) return null; + // The driver's own content origin, not a second reading of the frame's + // box. `boundingBox()` reports the BORDER box, so building the origin + // here would place every indicator rectangle `inset * scale` out on any + // bordered canvas — the same fault, in a third place, which is the sign + // that no caller should be assembling this at all. + const origin = await driver.frameOrigin(); // Read the live transform rather than assume 1. Without this the rect // reported under a scaled canvas is wrong by the scale factor, and a @@ -483,11 +487,7 @@ export function createPocDriver(page: Page): CanvasDriver { // wrong, not because the canvas is. const scale = await frameScale(); - return mapFrameRectToHost( - inFrame, - { x: frameOrigin.x, y: frameOrigin.y }, - scale - ); + return mapFrameRectToHost(inFrame, origin, scale); }, async readTreeShape() { From 10dc87fa4d7f1b10700856522d818e599a2d8db3 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 04:39:38 +0500 Subject: [PATCH 11/18] docs(builder): describe the guard's reach, not its history A comment recounted how the scan had been narrowed over time, which is review history rather than a description of the code, and the convention is that comments describe the code only. Replaced with what the scan does and does not recognise. The module header also read as though the sibling test enforced single ownership. It does not: it recognises a bounded set of spellings and its own tests assert that a computed name walks past. Nothing checks the arithmetic half at all. Both are review-time conventions and now say so, so the header cannot be read as a guarantee the code does not provide. --- .../builder/src/geometry-ownership.test.ts | 21 +++++++--------- packages/builder/src/geometry.ts | 24 +++++++++++++------ 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index d51665983f..8ba3ff94ab 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -201,21 +201,16 @@ describe("rectangles are read across the frame in one place", () => { }); it("does not claim to see a read routed through a computed name", () => { - // Recorded as a LIMIT rather than left for the next reader to discover. + // The limit of the scan, asserted rather than left to be discovered. // - // Three narrowings of this guard have each been followed by a finding of - // the same shape — another spelling it did not recognise — and that is the - // signal that the design is the problem, not the coverage. A scan over - // syntax has an unbounded surface: a name assembled at runtime, a - // `Reflect.get`, a property descriptor, an `eval`. Adding a fourth case - // would move the boundary without closing it. + // A scan over syntax has an unbounded surface: a name assembled at runtime, + // a `Reflect.get`, a property descriptor, an `eval`. Recognising one more + // spelling moves the edge without closing it, so this is a REVIEW AID over + // a bounded set of spellings, NOT a boundary the code cannot cross. // - // So the claim is narrowed to what is true. This is a REVIEW AID over a - // bounded set of spellings, NOT a boundary the code cannot cross. The - // enforceable half is elsewhere: `geometry.ts` owns the arithmetic and - // every caller is expected to ask it, which review checks. This assertion - // exists so that fact is written down as a passing test rather than as a - // sentence someone may stop believing. + // The enforceable half is elsewhere: `geometry.ts` owns the arithmetic and + // every caller asks it. Writing the limit as a passing assertion keeps it + // true, where a sentence in a header stops being read. const computed = crossFrameReads( 'const name = "getBounding" + "ClientRect"; const r = el[name]();', "probe.ts" diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts index 85e82a5467..10a20460fc 100644 --- a/packages/builder/src/geometry.ts +++ b/packages/builder/src/geometry.ts @@ -13,13 +13,23 @@ * bug is not caught by either module's own tests, because each is correct about * the question it asked. * - * How much of that is ENFORCED, stated plainly because the difference matters: - * a sibling test refuses a rectangle READ across the frame anywhere else in this - * package, and the e2e helper adapts these functions rather than restating them. - * Neither can stop a module that is handed an origin and a scale from - * open-coding the arithmetic — two numbers multiplied and added look like any - * other code — so that half is held at review, the same way the builder's - * "draws with `blocks-react`" rule is. + * How much of that is ENFORCED, stated plainly because the difference matters — + * and the honest answer is: none of it, by a boundary. + * + * A sibling test scans for a rectangle READ across the frame elsewhere in this + * package, and it recognises a bounded set of spellings: a property access, a + * string element access, a destructured binding. A name assembled at runtime, a + * `Reflect.get` or a property descriptor all walk past it, and its own tests say + * so. It narrows the easy paths; it is not a wall. + * + * Nothing checks the other half at all. A module handed an origin and a scale can + * open-code the arithmetic without touching the DOM, and two numbers multiplied + * and added look like any other code. + * + * So treat both as review-time conventions, the same way the builder's "draws + * with `blocks-react`" rule is. What is real is that the e2e helper adapts these + * functions rather than restating them, and that every caller needing a content + * origin asks {@link frameContentOrigin} for one. * * The functions are pure and take plain numbers rather than DOM nodes, so the * mapping can be exercised without a browser and the DOM reads stay at the edge. From bd9b334404c0ffd9dacfa2c1ca52667623b69bf2 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 04:40:51 +0500 Subject: [PATCH 12/18] fix(builder): map the nearest-zone centre with the shared helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zone centre was scaled and added inline instead of going through mapFramePointToHost. It used the right origin, so it was correct — but written out it is two numbers multiplied and added, which is the shape no import scan can separate from ordinary arithmetic, and therefore the one that keeps a stale copy of the mapping after the mapping is corrected. --- e2e/tests/canvas/poc-driver.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index 149a22772f..b98d1d3153 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -9,6 +9,7 @@ import { gotoAdmin } from "../support/admin"; import { frameContentOrigin, + mapFramePointToHost, mapFrameRectToHost, type FrameInset, } from "./coordinate-mapping"; @@ -233,7 +234,15 @@ export function createPocDriver(page: Page): CanvasDriver { let best = -1; let bestDistance = Number.POSITIVE_INFINITY; rects.forEach((rect, index) => { - const centre = origin.y + (rect.y + rect.height / 2) * scale; + // Mapped by the shared helper rather than multiplied out here. Written + // inline this is two numbers scaled and added, which is exactly the + // shape no import scan can tell from ordinary arithmetic — so it is the + // one that drifts silently when the mapping is corrected. + const centre = mapFramePointToHost( + { x: 0, y: rect.y + rect.height / 2 }, + origin, + scale + ).y; const distance = Math.abs(pointerY - centre); if (distance < bestDistance) { bestDistance = distance; From 10989ca8633d12e65ce5d9014354971d4569a6bd Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 05:09:40 +0500 Subject: [PATCH 13/18] docs(builder): call the scan a review aid at the top of the file too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module header still read as enforcement — "a convention cannot hold that, so it is checked" — while the last test in the same file asserts a computed name walks past. The same claim was corrected in geometry.ts and left standing here, which is worse than either alone: a reader who opens the guard sees the stronger statement. --- packages/builder/src/geometry-ownership.test.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index 8ba3ff94ab..73f52225f4 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -21,10 +21,17 @@ * about their own question and disagree about the shared one — the indicator * lands a few pixels off the gap it names, and neither module's tests fail. * - * A convention cannot hold that, because the second implementation looks - * reasonable in isolation and arrives when someone needs a rectangle and has a - * DOM node to hand. So it is checked: `getBoundingClientRect` may be read in - * `geometry.ts` and nowhere else in this package. + * So this scans for that read outside `geometry.ts` — and it is a REVIEW AID, + * not a boundary. It recognises a bounded set of spellings: a property access, + * a string element access, and a destructured binding in either form. A name + * assembled at runtime, a `Reflect.get`, a property descriptor and an `eval` + * all walk past it, and the last test in this file asserts one of them does. + * + * That is worth saying at the top rather than only at the bottom, because a + * scan is easy to read as a guarantee. It narrows the paths someone takes by + * accident, which is the failure it is aimed at: the second implementation + * looks reasonable in isolation and arrives when someone needs a rectangle and + * has a DOM node to hand. It cannot stop one written deliberately. * * Read from the AST rather than by matching text, so a call written as * `el["getBoundingClientRect"]()` is seen as the same thing — the spelling that From a2182e619f12201334619b31a62e1705bfd934ac Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 05:30:12 +0500 Subject: [PATCH 14/18] docs(builder): qualify what the release note claims the guard does The note said a guard ensures no other module reads a rectangle across the frame. The scan recognises a bounded set of spellings and its own test asserts a computed name walks past, so the note promised users a guarantee the code does not make. --- .changeset/builder-frame-geometry.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/builder-frame-geometry.md b/.changeset/builder-frame-geometry.md index d873ebe6ce..036cce7ba0 100644 --- a/.changeset/builder-frame-geometry.md +++ b/.changeset/builder-frame-geometry.md @@ -24,4 +24,4 @@ "@nextlyhq/builder": patch --- -Add the builder's host-canvas coordinate mapping: one module converts between the canvas frame and the host page, with a guard that no other module reads a rectangle across the frame. +Add the builder's host-canvas coordinate mapping: one module converts between the canvas frame and the host page, including the scaled border inset that places the frame's content origin. A sibling test scans for cross-frame rectangle reads elsewhere in the package, recognising a bounded set of spellings; it narrows the paths taken by accident rather than enforcing single ownership. From ada39f5411e1af8cd4c49c51c4085f0aa07c8d16 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 07:51:53 +0500 Subject: [PATCH 15/18] docs(builder): state why the origin is built here, not how it used to be The note recounted what an earlier version of itself had said and why that was wrong. That is edit history; a comment describes the code. Replaced with the reason the arithmetic lives with the module rather than at each call site. --- packages/builder/src/geometry.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts index 10a20460fc..2fe30cd74c 100644 --- a/packages/builder/src/geometry.ts +++ b/packages/builder/src/geometry.ts @@ -65,12 +65,10 @@ export interface Rect { * indicator feels slightly off" rather than as a bug. * * Build one with {@link frameContentOrigin} rather than adding that correction - * at the call site. An earlier version of this note pushed the arithmetic out to - * "whoever reads the DOM", on the grounds that only the caller holds - * `clientLeft`. That reasoning conflated the DOM READ with the arithmetic that - * follows it: reading `clientLeft` needs a browser, turning it into an origin is - * three multiplications over plain numbers. Two callers duly wrote the - * correction themselves, and both wrote it the same way round and both wrong. + * at the call site. Reading `clientLeft` needs a browser; turning it into an + * origin is arithmetic over plain numbers, and splitting the two puts the sum + * in every caller that measured a frame — where each writes it separately and + * a correction reaches only the one being edited. * * `scale` is the visual scale the host applies to the frame: a zoomed-out canvas * at 50% has `scale: 0.5`. From b2773818940f6c094260da92f232555d37722071 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 09:13:08 +0500 Subject: [PATCH 16/18] fix(builder): report a collapsed frame's scale instead of substituting one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit frameScale() ended in `|| 1`, which reads as "default when absent" and also fires on a measured 0. A collapsed frame therefore reached the mapping as a usable 1, so the FrameGeometryError for a zero scale could not fire through the driver at all — the guard existed and its only real path was blocked. Measured in Chromium: transform "none" parses to the identity whose `a` is already 1, so the fallback was not needed for the untransformed case either; scale(0) reports `a` 0, which is now preserved. Also removes review-process and edit-history wording from comments across the files this branch touches, per the convention that a comment describes the code. Ten sites, swept rather than fixed individually. --- e2e/tests/canvas/coordinate-mapping.spec.ts | 4 +- e2e/tests/canvas/coordinate-mapping.ts | 6 +-- e2e/tests/canvas/poc-driver.ts | 16 ++++++- .../builder/src/geometry-ownership.test.ts | 42 +++++++++---------- packages/builder/src/geometry.test.ts | 8 ++-- packages/builder/src/geometry.ts | 9 ++-- packages/builder/src/index.ts | 8 ++-- packages/builder/src/layering.test.ts | 4 +- packages/builder/src/source-modules.ts | 6 +-- 9 files changed, 55 insertions(+), 48 deletions(-) diff --git a/e2e/tests/canvas/coordinate-mapping.spec.ts b/e2e/tests/canvas/coordinate-mapping.spec.ts index 75f4f0ba9c..8c5b5b1443 100644 --- a/e2e/tests/canvas/coordinate-mapping.spec.ts +++ b/e2e/tests/canvas/coordinate-mapping.spec.ts @@ -44,8 +44,8 @@ async function groundTruth(page: import("@playwright/test").Page) { /** * How the frame's content origin is derived from its measured border box. * - * `"raw"` reproduces the arithmetic this file used to carry: add `clientLeft` - * to a post-transform corner without scaling it. It exists so the bordered test + * `"raw"` adds `clientLeft` to a post-transform corner without scaling it — + * the arithmetic a naive implementation writes. It exists so the bordered test * below can show its own tolerance is load-bearing, the same way the scale test * shows the scale term is. */ diff --git a/e2e/tests/canvas/coordinate-mapping.ts b/e2e/tests/canvas/coordinate-mapping.ts index a17d1a368e..63fa602519 100644 --- a/e2e/tests/canvas/coordinate-mapping.ts +++ b/e2e/tests/canvas/coordinate-mapping.ts @@ -74,8 +74,8 @@ export function mapFrameRectToHost( * Where the frame's content viewport starts, from a measured border box. * * Re-exported rather than wrapped: this is the arithmetic every caller needs - * after reading `boundingBox()` and `clientLeft`, and the two places that - * previously did it by hand both added the inset without scaling it. The - * measurement stays in the driver; the sums stay in the editor's module. + * after reading `boundingBox()` and `clientLeft`, and writing it at each call + * site is how one of them ends up adding the inset unscaled. The measurement + * stays in the driver; the sums stay in the editor's module. */ export { frameContentOrigin, type FrameInset }; diff --git a/e2e/tests/canvas/poc-driver.ts b/e2e/tests/canvas/poc-driver.ts index b98d1d3153..d01646e6b4 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -88,12 +88,24 @@ export function createPocDriver(page: Page): CanvasDriver { return frame; } - /** The frame's current transform scale; 1 when untransformed. */ + /** + * The frame's current transform scale; 1 when untransformed. + * + * Reported as measured, including zero. A collapsed frame maps the whole + * canvas onto a point, and the mapping refuses it — but only if the number + * reaches the mapping, so this must not substitute a usable-looking value for + * an unusable one. + * + * `|| 1` is what did that: it reads as "default when absent" and also fires + * on a measured 0. It is not needed for the untransformed case either, since + * `getComputedStyle` reports `"none"` there and `DOMMatrixReadOnly` parses + * that to the identity, whose `a` is already 1. + */ async function frameScale(): Promise { return page.evaluate(() => { const frame = document.querySelector("iframe"); if (!(frame instanceof HTMLElement)) return 1; - return new DOMMatrixReadOnly(getComputedStyle(frame).transform).a || 1; + return new DOMMatrixReadOnly(getComputedStyle(frame).transform).a; }); } diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index 73f52225f4..7c38d703c8 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -8,12 +8,10 @@ * here would see it. A duplicate mapping added alongside this file passes every * assertion below. * - * That half is a review-time convention rather than a checked one, for the same - * reason the builder's "draws with `blocks-react`" rule is: arithmetic on two - * numbers is indistinguishable from any other arithmetic, so no scan can tell a - * second implementation from ordinary code. What IS checkable is the DOM read - * that a mapping needs its inputs from, and narrowing the guard to that leaves - * the door it can actually hold. + * Nothing checks that half. Arithmetic on two numbers is indistinguishable + * from any other arithmetic, so no scan can tell a second implementation from + * ordinary code. What IS checkable is the DOM read a mapping needs its inputs + * from, and this is narrowed to that. * * The editor's chrome is drawn in the host document over a canvas that lives in * an iframe, so anything positioning an overlay has to convert between the two @@ -21,17 +19,16 @@ * about their own question and disagree about the shared one — the indicator * lands a few pixels off the gap it names, and neither module's tests fail. * - * So this scans for that read outside `geometry.ts` — and it is a REVIEW AID, - * not a boundary. It recognises a bounded set of spellings: a property access, - * a string element access, and a destructured binding in either form. A name - * assembled at runtime, a `Reflect.get`, a property descriptor and an `eval` - * all walk past it, and the last test in this file asserts one of them does. + * So this scans for that read outside `geometry.ts`. Its detection is bounded: + * it recognises a property access, a string element access, and a destructured + * binding in either form. A name assembled at runtime, a `Reflect.get`, a + * property descriptor and an `eval` all walk past it, and the last test in this + * file asserts one of them does. * - * That is worth saying at the top rather than only at the bottom, because a - * scan is easy to read as a guarantee. It narrows the paths someone takes by - * accident, which is the failure it is aimed at: the second implementation + * Stated at the top because a scan reads as a guarantee otherwise. It catches + * the spelling someone reaches for without thinking — a second implementation * looks reasonable in isolation and arrives when someone needs a rectangle and - * has a DOM node to hand. It cannot stop one written deliberately. + * has a DOM node to hand. It does not constrain one written deliberately. * * Read from the AST rather than by matching text, so a call written as * `el["getBoundingClientRect"]()` is seen as the same thing — the spelling that @@ -208,16 +205,15 @@ describe("rectangles are read across the frame in one place", () => { }); it("does not claim to see a read routed through a computed name", () => { - // The limit of the scan, asserted rather than left to be discovered. + // The limit of the scan, asserted so it stays true. // - // A scan over syntax has an unbounded surface: a name assembled at runtime, - // a `Reflect.get`, a property descriptor, an `eval`. Recognising one more - // spelling moves the edge without closing it, so this is a REVIEW AID over - // a bounded set of spellings, NOT a boundary the code cannot cross. + // Detection is by syntax, and syntax has an unbounded surface: a name + // assembled at runtime, a `Reflect.get`, a property descriptor, an `eval`. + // Recognising one more spelling moves the edge without closing it, so the + // set above is bounded on purpose rather than aspiring to completeness. // - // The enforceable half is elsewhere: `geometry.ts` owns the arithmetic and - // every caller asks it. Writing the limit as a passing assertion keeps it - // true, where a sentence in a header stops being read. + // Asserting the miss keeps the boundary honest: a sentence in a header can + // drift from the code, and this cannot. const computed = crossFrameReads( 'const name = "getBounding" + "ClientRect"; const r = el[name]();', "probe.ts" diff --git a/packages/builder/src/geometry.test.ts b/packages/builder/src/geometry.test.ts index 3d61533e78..922ac471ba 100644 --- a/packages/builder/src/geometry.test.ts +++ b/packages/builder/src/geometry.test.ts @@ -73,16 +73,16 @@ describe("locating the frame's content viewport", () => { }); it("agrees with adding the inset raw only at 100%", () => { - // Why the fault survived review: at scale 1 the two implementations are the - // same function, and 100% is the state a canvas is developed in. + // At scale 1 the scaled and raw inset arithmetic are the same function, so + // this case cannot distinguish them. It pins the identity, not the fix. expect( frameContentOrigin({ x: 100, y: 50 }, { left: 4, top: 8 }, 1) ).toEqual({ x: 104, y: 58 }); }); it("leaves the origin alone when the frame has no border", () => { - // The fixture case. It passes whether or not the inset is scaled, which is - // precisely why it could not have caught this. + // A borderless frame has a zero inset, so the scale term multiplies + // nothing. This case also cannot distinguish the two implementations. expect( frameContentOrigin({ x: 100, y: 50 }, { left: 0, top: 0 }, 0.5) ).toEqual({ x: 100, y: 50 }); diff --git a/packages/builder/src/geometry.ts b/packages/builder/src/geometry.ts index 2fe30cd74c..e8b5debff1 100644 --- a/packages/builder/src/geometry.ts +++ b/packages/builder/src/geometry.ts @@ -26,9 +26,8 @@ * open-code the arithmetic without touching the DOM, and two numbers multiplied * and added look like any other code. * - * So treat both as review-time conventions, the same way the builder's "draws - * with `blocks-react`" rule is. What is real is that the e2e helper adapts these - * functions rather than restating them, and that every caller needing a content + * What holds instead is narrower and true: the e2e helper adapts these + * functions rather than restating them, and every caller needing a content * origin asks {@link frameContentOrigin} for one. * * The functions are pure and take plain numbers rather than DOM nodes, so the @@ -189,8 +188,8 @@ export function pointToCanvas(point: Point, frame: FrameGeometry): Point { * A rectangle inside the canvas frame, in host coordinates. * * Its size scales with the frame. An overlay sized from the unscaled rectangle - * would be correct only at 100%, and would look correct there — which is how a - * zoom bug survives review. + * would be correct only at 100%, and would look correct there, which is the + * one configuration a canvas is usually looked at in. */ export function rectToHost(rect: Rect, frame: FrameGeometry): Rect { const origin = pointToHost({ x: rect.x, y: rect.y }, frame); diff --git a/packages/builder/src/index.ts b/packages/builder/src/index.ts index 73497c92ba..cc4ebf8445 100644 --- a/packages/builder/src/index.ts +++ b/packages/builder/src/index.ts @@ -11,10 +11,10 @@ * preparation, condition gating and slot pruning are consumed from the engine's * own entry points, never reproduced here. * - * It is held by review rather than by a test. Reimplementing rendering on React - * and the engine imports exactly the same packages as delegating to the - * renderer, so the layering guard cannot tell the two apart; it narrows what may - * be imported, which makes the shortcut inconvenient rather than impossible. + * No test enforces it. Reimplementing rendering on React and the engine + * imports exactly the same packages as delegating to the renderer, so the + * layering guard cannot tell the two apart; it narrows what may be imported, + * which makes the shortcut inconvenient rather than impossible. * * That rule is not stylistic. `plugin-page-builder` carries a second renderer of * its own, and the two disagree about condition gating in OPPOSITE directions — diff --git a/packages/builder/src/layering.test.ts b/packages/builder/src/layering.test.ts index 6e7d7a1395..3b97565795 100644 --- a/packages/builder/src/layering.test.ts +++ b/packages/builder/src/layering.test.ts @@ -36,8 +36,8 @@ import { BUNDLED_MODULE, TEST_MODULE } from "./source-modules"; * `blocks-react` rather than reimplementing rendering on top of React and * `@nextlyhq/blocks-engine` is a property of what the code does, not of what it * imports, and both spellings import exactly the same packages. The allowlist - * makes the shortcut inconvenient; it cannot make it impossible. Treat that rule - * as a design constraint reviewed by people, not as one enforced here. + * makes the shortcut inconvenient; it cannot make it impossible. That rule is a + * design constraint, and nothing in this file enforces it. */ // `import.meta.dirname` only exists from Node 20.11 and the package floor is diff --git a/packages/builder/src/source-modules.ts b/packages/builder/src/source-modules.ts index 8aa223db2b..7244e01b98 100644 --- a/packages/builder/src/source-modules.ts +++ b/packages/builder/src/source-modules.ts @@ -36,9 +36,9 @@ /** * The extensions TypeScript and tsup follow. * - * `.mts` and `.cts` are here because both tools resolve them. They were missing - * from every copy of this list, so a module written with either extension was - * invisible to every check in this package. + * `.mts` and `.cts` are here because both tools resolve them. A module written + * with either extension is a module the bundler follows, so a list omitting + * them leaves that file invisible to every check in this package. */ const MODULE_EXTENSIONS = [ "ts", From 02c1a9fa2cc01535bb92469346028b69259c5176 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 09:20:21 +0500 Subject: [PATCH 17/18] refactor(builder): both guards find their files by one rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The layering guard and the geometry-ownership guard each carried a byte-identical directory walk. The shared extension list cannot stop the walks themselves diverging — one skipping a directory, one matching a different part of the path — and a guard that walks past a file reports clean about code it never read. Reading the directory is injected rather than imported, so source-modules keeps importing nothing: a node:fs import there would put a Node dependency inside src, where the layering guard is entitled to refuse it. --- .../builder/src/geometry-ownership.test.ts | 15 +++++----- packages/builder/src/layering.test.ts | 15 +++++----- packages/builder/src/source-modules.ts | 30 +++++++++++++++++++ 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/packages/builder/src/geometry-ownership.test.ts b/packages/builder/src/geometry-ownership.test.ts index 7c38d703c8..ff56ac8776 100644 --- a/packages/builder/src/geometry-ownership.test.ts +++ b/packages/builder/src/geometry-ownership.test.ts @@ -40,7 +40,7 @@ import { fileURLToPath } from "node:url"; import ts from "typescript"; import { describe, expect, it } from "vitest"; -import { BUNDLED_MODULE } from "./source-modules"; +import { collectModules } from "./source-modules"; const SRC_DIR = dirname(fileURLToPath(import.meta.url)); @@ -73,14 +73,13 @@ function isModule(file: string, relativePath: string): boolean { */ const CROSS_FRAME_READS = new Set(["getBoundingClientRect", "getClientRects"]); +/** The package's modules, by the shared rule; only the file reading is local. */ function sourceFiles(dir: string): string[] { - const out: string[] = []; - for (const entry of readdirSync(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) out.push(...sourceFiles(full)); - else if (BUNDLED_MODULE.test(entry.name)) out.push(full); - } - return out; + return collectModules( + dir, + at => readdirSync(at, { withFileTypes: true }), + join + ); } /** Every cross-frame read a source text performs, by the name it used. */ diff --git a/packages/builder/src/layering.test.ts b/packages/builder/src/layering.test.ts index 3b97565795..1895ba272c 100644 --- a/packages/builder/src/layering.test.ts +++ b/packages/builder/src/layering.test.ts @@ -4,7 +4,7 @@ import { fileURLToPath } from "node:url"; import ts from "typescript"; import { describe, expect, it } from "vitest"; -import { BUNDLED_MODULE, TEST_MODULE } from "./source-modules"; +import { collectModules, TEST_MODULE } from "./source-modules"; /** * The package's layering contract, enforced rather than documented. @@ -105,14 +105,13 @@ const UNRESOLVABLE_SPECIFIER = ""; * anything, with the typecheck none the wiser because `allowJs` is off. */ +/** The package's modules, by the shared rule; only the file reading is local. */ function sourceFiles(dir: string): string[] { - const out: string[] = []; - for (const entry of readdirSync(dir, { withFileTypes: true })) { - const full = join(dir, entry.name); - if (entry.isDirectory()) out.push(...sourceFiles(full)); - else if (BUNDLED_MODULE.test(entry.name)) out.push(full); - } - return out; + return collectModules( + dir, + at => readdirSync(at, { withFileTypes: true }), + join + ); } /** diff --git a/packages/builder/src/source-modules.ts b/packages/builder/src/source-modules.ts index 7244e01b98..cc196104a6 100644 --- a/packages/builder/src/source-modules.ts +++ b/packages/builder/src/source-modules.ts @@ -67,3 +67,33 @@ export const TEST_MODULE = new RegExp(`\\.test\\.(?:${ANY_EXTENSION})$`); /** The same set as {@link TEST_MODULE}, in the form `vitest.config.ts` takes. */ export const TEST_GLOBS = MODULE_EXTENSIONS.map(ext => `src/**/*.test.${ext}`); + +/** + * Every module beneath a directory, found by one rule. + * + * Both guards in this package walk `src` looking for files to inspect, and each + * had its own copy of this loop. Two walks can diverge in ways the shared + * extension list cannot prevent — one skipping a directory, one matching on a + * different part of the path — and a guard that walks past a file reports clean + * about code it never read. + * + * Reading the directory is INJECTED rather than imported, so this module keeps + * importing nothing: a `node:fs` import here would put a Node dependency inside + * `src`, where the layering guard is entitled to refuse it. The caller supplies + * the two functions; the rule about what counts and where to recurse lives here. + */ +export function collectModules( + dir: string, + readdir: ( + at: string + ) => ReadonlyArray<{ name: string; isDirectory: () => boolean }>, + join: (...parts: string[]) => string +): string[] { + const out: string[] = []; + for (const entry of readdir(dir)) { + const full = join(dir, entry.name); + if (entry.isDirectory()) out.push(...collectModules(full, readdir, join)); + else if (BUNDLED_MODULE.test(entry.name)) out.push(full); + } + return out; +} From 348c63d3358c3cbb6aced7f338bcc5522615ad93 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Wed, 12 Aug 2026 09:20:54 +0500 Subject: [PATCH 18/18] test(builder): pin that the walk covers every extension it claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing positive control asserts the file list is non-empty and contains an index.ts. Both survive a walk narrowed to .ts alone, so neither separates full coverage from partial — and a scan going quiet on one extension is the dangerous direction, since the files it stops reading are the ones it reports clean. Naming a less common extension is what separates them; narrowing the walk now fails this and nothing else. --- packages/builder/src/layering.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/builder/src/layering.test.ts b/packages/builder/src/layering.test.ts index 1895ba272c..81b89dd74c 100644 --- a/packages/builder/src/layering.test.ts +++ b/packages/builder/src/layering.test.ts @@ -474,6 +474,16 @@ describe("the builder's layering contract", () => { expect(files.some(f => f.endsWith("index.ts"))).toBe(true); }); + it("reads every extension it claims to, not only the common ones", () => { + // `length > 0` and "an index.ts is present" both survive a walk narrowed to + // `.ts` alone, so neither separates full coverage from partial. A file in a + // less common extension has to be named for that. + // + // The scan going quiet on one extension is the dangerous direction: the + // files it stops reading are the ones it then reports clean. + expect(files.some(f => f.endsWith(".mts"))).toBe(true); + }); + it("never imports @nextlyhq/admin directly", () => { // The one route to admin is `@nextlyhq/plugin-sdk/admin`. Asserted as a // prefix so `@nextlyhq/admin/anything` is caught too — a subpath import is