From 7114898f01406e2f3f36de0923ca2cb7a9599ee6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 15:31:04 +0000 Subject: [PATCH] fix(app-shell): draw real canvas chrome for renderable-but-unoffered block types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PageBlockCanvas` read each node's icon and colour tone from `BLOCK_TYPE_META`, which is the palette's OFFER list ("what may an author drag in"). The canvas is answering a different question ("what may an author already have in this page"). The two diverge for an alias pair: `record:discussion` and `record:chatter` are one renderer under two names, so the palette advertises exactly one of them and the other spelling — which renders perfectly — fell through to the unknown-block box and the neutral `misc` grey. `block-types.ts` now declares `BLOCK_RENDERER_ALIAS_GROUPS` and resolves display chrome through `resolveBlockDisplayMeta()`, which falls back to an alias sibling's entry. Keyed on renderer IDENTITY, not on "is excluded": the guard test asserts every group member resolves through `ComponentRegistry` to the same component, so exclusions that genuinely are not page blocks keep the generic box. The palette offer list is unchanged — nothing became draggable. --- .changeset/rotten-pugs-invent.md | 9 + .../previews/PageBlockCanvas.tsx | 8 +- .../__tests__/canvas-display-meta.test.tsx | 259 ++++++++++++++++++ .../metadata-admin/previews/block-types.ts | 72 ++++- 4 files changed, 345 insertions(+), 3 deletions(-) create mode 100644 .changeset/rotten-pugs-invent.md create mode 100644 packages/app-shell/src/views/metadata-admin/previews/__tests__/canvas-display-meta.test.tsx diff --git a/.changeset/rotten-pugs-invent.md b/.changeset/rotten-pugs-invent.md new file mode 100644 index 0000000000..752b948778 --- /dev/null +++ b/.changeset/rotten-pugs-invent.md @@ -0,0 +1,9 @@ +--- +'@object-ui/app-shell': patch +--- + +Page-block canvas: draw the real icon and colour tone for block types that render fine but are not offered in the palette. + +The canvas read its per-node icon and tone from `BLOCK_TYPE_META`, which is the palette's *offer* list ("what may an author drag in") — while the canvas is answering a different question ("what may an author already have in this page"). The two diverge for an alias pair: `record:discussion` and `record:chatter` are one renderer under two names, so exactly one spelling is always unoffered, and a page carrying it showed a plain unknown-block box and neutral grey instead of the message icon and the blue record tone its twin gets. + +`block-types.ts` now declares `BLOCK_RENDERER_ALIAS_GROUPS` — spellings that resolve to the same registered renderer — and the canvas resolves display chrome through `resolveBlockDisplayMeta()`, which falls back to an alias sibling's entry. Keyed on renderer identity, not on "is excluded": palette exclusions that genuinely are not page blocks (`element:text_input`, `element:record_picker`, `element:form`, `ai:chat_window`) keep the generic box, and nothing became draggable — the palette offer list is unchanged. diff --git a/packages/app-shell/src/views/metadata-admin/previews/PageBlockCanvas.tsx b/packages/app-shell/src/views/metadata-admin/previews/PageBlockCanvas.tsx index 160d6a38de..171a5efb7d 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/PageBlockCanvas.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/PageBlockCanvas.tsx @@ -31,6 +31,7 @@ import { TYPES_BY_CATEGORY, CATEGORY_LABEL_EN, UnknownBlockIcon, + resolveBlockDisplayMeta, resolveBlockTone, type BlockTypeId, } from './block-types.js'; @@ -619,7 +620,12 @@ function BlockRow({ onRenameLabel: (nextLabel: string) => void; }) { const typeStr = String(block.type ?? ''); - const meta = BLOCK_TYPE_META[typeStr as BlockTypeId]; + // DISPLAY meta, not the palette catalogue. `BLOCK_TYPE_META` answers "what may + // an author drag in"; a node already in the page may be a spelling the palette + // does not offer yet the app renders fine — the `record:discussion` / + // `record:chatter` alias pair. See BLOCK_RENDERER_ALIAS_GROUPS in + // block-types.ts; nothing here makes a type offerable. + const meta = resolveBlockDisplayMeta(typeStr); const Icon = meta?.Icon ?? UnknownBlockIcon; const tone = resolveBlockTone(typeStr); const label = blockLabel(block); diff --git a/packages/app-shell/src/views/metadata-admin/previews/__tests__/canvas-display-meta.test.tsx b/packages/app-shell/src/views/metadata-admin/previews/__tests__/canvas-display-meta.test.tsx new file mode 100644 index 0000000000..b15bb58cb5 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/__tests__/canvas-display-meta.test.tsx @@ -0,0 +1,259 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#5837 — the page canvas draws chrome for what a page CAN CONTAIN, + * not for what the palette OFFERS. + * + * `BLOCK_TYPE_META` is the palette's offer list ("what may an author drag in"). + * `PageBlockCanvas` was reading it to pick a node's icon and colour tone, which + * is a different question ("what may an author already have in this page"). + * The two coincide for every palette exclusion whose reason is that the block + * is not page content at all — and diverge for an alias pair, where one renderer + * has two spellings and the palette deliberately advertises only one of them. + * The unadvertised spelling renders perfectly and got the unknown-block box. + * + * ## What this file pins, and how it avoids two easy ways of being wrong + * + * 1. RENDERABILITY, NOT EXCLUSION. `BLOCK_RENDERER_ALIAS_GROUPS` is only + * allowed to list spellings that the runtime registry resolves to the very + * same component. That is asserted here against the real `ComponentRegistry` + * (identity, not "both defined"), so the declaration cannot quietly grant + * friendly chrome to a type that has no renderer behind it. The counter-probe + * is `element:text_input`: excluded for an unrelated reason, no twin, and it + * must keep the generic box. + * + * 2. NO ORIENTATION IS HARD-CODED. Which member of the pair the palette + * advertises is a maintainer decision that already flipped once (#5495 moved + * it from the legacy alias to the canonical name). Every assertion below + * DERIVES the offered/unoffered split from `BLOCK_TYPE_META` instead of + * naming a side, and one test flips the catalogue outright to prove the + * resolver follows. + * + * The offer list itself must not move: "the icon appears" is also satisfiable by + * quietly making the type draggable, which is exactly the invariant #2943 built + * `PALETTE_EXCLUSIONS` to protect. That is the third block of tests. + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { ComponentRegistry } from '@object-ui/core'; +// Side-effect import: registers `record:chatter` and `record:discussion`. The +// app-shell test setup does not pull plugin-detail in, and relying on another +// file having imported it first would make this suite order-dependent. +import '@object-ui/plugin-detail'; +import { + BLOCK_TYPE_META, + BLOCK_CATEGORY_TONE, + BLOCK_RENDERER_ALIAS_GROUPS, + PALETTE_EXCLUSIONS, + TYPES_BY_CATEGORY, + UnknownBlockIcon, + resolveBlockDisplayMeta, + resolveBlockTone, +} from '../block-types'; +import { PageBlockCanvas } from '../PageBlockCanvas'; + +afterEach(cleanup); + +const offered = BLOCK_TYPE_META as Record; +const mutableMeta = BLOCK_TYPE_META as unknown as Record; + +/** The declared groups, split into what the palette advertises and what it does not. */ +const groupSplits = BLOCK_RENDERER_ALIAS_GROUPS.map((group) => ({ + group, + advertised: group.filter((t) => offered[t] !== undefined), + unadvertised: group.filter((t) => offered[t] === undefined), +})); + +describe('BLOCK_RENDERER_ALIAS_GROUPS — the declaration is keyed on renderer identity (#5837)', () => { + const config = (type: string) => + ComponentRegistry.getConfig(type) as { component?: React.ComponentType } | undefined; + + it('declares at least one group, each with at least two spellings', () => { + // Non-vacuity: every "for each group" assertion below passes over an empty + // list, and a one-member group has nothing to borrow from. + expect(BLOCK_RENDERER_ALIAS_GROUPS.length).toBeGreaterThan(0); + for (const group of BLOCK_RENDERER_ALIAS_GROUPS) { + expect(group.length, `${JSON.stringify(group)} is not a pair`).toBeGreaterThan(1); + } + }); + + it('every spelling in a group resolves to the SAME registered renderer', () => { + // This is how "renderable" is determined — not from the exclusion ledger, + // which records unrelated reasons too, but from the registry itself. Two + // independently registered renderers that happened to diverge would pass a + // defined/defined check; identity is what makes borrowing chrome honest. + for (const group of BLOCK_RENDERER_ALIAS_GROUPS) { + const configs = group.map((t) => ({ type: t, cfg: config(t) })); + for (const { type, cfg } of configs) { + expect(cfg, `\`${type}\` is declared an alias but nothing registers it`).toBeDefined(); + expect(cfg!.component, `\`${type}\` registers no component`).toBeDefined(); + } + const [first, ...rest] = configs; + for (const other of rest) { + expect( + other.cfg!.component, + `\`${other.type}\` and \`${first.type}\` are declared one renderer but resolve to two`, + ).toBe(first.cfg!.component); + } + } + // Non-vacuity for the lookup: a `getConfig` that returned a truthy object + // for anything at all would make every assertion above meaningless. + expect(config('record:zzNotARegisteredType')).toBeUndefined(); + }); + + it('each group has exactly one advertised spelling, and the rest carry an exclusion reason', () => { + for (const { group, advertised, unadvertised } of groupSplits) { + expect( + advertised.length, + `${JSON.stringify(group)} must have exactly one palette entry — one entry per renderer`, + ).toBe(1); + for (const type of unadvertised) { + expect( + PALETTE_EXCLUSIONS[type], + `\`${type}\` is unoffered but absent from the exclusion ledger`, + ).toBeTruthy(); + } + } + }); +}); + +describe('canvas chrome for a renderable-but-unoffered spelling (#5837)', () => { + it('the unadvertised spelling borrows the icon and tone of its twin', () => { + for (const { advertised, unadvertised } of groupSplits) { + const twin = offered[advertised[0]]!; + for (const type of unadvertised) { + const display = resolveBlockDisplayMeta(type); + expect(display, `\`${type}\` still resolves to no display meta`).toBeDefined(); + expect(display!.Icon, `\`${type}\` draws a different icon than \`${advertised[0]}\``) + .toBe(twin.Icon); + expect(display!.Icon).not.toBe(UnknownBlockIcon); + expect(display!.category).toBe(twin.category); + expect(resolveBlockTone(type)).toBe(resolveBlockTone(advertised[0])); + expect(resolveBlockTone(type)).not.toBe(BLOCK_CATEGORY_TONE.misc); + } + } + }); + + it('an offered type still resolves to its own entry, unchanged', () => { + for (const [type, meta] of Object.entries(BLOCK_TYPE_META)) { + const display = resolveBlockDisplayMeta(type); + expect(display!.Icon, type).toBe(meta.Icon); + expect(display!.category, type).toBe(meta.category); + } + }); + + it('follows the pair when the palette flips which spelling it advertises', () => { + // #5495 moved the advertised spelling from the legacy alias to the canonical + // name; nothing stops a future ruling moving it back. A fix that named a + // direction would re-open this gap on that day, so the resolver is proved + // symmetric here rather than asserted in prose. + const { advertised, unadvertised } = groupSplits[0]; + const wasOffered = advertised[0]; + const wasUnoffered = unadvertised[0]; + const saved = mutableMeta[wasOffered]; + try { + delete mutableMeta[wasOffered]; + mutableMeta[wasUnoffered] = saved; + const flipped = resolveBlockDisplayMeta(wasOffered); + expect(flipped, 'the formerly-offered spelling lost its chrome after the flip').toBeDefined(); + expect(flipped!.Icon).toBe((saved as { Icon: unknown }).Icon); + expect(resolveBlockTone(wasOffered)).not.toBe(BLOCK_CATEGORY_TONE.misc); + } finally { + delete mutableMeta[wasUnoffered]; + mutableMeta[wasOffered] = saved; + } + // The catalogue is back exactly as it was — a leaked mutation would make + // every later assertion in this process report on a palette nobody ships. + expect(offered[wasOffered]).toBe(saved); + expect(offered[wasUnoffered]).toBeUndefined(); + }); + + it('an exclusion with no renderer twin keeps the generic box', () => { + // Counter-probe. `element:text_input` is unoffered for its own reason and is + // in no alias group; a fix keyed on "is excluded" would hand it a friendly + // icon it has not earned. + for (const type of ['element:text_input', 'element:record_picker', 'element:form', 'ai:chat_window']) { + expect(PALETTE_EXCLUSIONS[type], `${type} is not an exclusion any more`).toBeTruthy(); + expect(resolveBlockDisplayMeta(type), `${type} borrowed display meta`).toBeUndefined(); + expect(resolveBlockTone(type), `${type} borrowed a tone`).toBe(BLOCK_CATEGORY_TONE.misc); + } + // And an outright unknown type, which is what the fallback exists for. + expect(resolveBlockDisplayMeta('zz:not-a-block')).toBeUndefined(); + expect(resolveBlockTone('zz:not-a-block')).toBe(BLOCK_CATEGORY_TONE.misc); + }); +}); + +describe('the palette offer list is untouched (#2943 ledger invariant)', () => { + it('no alias-group member became offerable', () => { + for (const { unadvertised } of groupSplits) { + for (const type of unadvertised) { + expect(type in (BLOCK_TYPE_META as Record)).toBe(false); + } + } + }); + + it('the drag-in list is exactly the palette catalogue, nothing more', () => { + // `TYPES_BY_CATEGORY` is what the Add-block picker iterates. Deriving the + // expectation from `BLOCK_TYPE_META` keeps this true across palette edits + // while still failing the moment a display-only type leaks into the picker. + const draggable = TYPES_BY_CATEGORY.flatMap((g) => g.types).sort(); + expect(draggable).toEqual(Object.keys(BLOCK_TYPE_META).sort()); + for (const excluded of Object.keys(PALETTE_EXCLUSIONS)) { + expect(draggable, `\`${excluded}\` became draggable`).not.toContain(excluded); + } + }); +}); + +describe('PageBlockCanvas — the chrome reaches the DOM (#5837)', () => { + const pair = groupSplits[0]; + const advertised = pair.advertised[0]; + const unadvertised = pair.unadvertised[0]; + + const draftWith = (types: string[]) => ({ + name: 'p', + type: 'record', + regions: [{ name: 'main', components: types.map((t) => ({ type: t })) }], + }); + + it('the unadvertised spelling gets the tone of its twin on the type badge', () => { + render(); + const badgeClass = (type: string) => screen.getByText(type).getAttribute('class') ?? ''; + + const twinClass = badgeClass(advertised); + expect(badgeClass(unadvertised)).toBe(twinClass); + // Positive half: the shared class really is the record tone, not two nodes + // that both fell through to grey. + expect(twinClass).toContain(BLOCK_CATEGORY_TONE[offered[advertised]!.category as 'record'].badge); + // Counter-probe on the same render: the un-twinned exclusion stays grey. + expect(badgeClass('element:text_input')).toContain(BLOCK_CATEGORY_TONE.misc.badge); + expect(badgeClass('element:text_input')).not.toBe(twinClass); + }); + + it('the unadvertised spelling gets the icon of its twin', () => { + // The canvas draws a node's ICON in its container branch (a leaf node shows + // a live preview instead), so the probe uses container-shaped nodes — an + // empty `properties.children` is all `childGroups` needs. + const container = (type: string) => ({ type, properties: { children: [] } }); + render( + , + ); + const iconClass = (type: string) => { + // A container row prints the type twice — once as the fallback label + // (`blockLabel` falls back to the raw type) and once in the badge — and + // both live inside the same row button, so either match finds the row. + const row = screen.getAllByText(type)[0].closest('button'); + return row?.querySelector('svg.lucide')?.getAttribute('class') ?? ''; + }; + expect(iconClass(advertised)).not.toBe(''); + expect(iconClass(unadvertised)).toBe(iconClass(advertised)); + expect(iconClass('element:text_input')).not.toBe(iconClass(advertised)); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/block-types.ts b/packages/app-shell/src/views/metadata-admin/previews/block-types.ts index 2f3ec65393..175385a2ed 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/block-types.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/block-types.ts @@ -182,6 +182,41 @@ export const PALETTE_EXCLUSIONS: Record = { 'record:chatter': 'compatibility alias — same renderer as the offered canonical `record:discussion`; still renders, just no longer advertised', }; +/** + * Block types that are ONE renderer under several spellings — the DISPLAY + * counterpart to {@link PALETTE_EXCLUSIONS}. + * + * {@link BLOCK_TYPE_META} answers "what may an author drag IN". The page canvas + * asks a different question — "what may an author already HAVE in this page" — + * and for most exclusions the two answers coincide: `ai:chat_window`, + * `element:form`, `element:record_picker` and `element:text_input` are not page + * blocks an author composes with, so a node bearing one of those names is + * something the canvas cannot draw meaningfully, and {@link UnknownBlockIcon} + * plus the neutral `misc` tone is the honest chrome for it. + * + * An alias pair is where the two questions diverge. `record:discussion` and + * `record:chatter` are registered against the SAME renderer function + * (`plugin-detail/src/index.tsx`), and the palette deliberately offers exactly + * one of them so there is one entry per renderer — which leaves the other + * spelling rendering perfectly while the canvas drew it as an unknown grey box. + * + * The key here is RENDERER IDENTITY, not "is excluded". A group may only list + * spellings that resolve, through `ComponentRegistry`, to the very same + * component; `canvas-display-meta.test.tsx` asserts exactly that against the + * real registry, so a group cannot claim a renderability it does not have. That + * is what keeps `element:text_input` — excluded for an unrelated reason, and + * with no twin — from borrowing an icon it has not earned. + * + * Groups are UNORDERED and orientation-agnostic on purpose. Which spelling the + * palette advertises is a maintainer decision that has already flipped once + * (objectui#5495 moved it from the alias to the canonical name), so the + * resolver borrows from whichever member is offered TODAY rather than + * hard-coding a direction; a future flip cannot re-open this gap. + */ +export const BLOCK_RENDERER_ALIAS_GROUPS: readonly (readonly string[])[] = [ + ['record:discussion', 'record:chatter'], +]; + export const CATEGORY_LABEL_EN: Record = { data: 'Data', layout: 'Layout', @@ -250,8 +285,41 @@ export const BLOCK_CATEGORY_TONE: Record = { }, }; +/** + * Icon + tone inputs for any block type the canvas can ENCOUNTER, as opposed to + * {@link BLOCK_TYPE_META}, which lists what the palette OFFERS. + * + * Deliberately narrower than {@link BlockTypeMeta}: no `label`. `blockLabel()` + * in `PageBlockCanvas` never consulted this catalogue — it falls back to the + * raw type string — and widening display resolution into labels would change + * author-visible naming, a separate decision from chrome. + */ +export interface BlockDisplayMeta { + Icon: LucideIcon; + category: BlockCategory; +} + +/** + * Resolve the canvas chrome for a block `type`: its palette entry when it has + * one, otherwise an alias sibling's ({@link BLOCK_RENDERER_ALIAS_GROUPS}). + * `undefined` for everything else, so the caller draws + * {@link UnknownBlockIcon}. Nothing here makes a type offerable — the palette + * is built from {@link BLOCK_TYPE_META} alone ({@link TYPES_BY_CATEGORY}). + */ +export function resolveBlockDisplayMeta(type: string): BlockDisplayMeta | undefined { + const direct = BLOCK_TYPE_META[type as BlockTypeId]; + if (direct) return { Icon: direct.Icon, category: direct.category }; + for (const group of BLOCK_RENDERER_ALIAS_GROUPS) { + if (!group.includes(type)) continue; + for (const sibling of group) { + const meta = BLOCK_TYPE_META[sibling as BlockTypeId]; + if (meta) return { Icon: meta.Icon, category: meta.category }; + } + } + return undefined; +} + /** Resolve a category tone for any block `type` string (handles unknowns). */ export function resolveBlockTone(type: string): BlockCategoryTone { - const meta = BLOCK_TYPE_META[type as BlockTypeId]; - return BLOCK_CATEGORY_TONE[meta?.category ?? 'misc']; + return BLOCK_CATEGORY_TONE[resolveBlockDisplayMeta(type)?.category ?? 'misc']; }