From 1895c09651c9ade6d4594f9772a38531944865b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 03:04:33 +0000 Subject: [PATCH] fix(plugin-detail): let DetailSection's heuristic own the empty-section default `RecordDetailsRenderer` mapped every authored section with `hideEmpty: s.hideEmpty ?? true`. `DetailSection` already states the correct rule in its own heuristic -- "If a section is entirely empty (e.g., loading state, brand-new record), do NOT auto-hide -- the labels themselves are useful as a structural skeleton" -- and the forced default overrode exactly the case that sentence reserves. On a hand-created sparse record whole sections disappeared and the body collapsed to a couple of rows. The renderer now passes the authored value through untouched. An unauthored section reaches DetailSection as `undefined` and the heuristic decides; an authored `hideEmpty` keeps its exact former meaning. Also drops a non-English comment from the slot (AGENTS.md commandment #-1). Pinned in record-details.emptySectionDefault.test.tsx: the all-empty skeleton, the below-threshold empty row, the intact label-graveyard guard, and both authored directions. Maintainer ruling 2026-08-31; objectui#7064. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM --- .changeset/7064-empty-section-default.md | 46 ++++ ...ecord-details.emptySectionDefault.test.tsx | 204 ++++++++++++++++++ .../src/renderers/record-details.tsx | 27 ++- 3 files changed, 271 insertions(+), 6 deletions(-) create mode 100644 .changeset/7064-empty-section-default.md create mode 100644 packages/plugin-detail/src/renderers/__tests__/record-details.emptySectionDefault.test.tsx diff --git a/.changeset/7064-empty-section-default.md b/.changeset/7064-empty-section-default.md new file mode 100644 index 0000000000..146b859d39 --- /dev/null +++ b/.changeset/7064-empty-section-default.md @@ -0,0 +1,46 @@ +--- +'@object-ui/plugin-detail': minor +--- + +**Behaviour change.** `record:details` no longer forces `hideEmpty` on the +sections it synthesizes, so a sparse record keeps its section skeleton instead +of collapsing. Applications relying on the old auto-hide of *unauthored* +sections will now see headings, field labels and empty-value placeholders where +rows used to vanish. This is the loud-over-silent direction, ruled by the +maintainer on 2026-08-31: an empty detail body is a platform concern, and a +metadata application should not have to author its way out of one. + +`RecordDetailsRenderer` mapped every authored section with +`hideEmpty: s.hideEmpty ?? true`. `DetailSection` already states the correct +rule in its own heuristic — *"If a section is entirely empty (e.g., loading +state, brand-new record), do NOT auto-hide — the labels themselves are useful +as a structural skeleton"* — and the forced default overrode exactly the case +that sentence reserves. On a hand-created record whole sections disappeared and +the body collapsed to a couple of rows; seeded demo data hid it. Every +application then had to hand-write `hideEmpty: false` per section to stop +looking broken, which is per-app tax for a platform defect. The renderer now +passes the authored value through untouched and lets the heuristic own the +default. + +What changes, precisely: + +- an **all-empty** section renders its heading, every field label and one + empty-value placeholder per field (it used to render nothing at all); +- a **small** partly-empty section — below `DetailSection`'s auto-hide + threshold of 4 fields / 25% empty (3 / 20% on mobile) — now shows its empty + rows; +- a **large** mostly-empty section with at least one filled row still + auto-hides, with the "Show N empty fields" toggle unchanged: the + label-graveyard guard is intact and this is not a return to dense-by-default; +- empty rows are now visible while inline-editing a section, so an unwritten + field can be filled in place. + +What does **not** change: an authored `hideEmpty` keeps its exact former +meaning. `hideEmpty: true` remains the explicit opt-in to hiding, and +`hideEmpty: false` remains what it always was — "not `true`", not an override +of the auto-hide heuristic (measured, and pinned as pre-existing). + +Reference-app hit inside this repo: the Studio metadata-admin page preview +(`PagePreview`) binds a real sample record, so a `record:details` block over a +sparse sample now previews the skeleton rather than a collapsed body. No +application metadata needs editing — that is the point of the change. diff --git a/packages/plugin-detail/src/renderers/__tests__/record-details.emptySectionDefault.test.tsx b/packages/plugin-detail/src/renderers/__tests__/record-details.emptySectionDefault.test.tsx new file mode 100644 index 0000000000..a09885b162 --- /dev/null +++ b/packages/plugin-detail/src/renderers/__tests__/record-details.emptySectionDefault.test.tsx @@ -0,0 +1,204 @@ +/** + * 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. + */ + +/** + * `record:details` — who owns the empty-section default (objectui#7064). + * + * `RecordDetailsRenderer` used to map every authored section with + * `hideEmpty: s.hideEmpty ?? true`. That forced default overrode the one case + * `DetailSection`'s own heuristic explicitly reserves: + * + * "If a section is entirely empty (e.g., loading state, brand-new record), + * do NOT auto-hide — the labels themselves are useful as a structural + * skeleton." + * + * With the force in place an all-empty section took `DetailSection`'s + * all-fields-hidden early return instead, so a hand-created record lost whole + * sections and collapsed to a two-row body, and every application had to + * hand-write `hideEmpty: false` per section to stop looking broken — per-app + * tax for a platform concern (maintainer ruling 2026-08-31). + * + * The renderer now passes the authored value through untouched. These pins + * hold both halves of that contract: + * - the UNAUTHORED default is the heuristic's, not the renderer's; + * - an AUTHORED value keeps its exact former meaning. + * + * Deliberately no i18n provider: `fieldLabel` falls back to the value the + * renderer hands it, which for the spec's bare-string section fields is the + * field NAME. So the "labels" a skeleton shows here read as field names — the + * same DOM nodes a translated app fills with translated labels. + */ + +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import * as React from 'react'; +import { RecordContextProvider } from '@object-ui/react'; +import { RecordDetailsRenderer } from '../record-details'; + +/** + * No `name` / `title` / `subject` / `display_name` key anywhere: the renderer + * drops the page-H1 title field from the body (`titleCandidates`), which would + * make an absence assertion below pass for the wrong reason. + */ +const objectSchema = { + fields: { + industry: { type: 'text', label: 'Industry' }, + stage: { type: 'text', label: 'Stage' }, + amount: { type: 'text', label: 'Amount' }, + close_date: { type: 'text', label: 'Close Date' }, + next_step: { type: 'text', label: 'Next Step' }, + }, +}; + +/** A hand-created sparse record: one filled field, everything else unwritten. */ +const sparseData = { industry: 'Manufacturing' }; + +const renderDetails = (schema: Record, data: Record = sparseData) => + render( + + + , + ); + +/** The empty-value placeholder `DetailSection` draws for a field with no value. */ +const emptyPlaceholders = () => screen.queryAllByTitle('No value'); + +describe('record:details — the UNAUTHORED empty-section default is DetailSection\'s heuristic (#7064)', () => { + it('an ALL-empty section renders its skeleton: heading, every field label, an empty placeholder each', () => { + renderDetails({ + sections: [ + { name: 'deal_terms', label: 'Deal Terms', fields: ['stage', 'amount', 'close_date', 'next_step'] }, + ], + }); + + // The heading survives — the whole section used to disappear here. + expect(screen.getByText('Deal Terms')).toBeInTheDocument(); + + // Every field keeps its row, so the record reads as a structure waiting to + // be filled rather than as a blank page. + for (const label of ['stage', 'amount', 'close_date', 'next_step']) { + expect(screen.getByText(label)).toBeInTheDocument(); + } + expect(emptyPlaceholders()).toHaveLength(4); + }); + + it('a SMALL partly-empty section (below the auto-hide threshold) now shows its empty row', () => { + // 2 fields, 1 empty: under DetailSection's minimum field count in both the + // desktop (4) and mobile (3) variant, so the auto-hide heuristic never + // fires and the empty row is shown. Under the old forced default this row + // was hidden. This is the second half of the user-visible behaviour change + // the changeset names — it is not limited to all-empty sections. + renderDetails({ + sections: [ + { name: 'summary', label: 'Summary', fields: ['industry', 'stage'] }, + ], + }); + + expect(screen.getByText('Summary')).toBeInTheDocument(); + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + expect(screen.getByText('stage')).toBeInTheDocument(); + expect(emptyPlaceholders()).toHaveLength(1); + }); + + it('the label-graveyard guard is INTACT: a large mostly-empty section still auto-hides', () => { + // 4 fields, 3 empty, 1 filled — at/above both threshold variants + // (min fields 4/3, empty ratio 25%/20%) with at least one filled row, so + // `shouldAutoHideEmpty` still fires exactly as before. Flipping the + // unauthored default did NOT turn populated pages into label graveyards; + // it only stopped overriding the all-empty case the heuristic reserves. + renderDetails({ + sections: [ + { + name: 'deal_terms', + label: 'Deal Terms', + fields: ['industry', 'stage', 'amount', 'close_date'], + }, + ], + }); + + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + expect(screen.queryByText('stage')).not.toBeInTheDocument(); + expect(emptyPlaceholders()).toHaveLength(0); + // …and the user-facing escape hatch is offered for the rows it hid. + expect(screen.getByRole('button', { name: /empty fields/i })).toBeInTheDocument(); + }); +}); + +describe('record:details — an AUTHORED `hideEmpty` keeps its exact former meaning (#7064)', () => { + it('`hideEmpty: true` still hides an all-empty section entirely', () => { + renderDetails({ + sections: [ + { name: 'deal_terms', label: 'Deal Terms', fields: ['stage', 'amount', 'close_date', 'next_step'], hideEmpty: true }, + // CONTROL: a sibling section that MUST render, so the absences below + // are a decision by `hideEmpty` and not a render that never happened. + { name: 'firmographics', label: 'Firmographics', fields: ['industry'] }, + ], + }); + + expect(screen.getByText('Firmographics')).toBeInTheDocument(); + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + + expect(screen.queryByText('Deal Terms')).not.toBeInTheDocument(); + expect(screen.queryByText('stage')).not.toBeInTheDocument(); + expect(emptyPlaceholders()).toHaveLength(0); + }); + + it('`hideEmpty: true` still hides the empty rows of a partly-filled section', () => { + renderDetails({ + sections: [ + { name: 'summary', label: 'Summary', fields: ['industry', 'stage'], hideEmpty: true }, + ], + }); + + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + expect(screen.queryByText('stage')).not.toBeInTheDocument(); + expect(emptyPlaceholders()).toHaveLength(0); + }); + + it('`hideEmpty: false` shows the empty rows the heuristic would not have hidden anyway', () => { + renderDetails({ + sections: [ + { name: 'summary', label: 'Summary', fields: ['industry', 'stage'], hideEmpty: false }, + ], + }); + + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + expect(screen.getByText('stage')).toBeInTheDocument(); + expect(emptyPlaceholders()).toHaveLength(1); + }); + + it('MEASURED, not endorsed: `hideEmpty: false` is "not true", NOT an override of the auto-hide heuristic', () => { + // `DetailSection` computes `shouldAutoHideEmpty` from `!section.hideEmpty`, + // so an authored `false` is indistinguishable from an unauthored section + // and the heuristic still hides empty rows once the thresholds are met. + // This is PRE-EXISTING and is NOT changed by #7064 — under the old forced + // default the same fixture took the same path, because `?? true` preserved + // an authored `false` too. Pinned so a future reader can see that the flip + // left this precedence exactly where it found it; whether `false` SHOULD + // become a hard override is a separate contract question. + renderDetails({ + sections: [ + { + name: 'deal_terms', + label: 'Deal Terms', + fields: ['industry', 'stage', 'amount', 'close_date'], + hideEmpty: false, + }, + ], + }); + + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + expect(screen.queryByText('stage')).not.toBeInTheDocument(); + expect(emptyPlaceholders()).toHaveLength(0); + }); +}); diff --git a/packages/plugin-detail/src/renderers/record-details.tsx b/packages/plugin-detail/src/renderers/record-details.tsx index 83f46137cf..bee9ec8811 100644 --- a/packages/plugin-detail/src/renderers/record-details.tsx +++ b/packages/plugin-detail/src/renderers/record-details.tsx @@ -202,12 +202,27 @@ export const RecordDetailsRenderer: React.FC = ({ // flat sections stay borderless so the page chrome alone provides // containment. Authors can override explicitly via `showBorder`. showBorder: s.showBorder ?? (translatedTitle ? true : false), - // Phase N: default to hide-empty so pages don't render as label - // graveyards on first load. Authors can opt back in to showing - // empty rows by setting `hideEmpty: false` explicitly. The - // "显示 N 个空字段" toggle in DetailSection still works as the - // user-facing escape hatch. - hideEmpty: s.hideEmpty ?? true, + // Deliberately NOT defaulted. The authored value passes through + // verbatim, so an UNAUTHORED section reaches DetailSection as + // `undefined` and that component's own stated heuristic decides: + // auto-hide empty rows only while the section still has at least one + // filled row, and never on an all-empty section — there the labels + // ARE the structural skeleton a sparse or brand-new record needs. + // + // This slot used to force `s.hideEmpty ?? true`, which overrode + // exactly the case that heuristic reserves: a hand-created record + // collapsed to a two-row body and whole sections vanished, and every + // app had to hand-write `hideEmpty: false` per section to stop looking + // broken. That is per-app tax for a platform concern (maintainer + // ruling 2026-08-31: this is a platform problem; metadata + // applications should not have to think about these details). + // + // An AUTHORED value is still honoured exactly as before — + // `hideEmpty: true` remains the explicit opt-in to hiding, and + // `hideEmpty: false` the explicit opt-out. Only the unauthored + // default flips. DetailSection's "Show N empty fields" toggle remains + // the user-facing escape hatch wherever the heuristic does hide rows. + hideEmpty: s.hideEmpty, fields: dropHidden(normaliseList(filterList(s.fields))), }); })