diff --git a/.changeset/9462-row-click-modifier-payload-forward.md b/.changeset/9462-row-click-modifier-payload-forward.md new file mode 100644 index 0000000000..002f501737 --- /dev/null +++ b/.changeset/9462-row-click-modifier-payload-forward.md @@ -0,0 +1,37 @@ +--- +'@object-ui/components': minor +'@object-ui/plugin-view': minor +'@object-ui/types': minor +--- + +Forward the row-click modifier payload through the three hops that were dropping it +(objectui#9462), so Cmd/Ctrl/middle-click on a row reaches a host handler. + +`useNavigationOverlay`'s `handleClick` has always invoked the handler it is given with +two arguments — the record, and the modifier payload (`metaKey` / `ctrlKey` / `button`) +a host needs to answer "open this record in a new tab". objectui#9360 made the hook's +option declare that payload and objectui#9460 made the component props on the path +declare it. Three hops in the middle received it and passed one argument on, so a host +wiring a modifier-click handler to `ObjectGrid`, `ListView` or `plugin-view`'s +`ObjectView` always read `undefined` as the second argument and the click silently +degraded to an ordinary navigation. + +**What changed.** `data-table`'s renderer now forwards the DOM event from both of its +call sites — the row's own click handler and the hover "open record" button, whose +handler had already bound the event as `e` for `stopPropagation` and still did not pass +it on. `plugin-view`'s `ObjectView` forwards it from `handleRowClick` to the component's +own `onRowClick` prop. The payload's contents are unchanged: this is a forward, not a +redefinition. + +**Declarations widened with the hops, and only those.** `DataTableSchema.onRowClick` +(`@object-ui/types`) and `ObjectViewProps.onRowClick` — plus the `onRowClick` that +`ObjectView` hands a host's `renderListView` — now declare +`(record, event?)`. `ObjectViewProps` is exported from `@object-ui/plugin-view`'s entry, +so this moves a published declaration. Source-compatible in both directions: a +one-parameter handler still satisfies the widened prop, and the second parameter is +spelled `any`, so a handler that annotated it `React.MouseEvent` is not refused +contravariantly. + +**Not in this change.** `ObjectDataTableSchema.onRowClick` (`@object-ui/types`) feeds +the same `data-table` channel and therefore now receives the payload too, while still +declaring one parameter. It is reported separately rather than widened here. diff --git a/packages/app-shell/src/__tests__/rowClickModifierPayloadForward-9462.test.tsx b/packages/app-shell/src/__tests__/rowClickModifierPayloadForward-9462.test.tsx new file mode 100644 index 0000000000..df0a85a532 --- /dev/null +++ b/packages/app-shell/src/__tests__/rowClickModifierPayloadForward-9462.test.tsx @@ -0,0 +1,229 @@ +/** + * 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. + */ + +/** + * objectui#9462 — the modifier payload SURVIVES every hop between + * `useNavigationOverlay.handleClick` and a host handler. + * + * `handleClick` invokes the handler it is given with TWO arguments — the + * record, and the modifier payload (`metaKey` / `ctrlKey` / `button`) a host + * needs to answer Cmd/Ctrl/middle-click with "open this record in a new tab". + * objectui#9360 made the hook's option declare that and objectui#9460 made the + * component props on the path declare it. Three hops in the middle RECEIVED the + * payload and dropped it, so the declarations were correct about what the hook + * delivers and still correct about what these hops delivered — which was + * nothing, and a host's second argument was always `undefined`. + * + * ## Why this file lives in `app-shell` + * + * The three subjects and the control sit in four different packages + * (`@object-ui/components`, `@object-ui/plugin-view`, `@object-ui/plugin-list`), + * and the control has to share the file — a red that cannot be told apart from + * a broken harness is half a measurement. `app-shell` is the one package that + * already depends on all of them, and it is also the real host: its own view + * code already spells its row-click handlers `(record: any, event?: any)`, + * which is the population this card is repairing the supply for. Putting the + * pin here needs no new dependency edge anywhere. + * + * ## THREE REDS, one per hop — measured on this branch with the fix reverted + * + * Each case names the hop it observes, because "the payload arrives" is true of + * all three at once only after all three are repaired: + * + * 1. the row's own click handler in `data-table`'s renderer, reached through + * `ObjectGrid` (which puts `navigation.handleClick` on the table node); + * 2. the hover "open record" button in the same renderer — the interesting + * one, because its handler already bound the DOM event as `e` to call + * `stopPropagation`, so the payload was IN SCOPE on that line and still was + * not passed on; + * 3. `plugin-view`'s `ObjectView`, whose `handleRowClick` is what the grid + * hands the hook and which forwarded one argument to the component's own + * `onRowClick` prop. + * + * ## THE CONTROL, in the same file and the same harness + * + * `ObjectGallery` already forwards — its card's `onClick` reads + * `navigation.handleClick(item, e)`. It is GREEN both before and after the + * change, which is what makes the three reds a reading about the three hops + * rather than about `fireEvent`, happy-dom or the hook. ⛔ Do not "simplify" it + * away: without it, a harness that never delivered `metaKey` at all would + * produce exactly the same three failures. + */ + +import React from 'react'; +import { describe, it, expect, vi, type Mock } from 'vitest'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom'; + +import { ObjectGrid } from '@object-ui/plugin-grid'; +import { ObjectView } from '@object-ui/plugin-view'; +import { ObjectGallery } from '@object-ui/plugin-list'; +import { ActionProvider, SchemaRendererProvider } from '@object-ui/react'; +import type { DataSource } from '@object-ui/types'; + +const rows = [ + { id: 'r1', name: 'Alice', amount: 100 }, + { id: 'r2', name: 'Bob', amount: 200 }, +]; + +const objectSchema = { + label: 'Test object', + fields: { + id: { name: 'id', type: 'text', label: 'Id' }, + name: { name: 'name', type: 'text', label: 'Name' }, + amount: { name: 'amount', type: 'number', label: 'Amount' }, + }, +}; + +function makeDataSource(): DataSource { + return { + find: vi.fn().mockResolvedValue(rows), + findOne: vi.fn().mockResolvedValue(rows[0]), + create: vi.fn().mockResolvedValue({}), + update: vi.fn().mockResolvedValue({}), + delete: vi.fn().mockResolvedValue({}), + getObjectSchema: vi.fn().mockResolvedValue(objectSchema), + } as unknown as DataSource; +} + +/** The host-facing shape all four subjects declare, once. */ +type HostRowClick = (record: Record, event?: unknown) => void; + +/** + * What every case reads: the SECOND argument of the host handler's first call. + * Spelled once so the three reds and the control are literally the same + * assertion pointed at four different components. + */ +function secondArgumentOf(spy: Mock): unknown { + expect(spy, 'the host handler was never called — the probe did not reach it').toHaveBeenCalled(); + return spy.mock.calls[0][1]; +} + +/** A grid that owns its rows inline, with a navigation config, as the card names it. */ +function renderGrid(onRowClick: Mock) { + const schema: any = { + type: 'object-grid', + objectName: 'test_object', + columns: [ + { field: 'name', label: 'Name' }, + { field: 'amount', label: 'Amount', type: 'number' }, + ], + navigation: { mode: 'drawer' }, + data: { provider: 'value', items: rows }, + }; + return render( + + + , + ); +} + +describe('objectui#9462 — RED 1: the data-table row click hands the host the modifier payload', () => { + it('a ⌘-click on a row reaches the host with the event, not with `undefined`', async () => { + const onRowClick = vi.fn(); + const { container } = renderGrid(onRowClick); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + const row = container.querySelector('tbody tr'); + expect(row, 'no data row rendered — the harness, not the subject, is broken').not.toBeNull(); + + fireEvent.click(row as Element, { metaKey: true }); + + const event = secondArgumentOf(onRowClick) as { metaKey?: boolean } | undefined; + expect(event, 'the row handler called `schema.onRowClick(row)` — one argument').toBeDefined(); + expect(event!.metaKey).toBe(true); + // The record still arrives first and unchanged: this card forwards a second + // argument, it does not re-shape the first. + expect(onRowClick.mock.calls[0][0]).toMatchObject({ id: 'r1' }); + }); +}); + +describe('objectui#9462 — RED 2: the hover "open record" button forwards the event it already bound', () => { + it('a Ctrl-click on the row-expand button reaches the host with the event', async () => { + const onRowClick = vi.fn(); + renderGrid(onRowClick); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + const expand = await screen.findAllByTestId('row-expand-button'); + + fireEvent.click(expand[0], { ctrlKey: true }); + + const event = secondArgumentOf(onRowClick) as { ctrlKey?: boolean } | undefined; + expect( + event, + 'the button handler called `schema.onRowClick?.(row)` — the DOM event was bound as `e` for `stopPropagation` and dropped', + ).toBeDefined(); + expect(event!.ctrlKey).toBe(true); + }); +}); + +describe("objectui#9462 — RED 3: ObjectView's own onRowClick prop receives the payload", () => { + it('the handler the view hands its list view forwards both arguments to the host', async () => { + const onRowClick = vi.fn(); + const dataSource = makeDataSource(); + + render( + + ( + + )} + /> + , + ); + + const trigger = await screen.findByTestId('custom-list-row'); + fireEvent.click(trigger, { metaKey: true }); + + const event = secondArgumentOf(onRowClick) as { metaKey?: boolean } | undefined; + expect(event, "`handleRowClick` called `onRowClick(record)` — the view truncated the call").toBeDefined(); + expect(event!.metaKey).toBe(true); + expect(onRowClick.mock.calls[0][0]).toMatchObject({ id: 'r1' }); + }); +}); + +describe('objectui#9462 — CONTROL: ObjectGallery already forwarded, and still does', () => { + it('a ⌘-click on a gallery card reaches the host with the event (green before and after)', async () => { + const onRowClick = vi.fn(); + const dataSource = makeDataSource(); + + render( + + + , + ); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + const cards = screen.getAllByRole('listitem'); + fireEvent.click(cards[0], { metaKey: true }); + + const event = secondArgumentOf(onRowClick) as { metaKey?: boolean } | undefined; + expect( + event, + 'the control is RED — the probe itself is broken, so the three cases above measure nothing', + ).toBeDefined(); + expect(event!.metaKey).toBe(true); + }); +}); diff --git a/packages/components/src/renderers/complex/data-table.tsx b/packages/components/src/renderers/complex/data-table.tsx index 1cadfff6f8..d286d5e6d2 100644 --- a/packages/components/src/renderers/complex/data-table.tsx +++ b/packages/components/src/renderers/complex/data-table.tsx @@ -2309,7 +2309,15 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { ) { return; } - schema.onRowClick(row); + // objectui#9462 — the DOM event goes with the row. + // `ObjectGrid` and `ListView` put the navigation + // hook's own `handleClick` on this slot, and that + // hook reads `metaKey` / `ctrlKey` / `button` off a + // second argument to decide "open in a new tab". + // Calling with one argument dropped the payload + // here, so Cmd/Ctrl/middle-click reached no host + // handler and degraded to an ordinary navigation. + schema.onRowClick(row, e); } }} > @@ -2342,7 +2350,14 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { data-testid="row-expand-button" onClick={(e) => { e.stopPropagation(); - schema.onRowClick?.(row); + // objectui#9462 — same forward as the row's own + // handler above. `e` was already bound here for + // `stopPropagation`, so the payload was in + // scope on this line and still was not handed + // on: the hover "open record" affordance + // answered a Cmd/Ctrl-click exactly like a + // plain one. + schema.onRowClick?.(row, e); }} title="Open record" > diff --git a/packages/plugin-view/src/ObjectView.tsx b/packages/plugin-view/src/ObjectView.tsx index 76fcb70cee..ebe80b3abf 100644 --- a/packages/plugin-view/src/ObjectView.tsx +++ b/packages/plugin-view/src/ObjectView.tsx @@ -394,8 +394,20 @@ export interface ObjectViewProps { /** * Callback when a row is clicked (for record detail navigation) + * + * TWO parameters since objectui#9462, and the second is not decoration. The + * view's own `handleRowClick` is what `ObjectGrid` hands to + * `useNavigationOverlay`, and that hook invokes it as + * `onRowClick(record, event)` — the modifier payload (`metaKey` / `ctrlKey` / + * `button`) a host needs to implement Cmd/Ctrl/middle-click for itself. + * `handleRowClick` used to forward one argument, so this prop delivered + * nothing on the second and a host's "open in a new tab" degraded to an + * ordinary navigation. Spelled `any` and not `HandleClickModifiers` for the + * reason objectui#9341 measured on `ObjectKanbanSchema.onCardClick`: a host + * that discovered the payload from the implementation annotated it + * `React.MouseEvent`, which a narrower declaration refuses contravariantly. */ - onRowClick?: (record: Record) => void; + onRowClick?: (record: Record, event?: any) => void; /** * Callback when edit is triggered on a record @@ -417,7 +429,13 @@ export interface ObjectViewProps { schema: any; dataSource: DataSource; onEdit?: (record: Record) => void; - onRowClick?: (record: Record) => void; + /** + * The view's own `handleRowClick`, verbatim — so TWO parameters since + * objectui#9462, for the same reason {@link ObjectViewProps.onRowClick} + * carries them: a custom list view that has a DOM click event in hand may + * pass it, and it reaches the host through the forward below. + */ + onRowClick?: (record: Record, event?: any) => void; className?: string; /** Current refresh counter — increment signals that a mutation occurred */ refreshKey?: number; @@ -1151,9 +1169,17 @@ export const ObjectView: React.FC = ({ }, [layout, schema]); // Handle row click - respects NavigationConfig - const handleRowClick = useCallback((record: Record) => { + // + // objectui#9462 — `event` is forwarded, not consumed. This callback is what + // `ObjectGrid` feeds to `useNavigationOverlay`, and `handleClick` invokes it + // with the modifier payload; truncating to `onRowClick(record)` here meant a + // host wired to this component's own prop never saw it. The branches below + // deliberately do NOT read it: what Cmd/Ctrl/middle-click should do when no + // host handler is present is the hook's own decision, taken before this + // callback runs. + const handleRowClick = useCallback((record: Record, event?: any) => { if (onRowClick) { - onRowClick(record); + onRowClick(record, event); return; } diff --git a/packages/react/src/hooks/__tests__/navigationOverlayConsumers.onRowClickArity-9357.test.ts b/packages/react/src/hooks/__tests__/navigationOverlayConsumers.onRowClickArity-9357.test.ts index 414094b219..ebae30b6cf 100644 --- a/packages/react/src/hooks/__tests__/navigationOverlayConsumers.onRowClickArity-9357.test.ts +++ b/packages/react/src/hooks/__tests__/navigationOverlayConsumers.onRowClickArity-9357.test.ts @@ -26,9 +26,22 @@ * ⛔ A declaration is NOT in scope merely because it is spelled with one * parameter and is named `onRowClick`. The census this card was handed counted * 24 one-parameter occurrences and that number is a POPULATION, not a defect - * count. The `CONTROLS` block below pins five of them as deliberately OUT, each - * for a different reason, and they are controls in the strict sense: they share - * the file, the name and the shape with the IN sites and vary only the claim. + * count. The `CONTROLS` block below pins the ones that are deliberately OUT, + * each for a different reason, and they are controls in the strict sense: they + * share the file, the name and the shape with the IN sites and vary only the + * claim. + * + * ⭐ THE LEDGER MOVED ONCE, AND THAT IS THE HANDSHAKE THIS FILE WAS BUILT FOR + * (objectui#9462). Two of the original controls were OUT on a claim about + * BEHAVIOUR, not about spelling: the `data-table` renderer and `ObjectView`'s + * `handleRowClick` each received the payload and passed one argument on, so + * widening their declarations would have promised what those hops never + * delivered. objectui#9462 repaired the hops — the renderer's two call sites + * now read `schema.onRowClick(row, e)` and the view forwards + * `onRowClick(record, event)` — which made the same declarations understated + * instead of accurate, so they moved into `IN_SITES` below rather than being + * edited in place. The one control still spelled with one parameter in that + * family (`ObjectDataTableSchema.onRowClick`) states its own, different reason. * * ## Why the instrument is bytes and not assignability * @@ -241,6 +254,22 @@ const IN_SITES: Array<{ rel: string; member: string; why: string; hop: RegExp; a { rel: 'packages/plugin-kanban/src/index.tsx', member: 'onCardClick', why: 'handed to KanbanImpl, whose SortableCard invokes it with the DOM event', hop: /onCardClick=\{schema\.onCardClick\}/ }, + // ⭐ The three entries below arrived by objectui#9462, which repaired the + // hops rather than the spellings. Each was a CONTROL until that card: they + // were OUT because the hop that CALLS them passed one argument, so widening + // them would have declared a payload that never arrived. The hops now pass + // the payload, so the same declarations understate a real call and the + // criterion at the top of this file puts them IN. The `hop` of each is the + // forwarding call itself, quoted in the declaring file. + { rel: 'packages/types/src/data-display.ts', member: 'onRowClick', + why: "the slot `data-table`'s renderer calls; ObjectGrid, ObjectDataTable and RelatedList all supply it", + hop: /`schema\.onRowClick\(row, e\)`/ }, + { rel: 'packages/plugin-view/src/ObjectView.tsx', member: 'onRowClick', + why: "the view's own host-facing prop; `handleRowClick` forwards both arguments to it", + hop: /onRowClick\(record, event\)/, after: 'export interface ObjectViewProps' }, + { rel: 'packages/plugin-view/src/ObjectView.tsx', member: 'onRowClick', + why: "the same `handleRowClick`, handed verbatim to a host's custom list view through `renderListView`", + hop: /onRowClick: handleRowClick/, after: ' renderListView?: (props: {' }, ]; /** @@ -254,17 +283,24 @@ const OUT_SITES: Array<{ rel: string; member: string; arity: number; param: RegE param: /^index: number$/, why: 'its second parameter is `index: number` — a DIFFERENT contract, not this one' }, { rel: 'packages/plugin-view/src/ManageViewsDialog.tsx', member: 'onRowClick', arity: 1, param: /^id: string$/, why: 'invoked as `onRowClick?.(view.id)` — a view id, not a record callback at all' }, - { rel: 'packages/types/src/data-display.ts', member: 'onRowClick', arity: 1, - param: /^row: any$/, why: '`data-table` invokes `schema.onRowClick(row)` with ONE argument; the declaration is accurate, and widening it would promise a payload that renderer never hands over' }, { rel: 'packages/types/src/objectql.ts', member: 'onRowClick', arity: 1, - param: /^row: any$/, why: 'ObjectDataTable forwards it into the same `data-table` channel above', + param: /^row: any$/, + // ⚠️ RE-JUSTIFIED, not left standing, by objectui#9462. This entry used to + // read "ObjectDataTable forwards it into the same `data-table` channel + // above" — an appeal to the `data-display.ts` control, which that card + // moved into IN_SITES. The channel now hands over two arguments, so this + // declaration DOES understate a real call and is a live instance of + // objectui#9357's own class, one hop further out. It stays OUT only + // because objectui#9462 was scope-pinned to three hops and the widening + // here is a separate published-face change; it is reported on that card + // rather than smuggled in. ⇒ this is a KNOWN GAP under measurement, not a + // declaration anyone has argued is accurate. + why: 'ObjectDataTable forwards it into the `data-table` channel, which now delivers the payload — an understated declaration deliberately left to its own card, not an accurate one', // Anchored since objectui#7804: `ObjectGallerySchema.onRowClick` now stands // EARLIER in this same file with the opposite contract, so a file-scoped // read would answer about the IN site and score this control green for the // wrong declaration. after: 'export interface ObjectDataTableSchema' }, - { rel: 'packages/plugin-view/src/ObjectView.tsx', member: 'onRowClick', arity: 1, - param: /^record: Record$/, why: 'its own `handleRowClick` truncates to `onRowClick(record)`; that hop DROPS the payload, which is a separate defect from an understated declaration and is reported rather than fixed here' }, ]; describe('objectui#9357 — the arity counter, before it is pointed at the tree', () => { diff --git a/packages/types/src/data-display.ts b/packages/types/src/data-display.ts index dfd838aa22..591f90256d 100644 --- a/packages/types/src/data-display.ts +++ b/packages/types/src/data-display.ts @@ -1378,13 +1378,26 @@ export interface DataTableSchema extends BaseSchema { * function, NOT authorable metadata: JSON has no function value, so the zod * twin refuses this key by name and points at the node-type spelling. Kept * callable here because it is called by `renderers/complex/data-table.tsx` - * (`schema.onRowClick(row)`, gated on `!e.defaultPrevented`). THREE suppliers, - * measured: `ObjectGrid` passes `navigation.handleClick`, `ObjectDataTable` - * passes `schema.onRowClick ?? handleRowClick` (its own forwarding face, - * `ObjectDataTableSchema.onRowClick`, carries the same refusal arm), and - * `RelatedList` forwards its own React prop of this name. - */ - onRowClick?: (row: any) => void; + * (`schema.onRowClick(row, e)`, gated on `!e.defaultPrevented`). THREE + * suppliers, measured: `ObjectGrid` passes `navigation.handleClick`, + * `ObjectDataTable` passes `schema.onRowClick ?? handleRowClick` (its own + * forwarding face, `ObjectDataTableSchema.onRowClick`, carries the same + * refusal arm), and `RelatedList` forwards its own React prop of this name. + * + * TWO parameters since objectui#9462, and the second is the reason that card + * exists: the renderer's two call sites — the row's own click handler and the + * hover "open record" button, `schema.onRowClick?.(row, e)` — each had the + * DOM event in hand and passed only the row. The suppliers above route this + * slot straight into `useNavigationOverlay`'s `handleClick`, which reads + * `metaKey` / `ctrlKey` / `button` off that second argument, so a host wiring + * Cmd/Ctrl/middle-click received `undefined` and "open in a new tab" silently + * became an ordinary navigation. Spelled `any` and not `HandleClickModifiers` + * for the reason objectui#9341 measured on `ObjectKanbanSchema.onCardClick`: + * that interface lives in `@object-ui/react`, which depends on THIS package, + * so naming it here is a phantom dependency and closes a cycle. `BaseSchema`'s + * own `onClick` / `onChange` / `onSubmit` already use this spelling. + */ + onRowClick?: (row: any, event?: any) => void; /** * Dynamic row class name * Function that returns a CSS class string for each row