fix(builder): include iframe padding in the frame's content origin - #700
Conversation
An iframe's nested viewport begins at the CONTENT box, so padding displaces it exactly as a border does. Callers built the inset from clientLeft/clientTop, which report the border alone, so every frame-local point mapped toward the border by the scaled padding. Measured in Chromium: an 8px border with 12px padding puts the border box at x=8 and the inner element at host x=28, while border-only arithmetic computes 16. The 12px gap is the padding exactly. The bordered acceptance case could not expose it because it set only a border; it now sets padding too and asserts both applied, since a case that silently loses its inline style degrades to the at-rest case and passes. Also anchors the discovery check outside the derived glob. The .mts control proves the runner follows that extension by executing, but a glob set that loses .mts stops collecting the control, so the evidence vanishes with the behaviour and the run just reports fewer tests. The assertion now lives in a .ts file, which any glob set that collects anything will collect.
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffe53b784c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… recipe The padding fix duplicated the very thing it was fixing: clientLeft + padding was written independently in the acceptance mapping and in the driver, so a future correction to one would recreate the silent drift. frameInsetOf is now the single reader, exported from the package. It is passed INTO the page rather than called on the Node side — Playwright serializes the function, which works only because it closes over nothing — so one definition serves the editor and the harness alike. Both public recipes corrected too. The README and the FrameGeometry JSDoc still defined the inset as clientLeft/clientTop, so a consumer following either instruction reproduced the offset this branch fixes. They now name the function instead of restating a recipe, because the recipe was documented and three call sites still got it wrong.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36e57110bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Deriving the vitest globs, the layering guard's idea of a test and the file walk from one extension list means a single edit narrows all of them at once. Dropping "ts" from MODULE_EXTENSIONS leaves one .mts control collected and the run reports 1 passed (1) in green, with every layering and geometry guard silently absent. The previous anchor sat in layering.test.ts, reasoning that any glob set collecting anything would collect that file. The narrowing un-collects it too, which is how the mutation went green; its comment now says what it actually covers. The check moves to globalSetup, which runs before any file is collected, so no glob decides whether it executes. Both outcomes are loud: with files collected it throws, and with none collected vitest raises FilesNotFoundError itself. It reads neither BUNDLED_MODULE nor MODULE_EXTENSIONS, because both narrow with the mutation and the check would agree with itself and pass. It walks every file, spells out what a test is named like, and compares that against the globs the runner was handed. An empty walk throws rather than reporting success.
The branch changes runtime behaviour in a published package and adds a public export, so it needs a release record of its own to schedule the train.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Two follow-ups raised against #683 after it merged.
The iframe's content origin excluded padding
An iframe's nested viewport begins at the content box, so padding displaces it exactly as a border does. Callers built the inset from
clientLeft/clientTop, which report the border alone, so every frame-local point mapped toward the border by the scaled padding width.Measured in Chromium rather than reasoned about — an 8px border with 12px padding:
xclientLeftpadding-leftThe 12px gap is the padding exactly.
FrameInsetis now documented as the COMPLETE border-to-content offset, and both DOM readers measureclientLeft + paddingLeft. The geometry module itself is unchanged — it was always correct about the numbers it was given, and the fault was in what it was given.The acceptance case could not have caught this: it set a border and no padding, so it passed whether or not padding was accounted for. It now sets both and asserts both applied, because a case whose inline style loses to the canvas stylesheet degrades to the at-rest case and passes while testing nothing.
The discovery control sat inside the glob it verifies
module-extensions.test.mtsproves the runner follows.mtsby executing. But ifTEST_GLOBSloses that extension, the control stops being collected — so the evidence disappears along with the behaviour it was evidence for, and the run simply reports fewer tests passing.The first attempt at this was wrong, and measurably so. It anchored the assertion in
layering.test.tson the reasoning that a.tsfile is collected by any glob set that collects anything. But the globs are derived from one extension list, so narrowing that list un-collects the anchor too. DroppingtsfromMODULE_EXTENSIONSreports:Green, with every layering and geometry guard silently absent.
A check cannot survive a mutation it depends on, so it moved out of the suite entirely.
vitest.global-setup.tsruns before any file is collected, where no glob decides whether it executes, and both outcomes are loud: with files collected it throws, and with none collected vitest raisesFilesNotFoundErroron its own.It reads neither
BUNDLED_MODULEnorMODULE_EXTENSIONS— both narrow with the mutation, and the check would agree with itself and pass. It walks every file, spells out what a test is named like as a literal, and compares that against the globs the runner was actually handed. An empty walk throws rather than reporting success.Verified against three injected mutations:
tsdropped fromMODULE_EXTENSIONS1 passed (1), greenmtsdroppedmodule-extensions.test.mtslayering.test.tskeeps its glob/extension consistency check, which is a real internal-consistency property, with a comment that no longer claims it survives narrowing.Verification
Builder: 60 tests,
check-typesand lint clean.e2etypechecks and lints clean. The browser suite is CI's to run; I have not re-run it locally against this commit and am not implying otherwise.