From 773c6463e8b42b7a01d5cf37298748f28b7d39c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 16:20:22 +0000 Subject: [PATCH] fix(plugin-tree): format tree-grid cells like the flat table (objectui#6014) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 「组织架构」 tree tab rendered a lookup column as a bare record id and a select column as its raw stored value, while the flat-table tab on the same page rendered the display name and the translated label over the same records. The defect was in the fetch, not the formatter. ObjectTree treated "the host passed inline `data`" as "I do not need the object schema" and skipped getObjectSchema, but its record-fetch branch prefers a live object dataSource over any inline data. On ListView's real mount shape (objectName + dataSource + its own pre-fetched data) the tree therefore ran its own query with buildExpandFields(undefined) -> [] -> no $expand, and had no field definitions to format cells from. Both symptoms fall out of that one gap. - Fetch the object schema whenever the dataSource can serve one. - Hold the record fetch until that schema has SETTLED (not necessarily succeeded), which also drops a wasted first query whose lookup columns came back as bare ids and were painted before the real query landed. - Route cells through a field-aware formatter that delegates both decisions: option labels via the same `translateOptions` seam ObjectGrid uses, expanded references via getRecordDisplayName (ADR-0079), family judged by isExpandableFieldType -- the same predicate that built $expand. No new exports, no new package dependencies. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- .changeset/6014-tree-grid-field-formatting.md | 47 ++++ .../src/ObjectTree.fieldFormatting.test.tsx | 222 ++++++++++++++++++ packages/plugin-tree/src/ObjectTree.tsx | 172 +++++++++++++- 3 files changed, 428 insertions(+), 13 deletions(-) create mode 100644 .changeset/6014-tree-grid-field-formatting.md create mode 100644 packages/plugin-tree/src/ObjectTree.fieldFormatting.test.tsx diff --git a/.changeset/6014-tree-grid-field-formatting.md b/.changeset/6014-tree-grid-field-formatting.md new file mode 100644 index 0000000000..94d9c492ed --- /dev/null +++ b/.changeset/6014-tree-grid-field-formatting.md @@ -0,0 +1,47 @@ +--- +'@object-ui/plugin-tree': patch +--- + +`ObjectTree` formats its cells the way the flat table does: a lookup column renders the +referenced record's display name and a select column renders its translated option label, +instead of a raw record id and the raw stored value (objectui#6014). + +Reported against the built-in 业务单元 (`sys_business_unit`) page, whose 「组织架构」 tree tab +showed the manager column as a bare user id and the type column as `department`, while the +flat-table tab on the same page — over the same expanded records — showed the user's name +and 「部门」. + +The card carried its own control, and it pointed at the fetch rather than the formatter. The +tree treated "the host passed inline `data`" as "I do not need the object schema" and skipped +`getObjectSchema`, but its record-fetch branch prefers a live object dataSource over any +inline data. On the one mount shape `ListView` actually uses — `objectName` + a dataSource + +its own pre-fetched `data` — the tree therefore issued its OWN query with +`buildExpandFields(undefined)` → `[]` → no `$expand` at all, and had no field definitions to +format cells from. Both reported symptoms fall out of that single gap, which is why the flat +tab was unaffected and why the tree's existing tests (inline data, no dataSource — a path +that never runs the tree's own fetch) could not see it. + +Three changes, all inside `packages/plugin-tree`: + +- The object schema is fetched whenever the dataSource can serve one, not only when no host + passed inline data. The guard inside the fetch already no-ops without a dataSource, so the + pure inline/static path is unaffected. +- Records are no longer fetched until that schema has settled — settled, not necessarily + successful, so a rejected or inapplicable schema fetch can never block the tree. This also + removes a wasted first query whose lookup columns came back as bare ids and were painted + for a moment before the real query landed. +- Cell values route through a field-aware formatter that delegates both decisions rather than + re-deciding them: option labels through the `translateOptions` seam `ObjectGrid` already + uses for the flat tab (so both tabs read one `fieldOptions.*` i18n key, with the same + exact-then-case-insensitive match and `humanizeLabel` fallback as `SelectCellRenderer`), and + expanded references through `getRecordDisplayName`, the unified display-name resolver + (ADR-0079), with the family judged by `isExpandableFieldType` — the same predicate that + decided what to put in `$expand`. + +No new exports and no new package dependencies: both resolvers were already published from +`@object-ui/core`, and `translateOptions` was already reachable through the `useSafeFieldLabel` +hook this component calls for its column headers. + +One visible consequence beyond the report: an expanded record that comes back with no name-ish +field now reads as ADR-0079's `Record #` floor — the string every other surface shows for +it — rather than as the bare id. diff --git a/packages/plugin-tree/src/ObjectTree.fieldFormatting.test.tsx b/packages/plugin-tree/src/ObjectTree.fieldFormatting.test.tsx new file mode 100644 index 0000000000..729982f641 --- /dev/null +++ b/packages/plugin-tree/src/ObjectTree.fieldFormatting.test.tsx @@ -0,0 +1,222 @@ +/** + * 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. + */ + +/** + * The tree-grid formats its cells the way the flat table does — objectui#6014. + * + * ── The repro this file pins ─────────────────────────────────────────────── + * On the built-in 业务单元 (`sys_business_unit`) page the 「组织架构」 tree tab + * rendered the manager `lookup` column as a raw record id and the `type` + * `select` column as the raw stored value (`department`), while the flat-table + * tab on the SAME page rendered the user's name and the translated 「部门」. + * + * ── The mount shape matters, and it is why this was invisible ────────────── + * `ListView` renders the tree through `SchemaRenderer` with BOTH `objectName` + * (so `getDataConfig` yields `provider: 'object'`) and its own already-fetched + * `data` array. `ObjectTree` treats "a host passed inline data" as "I do not + * need the object schema" and skips `getObjectSchema`, yet its record-fetch + * branch still prefers a live object dataSource — so it issued its OWN query + * with `buildExpandFields(undefined)` → `[]` → no `$expand` at all, and had no + * field definitions to read `options` from either. Both halves of the card fall + * out of that one gap, which is why the tests below mount the tree the way + * `ListView` really does (dataSource AND inline `data`) rather than the way the + * older tests do (inline `data` only, no dataSource — a path that never runs + * the tree's own fetch and therefore cannot see this defect). + * + * ── The two halves are two mechanisms ───────────────────────────────────── + * The lookup half is object unwrapping over an `$expand`ed record; the select + * half is option-label resolution (`field.options` + the `fieldOptions.*` i18n + * convention). Fixing one does not fix the other, so both are asserted here. + */ + +import React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { I18nProvider } from '@object-ui/i18n'; +import { ObjectTree } from './ObjectTree'; + +afterEach(cleanup); + +/** `sys_business_unit`, trimmed to the two columns the card names. */ +const OBJECT_SCHEMA = { + name: 'sys_business_unit', + fields: { + name: { type: 'text', label: 'Name' }, + parent_id: { type: 'lookup', reference: 'sys_business_unit', label: 'Parent' }, + manager_user_id: { type: 'lookup', reference: 'sys_user', label: 'Manager' }, + type: { + type: 'select', + label: 'Type', + // Labels deliberately NOT derivable from their value: `humanizeLabel` + // (the unmatched-value fallback) turns `department` into `Department`, + // so an assertion expecting `Department` would pass even with option + // matching completely broken. `Business Department` can only come from + // the options array. + options: [ + { label: 'Business Department', value: 'department' }, + { label: 'Holding Company', value: 'company' }, + ], + }, + }, +}; + +/** What the server returns when `$expand` names the manager lookup. */ +const EXPANDED_ROWS = [ + { + id: 'bu1', + name: 'Acme', + parent_id: null, + manager_user_id: { id: 'u1', name: 'Zhang San' }, + type: 'department', + }, +]; + +/** What the server returns when `$expand` is absent — the reported symptom. */ +const BARE_ROWS = [ + { id: 'bu1', name: 'Acme', parent_id: null, manager_user_id: 'u1', type: 'department' }, +]; + +const TREE_SCHEMA = { + type: 'object-tree', + objectName: 'sys_business_unit', + labelField: 'name', + fields: ['name', 'manager_user_id', 'type'], +}; + +/** + * A dataSource that answers honestly: it expands only what it was ASKED to + * expand. A stub that always returned the expanded shape would make this whole + * file pass without a fix — the defect is in the request, not the response. + */ +function makeDataSource(rows?: any[]) { + const expandArgs: (string[] | null)[] = []; + const dataSource = { + find: async (_object: string, query: any) => { + const expand = query?.$expand ?? null; + expandArgs.push(expand); + if (rows) return rows; + return expand && expand.includes('manager_user_id') ? EXPANDED_ROWS : BARE_ROWS; + }, + getObjectSchema: async () => OBJECT_SCHEMA, + } as any; + return { dataSource, expandArgs }; +} + +/** The cells of the single rendered row, as text — what the user sees. */ +async function renderRowCells(children: (ds: any) => React.ReactElement) { + const { dataSource, expandArgs } = makeDataSource(); + render(children(dataSource)); + await waitFor(() => expect(screen.getByTestId('object-tree')).toBeInTheDocument()); + const row = screen.getAllByTestId('object-tree-row')[0]; + const cells = Array.from(row.querySelectorAll('td')).map((td) => td.textContent?.trim() ?? ''); + return { cells, expandArgs }; +} + +/** The ListView mount shape: `objectName` + a live dataSource + inline `data`. */ +function listViewShapedTree(dataSource: any) { + return ( + + ); +} + +describe('ObjectTree field formatting (objectui#6014)', () => { + it('renders a lookup column as the referenced record display name, not its id', async () => { + const { cells } = await renderRowCells(listViewShapedTree); + + // The manager column is the second cell (label column first). + expect(cells[1]).toBe('Zhang San'); + expect(cells[1]).not.toBe('u1'); + }); + + it('asks the dataSource to expand the lookup columns it is going to render', async () => { + // The mechanism behind the assertion above: a host passing inline `data` + // must not suppress the object-schema fetch that `$expand` is derived from. + const { expandArgs } = await renderRowCells(listViewShapedTree); + + expect(expandArgs.length).toBeGreaterThan(0); + expect(expandArgs[expandArgs.length - 1]).toContain('manager_user_id'); + }); + + it('renders a select column as its option label, not the raw stored value', async () => { + const { cells } = await renderRowCells(listViewShapedTree); + + // The type column is the third cell. + expect(cells[2]).toBe('Business Department'); + // The reported symptom: the raw stored value. + expect(cells[2]).not.toBe('department'); + // …and not the `humanizeLabel` fallback either. Without this line the + // assertion above could be satisfied by the fallback alone. + expect(cells[2]).not.toBe('Department'); + }); + + it('matches a stored value to its option case-insensitively, and humanizes an unmatched one', async () => { + // Mirrors `SelectCellRenderer`: seed data stores `Department` against a + // declared `department`, and a value no option declares still reads as + // words rather than as a raw token. + // Served through the dataSource, not as inline `data`: on this mount shape + // the tree's own query wins over anything the host passed down — the very + // precedence that made objectui#6014 invisible from the inline path. + const { dataSource } = makeDataSource([ + { id: 'bu9', name: 'Mixed', parent_id: null, manager_user_id: null, type: 'Department' }, + { id: 'bu8', name: 'Unknown', parent_id: null, manager_user_id: null, type: 'joint_venture' }, + ]); + render(); + await waitFor(() => expect(screen.getByTestId('object-tree')).toBeInTheDocument()); + const rows = screen.getAllByTestId('object-tree-row'); + const typeCellOf = (label: string) => { + const row = rows.find((r) => r.textContent?.includes(label))!; + return Array.from(row.querySelectorAll('td'))[2]?.textContent?.trim(); + }; + + expect(typeCellOf('Mixed')).toBe('Business Department'); + expect(typeCellOf('Unknown')).toBe('Joint Venture'); + }); + + it('renders a select option label through the fieldOptions i18n convention', async () => { + // The exact repro: a zh session must read 「部门」, not `department`. + // Key convention: `{appNamespace}.fieldOptions.{object}.{field}.{value}` + // — the same one `translateOptions` resolves for the flat grid. + const { dataSource } = makeDataSource(); + render( + + + , + ); + await waitFor(() => expect(screen.getByTestId('object-tree')).toBeInTheDocument()); + const row = screen.getAllByTestId('object-tree-row')[0]; + const cells = Array.from(row.querySelectorAll('td')).map((td) => td.textContent?.trim() ?? ''); + + expect(cells[2]).toBe('部门'); + }); + + it('still renders a plain value column untouched', async () => { + // Counter-probe: the formatting chain must not swallow ordinary values. + // Green both before and after the fix — it is here so a green on the four + // assertions above cannot be explained by "the tree renders nothing". + const { cells } = await renderRowCells(listViewShapedTree); + + expect(cells[0]).toBe('Acme'); + }); +}); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index 29fdf4a297..9b1d1e3f6d 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -24,7 +24,14 @@ import type { DataSource, ViewData } from '@object-ui/types'; import { useNavigationOverlay, useSafeFieldLabel } from '@object-ui/react'; import { NavigationOverlay, cn } from '@object-ui/components'; import { createSafeTranslation } from '@object-ui/i18n'; -import { extractRecords, buildExpandFields, columnIdentity } from '@object-ui/core'; +import { + extractRecords, + buildExpandFields, + columnIdentity, + isExpandableFieldType, + getRecordDisplayName, + humanizeLabel, +} from '@object-ui/core'; import { ChevronRight, ChevronDown } from 'lucide-react'; /** @@ -224,8 +231,97 @@ function initialExpanded(roots: TreeNode[], depth?: number): Set { return set; } -function formatValue(value: any): string { +/** + * One entry of a field's `options`. The index signature is not incidental — it + * is what `useSafeFieldLabel().translateOptions` declares, and this alias exists + * to be assignable to that signature rather than to re-describe it. + */ +interface FieldOption { + value: string; + label: string; + [key: string]: unknown; +} + +/** Translates one field's `options` for the session locale. */ +type TranslateOptions = ( + objectName: string, + fieldName: string, + options: FieldOption[], +) => FieldOption[]; + +/** What {@link formatCellValue} needs to format one cell of one column. */ +interface CellFormatContext { + /** The object schema's definition for this column, when one was fetched. */ + fieldDef: any; + /** The column's field key — the i18n option keys are scoped by it. */ + fieldName: string; + /** The object the tree is rendering; absent for a schema-less inline mount. */ + objectName?: string; + /** `useSafeFieldLabel().translateOptions` — identity without a provider. */ + translateOptions: TranslateOptions; +} + +/** + * Format one cell the way the flat table formats the same field — objectui#6014. + * + * Both branches DELEGATE the decision rather than re-deciding it, so the tree + * cannot drift from the surfaces it is supposed to agree with: + * + * - **select-family** (any field carrying `options`): the stored value is + * resolved to its option label through `translateOptions`, which is the + * exact call `ObjectGrid` makes when it builds a column's `fieldMeta` + * (`packages/plugin-grid/src/ObjectGrid.tsx`, the `fieldMeta.options = + * translateOptions(...)` line), so both tabs read one `fieldOptions.*` i18n + * key. Matching is exact-then-case-insensitive and falls back to + * `humanizeLabel`, mirroring `SelectCellRenderer` in `@object-ui/fields` + * (seed data stores `Referral` against a declared `referral`). Keying the + * branch on "has options" rather than on a list of select spellings is + * deliberate: `select` / `status` / `multiselect` / `radio` / `checkboxes` / + * `tags` all resolve identically, and a copied type list is one more thing + * that can fall behind the registry. + * + * - **reference-family**: an expanded record resolves through + * `getRecordDisplayName`, THE unified display-name resolver (ADR-0079), and + * the family is judged by `isExpandableFieldType` — the SAME predicate that + * decided what to put in `$expand` a few lines up, so "what we expanded" and + * "what we unwrap as a reference" cannot disagree. + * + * A value with no field definition (an untyped column, or a mount that never + * fetched a schema) keeps the previous conservative unwrap. + */ +function formatCellValue(value: any, ctx?: CellFormatContext): string { if (value == null) return ''; + + const options: FieldOption[] | null = Array.isArray(ctx?.fieldDef?.options) + ? (ctx!.fieldDef.options as FieldOption[]) + : null; + if (options && options.length > 0) { + const translated = ctx!.objectName + ? ctx!.translateOptions(ctx!.objectName, ctx!.fieldName, options) + : options; + const labelFor = (raw: unknown): string => { + const exact = translated.find((opt) => opt?.value === raw); + if (exact) return String(exact.label ?? raw); + const normalized = String(raw).toLowerCase(); + const insensitive = translated.find( + (opt) => String(opt?.value).toLowerCase() === normalized, + ); + if (insensitive) return String(insensitive.label ?? raw); + return humanizeLabel(String(raw)); + }; + return Array.isArray(value) + ? value.filter((v) => v != null).map(labelFor).join(', ') + : labelFor(value); + } + + if (typeof value === 'object' && isExpandableFieldType(ctx?.fieldDef)) { + // No schema for the REFERENCED object here, so this lands on ADR-0079's + // record-key derivation (`name` / `full_name` / `*_name` / …) and, for an + // expanded record that came back without any name-ish field, its + // `Record #` floor — the same string every other surface shows. + return getRecordDisplayName(undefined, value); + } + if (typeof value === 'object') { return String(value.name ?? value.label ?? value.id ?? value._id ?? ''); } @@ -243,14 +339,41 @@ export const ObjectTree: React.FC = ({ const [loading, setLoading] = useState(true); const [error, setError] = useState(null); const [objectSchema, setObjectSchema] = useState(null); + /** + * Whether the object-schema fetch below has finished — settled, not + * successful: a dataSource that cannot serve a schema, an object with no + * name, and a rejected fetch all count, so the record fetch can never be + * blocked forever by a schema that is never going to arrive. + * + * A one-way latch on purpose. Re-arming it on every run of that effect would + * mean a `setState` in the effect body, and this component's dependency list + * includes `dataConfig` — a `useMemo` over the `schema` PROP object — so a + * host that rebuilds its schema each render would turn a benign re-run into a + * render loop. Not re-arming costs at most one fetch against a stale schema + * when `objectName` changes mid-life, which is exactly what happened on every + * fetch before this. + */ + const [schemaSettled, setSchemaSettled] = useState(false); const dataConfig = useMemo(() => getDataConfig(schema), [schema]); - const hasInlineData = - Array.isArray((rest as any).data) || - Array.isArray((schema as any).data) || - dataConfig?.provider === 'value'; - // Fetch object schema (for parent-field auto-detection + column labels). + // Fetch the object schema whenever the dataSource can serve one. + // + // It feeds FOUR things: parent-field auto-detection, column labels, the + // `$expand` list built below, and (objectui#6014) the per-field definitions + // the cell formatter reads to resolve select options and reference values. + // + // This used to be gated on "the host passed no inline data", which read as a + // cheap skip but disagreed with the record-fetch effect below: THAT branch + // prefers a live object dataSource over any inline `data`, so on the one + // mount shape `ListView` actually uses (objectName + dataSource + its own + // pre-fetched `data`) the tree ran its own query with + // `buildExpandFields(undefined)` → `[]` → no `$expand` at all, and had no + // field definitions to format cells with. That is the whole of objectui#6014: + // lookups rendered as bare ids and selects as raw stored values, on the very + // page whose flat-table tab rendered both correctly. The guard inside + // `fetchSchema` already no-ops without a dataSource, so dropping the gate + // costs nothing on the pure inline/static path. useEffect(() => { let cancelled = false; const fetchSchema = async () => { @@ -263,13 +386,17 @@ export const ObjectTree: React.FC = ({ if (!cancelled) setObjectSchema(result); } catch (err) { console.error('[ObjectTree] Failed to fetch object schema:', err); + } finally { + // `finally`, so the two early `return`s and a rejected fetch all settle + // too — see the latch's docstring. + if (!cancelled) setSchemaSettled(true); } }; - if (!hasInlineData) fetchSchema(); + fetchSchema(); return () => { cancelled = true; }; - }, [schema.objectName, dataSource, dataConfig, hasInlineData]); + }, [schema.objectName, dataSource, dataConfig]); // Fetch records. useEffect(() => { @@ -286,6 +413,13 @@ export const ObjectTree: React.FC = ({ // tree. Fetching our own records (no column projection) guarantees the // parent field is present so the hierarchy resolves. if (dataConfig?.provider === 'object' && dataSource && typeof dataSource.find === 'function') { + // Wait for the schema before querying. `$expand` is DERIVED from it, + // so firing early guaranteed one query whose lookup columns came back + // as bare ids — the user saw those raw ids painted, then replaced a + // moment later once the real query landed. `loading` stays true here + // so the tree shows its spinner instead of a wrong first answer, and + // this effect re-runs the moment the latch flips. + if (!schemaSettled) return; const expand = buildExpandFields(objectSchema?.fields); const result = await dataSource.find(dataConfig.object, { $filter: schema.filter, @@ -331,7 +465,7 @@ export const ObjectTree: React.FC = ({ return () => { cancelled = true; }; - }, [dataConfig, dataSource, schema.filter, objectSchema, (rest as any).data]); + }, [dataConfig, dataSource, schema.filter, objectSchema, schemaSettled, (rest as any).data]); const config = useMemo(() => getTreeConfig(schema), [schema]); const parentField = useMemo( @@ -375,6 +509,18 @@ export const ObjectTree: React.FC = ({ return headerObjectName ? i18n.fieldLabel(headerObjectName, field, fallback) : fallback; }; + /** + * Everything {@link formatCellValue} needs for one column. Built per cell + * from the SAME `objectSchema.fields` map the header labels read, so a column + * cannot be labelled from the schema and then formatted without it. + */ + const cellContext = (field: string): CellFormatContext => ({ + fieldDef: objectSchema?.fields?.[field], + fieldName: field, + objectName: headerObjectName, + translateOptions: i18n.translateOptions, + }); + const navigation = useNavigationOverlay({ navigation: (schema as any).navigation, objectName: schema.objectName, @@ -461,7 +607,7 @@ export const ObjectTree: React.FC = ({ )} - {formatValue(node.record[config.labelField]) || '—'} + {formatCellValue(node.record[config.labelField], cellContext(config.labelField)) || '—'} @@ -469,7 +615,7 @@ export const ObjectTree: React.FC = ({ .filter((f) => f !== config.labelField) .map((f) => ( - {formatValue(node.record[f])} + {formatCellValue(node.record[f], cellContext(f))} ))} @@ -495,7 +641,7 @@ export const ObjectTree: React.FC = ({ {key.replace(/_/g, ' ')} - {formatValue(value) || '—'} + {formatCellValue(value, cellContext(key)) || '—'} ))}