feat(plugin-page-builder): adopt the builder shell for editor chrome - #865
Conversation
The editor mounts standalone, where leaving means navigating away from unsaved canvas state, and embedded as a field inside an entry form, where the author is already on the page an exit would return them to. onExit becomes optional. Omitted, no exit affordance renders anywhere — and in the narrow-viewport notice the sentence promising an escape goes with the button. Copy and control are one unit: keeping the sentence instructs the author to leave and offers nothing to leave with, and a button carrying no handler still takes focus and depresses, which teaches that leaving does nothing. The handler is the capability rather than a mode flag beside it, so the state where a standalone boolean and a missing handler disagree cannot be represented. The device-preview switcher moves out of the plugin toolbar into its own component, so it can be passed to the shell top bar rather than deleted with the bar it used to sit in. The shell has no breakpoint UI to adopt: its breakpoint dialog edits which breakpoints exist and is exported from no entry.
The editor hand-rolled a three-pane layout, a toolbar and a breakpoint switcher. The shell supplies all of that as slots, so the surface passes canvas, inspector, library and top bar into it instead of laying them out. The drag provider and its overlay stay exactly where they were. The shell never looks inside the canvas slot and owns no drag machinery, so the refusal pill and its live region move with the subtree unchanged. The invalid-slot banner stays a sibling above the shell rather than moving into the canvas slot: the blocks it reports are not drawn on the canvas at all, so anchoring it there would put the notice beside the one place that cannot show what it is about. Neither mount passes onExit. Both render inside the admin rather than as a full-screen takeover, so there is nowhere to exit to and the shell draws no exit affordance.
The canvas region was a main. Every mount the shell has sits inside a host that already renders one — the admin dashboard layout does, and the editor is embedded in it — so a second gave assistive technology two competing primary landmarks and made strict main locators ambiguous. A named section is still exposed as a landmark, and for an editor whose surrounding page owns the primary content, region is the more accurate description. The ref and tabIndex move with the element: F6 cycling focuses this node, and losing either would read as a focus bug rather than a markup change. No prop for it. Every real mount wants the same answer, so a mode would default to the wrong value everywhere and carry a branch nothing could exercise correctly. The four locators that found the canvas by its main role now ask for the region.
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 36 minutes Limit details: You’ve used all 1 included review currently available under your plan. 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 (2)
📒 Files selected for processing (20)
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 |
github-actions
Bot
added
type: docs
@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: 8a3522bd1b
ℹ️ 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".
Review from the
|
The shell opens no left panel by default, which is right for an editor whose author already knows the document and wrong for a page builder, where the library is how you begin. Adopting the shell therefore hid a pane that had been permanently visible, behind a rail button an author has no reason to press yet. Seeded through the shell preference port rather than a new prop: the port already lets a host decide what nothing-stored-yet means, and the shell falls back to its own defaults only when read answers null. Answering with a seed instead needs no API. It is a first-run default and not a policy. Once the author opens or closes a panel their choice persists, closing the library included. Found by the canvas drag specs failing to locate any library entry, not by a unit test, so there is one now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4025563dce
ℹ️ 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".
…allow The manifest offered react 18 while depending on blocks-react, which requires 19. That combination gives nobody react 18 support: it hands a react 18 app an unsatisfiable peer graph and an error naming a transitive package they never installed. The promise predates the builder dependency and was never keepable. Narrowed to 19, which is what the repo installs, what the renderer serving published pages already requires, and what nothing here has ever tested against anything else. Guarded by deriving both sides from the manifests rather than restating a literal: a hardcoded expectation keeps passing after a dependency widens, which is the case worth catching. The check asserts it read some dependencies before judging them, and reports an unreadable manifest rather than skipping it.
…ditor The editor renders the builder shell, whose layout rules and --nx-builder-* tokens live in that package sheet. This plugin documents and exports exactly one stylesheet, so a host following the documentation precisely still got a shell with no layout and no colours. An @import rather than a component-level import: this package copies its styles directory into dist instead of running CSS through the bundler, so a side-effect import in a component would never be processed. Placed first, because CSS drops an @import that follows a style rule. The test pins the ORDER as well as the presence, since an import at the bottom reads as configured and is silently ignored by every browser.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec30b5d5e9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0a6e74683
ℹ️ 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".
A form may embed several page-builder fields. Every store read and wrote one key, so opening one surface applied the panel selection and widths the other had last written — which reads as the editor forgetting the choice at random. Keyed by the same identifier the provider takes as its draft key, so the two cannot disagree. The standalone edit view passes none and keeps the bare key, which is right for the only surface of its kind on a page. The full edit view also now supplies its cancel handler as the exit. It was the one mount with somewhere to go, and without it no exit affordance rendered anywhere at all — the optional prop was doing nothing but hide the control.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e306c299
ℹ️ 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".
The rail drew all seven panels while a host might fill one, so six controls opened a reserved panel and shrank the canvas to show nothing — a broken control rather than an absent feature. They are shown and disabled rather than hidden: the set is the editor shape, and hiding the unbuilt ones would make the chrome change under an author as features land. Each says what it is and that it is coming. The host passes the same set renderPanel returns content for, so the rail cannot disagree with the panel body. Omitting it treats every panel as available, which keeps a host that fills all of them from enumerating them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d70acd86ab
ℹ️ 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".
…not merely present
…dits Wiring the host cancel handler into Exit exposed a path that navigates away and consults nothing. The canvas holds unsaved edits in memory and its local draft is restored nowhere, so a click discarded the whole edit — fastest right after a change, while the draft write was still inside its debounce. The confirmation belongs to the editor rather than the host: the host cannot see dirty state, and every host would otherwise have to remember this. A clean document still exits on one click. The shell also normalises a restored panel selection against what the host can fill. Disabling the rail button did not cover it — nobody clicked, the selection came out of storage — so the layout still reserved a panel whose content renders nothing. One predicate now answers both, so the button and the reserved panel cannot disagree.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfe5c13c41
ℹ️ 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".
The guard asked whether a range mentioned 18, which answers a narrower question than the contract it states. It was silent on this package advertising ^19.0.0 against a dependency that has moved to ^20.0.0, silent on the package widening to ^19.0.0 || ^20.0.0, and would have reported 19.x against ^19.0.0 despite the two denoting the same set. Compatibility is decided by whether every React version satisfying our range also satisfies the dependency's, so ask that directly. An unparseable range on either side is reported rather than skipped, since treating it as compatible would let a typo silence the check for that dependency. The predicate is exercised on known answers as well, because the manifest-driven assertion reports nothing while the workspace is consistent, and that silence is the same output whether the comparison works or cannot see anything. The population assertion moves from a count to naming blocks-react: a count is satisfied by a selector that drops the renderer and picks up something else.
Title and slug lived in SaveShell's own useState while every consumer asked EditorProvider's dirty flag for whether unsaved work existed. A component local copy cannot move that flag, so retyping a page's title and nothing else left the editor reporting itself clean — and the exit confirmation, which reads that flag, navigated away and discarded the edit without asking. Two owners of one question is what produced it, so the repair removes one rather than teaching the guard about the second: the editor state now holds the page fields, SET_PAGE_METADATA marks the page unsaved like any other edit, and MARK_SAVED clears them together. A field added later inherits this by being stored here, rather than by whoever adds it remembering that a second place exists. The draft write carries the metadata for the same reason, and REPLACE preserves it — rebuilding without it would blank the inputs mid-edit while also clearing dirty, so the loss would be silent.
The binding is registered on the document, so every mounted shell sees every press, and eligibility came from viewport state alone. A shell embedded as a field in an entry form therefore answered F6 from a sibling input and moved focus into an editor the author was not in; with several such fields, the most recently registered one won. Eligibility now asks where focus IS. Containment is asked of the regions rather than of a separate root element, since they are what cycling already moves between and a second ref for the same question could disagree with the first. Focus resting on the body still enters, which is what keeps the key working on a page whose editor owns it and has not been clicked yet — but only while one shell is mounted, because with more than one the press names none of them.
|
@codex please review this PR |
The count deciding whether one shell is mounted is what allows F6 to enter from a page where focus rests on the body. A count that only rose would decline that entry permanently, which is the defect the focus rule fixes arriving from the other side — and it would appear first in dev, where a hot reload remounts. Asserting the count after a single mount cannot separate the two: it passes on a leaking counter the first time. Mounting, tearing down and mounting again is what distinguishes them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eb18d7421
ℹ️ 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".
Asking the region list who owns the key rejected the shell whenever focus sat in the chrome header. The header carries the exit button and the host's top bar, and it is a sibling of the region container rather than inside one, so F6 did nothing from the very chrome it belongs to. Root and regions answer different questions that looked like one: the root says whether the author is inside this editor, the region list says where cycling can land. Ownership moves to the existing chrome ref, which already wraps both the header and the regions; the region list still decides the cycle. Declining presses from outside the shell is unchanged.
… sheet
The ordering assertion took `.nx-pb-editor {` as the first style rule, so a
rule inserted above the import but below that selector invalidated the
import while the comparison still read correctly. Every browser drops an
`@import` that follows a style rule, and the file would have shipped with
no chrome styles at all.
Both operands have now defeated a text search in turn: the bare word
`@import` matched the comment above the statement, and one selector stands
in for every rule. Neither is a property of the file. Walking the parsed
stylesheet's top-level children asks what CSS asks — does any style rule
precede the import.
`generate` produces the prelude rather than the node's shape being guessed
at; css-tree gives an `AtrulePrelude` here, and a wrong guess matches
nothing, which reads as a missing import rather than a bad probe.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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
What
The page-builder editor hand-rolled a three-pane layout, a toolbar and a device switcher.
@nextlyhq/builderalready ships all of that as a shell with slots. This is A-2 + A-3 of the B-6 integration sequence (2026-08-15-op-model-decision.md§5, founder-confirmed): the editor now passes its canvas, inspector, block library and device switcher into the shell rather than laying them out itself.Why one PR
Splitting A-2 from A-3 would touch
EditorSurface.tsxtwice and leave an interim state where the shell wraps a hand-rolled toolbar. One claim: the editor's chrome is now the shell's.Two defects found on the way, both pre-existing
1. The shell claimed the document's primary landmark. Its canvas region was
<main aria-label="Canvas">(builder-shell.tsx:701), andpackages/admin/src/layout/DashboardLayout.tsx:105already renders<main>. Every mount of this shell is inside a host that owns one — so adopting it put two non-hiddenmainelements in one document: invalid markup, two competing primary landmarks, and every strictmainlocator made ambiguous.Now a named
section, which is still exposed as a landmark (region) and is the more accurate description of an editor embedded in a page whose primary content is the surrounding form. No prop for it — every real mount wants the same answer, so a mode would default to the wrong value everywhere and carry a branch nothing could exercise correctly. TherefandtabIndex={-1}move with the element: F6 region cycling focuses that node, and losing either would read as a focus bug rather than a markup change.Four locators that found the canvas by
role="main"now ask forrole="region"— the builder's unit tests,e2e/tests/shell/driver.ts, andshell.spec.ts.2. A guard blind to the case it was written for.
one-main-per-document.test.tsscans only this package's admin directory, so the shell's<main>— in another package — was never in its population. It went red here for an adjacent reason (its third assertion about markup that legitimately moved) and happened to land on the real defect.Fixing the markup makes it green and leaves the blindness. Rather than let that read as coverage, the blind spot is now stated in the file: read a green result as "this package adds no second landmark", never as "the document has one
main". Widening the population to coverpackages/builder/srcis tracked separately and owned by that package's lane.onExitis optional nowThe editor mounts standalone and embedded as a field inside an entry form, where there is nowhere to exit to. Omitted, no exit affordance renders — and in the narrow-viewport notice the sentence promising an escape goes with the button. Copy and control are one unit: keeping the sentence instructs the author to leave and offers nothing to leave with; keeping a button with no handler still takes focus and depresses, teaching that leaving does nothing.
The handler is the capability rather than a mode flag beside it, so the state where a
standaloneboolean and a missing handler disagree cannot be represented.What deliberately did NOT move
The
<DragDropProvider>and its<DragOverlay>— including #847's refusal pill and its live region — stay exactly where they were. Measured:builder-shell.tsxis 828 lines with zero matches forDragOverlay|DndContext|DragDropProvider|useDraggable|dnd-kit, and itschildrendocblock reads "The canvas. The shell never looks inside it." So the shell wraps the canvas rather than hosting a drag context, and no thirdrole="status"region is introduced (there are already two in source plus dnd-kit's at runtime — a filed, separate defect).The invalid-slot banner stays a sibling above the shell rather than moving into the canvas slot: the blocks it reports are not drawn on the canvas at all.
Verification
Every guard broken on purpose and required to fail for the intended reason:
× renders no exit affordance at all× drops the escape sentence WITH the button× delegates the canvas landmark to the shellplugin-page-builder: 802/802 pass, both tsconfig programs clean (includingtsconfig.tests.json, new onmainin #864).builder: 29/29. Lint 21/21. Sherif: 0 errors.Coordination
EditorSurface.tsxwas cleared by lane 66 (feat(plugin-page-builder): tell an author why a drop was refused #847, fix(root): size the settle allowance to a frame, and check the specs nothing read #864 both merged) and by the canvas lane, which reaches its store wiring at step 3 at the earliest.BuilderShellPropschanges were both agreed with the builder package's lane before writing, per a standing freeze on that surface. They ruled against my first proposal on the landmark and were right.Known, not fixed here
The narrow-viewport notice triggers on the viewport via
matchMedia, while the shell sizes to its container. So an embedded shell in a narrow column on a wide screen shows no notice, and one in a wide column on a narrow screen does. Two lanes have now seen it; it is arguably its own defect and is out of scope for a wiring PR.