diff --git a/.changeset/builder-frame-geometry.md b/.changeset/builder-frame-geometry.md new file mode 100644 index 0000000000..036cce7ba0 --- /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, 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. 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.spec.ts b/e2e/tests/canvas/coordinate-mapping.spec.ts index 0a97a643df..8c5b5b1443 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"` 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. + */ +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}"]`); @@ -50,10 +66,27 @@ 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(); + // 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 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!, { x: origin!.x, y: origin!.y }, scale); + return mapFrameRectToHost(frameRect!, contentOrigin, scale); } /** Largest absolute difference across all four rect components. */ @@ -174,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 50f26ce433..63fa602519 100644 --- a/e2e/tests/canvas/coordinate-mapping.ts +++ b/e2e/tests/canvas/coordinate-mapping.ts @@ -1,58 +1,81 @@ +/** + * 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 { + frameContentOrigin, + pointToCanvas, + pointToHost, + rectToHost, + type FrameGeometry, + type FrameInset, +} 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)); } + +/** + * 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 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 07065cb2ca..d01646e6b4 100644 --- a/e2e/tests/canvas/poc-driver.ts +++ b/e2e/tests/canvas/poc-driver.ts @@ -7,7 +7,12 @@ import { expect, type Frame, type Page } from "@playwright/test"; import { gotoAdmin } from "../support/admin"; -import { mapFrameRectToHost } from "./coordinate-mapping"; +import { + frameContentOrigin, + mapFramePointToHost, + mapFrameRectToHost, + type FrameInset, +} from "./coordinate-mapping"; import type { ActiveTargetReader, CanvasDriver, @@ -83,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; }); } @@ -156,9 +173,26 @@ 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. + // + // 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 frameContentOrigin(box, inset, await frameScale()); }, async readBlockBoxes() { @@ -212,7 +246,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; @@ -453,8 +495,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 @@ -462,11 +508,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() { 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/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 new file mode 100644 index 0000000000..ff56ac8776 --- /dev/null +++ b/packages/builder/src/geometry-ownership.test.ts @@ -0,0 +1,223 @@ +/** + * 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. + * + * 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 + * 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. + * + * 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. + * + * 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 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 + * 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"; +import { collectModules } from "./source-modules"; + +const SRC_DIR = dirname(fileURLToPath(import.meta.url)); + +/** + * The one module allowed to convert between the frame and the host, matched by + * its exact file name. + * + * 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 file IS the named module, by its path beneath `src`. */ +function isModule(file: string, relativePath: string): boolean { + return relative(SRC_DIR, file) === relativePath; +} + +/** + * 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"]); + +/** The package's modules, by the shared rule; only the file reading is local. */ +function sourceFiles(dir: string): string[] { + return collectModules( + dir, + at => readdirSync(at, { withFileTypes: true }), + join + ); +} + +/** 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); + } + // `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); + }; + + visit(source); + return found; +} + +describe("rectangles are read across 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 => !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}`); + }); + + expect(offenders).toEqual([]); + }); + + 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(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", () => { + // 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"]); + }); + + 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", () => { + // The limit of the scan, asserted so it stays true. + // + // 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. + // + // 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" + ); + + expect(computed).toEqual([]); + }); +}); diff --git a/packages/builder/src/geometry.test.ts b/packages/builder/src/geometry.test.ts new file mode 100644 index 0000000000..922ac471ba --- /dev/null +++ b/packages/builder/src/geometry.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it } from "vitest"; + +import { + FrameGeometryError, + frameContentOrigin, + 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("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%", () => { + // 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", () => { + // 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 }); + }); + + 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 }], + ["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..e8b5debff1 --- /dev/null +++ b/packages/builder/src/geometry.ts @@ -0,0 +1,202 @@ +/** + * 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. + * + * 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. + * + * 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 + * 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 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. + * + * Build one with {@link frameContentOrigin} rather than adding that correction + * 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`. + * + * 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.` + ); + } +} + +/** + * 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); + 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 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); + return { + x: origin.x, + y: origin.y, + width: rect.width * frame.scale, + height: rect.height * frame.scale, + }; +} diff --git a/packages/builder/src/index.ts b/packages/builder/src/index.ts index eed78064d0..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 — @@ -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 */ @@ -41,3 +42,23 @@ * 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, + frameContentOrigin, + pointToCanvas, + pointToHost, + rectToHost, + type FrameGeometry, + type FrameInset, + type Point, + type Rect, +} from "./geometry"; diff --git a/packages/builder/src/layering.test.ts b/packages/builder/src/layering.test.ts index e400b6a92f..81b89dd74c 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 { collectModules, TEST_MODULE } from "./source-modules"; /** * The package's layering contract, enforced rather than documented. @@ -35,8 +36,8 @@ import { describe, expect, it } from "vitest"; * `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 @@ -103,16 +104,14 @@ 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)$/; +/** 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 + ); } /** @@ -475,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 @@ -493,7 +502,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 +512,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 new file mode 100644 index 0000000000..cc196104a6 --- /dev/null +++ b/packages/builder/src/source-modules.ts @@ -0,0 +1,99 @@ +/** + * 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. + * + * 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 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. + * + * 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. 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", + "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. + * + * 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 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; +} 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, }, }); 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 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