From 5ed30e2e240fe8c481456f4cafe9919da2f4fba5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 00:28:41 +0000 Subject: [PATCH] fix(plugin-detail): copy object-valued cells as JSON, not `[object Object]` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DetailSection`'s click-to-copy built its payload with `String(value)`, and `String()` of an object is the literal text `[object Object]`. The affordance is offered for every non-empty value — objects included — so a reader clicking the copy button, the row, or pressing Enter on an address, geolocation, JSON, file, expanded-lookup, repeater or image cell silently got a placeholder on the clipboard, while the cell beside the button rendered that same value correctly. Objects are now serialized with `JSON.stringify`. Non-objects keep `String()` byte-for-byte: a number still copies `16` (not the rendered `16.00`), a percent `0.123` (not `12%`), a date `2026-03-04` (not `Mar 4`). Copying the RENDERED text was measured and rejected — it is the worse contract for 9 of 17 field types and loses data silently (an `image` or `boolean` cell renders no text at all, so it would copy the empty string). The JSON blob is a defensible default, not a settled contract; the read site says so in place. Card: objectui#8395 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- .changeset/8395-detail-copy-object-values.md | 34 ++ packages/plugin-detail/src/DetailSection.tsx | 56 ++- ...tailSection.copyObjectValues-8395.test.tsx | 370 ++++++++++++++++++ 3 files changed, 459 insertions(+), 1 deletion(-) create mode 100644 .changeset/8395-detail-copy-object-values.md create mode 100644 packages/plugin-detail/src/__tests__/DetailSection.copyObjectValues-8395.test.tsx diff --git a/.changeset/8395-detail-copy-object-values.md b/.changeset/8395-detail-copy-object-values.md new file mode 100644 index 0000000000..26c1f942e0 --- /dev/null +++ b/.changeset/8395-detail-copy-object-values.md @@ -0,0 +1,34 @@ +--- +'@object-ui/plugin-detail': minor +--- + +`DetailSection`'s click-to-copy no longer writes `[object Object]` for object-valued +cells (objectui#8395). + +The copy affordance built its payload with `String(value)`, and `String()` of an object +is the literal text `[object Object]`. Because the affordance is offered for every +non-empty value — objects included — a reader clicking the copy button, the row, or +pressing Enter on an **address**, **geolocation**, **JSON**, **file/attachment**, +**expanded lookup**, **repeater** or **image** cell silently got that placeholder on the +clipboard, while the cell beside the button rendered the same value correctly. Nothing +errored; it was noticed only on paste. + +Objects are now serialized with `JSON.stringify`, so those cells copy the stored value +losslessly and parseably. + +**Non-objects are byte-identical.** A number still copies `16` (not the rendered +`16.00`), a currency `1234.5` (not `1,234.50`), a percent `0.123` (not `12%`), a date +`2026-03-04` (not `Mar 4`), and a select its stored `won` (not `Closed Won`). Copying +the *rendered* text was measured and rejected: it is the worse contract for 9 of 17 +field types and loses data silently. + +**One payload moves without having been broken:** a multiselect stored as +`['alpha','beta']` copied `alpha,beta` and now copies `["alpha","beta"]` — an array is +an object. The new form is lossless where the old one was ambiguous for any value +containing a comma. + +The JSON blob is a **defensible default, not a settled contract**. What an object cell +*should* copy (a formatted postal address, `lat, lng`, a filename) is a per-kind product +question tracked separately as objectui#8395's option B; the read site says so in place +so the default is not mistaken for the answer. The `password` / `secret` branch of the +same handler is untouched here — it is its own open card. diff --git a/packages/plugin-detail/src/DetailSection.tsx b/packages/plugin-detail/src/DetailSection.tsx index c8f72e5ed1..014ec9dc9f 100644 --- a/packages/plugin-detail/src/DetailSection.tsx +++ b/packages/plugin-detail/src/DetailSection.tsx @@ -216,8 +216,62 @@ export const DetailSection: React.FC = ({ */ const serverFieldErrors = useInlineEdit()?.fieldErrors ?? null; + /** + * What the copy affordance WRITES (objectui#8395). + * + * `String(value)` on an object is the literal text `[object Object]`, so + * every object-valued cell — address, geolocation, JSON, file, expanded + * lookup, repeater — silently put a placeholder on the clipboard, while the + * cell BESIDE the button rendered that same value correctly. Objects are + * serialized as JSON here instead: lossless, parseable, never + * `[object Object]`. + * + * ## The JSON blob is a DEFENSIBLE DEFAULT, NOT A SETTLED CONTRACT + * + * What an object cell *should* put on the clipboard is a product question + * with several defensible answers per kind — the formatted postal address + * the reader can see, `lat, lng` for a geolocation, a filename for a file, + * the option labels or the stored values for a multiselect. That contract is + * objectui#8395's OPTION B (a shared value-to-text formatter that REUSES the + * cell renderers' own formatters — `formatAddress`, objectui#4037 — rather + * than re-spelling them), and it is a SEPARATE, still-unspecified card. + * ⛔ Do not read this line as the answer to it. + * + * ## Two shapes were measured and REJECTED — do not reach for them + * + * Copying the cell's RENDERED text is the worse contract for 9 of 17 field + * types and loses data silently: `date` renders `Mar 4` (the year is gone), + * `percent` renders `12%` against a stored `0.123` (a different quantity), + * `datetime` concatenates to an unparseable `3/4/20265:06 am`, and `image` + * and `boolean` render no text at all — so it would copy the empty string, + * which is strictly worse than the defect it set out to fix. And withdrawing + * the affordance from text-less cells would narrow `canCopy` away from + * `hasCellValue`, whose three readers MUST agree (see its docblock above). + * + * ## The non-regression half + * + * Non-objects keep `String(value)` BYTE-FOR-BYTE: a number still copies + * `16`, never the rendered `16.00`; a select still copies its stored `won`, + * never the rendered `Closed Won`. Pinned per kind — both halves — in + * `__tests__/DetailSection.copyObjectValues-8395.test.tsx`. + */ const handleCopyField = React.useCallback((fieldName: string, value: any) => { - const textValue = value !== null && value !== undefined ? String(value) : ''; + let textValue: string; + if (value === null || value === undefined) { + textValue = ''; + } else if (typeof value === 'object') { + // The same guard `JsonCellRenderer` already applies to this exact + // operation on this exact value: a structure `JSON.stringify` cannot + // represent (a cycle) keeps today's string form rather than throwing out + // of a click handler, and the row stays consistent with its own cell. + try { + textValue = JSON.stringify(value); + } catch { + textValue = String(value); + } + } else { + textValue = String(value); + } navigator.clipboard.writeText(textValue).then(() => { setCopiedField(fieldName); setTimeout(() => setCopiedField(null), 2000); diff --git a/packages/plugin-detail/src/__tests__/DetailSection.copyObjectValues-8395.test.tsx b/packages/plugin-detail/src/__tests__/DetailSection.copyObjectValues-8395.test.tsx new file mode 100644 index 0000000000..5b129e47fc --- /dev/null +++ b/packages/plugin-detail/src/__tests__/DetailSection.copyObjectValues-8395.test.tsx @@ -0,0 +1,370 @@ +/** + * 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. + */ + +/** + * What `DetailSection`'s click-to-copy WRITES, per field kind (objectui#8395). + * + * ## The defect + * + * `handleCopyField` built its payload with `String(value)`, and `String()` of an + * object is the literal text `[object Object]`. The affordance is offered for + * every non-empty value (`canCopy` is `hasCellValue`, objectui#8376) — objects + * included — so a reader clicking the copy button, the row, or pressing Enter on + * an address, geolocation, JSON, file, expanded-lookup, repeater or image cell + * got a placeholder on the clipboard **while the cell beside the button rendered + * that same value correctly**. Nothing errors; it is noticed only on paste. + * + * ## What this file pins — BOTH halves + * + * 1. **The fix.** Every object-valued kind copies parseable JSON of the STORED + * value. Each such case also asserts the cell RENDERED its value, because + * "the payload is not `[object Object]`" is satisfied by a cell that rendered + * nothing and copied nothing — an implementation strictly worse than the bug. + * Nothing here asserts a negation; every row asserts its exact payload. + * 2. ⭐ **The non-regression.** The scalar kinds that were already correct stay + * BYTE-IDENTICAL, and each of those cases asserts the rendered text too, so + * the pin is red for any future "just copy what the cell renders" rewrite. + * That shape was measured and rejected on this card: it is the worse contract + * for 9 of 17 field types (`date` renders `Mar 4` — the year is gone; + * `percent` renders `12%` against a stored `0.123`; `image` and `boolean` + * render no text at all, so they would copy the empty string). + * + * ## The contract status of the JSON blob + * + * A JSON blob is a DEFENSIBLE DEFAULT, not a settled contract — see the read + * site's docblock. The per-kind contract (a formatted postal address, `lat, + * lng`, a filename …) is objectui#8395's option B, a separate card. These cases + * pin today's answer so a future option-B change is a deliberate edit here. + * + * ## Instruments + * + * - What is observed is the ARGUMENT to `navigator.clipboard.writeText`, spied + * through `Object.defineProperty(navigator, 'clipboard', …)` — the pattern + * `app-shell/src/console/organizations/__tests__/acceptInvitationLink.mount.test.tsx` + * already uses. There is no `execCommand` fallback on this path. + * - ⚠️ The clickable row is found by `[role="button"]`, NEVER by + * `querySelector('button')`: `BooleanCellRenderer` renders a Radix Checkbox, + * which IS a `button` element, so that selector matches the checkbox instead + * of the copy button and the click copies nothing. Pinned below. + * - `window.innerWidth` is 1280: `DetailSection` has mobile and desktop row + * shapes and an `isMobile` auto-hide threshold. + * - Presence is read through `queryByText` into `expect(value, message)`, never + * `getByText`, so a missing row fails with the reason rather than with + * `TestingLibraryElementError` before `expect` runs. + * + * ## Deliberately NOT pinned here + * + * The `password` / `secret` branch of this same handler — copy writes the raw + * secret while the cell renders the mask — is objectui#8440, in the maintainer's + * decision box. This card leaves that path exactly as it found it, and asserts + * nothing about it, so whichever way #8440 is answered no case here turns red. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; +import * as React from 'react'; +import { DetailSection } from '../DetailSection'; +import type { DetailViewSection } from '@object-ui/types'; + +let writeText: ReturnType; + +beforeAll(() => { + Object.defineProperty(window, 'innerWidth', { configurable: true, value: 1280 }); +}); + +beforeEach(() => { + writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, 'clipboard', { configurable: true, value: { writeText } }); +}); + +afterEach(cleanup); + +const objectSchema = { + fields: { + // — object-valued kinds: the defect — + billing_address: { type: 'address', label: 'Billing Address' }, + office_location: { type: 'location', label: 'Office Location' }, + payload: { type: 'json', label: 'Payload' }, + contract: { type: 'file', label: 'Contract' }, + owner_ref: { type: 'lookup', label: 'Owner', reference_to: 'account' }, + attachments: { type: 'file', label: 'Attachments', multiple: true }, + line_items: { type: 'repeater', label: 'Line Items' }, + logo: { type: 'image', label: 'Logo' }, + tags: { + type: 'multiselect', + label: 'Tags', + options: [ + { value: 'alpha', label: 'Alpha' }, + { value: 'beta', label: 'Beta' }, + ], + }, + // — scalar kinds: already correct, must not move — + amount: { type: 'number', label: 'Amount', scale: 2 }, + price: { type: 'currency', label: 'Price' }, + ratio: { type: 'percent', label: 'Ratio' }, + due: { type: 'date', label: 'Due' }, + stage: { + type: 'select', + label: 'Stage', + options: [{ value: 'won', label: 'Closed Won' }], + }, + name: { type: 'text', label: 'Name' }, + active: { type: 'boolean', label: 'Active' }, + }, +}; + +/** + * Stored values. The geolocation carries MORE precision than the cell displays + * (the cell rounds to 4 decimals) so the payload assertions below cannot be + * satisfied by re-deriving the payload from the rendered text. + */ +const DATA: Record = { + billing_address: { + // `postalCode`, the spec spelling (objectstack#5143). `formatAddress` reads + // that key and the legacy `zipCode` only, so an off-spec `postal_code` + // would be dropped from the RENDERED line while still riding along in the + // JSON payload — which would make the control below weaker than it looks. + street: '1 Main St', + city: 'Springfield', + state: 'IL', + postalCode: '62704', + country: 'USA', + }, + office_location: { latitude: 30.2741567, longitude: 120.1551234 }, + payload: { a: 1, b: ['x', 'y'] }, + contract: { name: 'contract.pdf', url: 'https://cdn.example.com/contract.pdf' }, + owner_ref: { id: 'acct-1', name: 'Acme Corp' }, + attachments: [{ name: 'a.pdf' }, { name: 'b.pdf' }], + line_items: [{ label: 'one' }, { label: 'two' }], + logo: { name: 'logo.png', url: 'https://cdn.example.com/logo.png' }, + tags: ['alpha', 'beta'], + amount: 16, + price: 1234.5, + ratio: 0.123, + due: '2026-03-04', + stage: 'won', + name: 'Plain String Value', + active: true, +}; + +/** + * The row LABEL is the section field's own `label` (`field.label || field.name` + * at the render site), so it is spelled here rather than inherited from + * `objectSchema` — which supplies the TYPE and the options, not the label. + */ +const section = { + title: 'Details', + fields: Object.entries(objectSchema.fields).map(([name, def]) => ({ + name, + label: (def as { label: string }).label, + })), +} as DetailViewSection; + +const renderAll = () => + render(); + +/** + * The clickable row for a field, located by its LABEL and taken by + * `[role="button"]` — see the docblock on why not `querySelector('button')`. + */ +const rowOf = (label: string): HTMLElement => { + const labelEl = screen.queryByText(label); + expect(labelEl, `CONTROL: a row labelled "${label}" is on screen`).not.toBeNull(); + const row = (labelEl as HTMLElement).parentElement!.querySelector('[role="button"]'); + expect(row, `the "${label}" row offers the copy affordance`).not.toBeNull(); + return row as HTMLElement; +}; + +/** Click the row and return the exact argument handed to `writeText`. */ +const copyFrom = (label: string): unknown => { + writeText.mockClear(); + fireEvent.click(rowOf(label)); + expect( + writeText.mock.calls.length, + `clicking the "${label}" row wrote to the clipboard exactly once`, + ).toBe(1); + return writeText.mock.calls[0][0]; +}; + +describe('DetailSection copy payloads — object cells copy JSON, scalars are unchanged (#8395)', () => { + /** + * The seven object-valued kinds whose cell renders TEXT. `rendered` is the + * control: it proves the cell drew the value, so no row here can pass on a + * cell that rendered nothing. + */ + const OBJECT_KINDS = [ + { + label: 'Billing Address', + rendered: '1 Main St, Springfield, IL 62704, USA', + clipboard: + '{"street":"1 Main St","city":"Springfield","state":"IL","postalCode":"62704","country":"USA"}', + }, + { + label: 'Office Location', + rendered: '30.2742, 120.1551', + clipboard: '{"latitude":30.2741567,"longitude":120.1551234}', + }, + { + label: 'Payload', + rendered: '{"a":1,"b":["x","y"]}', + clipboard: '{"a":1,"b":["x","y"]}', + }, + { + label: 'Contract', + rendered: 'contract.pdf', + clipboard: '{"name":"contract.pdf","url":"https://cdn.example.com/contract.pdf"}', + }, + { + label: 'Owner', + rendered: 'Acme Corp', + clipboard: '{"id":"acct-1","name":"Acme Corp"}', + }, + { + label: 'Attachments', + rendered: '2 files', + clipboard: '[{"name":"a.pdf"},{"name":"b.pdf"}]', + }, + { + // The repeater cell prints a COUNT. Only the count is asserted: the label + // beside it is a hardcoded Chinese string (objectui#8441), and pinning + // that spelling here would turn this file red when #8441 fixes it. + label: 'Line Items', + rendered: '2', + clipboard: '[{"label":"one"},{"label":"two"}]', + }, + ]; + + it.each(OBJECT_KINDS)( + 'OBJECT — $label copies parseable JSON of the stored value, not `[object Object]`', + ({ label, rendered, clipboard }) => { + renderAll(); + const row = rowOf(label); + + // CONTROL — the cell drew the value. Without this, the payload assertion + // below would also pass for a cell that rendered and copied nothing. + expect( + row.textContent, + `CONTROL: the "${label}" cell rendered its value`, + ).toContain(rendered); + + expect(copyFrom(label), `"${label}" copies the stored value as JSON`).toBe(clipboard); + // Parseability is the property the JSON default was chosen for, so it is + // asserted rather than implied by the string above. + expect(() => JSON.parse(clipboard), `"${label}"'s payload parses`).not.toThrow(); + }, + ); + + it('OBJECT — an image cell copies its JSON even though it renders no text', () => { + // The image cell renders an , so its `textContent` is ''. Its control + // is therefore the element, not text — and it is exactly the row that would + // copy the EMPTY STRING under a "copy the rendered text" rewrite. + renderAll(); + const row = rowOf('Logo'); + + expect( + row.querySelector('img'), + 'CONTROL: the "Logo" cell rendered an for its value', + ).not.toBeNull(); + expect(row.textContent, 'the "Logo" cell renders no text at all').toBe(''); + + expect(copyFrom('Logo'), '"Logo" copies the stored file object as JSON').toBe( + '{"name":"logo.png","url":"https://cdn.example.com/logo.png"}', + ); + }); + + it('OBJECT — a multiselect copies its stored values as a JSON array', () => { + // ⚠️ DECLARED MOVE. An array is an object, so this payload changes from + // `alpha,beta` (`String(['alpha','beta'])`) to a JSON array. It is the one + // kind whose payload moves without having been `[object Object]` today: the + // new form is lossless where the old one was ambiguous for any value + // containing a comma. The LABELS ("Alpha", "Beta") are deliberately NOT what + // is copied — the stored values are. + renderAll(); + const row = rowOf('Tags'); + + expect(row.textContent, 'CONTROL: the "Tags" cell rendered its option badges').toBe( + 'AlphaBeta', + ); + expect(copyFrom('Tags'), '"Tags" copies the stored values, not the labels').toBe( + '["alpha","beta"]', + ); + }); + + /** + * ⭐ The non-regression half. `rendered` here is what the reader SEES and + * `clipboard` is what they GET — the two differ on purpose for four of these + * five rows, which is why "copy the rendered text" is not the contract. + */ + const UNCHANGED_SCALARS = [ + { label: 'Amount', rendered: '16.00', clipboard: '16' }, + { label: 'Price', rendered: '1,234.50', clipboard: '1234.5' }, + { label: 'Ratio', rendered: '12%', clipboard: '0.123' }, + { label: 'Due', rendered: 'Mar 4', clipboard: '2026-03-04' }, + { label: 'Stage', rendered: 'Closed Won', clipboard: 'won' }, + { label: 'Name', rendered: 'Plain String Value', clipboard: 'Plain String Value' }, + ]; + + it.each(UNCHANGED_SCALARS)( + 'SCALAR — $label still copies its stored value byte-for-byte', + ({ label, rendered, clipboard }) => { + renderAll(); + const row = rowOf(label); + + expect( + row.textContent, + `CONTROL: the "${label}" cell rendered its value as ${rendered}`, + ).toContain(rendered); + + expect(copyFrom(label), `"${label}" copies the STORED value, not the rendered one`).toBe( + clipboard, + ); + }, + ); + + it('SCALAR — a boolean copies `true`, and the copy target is not the row\'s first button', () => { + // ⚠️ The instrument pin. `BooleanCellRenderer` renders a Radix Checkbox, + // which IS a `button`, so `querySelector('button')` in this row returns the + // checkbox and clicking it copies nothing. Anyone writing a copy test for + // this surface must click `[role="button"]`, as `rowOf` does. + renderAll(); + const row = rowOf('Active'); + + const firstButton = row.querySelector('button'); + expect(firstButton, 'CONTROL: the boolean row does contain a `button` element').not.toBeNull(); + expect( + firstButton!.getAttribute('role'), + 'that first `button` is the checkbox, NOT the copy button', + ).toBe('checkbox'); + + expect(copyFrom('Active'), 'the boolean row copies its stored value').toBe('true'); + }); + + it('a value JSON cannot represent keeps its string form instead of throwing', () => { + // NOT a payload contract — the guard `JsonCellRenderer` applies to the same + // operation on the same value. A cycle cannot be serialized; retaining + // today's `[object Object]` for it keeps the click handler from throwing, + // and it is the ONLY input for which that text is still written. + const cyclic: Record = { self: null }; + cyclic.self = cyclic; + + render( + , + ); + + expect(() => copyFrom('Payload'), 'clicking must not throw out of the handler').not.toThrow(); + expect(writeText.mock.calls[0][0], 'an unserializable value keeps its string form').toBe( + '[object Object]', + ); + }); +});