diff --git a/.changeset/a-link-follows-the-element-the-page-shows.md b/.changeset/a-link-follows-the-element-the-page-shows.md new file mode 100644 index 0000000000..939fbc7145 --- /dev/null +++ b/.changeset/a-link-follows-the-element-the-page-shows.md @@ -0,0 +1,37 @@ +--- +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/builder": patch +"create-nextly-app": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/eslint-plugin": patch +"@nextlyhq/module-specifiers": patch +"nextly": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-mcp": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/ui": patch +--- + +A link saved in a pattern now points at the element the page actually shows. +When a selection held a visible renamed element and a hidden copy with the +same id (hidden by a visibility condition), a link from an unrelated block could +keep the old id, which nothing renders once the hidden copy is left off the +page. The visible element now decides where such a link points. A hidden +element decides only when nothing visible carries that id, so a link and its +target stay together when the condition later shows it. Hidden elements still +get their own ids put back. diff --git a/packages/blocks-engine/src/composition-planners.test.ts b/packages/blocks-engine/src/composition-planners.test.ts index 1e73e7c491..f865eda253 100644 --- a/packages/blocks-engine/src/composition-planners.test.ts +++ b/packages/blocks-engine/src/composition-planners.test.ts @@ -4825,11 +4825,91 @@ describe("a rename record names the nodes it renamed", () => { ).toBe("pricing"); }); - it("keeps a governed link pointing at a target that keeps its id", () => { + it("points an unrelated link at the visible renamed node past a gated namesake", () => { + // The namesake is gated, so the page renders only the renamed node. The + // link belongs to a component that renamed nothing; it has to follow the + // node that actually renders the id rather than keep one nothing renders. + const doc = insertedPage(); + const renamed = marked([...doc.nodes], "renamed"); + const mid = marked([...doc.nodes], "mid"); + const withGatedNamesake = applyOps(doc, [ + { + kind: "insert", + node: node("namesake", { + cssId: renamed.cssId, + props: { mark: "namesake" }, + visibility: { + conditions: [[{ field: "tier", op: "eq", value: "pro" }]], + }, + }), + at: { parentId: mid.id, slot: "children", index: 0 }, + }, + { + kind: "insert", + node: node("stranger", { + origin: { from: "component", id: "def-1" }, + attributes: { "aria-describedby": renamed.cssId ?? "" }, + props: { mark: "stranger" }, + } as Partial), + at: { parentId: mid.id, slot: "children", index: 0 }, + }, + ]).document; + + expect(stored(withGatedNamesake, "wrap", "renamed").cssId).toBe("pricing"); + expect(stored(withGatedNamesake, "wrap", "namesake").cssId).toBe( + renamed.cssId + ); + expect( + stored(withGatedNamesake, "wrap", "stranger").attributes?.[ + "aria-describedby" + ] + ).toBe("pricing"); + }); + + it("restores a pattern's link inside a gated container beside a visible namesake", () => { + // The container holding the renamed node and its link is gated, and an + // unrelated visible node carries the minted id. The link's own record knows + // where it pointed, so the visible namesake must not decide. + const doc = insertedPage(); + const renamed = marked([...doc.nodes], "renamed"); + const wrap = marked([...doc.nodes], "wrap"); + const mid = marked([...doc.nodes], "mid"); + const gatedWithNamesake = applyOps(doc, [ + { + kind: "update", + id: mid.id, + patch: { + visibility: { + conditions: [[{ field: "tier", op: "eq", value: "pro" }]], + }, + }, + }, + { + kind: "insert", + node: node("unrelated", { + origin: { from: "component", id: "def-1" }, + cssId: renamed.cssId, + props: { mark: "unrelated" }, + } as Partial), + at: { parentId: wrap.id, slot: "children", index: 1 }, + }, + ]).document; + + expect(stored(gatedWithNamesake, "wrap", "renamed").cssId).toBe("pricing"); + expect(stored(gatedWithNamesake, "wrap", "unrelated").cssId).toBe( + renamed.cssId + ); + expect( + stored(gatedWithNamesake, "wrap", "link").attributes?.["aria-describedby"] + ).toBe("pricing"); + }); + + it("restores a governed link from its own record past a namesake that keeps its id", () => { // The listed node stays on the page but outside the selection, so the - // record is live and the link's own record would put the source name back. - // The link's target in the saved forest is an unlisted namesake, which - // keeps the minted id — so the link follows it and keeps the id too. + // record is live and says what the link pointed at. The saved forest holds + // an unlisted namesake that keeps the minted id; the link's own record + // decides, so it goes back to the source name rather than following the + // namesake — as a link saved without its target already does. const doc = insertedPage(); const renamed = marked([...doc.nodes], "renamed"); const mid = marked([...doc.nodes], "mid"); @@ -4860,7 +4940,7 @@ describe("a rename record names the nodes it renamed", () => { expect(marked([...saved.nodes], "namesake").cssId).toBe(renamed.cssId); expect( marked([...saved.nodes], "link").attributes?.["aria-describedby"] - ).toBe(renamed.cssId); + ).toBe("pricing"); }); it("restores nothing when the recorded node id occurs twice", () => { diff --git a/packages/blocks-engine/src/composition-planners.ts b/packages/blocks-engine/src/composition-planners.ts index f376137e19..1679f9b464 100644 --- a/packages/blocks-engine/src/composition-planners.ts +++ b/packages/blocks-engine/src/composition-planners.ts @@ -3826,8 +3826,9 @@ function liveEntries( * namesake can carry one id, and a reference the run's record governs can sit * beside an unrelated author's reference to the same id — in each, the right * outcome differs by node. The copier decides which question a node is asked: - * what the id it renders was, or, for an id it references that the copy does not - * settle by its target, what that reference was. + * what the id it renders was, or what an id it references was. A reference takes + * its node's answer where there is one, and follows its target only where there + * is none. * * INVERTED from the record, which reads source → copy because that is the * direction an insert renames in. @@ -3904,7 +3905,8 @@ function restoredFor( * the scope lists the nodes it renamed and this is not one of them. * * Only for the node carrying the id itself. A reference is answered by the - * scope alone; whether it follows its target instead is the copier's question. + * scope alone, and that answer decides it; the copier falls back to the target + * only where the scope has none. */ function renderedAnswer( scope: RenameScope | undefined, diff --git a/packages/blocks-engine/src/tree.reid-with-map.test.ts b/packages/blocks-engine/src/tree.reid-with-map.test.ts index 3b77fb2dfb..cb44664d7e 100644 --- a/packages/blocks-engine/src/tree.reid-with-map.test.ts +++ b/packages/blocks-engine/src/tree.reid-with-map.test.ts @@ -880,9 +880,9 @@ describe('the "restoreEach" DOM id policy', () => { expect((nodes[1].props as { href: string }).href).toBe("#hero"); }); - it("keeps a reference whose one target keeps its id", () => { - // The holder alone would put the source name back, which would store a - // link naming an id its target no longer carries. + it("restores a reference from its holder's answer even when its target keeps its id", () => { + // The holder knows where its reference pointed, so its answer decides; the + // target is followed only for a reference the holder has no answer for. const { nodes } = reidForestWithMap( [ node("target", { cssId: "hero-7f3" }), @@ -892,7 +892,27 @@ describe('the "restoreEach" DOM id policy', () => { ); expect(nodes[0].cssId).toBe("hero-7f3"); - expect(nodes[1].attributes?.["aria-describedby"]).toBe("hero-7f3"); + expect(nodes[1].attributes?.["aria-describedby"]).toBe("hero"); + }); + + it("follows a target placed both open and gated as an open one", () => { + // One node object sits in an open slot and inside a gated container. It + // renders wherever its open placement does, so an unrelated reference + // follows it past a gated namesake — reading it as gated made the two look + // like competing gated targets and left the reference behind. + const sharedTarget = node("shared", { cssId: "shared-1" }); + const { nodes } = reidForestWithMap( + [ + sharedTarget, + node("wrapper", { visibility: gate }, [sharedTarget]), + node("namesake", { cssId: "shared-1", visibility: gate }), + node("holder", { attributes: { "aria-describedby": "shared-1" } }), + ], + { restoreEach: answers({ shared: { "shared-1": "shared" } }) } + ); + + expect(nodes[0].cssId).toBe("shared"); + expect(nodes[3].attributes?.["aria-describedby"]).toBe("shared"); }); it("lets each holder decide a reference nothing in the forest renders", () => { @@ -987,6 +1007,133 @@ describe('the "restoreEach" DOM id policy', () => { expect([...domIds.entries()]).toEqual([["hero-7f3", "hero"]]); }); + /** A condition gate the renderer prunes for a viewer who does not match. */ + const gate = { + conditions: [[{ field: "tier", op: "eq", value: "pro" }]], + } as BlockNode["visibility"]; + + it("follows the visible target past a gated namesake", () => { + // The gated namesake renders nothing, so the reference resolves to the + // visible node on the page. Counting both as targets made the id look + // contested and left the reference on an id nothing renders. + const { nodes } = reidForestWithMap( + [ + node("visible", { cssId: "shared-1" }), + node("hidden", { cssId: "shared-1", visibility: gate }), + node("holder", { attributes: { "aria-describedby": "shared-1" } }), + ], + { restoreEach: answers({ visible: { "shared-1": "shared" } }) } + ); + + expect(nodes[0].cssId).toBe("shared"); + expect(nodes[1].cssId).toBe("shared-1"); + expect(nodes[2].attributes?.["aria-describedby"]).toBe("shared"); + }); + + it("follows a gated target when nothing visible carries the id", () => { + // Excluding gated nodes outright would hand this reference to its holder, + // which answers nothing — storing a link to an id its target no longer + // carries once the gate opens. + const { nodes } = reidForestWithMap( + [ + node("hidden", { cssId: "shared-1", visibility: gate }), + node("holder", { attributes: { "aria-describedby": "shared-1" } }), + ], + { restoreEach: answers({ hidden: { "shared-1": "shared" } }) } + ); + + expect(nodes[0].cssId).toBe("shared"); + expect(nodes[1].attributes?.["aria-describedby"]).toBe("shared"); + }); + + it("treats a node inside a gated subtree as gated", () => { + // Gating is inherited: the renderer prunes the whole subtree, so a + // namesake under a gated parent renders nothing either. + const { nodes } = reidForestWithMap( + [ + node("visible", { cssId: "shared-1" }), + node("wrapper", { visibility: gate }, [ + node("nested", { cssId: "shared-1" }), + ]), + node("holder", { attributes: { "aria-describedby": "shared-1" } }), + ], + { restoreEach: answers({ visible: { "shared-1": "shared" } }) } + ); + + expect(nodes[0].cssId).toBe("shared"); + expect(nodes[2].attributes?.["aria-describedby"]).toBe("shared"); + }); + it("restores a link inside a gated subtree from its own answer past a visible namesake", () => { + // The link's holder has an answer, so it decides: the link must not be + // handed to an unrelated visible namesake just because that one always + // renders. + const { nodes } = reidForestWithMap( + [ + node("unrelated", { cssId: "shared-1" }), + node("wrapper", { visibility: gate }, [ + node("target", { cssId: "shared-1" }), + node("link", { attributes: { "aria-describedby": "shared-1" } }), + ]), + ], + { + restoreEach: answers({ + target: { "shared-1": "shared" }, + link: { "shared-1": "shared" }, + }), + } + ); + const inside = nodes[1].slots?.children ?? []; + + expect(nodes[0].cssId).toBe("shared-1"); + expect(inside[0]?.cssId).toBe("shared"); + expect(inside[1]?.attributes?.["aria-describedby"]).toBe("shared"); + }); + + it("restores a gated link from its own answer beside a namesake that keeps its id", () => { + // The only node rendering the id inside the gate keeps it, and nothing + // visible carries it. The link's holder knows where it pointed, so its + // answer decides rather than whichever node happens to share the id. + const { nodes } = reidForestWithMap( + [ + node("wrapper", { visibility: gate }, [ + node("namesake", { cssId: "shared-1" }), + node("link", { attributes: { "aria-describedby": "shared-1" } }), + ]), + ], + { restoreEach: answers({ link: { "shared-1": "shared" } }) } + ); + const inside = nodes[0].slots?.children ?? []; + + expect(inside[0]?.cssId).toBe("shared-1"); + expect(inside[1]?.attributes?.["aria-describedby"]).toBe("shared"); + }); + + it("restores a link under nested gates from its own answer past a visible namesake", () => { + // However many gates sit above the link, its holder's answer decides; a + // visible namesake that always renders does not. + const { nodes } = reidForestWithMap( + [ + node("unrelated", { cssId: "shared-1" }), + node("outer", { visibility: gate }, [ + node("target", { cssId: "shared-1" }), + node("inner", { visibility: gate }, [ + node("link", { attributes: { "aria-describedby": "shared-1" } }), + ]), + ]), + ], + { + restoreEach: answers({ + target: { "shared-1": "shared" }, + link: { "shared-1": "shared" }, + }), + } + ); + const outer = nodes[1].slots?.children ?? []; + const inner = outer[1]?.slots?.children ?? []; + + expect(outer[0]?.cssId).toBe("shared"); + expect(inner[0]?.attributes?.["aria-describedby"]).toBe("shared"); + }); }); describe("only the id a node RENDERS may be reminted", () => { diff --git a/packages/blocks-engine/src/tree.ts b/packages/blocks-engine/src/tree.ts index b84fab3763..94221b1459 100644 --- a/packages/blocks-engine/src/tree.ts +++ b/packages/blocks-engine/src/tree.ts @@ -1110,12 +1110,11 @@ export type DomIdPolicy = * asks with the ORIGINAL node: * * - for the id a node RENDERS, and moves that id to the answer; - * - for an id a node REFERENCES, only when the copy does not settle it. A - * reference follows its target: where the copied forest renders the id - * and every node rendering it ends up with one id, the reference takes - * that id, so a link and its target are never stored apart. Where - * nothing renders it, or its renderers end up with different ids, the - * node holding the reference decides. + * - for an id a node REFERENCES, and a reference takes that answer when + * there is one. The node holding it knows where it pointed. Only a + * reference its holder has no answer for follows its target: where the + * copied forest renders the id and the nodes settling it end up with one + * id, the reference takes that id; otherwise it keeps what it carries. * * Gating is not consulted, for the reason `restore` gives: putting an id * back asks nothing about the page. @@ -1506,8 +1505,12 @@ type RestoreEach = (node: BlockNode, value: string) => string | undefined; /** What the per-node copier carries from node to node. */ interface EachCopy { readonly nodeIds: Map; - /** Each rendered id → every id the nodes rendering it ended up with. */ + /** Each id a VISIBLE node renders → every id those nodes ended up with. */ readonly outcomes: Map>; + /** The same, for nodes inside a subtree the renderer prunes. */ + readonly gatedOutcomes: Map>; + /** Every node the renderer would prune, by identity. */ + readonly hidden: ReadonlySet; /** Each copy's id → the original it was made from, for the reference pass. */ readonly holders: Map; readonly restoreEach: RestoreEach; @@ -1518,18 +1521,32 @@ interface EachCopy { * * Two passes, as every policy takes. The first moves the id each node renders * to that node's own answer and notes, per id, every id its renderers ended up - * with. The second rewrites references: a reference to an id those notes - * settle takes the settled id, so it follows its target; any other reference is - * decided by the node holding it. - * - * No hidden-subtree set, and no shared memo of replacements. Putting an id back - * asks nothing about the page, so gating is irrelevant; and two nodes spelling - * one id may now answer differently, which a memo keyed by the id alone would - * collapse into one answer for both. - * - * `domIds` reports a rendered id only where every node rendering it moved to - * the same id — the one reading of "what this copy moved" that stays true when - * renderers disagree. + * with. The second rewrites references: a reference its holder has an answer + * for takes that answer, and only one it has none for follows the id those notes + * settle. + * + * The holder first, because it knows where its reference pointed. Settling a + * reference by its target instead means deciding which elements render + * together, and that question has no end: gated namesakes, nested gates, a node + * placed under two gates, sibling gates whose conditions agree, and conditions + * equal in meaning but written differently, which nothing can decide from the + * document. Following the target is kept for the reference nobody recorded. + * + * Gating decides only which nodes a reference follows, never which ids move. + * Putting an id back asks nothing about the page, so a gated node still has its + * own id restored. But a reference resolves to the element the page RENDERS, + * and a gated namesake renders nothing — so the nodes the renderer keeps settle + * a reference first, and gated ones only when no visible node carries the id. + * Treating both alike made a visible target and its gated namesake look + * contested, and left an unrelated link on an id nothing renders. + * + * No shared memo of replacements: two nodes spelling one id may answer + * differently, which a memo keyed by the id alone would collapse into one + * answer for both. + * + * `domIds` reports a rendered id only where the nodes that settle it all moved + * to the same id — the one reading of "what this copy moved" that stays true + * when renderers disagree. */ function reidForestRestoringEach( nodes: BlockNode[], @@ -1538,6 +1555,12 @@ function reidForestRestoringEach( const each: EachCopy = { nodeIds: new Map(), outcomes: new Map(), + gatedOutcomes: new Map(), + // Gated where no placement of the node is open. The pruning rule's own walk, + // read the other way: `hiddenSubtreeNodes` asks whether ANY placement is + // pruned, and a node an unrelated reference follows renders wherever any + // placement is not. + hidden: prunedInEveryPlacement(nodes), holders: new Map(), restoreEach, }; @@ -1553,7 +1576,7 @@ function reidForestRestoringEach( return { nodes: linked, nodeIds: each.nodeIds, - domIds: settledMoves(each.outcomes), + domIds: settledMoves(each), }; } @@ -1566,7 +1589,11 @@ function reidOneRestoringEach(node: BlockNode, each: EachCopy): BlockNode { const rendered = renderedDomId(node); if (rendered !== undefined) { const answer = each.restoreEach(node, rendered); - noteOutcome(each.outcomes, rendered, answer ?? rendered); + noteOutcome( + each.hidden.has(node) ? each.gatedOutcomes : each.outcomes, + rendered, + answer ?? rendered + ); if (answer !== undefined) { moveOwnIds( copy, @@ -1598,13 +1625,29 @@ function soleOutcome( return only; } -/** Each rendered id that moved, to the one id all of its renderers became. */ -function settledMoves( - outcomes: ReadonlyMap> -): Map { +/** + * The one id a reference to `value` follows, or nothing when the copy does not + * settle it. + * + * The nodes the page renders decide whenever any of them carries the id; gated + * nodes decide only where none does, so a link to a target that is gated today + * still follows it and is not split from it when the gate opens. + */ +function settledTarget(each: EachCopy, value: string): string | undefined { + const visible = each.outcomes.get(value); + if (visible !== undefined) return soleOutcome(visible); + return soleOutcome(each.gatedOutcomes.get(value)); +} + +/** Each rendered id that moved, to the one id the nodes settling it became. */ +function settledMoves(each: EachCopy): Map { const moved = new Map(); - for (const [value, became] of outcomes) { - const only = soleOutcome(became); + const values = new Set([ + ...each.outcomes.keys(), + ...each.gatedOutcomes.keys(), + ]); + for (const value of values) { + const only = settledTarget(each, value); if (only !== undefined && only !== value) moved.set(value, only); } return moved; @@ -1613,9 +1656,9 @@ function settledMoves( /** * What each id one node references should now be. * - * Settled by the copy where it can be: an id the forest renders, whose - * renderers all ended up with one id, is that id — kept or moved — whatever the - * holder would have said. Otherwise the holder decides. + * The holder's own answer where it has one. Otherwise settled by the copy where + * it can be: an id the forest renders, whose settling renderers all ended up + * with one id, is that id — kept or moved. Otherwise the reference is kept. * * A lookup rather than a map, because the answer depends on the holder and * building a whole map per node would put every id the records name into the @@ -1629,12 +1672,11 @@ function referenceAnswers( return { size: 1, get(value: string): string | undefined { - const became = each.outcomes.get(value); - if (became !== undefined && became.size === 1) { - const settled = soleOutcome(became); - return settled === value ? undefined : settled; - } - return holder === undefined ? undefined : each.restoreEach(holder, value); + const own = + holder === undefined ? undefined : each.restoreEach(holder, value); + if (own !== undefined) return own; + const settled = settledTarget(each, value); + return settled === value ? undefined : settled; }, }; } @@ -1757,15 +1799,48 @@ export function remapIdReferences( export function hiddenSubtreeNodes( nodes: readonly BlockNode[] ): ReadonlySet { - const hidden = new Set(); - const seen = new Map(); + return placementGating(nodes).gated; +} + +/** + * Every node no placement of which is open, by identity. + * + * The walk reaches a node object placed in two slots once per placement. A + * placement under a gate is pruned and one under none renders, so such a node + * renders wherever any placement is open — the question an unrelated reference + * following its target needs answered, where {@link hiddenSubtreeNodes} answers + * whether any placement is pruned. + */ +function prunedInEveryPlacement( + nodes: readonly BlockNode[] +): ReadonlySet { + const { gated, open } = placementGating(nodes); + return new Set([...gated].filter(node => !open.has(node))); +} + +/** + * Which nodes are reached under a gate on some placement, and which are reached + * under none on some placement. + * + * One walk for both readings, so the pruning rule and its converse cannot come + * to disagree about inheritance. A placement inherits from its parent's CURRENT + * visit, because the gate over a placement is the one on the path it was + * reached by. + */ +function placementGating(nodes: readonly BlockNode[]): { + gated: Set; + open: Set; +} { + const gated = new Set(); + const open = new Set(); + const current = new Map(); walkNodes([...nodes], (node, parent) => { - const inherited = parent !== undefined && seen.get(parent) === true; - const gated = inherited || isConditionGated(node); - seen.set(node, gated); - if (gated) hidden.add(node); + const inherited = parent !== undefined && current.get(parent) === true; + const isGated = inherited || isConditionGated(node); + current.set(node, isGated); + (isGated ? gated : open).add(node); }); - return hidden; + return { gated, open }; } /**