From 1eed51f80b9629abff382d645815b5c37563ca80 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 21:48:57 +0000 Subject: [PATCH 1/2] feat(app-shell)!: a record page shows a discussion panel only when it composes one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui#7298 half two, maintainer ruling 2026-09-12 (decision batch #120 item 5). `RecordDetailView` stops auto-appending a discussion panel below any record page whose tree places no `record:discussion` / `record:chatter` node, and the `(effectivePage as any).disableDiscussion` read goes with it — nothing reads an undeclared page key here any more. `PageSchema` is a `strictObject`, so that key was a hard parse error rather than a dropped one and the escape hatch the renderer's own comment documented could never be authored, while the panel it was meant to decline was hard-coded open for writes. `enable.feeds` stays the OBJECT's switch and now outranks the page: the composed tree is pruned by `stripDiscussionNodes` when the object opted out, so a declared (or synthesized) node no longer renders a panel over a feed the view deliberately never fetched. The prune is identity-preserving, so a feeds-on page is handed through by reference. `hasExplicitDiscussion` is replaced rather than left dead: its only question was whether to append, and the two walkers now share one container-key table so "is the panel there" and "remove the panel" cannot disagree. No transition window and no opt-in flag, per the ruling. Synthesized and slotted pages are unchanged — `buildDefaultPageSchema` composes the node itself. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa --- .../7298-discussion-explicit-composition.md | 59 ++++ content/docs/guide/slotted-pages.md | 47 +++ content/docs/plugins/plugin-detail.mdx | 12 +- .../__tests__/pageSchemaIntrospect.test.ts | 150 +++++--- .../src/utils/pageSchemaIntrospect.ts | 128 ++++++- ...iscussionExplicitComposition-7298.test.tsx | 325 ++++++++++++++++++ ...w.discussionFallbackPipeline-8983.test.tsx | 149 ++++---- .../RecordDetailView.feedLoading.test.tsx | 69 ++-- .../RecordDetailView.feedRecordScope.test.tsx | 43 ++- .../app-shell/src/views/RecordDetailView.tsx | 82 ++--- 10 files changed, 825 insertions(+), 239 deletions(-) create mode 100644 .changeset/7298-discussion-explicit-composition.md create mode 100644 packages/app-shell/src/views/RecordDetailView.discussionExplicitComposition-7298.test.tsx diff --git a/.changeset/7298-discussion-explicit-composition.md b/.changeset/7298-discussion-explicit-composition.md new file mode 100644 index 0000000000..54d67c0014 --- /dev/null +++ b/.changeset/7298-discussion-explicit-composition.md @@ -0,0 +1,59 @@ +--- +'@object-ui/app-shell': minor +--- + +A record page shows a discussion panel if and only if it composes one +(objectui#7298). + +**⚠️ Behaviour change on a shipped default — migration below.** `RecordDetailView` +used to append a discussion panel below any record page whose tree placed no +`record:discussion` / `record:chatter` node, and the only documented way to +decline it was `assignedPage.disableDiscussion = true`. That escape hatch could +never be written: `PageSchema` is a `strictObject`, so the key is a hard parse +error rather than a dropped one, and the renderer reached it through an +`as any`. Meanwhile the appended panel was hard-coded open for writes (comment +input, reactions, threading), so a deliberately read-only page — over a +`protection: { lock: 'full' }` platform object, where the object-side switch is +unreachable too — had no authorable way to say no. + +Maintainer ruling of 2026-09-12 (decision batch #120 item 5): *"a page is what +its author composes … nothing is appended by default and then removed by a +negative flag."* So the append is removed together with the unauthorable read — +nothing reads an undeclared page key any more — rather than the negative flag +being added to the protocol. + +**MIGRATION — pages that relied on the automatic panel add one +`record:discussion` node.** Put it where you want the conversation to sit: + +```ts +regions: [ + { + name: 'main', + components: [ + { type: 'page:header', properties: { title: '{name}' } }, + { type: 'record:details' }, + { type: 'record:discussion' }, + ], + }, +] +``` + +There is **no transition window and no opt-in flag** — this behaviour is gone in +this release. The node's own config is honoured as authored, so +`{ type: 'record:discussion', properties: { feed: { showCommentInput: false } } }` +is now the ordinary way to show a conversation without a composer. + +**Who is affected.** Only **authored full pages** that omit the node. Synthesized +default pages and slotted pages are unchanged — `buildDefaultPageSchema` +composes `record:discussion` itself, which is why the out-of-the-box record page +still has its panel. + +**Precedence, now enforced rather than documented.** `enable.feeds: false` on the +object stays the object's switch and outranks the page: an object with feeds off +shows no discussion panel, declared or not. Previously that gate sat on the +auto-append alone, so a declared (or synthesized) node rendered a panel on a +feeds-off object over a feed the view deliberately never fetched; the composed +tree is now pruned before it renders. Record pages for objects that never opted +out are untouched — the tree is handed through by reference. + +Docs: `content/docs/guide/slotted-pages.md`. diff --git a/content/docs/guide/slotted-pages.md b/content/docs/guide/slotted-pages.md index 5080326057..32c6fcc7df 100644 --- a/content/docs/guide/slotted-pages.md +++ b/content/docs/guide/slotted-pages.md @@ -42,6 +42,53 @@ Each slot accepts a single component schema or an array (arrays are flattened in place). Each slot is a **full replacement at the slot boundary** — there is no deep-merge or JSON-Patch in v1. +## The discussion panel is composed, never appended + +A record page shows a discussion panel **if and only if the page composes a +`record:discussion` node** — or its `record:chatter` alias, which is the same +renderer under a Salesforce-familiar name. There is no automatic panel, and +therefore no negative flag to switch one off. + +- **Synthesized** and **slotted** pages compose the node for you (it is the + `discussion` slot in the table above), so the out-of-the-box record page is + unchanged. +- **Full pages** (`kind: "full"`) author every region, so a full page places the + node itself — exactly like every other component it wants: + + +```ts +regions: [ + { + name: 'main', + components: [ + { type: 'page:header', properties: { title: '{name}' } }, + { type: 'record:details' }, + { type: 'record:discussion' }, // ← the panel is here because you put it here + ], + }, +] +``` + +The node's own config is honoured as authored, so the panel a page composes is +the panel it asked for: `{ type: 'record:discussion', properties: { feed: { +showCommentInput: false } } }` renders the conversation without a composer. + +**Precedence — the object outranks the page.** `enable.feeds: false` on the +object definition suppresses the discussion panel whether or not the page +composes the node; the view also skips the `sys_comment` read for such an +object, and the server rejects comment writes against it with +`403 FEEDS_DISABLED`. `enable.feeds` is opt-**out**: absent means on. + + + **Upgrading (objectui#7298).** A record page used to get a discussion panel + appended below its content whenever its tree placed no discussion node, and + the only way out was an `assignedPage.disableDiscussion` flag that `PageSchema` + — a `strictObject` — refuses, so no author could ever write it. Both are gone. + If one of your **authored full pages** relied on that automatic panel, add one + `record:discussion` node where you want it. Synthesized and slotted pages need + no change. + + ## Example: customize only the header ```ts diff --git a/content/docs/plugins/plugin-detail.mdx b/content/docs/plugins/plugin-detail.mdx index 94c4f32532..fd2073a909 100644 --- a/content/docs/plugins/plugin-detail.mdx +++ b/content/docs/plugins/plugin-detail.mdx @@ -334,10 +334,14 @@ decision as a defect is how a warning channel gets ignored: **Load more** rather than rendering whole. Two members of the shape are **not** read on this path and authoring them - does nothing here: `filterMode` and `enableMentions` (objectui#8968). And the - panel the host auto-appends when a page omits a discussion block does **not** - run this pipeline, so it still renders the feed unfiltered and whole - (objectui#8983). + does nothing here: `filterMode` and `enableMentions` (objectui#8968). + + There is no third path left to disagree with: the host used to auto-append a + panel below any page that omitted a discussion block, and objectui#7298 + retired that append — a record page shows a discussion panel if and only if it + composes one. Every panel a user sees is therefore an authored or synthesized + block running this pipeline. The object's `enable.feeds: false` still + outranks the page and suppresses the panel either way. ### RecordActivityTimeline Config diff --git a/packages/app-shell/src/utils/__tests__/pageSchemaIntrospect.test.ts b/packages/app-shell/src/utils/__tests__/pageSchemaIntrospect.test.ts index 8cc60c1ea8..79d20e58b9 100644 --- a/packages/app-shell/src/utils/__tests__/pageSchemaIntrospect.test.ts +++ b/packages/app-shell/src/utils/__tests__/pageSchemaIntrospect.test.ts @@ -1,49 +1,82 @@ import { describe, it, expect } from 'vitest'; -import { hasExplicitDiscussion, hasExplicitAttachments } from '../pageSchemaIntrospect'; +import { stripDiscussionNodes, hasExplicitAttachments } from '../pageSchemaIntrospect'; -describe('hasExplicitDiscussion', () => { - it('returns false for nullish and primitive inputs', () => { - expect(hasExplicitDiscussion(null)).toBe(false); - expect(hasExplicitDiscussion(undefined)).toBe(false); - expect(hasExplicitDiscussion('record:discussion')).toBe(false); - expect(hasExplicitDiscussion(42)).toBe(false); - }); +/** + * Every `type` string anywhere in a (non-cyclic) tree, in document order. + * Asserting on this rather than on a hand-written expected tree keeps the + * cases below readings about WHICH NODES SURVIVED — a strip that flattened or + * dropped a sibling would show up here, where a `not.toContain` on the + * serialized tree would not. + */ +function types(tree: unknown): string[] { + const out: string[] = []; + const walk = (node: any) => { + if (!node || typeof node !== 'object') return; + if (Array.isArray(node)) return node.forEach(walk); + if (typeof node.type === 'string') out.push(node.type); + for (const key of ['children', 'items', 'body', 'components', 'regions']) walk(node[key]); + if (node.properties) for (const key of ['children', 'items']) walk(node.properties[key]); + }; + walk(tree); + return out; +} - it('detects record:discussion at the root', () => { - expect(hasExplicitDiscussion({ type: 'record:discussion' })).toBe(true); +/** + * objectui#7298 — `enable.feeds: false` is the OBJECT's switch and it outranks + * whatever the page composed. `RecordDetailView` hands the composed tree + * through this before rendering it, so a declared (or synthesized) + * `record:discussion` never reaches a feeds-off object. + */ +describe('stripDiscussionNodes', () => { + it('returns nullish and primitive inputs unchanged', () => { + expect(stripDiscussionNodes(null)).toBeNull(); + expect(stripDiscussionNodes(undefined)).toBeUndefined(); + // A bare string is not a node, so it is not a discussion node either. + expect(stripDiscussionNodes('record:discussion')).toBe('record:discussion'); + expect(stripDiscussionNodes(42)).toBe(42); }); - it('detects the record:chatter alias', () => { - expect(hasExplicitDiscussion({ type: 'record:chatter' })).toBe(true); + it('returns null when the ROOT itself is the discussion node', () => { + // No container to remove it from — the total-function corner. + expect(stripDiscussionNodes({ type: 'record:discussion' })).toBeNull(); + expect(stripDiscussionNodes({ type: 'record:chatter' })).toBeNull(); }); - it('detects discussion nested inside children/items/body/components', () => { + it('removes discussion nested inside children/items/body/components', () => { const node = (key: string) => ({ type: 'foo', - [key]: [{ type: 'record:discussion' }], + [key]: [{ type: 'record:details' }, { type: 'record:discussion' }], }); - expect(hasExplicitDiscussion(node('children'))).toBe(true); - expect(hasExplicitDiscussion(node('items'))).toBe(true); - expect(hasExplicitDiscussion(node('body'))).toBe(true); - expect(hasExplicitDiscussion(node('components'))).toBe(true); + for (const key of ['children', 'items', 'body', 'components']) { + expect(types(stripDiscussionNodes(node(key)))).toEqual(['foo', 'record:details']); + } + }); + + it('removes the record:chatter alias too', () => { + const page = { type: 'foo', children: [{ type: 'record:chatter' }, { type: 'record:details' }] }; + expect(types(stripDiscussionNodes(page))).toEqual(['foo', 'record:details']); }); - it('detects discussion nested inside properties.children/items', () => { + it('removes discussion nested inside properties.children/items', () => { expect( - hasExplicitDiscussion({ - type: 'foo', - properties: { children: [{ type: 'record:discussion' }] }, - }), - ).toBe(true); + types( + stripDiscussionNodes({ + type: 'foo', + properties: { children: [{ type: 'record:discussion' }, { type: 'record:details' }] }, + }), + ), + ).toEqual(['foo', 'record:details']); expect( - hasExplicitDiscussion({ - type: 'foo', - properties: { items: [{ type: 'record:chatter' }] }, - }), - ).toBe(true); + types( + stripDiscussionNodes({ + type: 'foo', + properties: { items: [{ type: 'record:chatter' }] }, + }), + ), + ).toEqual(['foo']); }); - it('detects discussion nested inside regions[].components[] (synth + full pages)', () => { + it('removes discussion from regions[].components[] (synth + full pages)', () => { // Mirrors buildDefaultPageSchema output shape. const synthPage = { type: 'record', @@ -62,57 +95,80 @@ describe('hasExplicitDiscussion', () => { }, ], }; - expect(hasExplicitDiscussion(synthPage)).toBe(true); + const stripped = stripDiscussionNodes(synthPage) as any; + expect(types(stripped)).toEqual(['record', 'page:header', 'page:tabs', 'page:tab']); + // Everything else about the page survives, including the scalars that are + // not nodes at all. + expect(stripped.template).toBe('full-width'); + expect(stripped.regions[0].name).toBe('main'); + expect(stripped.regions[0].width).toBe('full'); }); - it('returns false when no discussion node exists anywhere in the tree', () => { + it('strips deep nesting (page:tabs > page:tab > record:discussion)', () => { const page = { - type: 'record', regions: [ { - name: 'main', components: [ - { type: 'page:header' }, { type: 'page:tabs', - items: [ - { type: 'page:tab', children: [{ type: 'record:details' }] }, - { type: 'page:tab', children: [{ type: 'record:history' }] }, - ], + items: [{ type: 'page:tab', children: [{ type: 'record:discussion' }] }], }, ], }, ], }; - expect(hasExplicitDiscussion(page)).toBe(false); + expect(types(stripDiscussionNodes(page))).toEqual(['page:tabs', 'page:tab']); }); - it('handles deep nesting (page:tabs > page:tab > record:discussion)', () => { + it('IDENTITY-PRESERVING — a tree with no discussion node comes back BY REFERENCE', () => { + // This is what keeps the feeds-ON path (every object that has not opted + // out) from handing a new page identity to the renderer on every render. const page = { + type: 'record', regions: [ { + name: 'main', components: [ + { type: 'page:header' }, { type: 'page:tabs', items: [ - { - type: 'page:tab', - children: [{ type: 'record:discussion' }], - }, + { type: 'page:tab', children: [{ type: 'record:details' }] }, + { type: 'page:tab', children: [{ type: 'record:history' }] }, ], }, ], }, ], }; - expect(hasExplicitDiscussion(page)).toBe(true); + expect(stripDiscussionNodes(page)).toBe(page); + }); + + it('copies only the path it had to change', () => { + const untouched = { type: 'page:header' }; + const page = { + type: 'record', + regions: [{ name: 'main', components: [untouched, { type: 'record:discussion' }] }], + }; + const stripped = stripDiscussionNodes(page) as any; + expect(stripped).not.toBe(page); + // The sibling that had nothing removed is the SAME object, not a clone. + expect(stripped.regions[0].components[0]).toBe(untouched); + }); + + it('prunes a SHARED subtree in every place it appears', () => { + // The walker memoizes by node, so a second occurrence must get the pruned + // result rather than the original it was handed on the way in. + const shared: any = { type: 'page:section', children: [{ type: 'record:discussion' }] }; + const page = { type: 'record', regions: [{ components: [shared] }, { components: [shared] }] }; + expect(types(stripDiscussionNodes(page))).toEqual(['record', 'page:section', 'page:section']); }); it('does not loop forever on cyclic schemas', () => { const a: any = { type: 'page:section' }; const b: any = { type: 'page:section', children: [a] }; a.children = [b]; - expect(hasExplicitDiscussion(a)).toBe(false); + expect(() => stripDiscussionNodes(a)).not.toThrow(); }); }); diff --git a/packages/app-shell/src/utils/pageSchemaIntrospect.ts b/packages/app-shell/src/utils/pageSchemaIntrospect.ts index d2f251f2b5..3242d872cc 100644 --- a/packages/app-shell/src/utils/pageSchemaIntrospect.ts +++ b/packages/app-shell/src/utils/pageSchemaIntrospect.ts @@ -3,11 +3,21 @@ * Copyright (c) 2024-present ObjectStack Inc. * * Helpers that introspect a page schema tree without needing the React - * runtime. Used by RecordDetailView to decide whether to auto-append - * a discussion / chatter slot at the bottom of the page. + * runtime. Used by RecordDetailView to decide which host-side fallback panels + * a page still needs, and to enforce the object-level `enable.feeds` switch + * over whatever the page composed. */ const DISCUSSION_TYPES = new Set(['record:discussion', 'record:chatter']); +/** + * Where a page tree nests component nodes. `regions` is what makes + * `buildDefaultPageSchema` output and full-Lightning authored pages visible to + * these walkers at all; without it the synthesized `record:discussion` is + * invisible and the host used to append a SECOND chatter panel on top of it. + */ +const CONTAINER_KEYS = ['children', 'items', 'body', 'components', 'regions'] as const; +/** The same nesting one level down, inside the spec `properties` envelope. */ +const NESTED_CONTAINER_KEYS = ['children', 'items'] as const; const ATTACHMENT_TYPES = new Set(['record:attachments']); const APPROVAL_TYPES = new Set(['record:approvals']); @@ -31,19 +41,12 @@ function hasNodeOfType(root: unknown, types: ReadonlySet): boolean { if (Array.isArray(node)) return node.some(walk); const t = node?.type; if (typeof t === 'string' && types.has(t)) return true; + // ⚠️ The SAME tables `stripDiscussionNodes` walks, deliberately shared: a + // container key that only one of the two knows about is a page shape where + // "is the panel there?" and "remove the panel" disagree. const candidates: any[] = [ - node.children, - node.items, - node.body, - node.components, - node.properties?.children, - node.properties?.items, - // Synth + full-Lightning pages nest components inside - // `regions[].components[]`. Without this branch the walker - // fails to see the `record:discussion` baked in by - // `buildDefaultPageSchema`, and the host appends a second - // chatter panel on top of it. - node.regions, + ...CONTAINER_KEYS.map((key) => (node as any)[key]), + ...NESTED_CONTAINER_KEYS.map((key) => node.properties?.[key]), ]; return candidates.some(walk); }; @@ -51,11 +54,100 @@ function hasNodeOfType(root: unknown, types: ReadonlySet): boolean { } /** - * True when the page schema already places a `record:discussion` / - * `record:chatter` node — the host must then skip its bottom auto-append. + * Return `root` with every `record:discussion` / `record:chatter` node removed + * from the containers this module walks. + * + * This is the object-level `enable.feeds: false` gate (#2707) applied to a + * COMPOSED page (objectui#7298). The maintainer ruling of 2026-09-12 makes the + * declared node the only way a discussion panel reaches a record page, and + * keeps `enable.feeds` as the object's switch *over* the page: an object with + * feeds off shows no panel, declared or not. `RecordDetailView` deliberately + * never fetches `sys_comment` for such an object, so a node left in the tree + * would render a composer and an empty feed the server would 403 anyway. + * + * ⚠️ Identity-preserving on purpose: a subtree with nothing to remove is + * returned by REFERENCE, so the feeds-ON path (every object that has not opted + * out) hands the very same object through and nothing downstream sees a new + * page identity each render. + * + * Shared subtrees are walked once and their result reused, so a node that + * appears twice is pruned in both places; a cycle is left as it stands rather + * than recursed into. + * + * Returns `null` when `root` ITSELF is a discussion node — there is no + * container to remove it from. Callers pass a page, so this is a total-function + * corner rather than a case that arises. */ -export function hasExplicitDiscussion(root: unknown): boolean { - return hasNodeOfType(root, DISCUSSION_TYPES); +export function stripDiscussionNodes(root: unknown): unknown { + const done = new WeakMap(); + const active = new WeakSet(); + + const isDiscussionNode = (node: any): boolean => + Boolean(node) && typeof node === 'object' && typeof node.type === 'string' + && DISCUSSION_TYPES.has(node.type); + + const walk = (node: any): any => { + if (!node || typeof node !== 'object') return node; + if (done.has(node)) return done.get(node); + // A cycle: return the node as it stands instead of recursing forever. + if (active.has(node)) return node; + active.add(node); + + let result: any = node; + + if (Array.isArray(node)) { + const next: any[] = []; + let changed = false; + for (const entry of node) { + if (isDiscussionNode(entry)) { + changed = true; + continue; + } + const walked = walk(entry); + if (walked !== entry) changed = true; + next.push(walked); + } + result = changed ? next : node; + } else { + const patch: Record = {}; + let changed = false; + for (const key of CONTAINER_KEYS) { + const value = (node as any)[key]; + if (value === undefined || value === null) continue; + const walked = walk(value); + if (walked !== value) { + patch[key] = walked; + changed = true; + } + } + const properties = (node as any).properties; + if (properties && typeof properties === 'object' && !Array.isArray(properties)) { + const propsPatch: Record = {}; + let propsChanged = false; + for (const key of NESTED_CONTAINER_KEYS) { + const value = properties[key]; + if (value === undefined || value === null) continue; + const walked = walk(value); + if (walked !== value) { + propsPatch[key] = walked; + propsChanged = true; + } + } + if (propsChanged) { + patch.properties = { ...properties, ...propsPatch }; + changed = true; + } + } + result = changed ? { ...node, ...patch } : node; + } + + active.delete(node); + done.set(node, result); + return result; + }; + + if (isDiscussionNode(root)) return null; + return walk(root); } /** diff --git a/packages/app-shell/src/views/RecordDetailView.discussionExplicitComposition-7298.test.tsx b/packages/app-shell/src/views/RecordDetailView.discussionExplicitComposition-7298.test.tsx new file mode 100644 index 0000000000..36bd4e064d --- /dev/null +++ b/packages/app-shell/src/views/RecordDetailView.discussionExplicitComposition-7298.test.tsx @@ -0,0 +1,325 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * ══════════════════════════════════════════════════════════════════════════ + * A record page shows a discussion panel IF AND ONLY IF it composes one + * (objectui#7298 half two, maintainer ruling 2026-09-12, decision batch #120 + * item 5) + * ══════════════════════════════════════════════════════════════════════════ + * + * `RecordDetailView` used to append a discussion panel to any authored record + * page that omitted `record:discussion` / `record:chatter`, and the only way to + * decline it was `assignedPage.disableDiscussion = true` — a key `PageSchema` + * is a `strictObject` about, so authoring it is a hard parse error. The panel + * the host appended was hard-coded open for writes (comment input, reactions, + * threading), which is why a deliberately read-only page over a + * `protection: { lock: 'full' }` platform object could not decline it at all. + * + * The ruling removes the append rather than declaring the negative flag: *"a + * page is what its author composes … nothing is appended by default and then + * removed by a negative flag."* So: + * + * DECLARED the page places `record:discussion` (or `record:chatter`, the + * same renderer under a Salesforce-familiar alias) → the panel + * renders, on the config the author wrote. + * NOT DECLARED nothing renders. No panel, no composer, no host fallback. + * FEEDS OFF `enable.feeds: false` is still the OBJECT's switch and it wins + * over the page: declared or not, no panel. + * + * ## What keeps the negative pin from passing trivially + * + * "No panel rendered" is satisfied by any harness in which the panel could + * never have rendered — a broken feed read, a page that never resolved, a + * component that throws. Every negative case below is therefore paired with a + * LIT CONTROL that differs from it in exactly one way (the page declares the + * node) and asserts the panel IS there, on the same fixture, in the same file: + * + * `no panel without the node` ⟷ `the declared node renders the panel` + * `feeds off suppresses it` ⟷ `feeds on renders the empty panel` + * + * ⚠️ The feeds-off pair cannot be read on feed ROWS: `enable.feeds: false` also + * skips the `sys_comment` fetch, so both sides would have zero rows for two + * different reasons. It is read on the panel's own chrome — the "Discussion" + * heading, its empty state and the composer — which is present whenever the + * panel mounts, empty feed or not. + * + * ## Resolution + * + * Nothing here resolves through any `dist/`: `./RecordDetailView` is this + * package's own source and `@object-ui/plugin-detail` is mapped to its `src` by + * the root `vitest.config.mts` alias table. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import { MetadataCtx } from '@object-ui/react'; + +vi.mock('@object-ui/auth', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ user: { id: 'u1', name: 'Ada', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => vi.fn(), +})); + +vi.mock('@object-ui/collaboration', async (importOriginal) => ({ + ...(await importOriginal>()), + useRecordPresence: () => [], + PresenceAvatars: () => null, +})); + +vi.mock('sonner', () => ({ + toast: Object.assign(vi.fn(), { + success: vi.fn(), + error: vi.fn(), + info: vi.fn(), + warning: vi.fn(), + loading: vi.fn(), + dismiss: vi.fn(), + }), +})); + +// Orthogonal chrome — same posture as the sibling feed tests in this folder, so +// the only asynchrony in this file is the feed read. +vi.mock('./ActionConfirmDialog', () => ({ ActionConfirmDialog: () => null })); +vi.mock('./ActionParamDialog', () => ({ ActionParamDialog: () => null })); +vi.mock('./ActionResultDialog', () => ({ ActionResultDialog: () => null })); +vi.mock('./FlowRunner', () => ({ FlowRunner: () => null })); +vi.mock('./MetadataInspector', () => ({ + MetadataPanel: () => null, + useMetadataInspector: () => ({ showDebug: false, toggle: () => {} }), +})); + +import { RecordDetailView } from './RecordDetailView'; + +const OBJECT_NAME = 'crm_account'; +const RECORD_ID = 'rec-alpha'; +const COMMENT_MARKER = 'A comment on the record'; + +/** The panel's own chrome — present whenever it mounts, empty feed or not. */ +const PANEL_HEADING = 'Discussion'; +const PANEL_EMPTY = 'No comments yet'; +/** `CommentInput`'s textarea — the write surface the ruling is about. Read by + * PLACEHOLDER, not by the submit button: that button mounts only once the box + * has text, so counting it reads zero on a panel that is fully open for + * writes. */ +const COMPOSER = /Leave a comment/; + +const baseObject = { + name: OBJECT_NAME, + label: 'Account', + managedBy: 'platform', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + }, +}; + +/** `enable.feeds` is opt-OUT: absent = on, only an explicit `false` disables. */ +const OBJECTS_FEEDS_ON = [baseObject]; +const OBJECTS_FEEDS_OFF = [{ ...baseObject, enable: { feeds: false } }]; + +function authoredPage(components: any[]) { + return { + name: 'account_record_page', + type: 'record', + pageType: 'record', + object: OBJECT_NAME, + regions: [{ name: 'main', components }], + }; +} + +const HEADER = { type: 'page:header', title: 'Account' }; + +/** An authored page that composes NO discussion node — the subject. */ +const PAGE_WITHOUT_DISCUSSION = authoredPage([HEADER]); +/** The lit control: the same page, plus the one node the ruling requires. */ +const PAGE_WITH_DISCUSSION = authoredPage([HEADER, { type: 'record:discussion' }]); +/** The Salesforce-familiar alias — one renderer under two registered names. */ +const PAGE_WITH_CHATTER = authoredPage([HEADER, { type: 'record:chatter' }]); +/** Author-written config on the declared node must be honoured as authored. */ +const PAGE_WITH_READONLY_DISCUSSION = authoredPage([ + HEADER, + { type: 'record:discussion', properties: { feed: { showCommentInput: false } } }, +]); + +function makeDataSource(comments: Array>) { + const find = vi.fn((objectName: string) => { + if (objectName === 'sys_comment') return Promise.resolve({ data: comments }); + return Promise.resolve({ data: [] }); + }); + return { + find, + findOne: vi.fn(async () => ({ id: RECORD_ID, name: 'Alpha' })), + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + } as any; +} + +const ONE_COMMENT = [ + { + id: 'c-1', + thread_id: `${OBJECT_NAME}:${RECORD_ID}`, + author_name: 'Ada', + body: COMMENT_MARKER, + created_at: '2026-01-01T00:00:00.000Z', + }, +]; + +function makeMetadata(objects: any[], pages: any[]) { + return { + objects, + pages, + loading: false, + error: null, + refresh: async () => {}, + invalidate: () => {}, + ensureType: async () => pages, + getItem: async () => null, + getItemsByType: (type: string) => (type === 'page' ? pages : []), + } as any; +} + +function renderDetail(opts: { objects: any[]; pages: any[]; comments?: Array> }) { + return render( + + + {}} + objectNameOverride={OBJECT_NAME} + recordIdOverride={RECORD_ID} + embedded + /> + + , + ); +} + +const count = (text: string) => screen.queryAllByText(text).length; +const composerCount = () => screen.queryAllByPlaceholderText(COMPOSER).length; + +/** Wait until the feed read has settled, so an absent panel is an answer. */ +async function settled() { + await waitFor(() => expect(screen.queryByTestId('activity-loading')).toBeNull()); + // The record itself resolves through `findOne`; the header proves the page + // rendered at all, which is what makes an absent panel a reading about the + // panel rather than about a page that never mounted. + await waitFor(() => expect(count('Account')).toBeGreaterThan(0)); +} + +beforeEach(() => { + cleanup(); + // Unrelated chrome on this view reaches for the platform API; in jsdom that + // is a real socket. Answer it locally so the feed read is the only asynchrony. + vi.stubGlobal( + 'fetch', + vi.fn(async () => + new Response(JSON.stringify({ data: [] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ), + ); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +describe('objectui#7298 — the discussion panel is composed, never appended', () => { + it('an authored page that does NOT declare the node renders NO panel', async () => { + renderDetail({ objects: OBJECTS_FEEDS_ON, pages: [PAGE_WITHOUT_DISCUSSION], comments: ONE_COMMENT }); + await settled(); + + expect(count(PANEL_HEADING)).toBe(0); + expect(count(PANEL_EMPTY)).toBe(0); + expect(count(COMMENT_MARKER)).toBe(0); + // The write surface the ruling is about: no composer reaches a page that + // did not ask for one. + expect(composerCount()).toBe(0); + }); + + it('LIT CONTROL — the same page WITH `record:discussion` renders the panel', async () => { + renderDetail({ objects: OBJECTS_FEEDS_ON, pages: [PAGE_WITH_DISCUSSION], comments: ONE_COMMENT }); + await settled(); + + await waitFor(() => expect(count(COMMENT_MARKER)).toBeGreaterThan(0)); + expect(count(PANEL_HEADING)).toBeGreaterThan(0); + expect(composerCount()).toBeGreaterThan(0); + }); + + it('the `record:chatter` alias composes the same panel', async () => { + renderDetail({ objects: OBJECTS_FEEDS_ON, pages: [PAGE_WITH_CHATTER], comments: ONE_COMMENT }); + await settled(); + + await waitFor(() => expect(count(COMMENT_MARKER)).toBeGreaterThan(0)); + expect(count(PANEL_HEADING)).toBeGreaterThan(0); + }); + + it("the declared node's own `feed` config is honoured as authored", async () => { + // The backwards workaround the card measured — "declare the component that + // opens three write surfaces, in order to close them" — is now just + // authoring: the node is the only way the panel arrives, and its config + // decides what it offers. + renderDetail({ + objects: OBJECTS_FEEDS_ON, + pages: [PAGE_WITH_READONLY_DISCUSSION], + comments: ONE_COMMENT, + }); + await settled(); + + await waitFor(() => expect(count(COMMENT_MARKER)).toBeGreaterThan(0)); + expect(count(PANEL_HEADING)).toBeGreaterThan(0); + expect(composerCount()).toBe(0); + }); + + it('the SYNTHESIZED default page still shows the panel — it declares the node', async () => { + // No authored page ⇒ `buildDefaultPageSchema`, which composes + // `record:discussion` itself. The ruling removes the host's append, not the + // default page's panel, so the out-of-the-box record page is unchanged. + renderDetail({ objects: OBJECTS_FEEDS_ON, pages: [], comments: ONE_COMMENT }); + await settled(); + + await waitFor(() => expect(count(COMMENT_MARKER)).toBeGreaterThan(0)); + expect(count(PANEL_HEADING)).toBeGreaterThan(0); + }); +}); + +describe('objectui#7298 — `enable.feeds` stays the OBJECT switch and wins over the page', () => { + it('LIT CONTROL — feeds ON, declared node, empty feed: the panel chrome is there', async () => { + // Read on chrome, not rows: the feeds-off case has zero rows for a second + // reason (the `sys_comment` fetch is skipped), so rows cannot discriminate. + renderDetail({ objects: OBJECTS_FEEDS_ON, pages: [PAGE_WITH_DISCUSSION], comments: [] }); + await settled(); + + await waitFor(() => expect(count(PANEL_HEADING)).toBeGreaterThan(0)); + expect(count(PANEL_EMPTY)).toBeGreaterThan(0); + expect(composerCount()).toBeGreaterThan(0); + }); + + it('feeds OFF suppresses the panel even when the page DECLARES it', async () => { + renderDetail({ objects: OBJECTS_FEEDS_OFF, pages: [PAGE_WITH_DISCUSSION], comments: ONE_COMMENT }); + await settled(); + + expect(count(PANEL_HEADING)).toBe(0); + expect(count(PANEL_EMPTY)).toBe(0); + expect(composerCount()).toBe(0); + }); + + it('feeds OFF suppresses the panel on the synthesized default page too', async () => { + renderDetail({ objects: OBJECTS_FEEDS_OFF, pages: [], comments: ONE_COMMENT }); + await settled(); + + expect(count(PANEL_HEADING)).toBe(0); + expect(count(PANEL_EMPTY)).toBe(0); + expect(composerCount()).toBe(0); + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.discussionFallbackPipeline-8983.test.tsx b/packages/app-shell/src/views/RecordDetailView.discussionFallbackPipeline-8983.test.tsx index e9aca38950..44035c7a19 100644 --- a/packages/app-shell/src/views/RecordDetailView.discussionFallbackPipeline-8983.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.discussionFallbackPipeline-8983.test.tsx @@ -6,34 +6,38 @@ * LICENSE file in the root directory of this source tree. * * ══════════════════════════════════════════════════════════════════════════ - * The TWO chatter surfaces on a record page render the SAME feed - * (objectui#8983) + * BOTH composition routes to the chatter panel render the SAME feed + * (objectui#8983, re-aimed by objectui#7298) * ══════════════════════════════════════════════════════════════════════════ * - * A record page can show its discussion through either of two surfaces, and - * which one a user gets is decided by the page schema, not by them: + * A record page shows its discussion because the page composes a + * `record:discussion` / `record:chatter` node, and there are two ways that node + * gets into the tree: * - * AUTHORED / SYNTHESIZED BLOCK a page that places `record:discussion` or - * `record:chatter` — which every synthesized - * default page does (`buildDefaultPageSchema` - * appends `record:discussion` unless - * `hideDiscussion`) — renders through - * `RecordChatterRenderer`. - * HOST FALLBACK an AUTHORED page that omits the block gets - * the host's bottom auto-append, gated by - * `showAutoDiscussion` in `RecordDetailView`. + * SYNTHESIZED BLOCK no authored record page ⇒ `buildDefaultPageSchema` + * composes `record:discussion` itself. + * AUTHORED BLOCK an authored page places the node in its own region, + * beside whatever else it composes. * - * objectui#8934 taught the renderer to run `applyFeedConfig`, so the protocol's - * `feed` members and its DEFAULTS became live on the first surface. The - * fallback kept handing its rows to `RecordChatterPanel` raw. For one round the - * two therefore disagreed on the same record and the same rows — a completed - * activity showed on one and not the other, and a feed past twenty rows - * rendered whole on one and paged on the other. + * Both resolve through `ComponentRegistry` to ONE `RecordChatterRenderer`, so + * both must render one feed. objectui#8934 taught that renderer to run + * `applyFeedConfig`, making the protocol's `feed` members and its DEFAULTS live + * — and this file is what turns red if a future edit gives either route its own + * items/config path instead. * - * objectui#8983 closed that by BINDING, not by copying: the fallback now mounts - * `RecordChatterRenderer` — the same component `ComponentRegistry` resolves - * both block names to — with no schema, so it runs the one pipeline instead of - * a second implementation of it kept in agreement by hand. + * ⚠️ WHAT CHANGED UNDER THIS FILE. The second surface used to be the HOST + * FALLBACK: an authored page that omitted the block got a panel auto-appended + * by `RecordDetailView`, and for one round that fallback handed its rows to + * `RecordChatterPanel` raw while the block ran the pipeline — a completed + * activity showed on one and not the other, and a feed past twenty rows + * rendered whole on one and paged on the other. objectui#8983 closed that by + * BINDING rather than copying (the fallback started mounting the renderer with + * no schema). objectui#7298 then retired the fallback outright — the maintainer + * ruling of 2026-09-12: a page shows a discussion panel if and only if it + * composes one. So the divergence this file was written about can no longer + * arise from THAT pair; the pair it guards now is the two ways the node is + * composed, and the pre-fix control below still measures what an UNBOUND + * surface would have rendered. * * ## What makes the equality below a reading rather than a tautology * @@ -42,12 +46,13 @@ * objectui#8934 too, when neither surface ran the pipeline. Three legs answer * that, and all three have to hold together: * - * PRE-FIX CONTROL `PreFixFallbackShape` below is the fallback's JSX as it - * was written before this card — the same panel, the same - * hard-coded three affordances, raw `items`. It is rendered - * on the SAME feed and asserted to produce a DIFFERENT row - * set. So the fixture provably discriminates: an unbound - * fallback fails the equality. + * PRE-FIX CONTROL `PreFixFallbackShape` below is the retired host + * fallback's JSX as it was written before objectui#8983 — + * the same panel, the same hard-coded three affordances, + * raw `items`. It is rendered on the SAME feed and asserted + * to produce a DIFFERENT row set. So the fixture provably + * discriminates: a surface that stops running the pipeline + * fails the equality. * AGREEMENT FIXTURE the shared row set is asserted by VALUE, and the value is * the pipeline's answer (completed row hidden, a 20-row * page window) rather than the raw feed. Both surfaces @@ -131,16 +136,22 @@ const OBJECTS = [ ]; /** - * An AUTHORED record page that places no discussion slot — the one thing that - * turns the host fallback on. A synthesized page always bakes in - * `record:discussion`, so without this the fallback branch is unreachable. + * An AUTHORED record page that composes the discussion node itself — the second + * of the two routes. Since objectui#7298 an authored page that OMITS the node + * renders no panel at all, which is pinned in + * `RecordDetailView.discussionExplicitComposition-7298`. */ -const AUTHORED_PAGE_WITHOUT_DISCUSSION = { +const AUTHORED_PAGE_WITH_DISCUSSION = { name: 'account_record_page', type: 'record', pageType: 'record', object: OBJECT_NAME, - regions: [{ name: 'main', components: [{ type: 'page:header', title: 'Account' }] }], + regions: [ + { + name: 'main', + components: [{ type: 'page:header', title: 'Account' }, { type: 'record:discussion' }], + }, + ], }; /** Zero-padded so no marker is a substring of another (`Comment 1` vs `10`). */ @@ -248,15 +259,15 @@ function renderDetail(fixture: Fixture, pages: any[]) { } /** - * The host fallback's JSX EXACTLY as it was written before objectui#8983 — - * `RecordChatterPanel` mounted directly, the three affordances hard-coded, raw - * `items`, no `hasMore` / `onLoadMore`. + * The retired host fallback's JSX EXACTLY as it was written before + * objectui#8983 — `RecordChatterPanel` mounted directly, the three affordances + * hard-coded, raw `items`, no `hasMore` / `onLoadMore`. * - * ⛔ Not production code and not a second implementation: it is the control - * that makes the equality assertions readings. Every case that asserts the two - * surfaces agree also asserts that THIS shape does not, on the same feed — so a - * fallback that stopped running the pipeline could not pass by rendering - * whatever the other surface happens to render. + * ⛔ Not production code, and since objectui#7298 not any code: it is the + * control that makes the equality assertions readings. Every case that asserts + * the two routes agree also asserts that THIS shape does not, on the same feed + * — so a route that stopped running the pipeline could not pass by rendering + * whatever the other one happens to render. */ const PreFixFallbackShape: React.FC<{ items: FeedItem[] }> = ({ items }) => (
@@ -299,9 +310,9 @@ async function readSurface(fixture: Fixture, pages: any[]) { return { rows: renderedRows(fixture), loadMore: loadMoreCount() }; } -const viaHostFallback = (fixture: Fixture) => readSurface(fixture, [AUTHORED_PAGE_WITHOUT_DISCUSSION]); -/** No authored page ⇒ the synthesized default, which bakes in the block. */ -const viaAuthoredBlock = (fixture: Fixture) => readSurface(fixture, []); +const viaAuthoredBlock = (fixture: Fixture) => readSurface(fixture, [AUTHORED_PAGE_WITH_DISCUSSION]); +/** No authored page ⇒ the synthesized default, which composes the block. */ +const viaSynthesizedBlock = (fixture: Fixture) => readSurface(fixture, []); function readPreFixShape(fixture: Fixture) { render(); @@ -329,26 +340,26 @@ afterEach(() => { vi.clearAllMocks(); }); -describe('both chatter surfaces render the same feed (objectui#8983)', () => { +describe('both composition routes render the same feed (objectui#8983)', () => { it('a COMPLETED activity is hidden on both — and the pre-fix fallback shape shows it', async () => { // The card's first probe row. `showCompleted` defaults false in the spec, - // so the pipeline drops the `task`; an unbound fallback renders it. + // so the pipeline drops the `task`; an unbound surface renders it. const fixture: Fixture = { comments: 3, withCompletedActivity: true }; const expected = ['Comment 01', 'Comment 02', 'Comment 03']; - const fallback = await viaHostFallback(fixture); - expect(fallback.rows).toEqual(expected); - - cleanup(); const authored = await viaAuthoredBlock(fixture); expect(authored.rows).toEqual(expected); - expect(fallback.rows).toEqual(authored.rows); - // PRE-FIX CONTROL — the same feed, the shape this branch used to have. + cleanup(); + const synthesized = await viaSynthesizedBlock(fixture); + expect(synthesized.rows).toEqual(expected); + expect(authored.rows).toEqual(synthesized.rows); + + // PRE-FIX CONTROL — the same feed, the shape the retired fallback had. cleanup(); const preFix = readPreFixShape(fixture); expect(preFix.rows).toEqual([...expected, TASK_MARKER]); - expect(preFix.rows).not.toEqual(fallback.rows); + expect(preFix.rows).not.toEqual(authored.rows); }); it('a feed past the page window pages on both — and the pre-fix fallback shape renders it whole', async () => { @@ -358,22 +369,22 @@ describe('both chatter surfaces render the same feed (objectui#8983)', () => { const fixture: Fixture = { comments: 25, withCompletedActivity: false }; const expected = Array.from({ length: 20 }, (_, i) => commentMarker(i + 6)); - const fallback = await viaHostFallback(fixture); - expect(fallback.rows).toEqual(expected); - expect(fallback.loadMore).toBe(1); - - cleanup(); const authored = await viaAuthoredBlock(fixture); expect(authored.rows).toEqual(expected); expect(authored.loadMore).toBe(1); - expect(fallback.rows).toEqual(authored.rows); + + cleanup(); + const synthesized = await viaSynthesizedBlock(fixture); + expect(synthesized.rows).toEqual(expected); + expect(synthesized.loadMore).toBe(1); + expect(authored.rows).toEqual(synthesized.rows); // PRE-FIX CONTROL — no pipeline, so no window and no paging affordance. cleanup(); const preFix = readPreFixShape(fixture); expect(preFix.rows).toHaveLength(25); expect(preFix.loadMore).toBe(0); - expect(preFix.rows).not.toEqual(fallback.rows); + expect(preFix.rows).not.toEqual(authored.rows); }); it('NEUTRAL FIXTURE — on a feed with nothing to filter, all three shapes agree', async () => { @@ -384,13 +395,13 @@ describe('both chatter surfaces render the same feed (objectui#8983)', () => { const fixture: Fixture = { comments: 3, withCompletedActivity: false }; const expected = ['Comment 01', 'Comment 02', 'Comment 03']; - const fallback = await viaHostFallback(fixture); - expect(fallback.rows).toEqual(expected); - expect(fallback.loadMore).toBe(0); - - cleanup(); const authored = await viaAuthoredBlock(fixture); expect(authored.rows).toEqual(expected); + expect(authored.loadMore).toBe(0); + + cleanup(); + const synthesized = await viaSynthesizedBlock(fixture); + expect(synthesized.rows).toEqual(expected); cleanup(); const preFix = readPreFixShape(fixture); @@ -399,14 +410,14 @@ describe('both chatter surfaces render the same feed (objectui#8983)', () => { }); }); -describe('the fallback got the paging PAIR, not just the affordance (objectui#8983)', () => { - it('"Load more" on the host fallback grows the window by one page', async () => { +describe('the authored block got the paging PAIR, not just the affordance (objectui#8983)', () => { + it('"Load more" on an authored `record:discussion` grows the window by one page', async () => { // `hasMore` without `onLoadMore` renders a button that does nothing — which // satisfies every assertion that only counts buttons. Clicking it is what // separates "the page window is wired" from "a dead affordance rendered". const fixture: Fixture = { comments: 25, withCompletedActivity: false }; - renderDetail(fixture, [AUTHORED_PAGE_WITHOUT_DISCUSSION]); + renderDetail(fixture, [AUTHORED_PAGE_WITH_DISCUSSION]); await settled(); await waitFor(() => expect(renderedRows(fixture).length).toBe(20)); diff --git a/packages/app-shell/src/views/RecordDetailView.feedLoading.test.tsx b/packages/app-shell/src/views/RecordDetailView.feedLoading.test.tsx index 55a0c9be31..13e9a39799 100644 --- a/packages/app-shell/src/views/RecordDetailView.feedLoading.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.feedLoading.test.tsx @@ -8,8 +8,8 @@ * the discussion feed, so it is the only place that can tell the panel * "still fetching". Until #3209 it never did: `feedItems` started `[]` and * both fetches filled it asynchronously with no loading state anywhere, so - * `DiscussionContextProvider` and the auto-appended `RecordChatterPanel` - * both went out without `loading`. The user-visible result was the panel + * `DiscussionContextProvider` and the panel it feeds both went out without + * `loading`. The user-visible result was the panel * asserting "No comments yet" — a factual claim about the record — for the * whole first leg of every record page, then contradicting itself when the * rows arrived. @@ -19,14 +19,19 @@ * are on the rendered outcome — the loading row vs. the empty copy — never * on "a prop was passed". * - * TWO delivery paths reach a user, and this file exercises both, because the - * fix wires them separately and either one alone still ships the bug: - * • the SYNTHESIZED page (no authored record page) bakes in a - * `record:discussion` node, so the panel arrives through + * TWO composition routes reach a user, and this file exercises both: + * • the SYNTHESIZED page (no authored record page) composes a + * `record:discussion` node itself, so the panel arrives through * `DiscussionContextProvider` → `record:chatter` renderer; - * • an AUTHORED page that omits the discussion slot gets the host's - * bottom auto-append, a `RecordChatterPanel` mounted directly with no - * context hop in between. + * • an AUTHORED page that composes the node explicitly — the same renderer, + * reached through the author's own tree and its `page:header` sibling. + * + * ⚠️ This file used to drive the second route through the host's bottom + * AUTO-APPEND, on an authored page that omitted the node. objectui#7298 (the + * maintainer ruling of 2026-09-12) removed that append: a record page shows a + * discussion panel if and only if it composes one, so an authored page without + * the node now has no panel and no loading signal to pin. The route that + * survives is the authored NODE, which is what the fixture below declares. * * The three cases that matter, and why: * 1. during the fetch → loading row, no empty copy; @@ -121,17 +126,21 @@ function makeDataSource(feed: FeedResponses) { } /** - * An AUTHORED record page that places no discussion slot — this is what - * turns on the host's bottom auto-append (`showAutoDiscussion`). The - * synthesized default page always bakes in `record:discussion`, so without - * an authored page the auto-append branch is unreachable. + * An AUTHORED record page that composes the discussion node explicitly — the + * second of the two routes, and since objectui#7298 the only way a panel + * reaches an authored page at all. */ -const AUTHORED_PAGE_WITHOUT_DISCUSSION = { +const AUTHORED_PAGE_WITH_DISCUSSION = { name: 'account_record_page', type: 'record', pageType: 'record', object: OBJECT_NAME, - regions: [{ name: 'main', components: [{ type: 'page:header', title: 'Account' }] }], + regions: [ + { + name: 'main', + components: [{ type: 'page:header', title: 'Account' }, { type: 'record:discussion' }], + }, + ], }; function makeMetadata(pages: any[]) { @@ -189,11 +198,11 @@ afterEach(() => { describe('RecordDetailView — the feed fetch produces a loading signal (#3209)', () => { it('renders the discussion through the context path when the page is synthesized', async () => { - // Pins WHICH path the cases below exercise: the synthesized page bakes - // in `record:discussion`, so the host suppresses its auto-append and the - // panel arrives via DiscussionContext. If this ever flips, the auto-append - // case (further down) would be the one under test twice and the context - // hop would go uncovered without any test turning red. + // Pins WHICH route the cases below exercise: with no authored page the + // synthesizer composes `record:discussion` itself, so the panel arrives + // through DiscussionContext. If this ever flips, the AUTHORED-node case + // (further down) would be the one under test twice and the synthesized + // route would go uncovered without any test turning red. const dataSource = makeDataSource({ sys_comment: pending, sys_activity: pending }); renderDetail(dataSource); @@ -327,16 +336,16 @@ describe('RecordDetailView — the feed fetch produces a loading signal (#3209)' }); }); -describe('RecordDetailView — the auto-appended panel is on the same chain (#3209)', () => { - // An authored page that omits the discussion slot gets the host's - // bottom-of-page `RecordChatterPanel`, mounted DIRECTLY — no - // DiscussionContext hop, therefore its own `loading` wire. Missing it - // would leave every authored record page still showing "No comments yet" - // mid-fetch, with the context path green. - it('shows the loading row on an authored page whose discussion slot is auto-appended', async () => { +describe('RecordDetailView — an AUTHORED discussion node is on the same chain (#3209)', () => { + // The author's own `record:discussion` reaches the same renderer through the + // author's tree rather than the synthesizer's. Both routes read `loading` off + // the one `DiscussionContextProvider`, and this half is what would leave an + // authored record page showing "No comments yet" mid-fetch while the + // synthesized path stayed green. + it('shows the loading row on an authored page that composes the discussion node', async () => { const dataSource = makeDataSource({ sys_comment: pending, sys_activity: pending }); - renderDetail(dataSource, [AUTHORED_PAGE_WITHOUT_DISCUSSION]); + renderDetail(dataSource, [AUTHORED_PAGE_WITH_DISCUSSION]); expect(await screen.findByTestId('activity-loading')).toBeTruthy(); expect(screen.queryByText(EMPTY_COMMENTS)).toBeNull(); @@ -348,7 +357,7 @@ describe('RecordDetailView — the auto-appended panel is on the same chain (#32 sys_activity: () => Promise.resolve({ data: [] }), }); - renderDetail(dataSource, [AUTHORED_PAGE_WITHOUT_DISCUSSION]); + renderDetail(dataSource, [AUTHORED_PAGE_WITH_DISCUSSION]); expect(await screen.findByText(EMPTY_COMMENTS)).toBeTruthy(); expect(screen.queryByTestId('activity-loading')).toBeNull(); @@ -360,7 +369,7 @@ describe('RecordDetailView — the auto-appended panel is on the same chain (#32 sys_activity: () => Promise.reject(new Error('404 sys_activity')), }); - renderDetail(dataSource, [AUTHORED_PAGE_WITHOUT_DISCUSSION]); + renderDetail(dataSource, [AUTHORED_PAGE_WITH_DISCUSSION]); expect(await screen.findByText(EMPTY_COMMENTS)).toBeTruthy(); expect(screen.queryByTestId('activity-loading')).toBeNull(); diff --git a/packages/app-shell/src/views/RecordDetailView.feedRecordScope.test.tsx b/packages/app-shell/src/views/RecordDetailView.feedRecordScope.test.tsx index f2f36f0217..1d371dc313 100644 --- a/packages/app-shell/src/views/RecordDetailView.feedRecordScope.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.feedRecordScope.test.tsx @@ -168,17 +168,28 @@ function makeDataSource(seed: Seed = {}) { } /** - * An AUTHORED record page with no discussion slot — this is what turns on the - * host's bottom auto-append, the `RecordChatterPanel` that carries the comment - * composer (`showCommentInput: true`). The optimistic round-trip test needs a - * real composer to post through, rather than reaching for the callback prop. + * An AUTHORED record page that composes the discussion node — the panel that + * carries the comment composer (`showCommentInput` defaults true on the + * renderer's own config). The optimistic round-trip test needs a real composer + * to post through, rather than reaching for the callback prop. + * + * ⚠️ This used to be a page with NO discussion slot, driving the host's bottom + * auto-append. objectui#7298 removed that append — a record page shows a panel + * if and only if it composes one — so the composer now has to be authored, and + * what this file pins (which record an optimistic comment belongs to) is + * unchanged by where the panel came from. */ -const AUTHORED_PAGE_WITHOUT_DISCUSSION = { +const AUTHORED_PAGE_WITH_DISCUSSION = { name: 'account_record_page', type: 'record', pageType: 'record', object: OBJECT_NAME, - regions: [{ name: 'main', components: [{ type: 'page:header', title: 'Account' }] }], + regions: [ + { + name: 'main', + components: [{ type: 'page:header', title: 'Account' }, { type: 'record:discussion' }], + }, + ], }; function makeMetadata(pages: any[]) { @@ -359,7 +370,7 @@ describe('RecordDetailView — the feed does not follow the user to the next rec }); describe('RecordDetailView — optimistic comments ride with their own record (#3268)', () => { - /** Post a comment through the real composer on the auto-appended panel. */ + /** Post a comment through the real composer on the authored panel. */ async function postComment(text: string) { const box = await screen.findByPlaceholderText(COMMENT_PLACEHOLDER); fireEvent.change(box, { target: { value: text } }); @@ -372,21 +383,21 @@ describe('RecordDetailView — optimistic comments ride with their own record (# // been posted but not yet come back from the server cannot be collateral. const dataSource = makeDataSource({}); - const { rerender } = render(tree(dataSource, REC_A, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + const { rerender } = render(tree(dataSource, REC_A, [AUTHORED_PAGE_WITH_DISCUSSION])); await screen.findByText(EMPTY_COMMENTS); await postComment('optimistic on A'); expect(await screen.findByText('optimistic on A')).toBeTruthy(); // …it must not follow the user to B… - rerender(tree(dataSource, REC_B, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + rerender(tree(dataSource, REC_B, [AUTHORED_PAGE_WITH_DISCUSSION])); expect(await screen.findByText(EMPTY_COMMENTS)).toBeTruthy(); expect(screen.queryByText('optimistic on A')).toBeNull(); // …and it must still be on A when the user comes back, exactly once: the // re-read now returns the persisted copy under the SAME id the optimistic // row was created with, so the union-by-id merge folds them together. - rerender(tree(dataSource, REC_A, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + rerender(tree(dataSource, REC_A, [AUTHORED_PAGE_WITH_DISCUSSION])); await waitFor(() => expect(screen.getAllByText('optimistic on A')).toHaveLength(1)); }); @@ -397,17 +408,17 @@ describe('RecordDetailView — optimistic comments ride with their own record (# const dataSource = makeDataSource({}); dataSource.create = vi.fn(async () => { throw new Error('503 offline'); }); - const { rerender } = render(tree(dataSource, REC_A, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + const { rerender } = render(tree(dataSource, REC_A, [AUTHORED_PAGE_WITH_DISCUSSION])); await screen.findByText(EMPTY_COMMENTS); await postComment('never persisted'); expect(await screen.findByText('never persisted')).toBeTruthy(); - rerender(tree(dataSource, REC_B, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + rerender(tree(dataSource, REC_B, [AUTHORED_PAGE_WITH_DISCUSSION])); expect(await screen.findByText(EMPTY_COMMENTS)).toBeTruthy(); expect(screen.queryByText('never persisted')).toBeNull(); - rerender(tree(dataSource, REC_A, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + rerender(tree(dataSource, REC_A, [AUTHORED_PAGE_WITH_DISCUSSION])); await waitFor(() => expect(screen.getAllByText('never persisted')).toHaveLength(1)); }); @@ -415,12 +426,12 @@ describe('RecordDetailView — optimistic comments ride with their own record (# // Posting on A then on B must give each panel exactly its own row. const dataSource = makeDataSource({}); - const { rerender } = render(tree(dataSource, REC_A, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + const { rerender } = render(tree(dataSource, REC_A, [AUTHORED_PAGE_WITH_DISCUSSION])); await screen.findByText(EMPTY_COMMENTS); await postComment('written on A'); await screen.findByText('written on A'); - rerender(tree(dataSource, REC_B, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + rerender(tree(dataSource, REC_B, [AUTHORED_PAGE_WITH_DISCUSSION])); await screen.findByText(EMPTY_COMMENTS); await postComment('written on B'); await screen.findByText('written on B'); @@ -431,7 +442,7 @@ describe('RecordDetailView — optimistic comments ride with their own record (# expect.objectContaining({ thread_id: `${OBJECT_NAME}:${REC_B}`, body: 'written on B' }), ); - rerender(tree(dataSource, REC_A, [AUTHORED_PAGE_WITHOUT_DISCUSSION])); + rerender(tree(dataSource, REC_A, [AUTHORED_PAGE_WITH_DISCUSSION])); await waitFor(() => expect(screen.getAllByText('written on A')).toHaveLength(1)); expect(screen.queryByText('written on B')).toBeNull(); }); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index f4fb0b7ae6..0c63ff1f02 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -10,7 +10,7 @@ import { useState, useEffect, useCallback, useMemo, useRef } from 'react'; import { useParams, useNavigate, useLocation, useSearchParams, Link } from 'react-router-dom'; -import { activityRowToFeedItem, RecordChatterRenderer, InlineEditSaveBar, buildDefaultPageSchema, deriveFieldGroupDetailSections, extractMentions, resolveTitleField, useRecordEditable } from '@object-ui/plugin-detail'; +import { activityRowToFeedItem, InlineEditSaveBar, buildDefaultPageSchema, deriveFieldGroupDetailSections, extractMentions, resolveTitleField, useRecordEditable } from '@object-ui/plugin-detail'; import { Empty, EmptyTitle, EmptyDescription } from '@object-ui/components'; import { useAuth, createAuthenticatedFetch } from '@object-ui/auth'; import { usePermissions } from '@object-ui/permissions'; @@ -24,7 +24,7 @@ import { SkeletonDetail } from '../skeletons/index.js'; import { ManagedByBadge } from '../components/ManagedByBadge.js'; import { resolveEffectiveCrudAffordances } from '../utils/crudAffordances.js'; import { deriveRelatedLists } from '../utils/deriveRelatedLists.js'; -import { hasExplicitDiscussion, hasExplicitAttachments, hasExplicitApprovals } from '../utils/pageSchemaIntrospect.js'; +import { stripDiscussionNodes, hasExplicitAttachments, hasExplicitApprovals } from '../utils/pageSchemaIntrospect.js'; import { ActionConfirmDialog, type ConfirmDialogState } from './ActionConfirmDialog.js'; import { ActionParamDialog, type ParamDialogState } from './ActionParamDialog.js'; import { ActionResultDialog, type ResultDialogState } from './ActionResultDialog.js'; @@ -2176,16 +2176,6 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // A page always exists past the guards above — authored (assignedPage) // or synthesized (buildDefaultPageSchema). - const disableDiscussion = (effectivePage as any)?.disableDiscussion === true; - // When the page schema embeds an explicit `record:discussion` / - // `record:chatter` slot, skip the bottom auto-append so the - // author placement (or synth default) wins. The walker recurses - // into `regions[]` so `buildDefaultPageSchema` output and - // full-Lightning authored pages are both detected. - const hasDiscussion = hasExplicitDiscussion(effectivePage as any); - // `enable.feeds: false` (#2707) suppresses the discussion panel outright — - // same opt-out contract the server enforces on sys_comment creation. - const showAutoDiscussion = !disableDiscussion && !hasDiscussion && feedsEnabled; // Synthesized pages place `record:attachments` beside the discussion feed // (objectstack#4358); the legacy bottom-of-page append below stays only as // the fallback for authored pages that don't slot the panel themselves. @@ -2331,7 +2321,7 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // synth/slotted pages AND authored full-Lightning pages without // mutating the assignedPage tree. `PageHeaderRenderer` dedupes by // name so authored business actions still win on collision. - const renderedPage = assignedPage + const composedPage = assignedPage ? effectivePage : buildDefaultPageSchema(objectDef as any, { sections: synthParts.sections, @@ -2348,6 +2338,30 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri ...(assignedSlots ? { slots: assignedSlots } : {}), }); + // objectui#7298 — A RECORD PAGE SHOWS A DISCUSSION PANEL BECAUSE IT COMPOSES + // ONE. Maintainer ruling 2026-09-12 (decision batch #120 item 5): *"a page is + // what its author composes … nothing is appended by default and then removed + // by a negative flag."* This view used to append `RecordChatterRenderer` + // below the page whenever the tree placed no `record:discussion` / + // `record:chatter` node, and the only documented way out was + // `assignedPage.disableDiscussion = true` — a key `PageSchema` is a + // `strictObject` about, so authoring it is a hard PARSE ERROR, not a dropped + // key. The append is gone with the read; the node is now the whole answer, + // and the panel's own `feed` config is honoured as authored. The synthesized + // default page still shows the panel because `buildDefaultPageSchema` + // composes `record:discussion` itself — out-of-the-box record pages are + // unchanged; AUTHORED pages that relied on the append add one node. + // + // `enable.feeds` (#2707) stays the OBJECT's switch and OUTRANKS the page: an + // object with feeds off shows no panel, declared or not. That is the one half + // of the ruling this tree did not already do — the old `feedsEnabled` gate + // sat on the append alone, so a declared (or synthesized) node rendered a + // panel on a feeds-off object, over a feed the view deliberately never + // fetched. Pruning the composed tree is what makes the precedence real rather + // than documented, and it is identity-preserving: a feeds-ON page is handed + // through untouched. + const renderedPage = feedsEnabled ? composedPage : stripDiscussionNodes(composedPage); + // Same split as attachments, but introspected on `renderedPage` — the tree // actually rendered — NOT `effectivePage` (#3461): the Approvals tab exists // only in the synthParts-aware rebuild above (it depends on runtime request @@ -2546,48 +2560,6 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri />
)} - {/* Auto-append the discussion feed only when the page schema - doesn't already place a `record:discussion` / - `record:chatter` component. Hard opt-out via - `assignedPage.disableDiscussion = true`. - - ── ONE PIPELINE, NOT TWO (objectui#8983) ────────────────── - This mounts `RecordChatterRenderer`: the very component - `ComponentRegistry` resolves BOTH `record:discussion` and - `record:chatter` to (`plugin-detail/src/index.tsx` registers - the two names against it; `recordChatterFeedMembersLive-8934` - pins that they stay one renderer). It used to reach PAST that - renderer to `RecordChatterPanel` with raw rows, and once - objectui#8934 taught the renderer to run `applyFeedConfig`, - that shortcut made the host fallback and the authored / - synthesized block render DIFFERENT feeds from the same rows: - a completed activity appeared here and not there (the spec's - `showCompleted` default is false), and a feed past twenty rows - rendered whole here and paged there. Nobody chose that - divergence; it fell out of closing the gap on one surface. - - ⚠️ The renderer takes no `items` prop — it reads the feed, the - loading flag, the mutation handlers and the mention - suggestions off the `DiscussionContextProvider` opened above, - carrying the very same `feedItems`. So this is a RE-BINDING of - an existing surface, not a second wiring, and it adds no - package dependency: `@object-ui/plugin-detail` is already this - package's peer dependency and already the source of this - file's detail imports. - - ⚠️ No authored schema is passed, deliberately. This is a host - affordance, not an authored block — there is no author to read - a `feed` config from — so it renders on the renderer's own - defaults, which are the same three affordances this branch - used to hard-code (`record-chatter.tsx` builds `position: - 'bottom'`, `collapsible: false`, reactions / threading / - comment input) plus the protocol's `feed` defaults. Routing a - synthesized node through `SchemaRenderer` instead would add - `hidden` / `disabled` expression evaluation over a node nobody - authored, to reach the component mounted here anyway. */} - {showAutoDiscussion && ( - - )} {/* Record-level inline-edit Save/Cancel bar (objectui#2407 P2) — commits the whole draft (highlights + body) in ONE atomic OCC update; renders (sticky) only while editing. */} From 26115bbeba9acf31ad83e6d879316bb7e1a57a14 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 22:07:52 +0000 Subject: [PATCH 2/2] fix(app-shell,docs): drop the useless initialiser; keep the record-page doc to plain markdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `no-useless-assignment` flagged `let result: any = node` in `stripDiscussionNodes` — both branches assign it, so the initialiser only hid a branch that forgot to. `content/docs/guide/slotted-pages.md` is a `.md` file and every `Callout` in this docs tree lives in `.mdx`; the upgrade note is a blockquote instead, which is the form the same file already uses. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa --- content/docs/guide/slotted-pages.md | 16 +++++++--------- .../app-shell/src/utils/pageSchemaIntrospect.ts | 4 +++- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/content/docs/guide/slotted-pages.md b/content/docs/guide/slotted-pages.md index 32c6fcc7df..a72efbec95 100644 --- a/content/docs/guide/slotted-pages.md +++ b/content/docs/guide/slotted-pages.md @@ -79,15 +79,13 @@ composes the node; the view also skips the `sys_comment` read for such an object, and the server rejects comment writes against it with `403 FEEDS_DISABLED`. `enable.feeds` is opt-**out**: absent means on. - - **Upgrading (objectui#7298).** A record page used to get a discussion panel - appended below its content whenever its tree placed no discussion node, and - the only way out was an `assignedPage.disableDiscussion` flag that `PageSchema` - — a `strictObject` — refuses, so no author could ever write it. Both are gone. - If one of your **authored full pages** relied on that automatic panel, add one - `record:discussion` node where you want it. Synthesized and slotted pages need - no change. - +> **Upgrading (objectui#7298).** A record page used to get a discussion panel +> appended below its content whenever its tree placed no discussion node, and +> the only way out was an `assignedPage.disableDiscussion` flag that `PageSchema` +> — a `strictObject` — refuses, so no author could ever write it. Both are gone. +> If one of your **authored full pages** relied on that automatic panel, add one +> `record:discussion` node where you want it. Synthesized and slotted pages need +> no change. ## Example: customize only the header diff --git a/packages/app-shell/src/utils/pageSchemaIntrospect.ts b/packages/app-shell/src/utils/pageSchemaIntrospect.ts index 3242d872cc..295b651276 100644 --- a/packages/app-shell/src/utils/pageSchemaIntrospect.ts +++ b/packages/app-shell/src/utils/pageSchemaIntrospect.ts @@ -93,7 +93,9 @@ export function stripDiscussionNodes(root: unknown): unknown { if (active.has(node)) return node; active.add(node); - let result: any = node; + // Assigned in both branches below; left uninitialised so a branch that + // forgot to set it is a type error rather than a silent pass-through. + let result: any; if (Array.isArray(node)) { const next: any[] = [];