From 50473025bb5de33ac4602f580df12ecbe6d585c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 00:21:43 +0000 Subject: [PATCH] fix(app-shell): RecordDetailView modal dispatch reports unresolvable targets instead of POSTing to /actions (#3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirror objectstack#3959 on the record page: type:'modal' is CLIENT-SIDE ONLY. The pre-#3959 fallthrough to serverActionHandler could never succeed — the framework rejects type:'modal' over REST with a 400 (headlessActionTypeError) — so an unresolvable target is now reported as the authoring error it is, with the same copy as the shared console runtime. Docblock rewritten so its parity claim is true again. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NVPjPzmmAJ2Ngtvgg5MSRa --- .../modal-dead-fallthrough-record-page.md | 5 + .../RecordDetailView.modalDispatch.test.tsx | 260 ++++++++++++++++++ .../app-shell/src/views/RecordDetailView.tsx | 32 ++- 3 files changed, 289 insertions(+), 8 deletions(-) create mode 100644 .changeset/modal-dead-fallthrough-record-page.md create mode 100644 packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx diff --git a/.changeset/modal-dead-fallthrough-record-page.md b/.changeset/modal-dead-fallthrough-record-page.md new file mode 100644 index 000000000..5f28c89e0 --- /dev/null +++ b/.changeset/modal-dead-fallthrough-record-page.md @@ -0,0 +1,5 @@ +--- +"@object-ui/app-shell": patch +--- + +RecordDetailView's `type:'modal'` dispatch no longer falls back to the server-side action handler when the target resolves to neither a page nor an object. That fallthrough could never succeed — the framework rejects `type:'modal'` over REST with a 400 (`headlessActionTypeError`) — so it only converted an authoring mistake into a confusing round-trip. The record page now reports the same descriptive authoring error as the shared console runtime (objectstack#3959), naming the action, the dud target, and the way out (`type:'script'` with `params`). diff --git a/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx b/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx new file mode 100644 index 000000000..6b9edf3e3 --- /dev/null +++ b/packages/app-shell/src/views/RecordDetailView.modalDispatch.test.tsx @@ -0,0 +1,260 @@ +/** + * 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. + */ + +/** + * RecordDetailView — `type: 'modal'` dispatch is CLIENT-SIDE ONLY + * (objectstack#3959 / objectui#3320). + * + * The record page wires its own `modal` handler into its ``, + * shadowing the shared console runtime for every action on the record surface. + * objectstack#3959 removed the "fall back to the server-side handler" branch + * from `useConsoleActionRuntime.modalActionHandler` — the framework's + * `headlessActionTypeError` rejects `type: 'modal'` over REST with a 400, so + * the fallthrough could never succeed and only converted an authoring mistake + * (a target naming no page) into a confusing round-trip. This copy kept the + * pre-#3959 shape, so the SAME dud modal action reported a descriptive + * authoring error on a list page and an opaque 400 on the record page. + * + * This file is the RecordDetailView-side counterpart of + * `useConsoleActionRuntime.test.tsx`'s "reports an unresolvable target instead + * of POSTing to /actions": it exercises the handler set the view REALLY hands + * to its ActionProvider (captured via a pass-through wrapper), so re-adding the + * fallthrough — `return serverActionHandler(action)` — turns it red. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, waitFor, act, cleanup } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +const authFetchSpy = vi.fn(async () => + new Response(JSON.stringify({ data: [] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), +); +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ user: { id: 'u1', name: 'Ada', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => authFetchSpy, +})); + +vi.mock('@object-ui/collaboration', () => ({ + 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(), + }), +})); + +// The dialogs / flow runner are orthogonal chrome; stubbing them keeps this +// file about the modal dispatch (same posture as the feed-signal tests). +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: () => {} }), +})); + +// The client modal transport is stubbed for the same reason as in +// useConsoleActionRuntime.test.tsx — importing it for real drags in +// and the whole plugin-form graph. `resolveTargetSpy` stands in +// for the page/object lookup so each case below chooses whether the target +// resolves; its real resolution rules are covered in +// useActionModal.resolve.test.tsx. +const modalHandlerSpy = vi.fn(async () => ({ success: true })); +const resolveTargetSpy = vi.fn(async (_schema: any): Promise => null); +vi.mock('../hooks/useActionModal', () => ({ + useActionModal: () => ({ + modalHandler: modalHandlerSpy, + modalElement: null, + closeModal: () => {}, + resolveModalTarget: resolveTargetSpy, + }), +})); + +// The server-side dispatch the dead fallthrough used to reach. Returns +// success so a re-added `return serverActionHandler(action)` flips BOTH the +// not-called assertion AND the `success: false` assertion below. +const serverActionSpy = vi.fn(async () => ({ success: true })); +vi.mock('../utils/consoleServerAction', () => ({ + createConsoleServerActionHandler: () => serverActionSpy, +})); + +// Capture the handler set each receives while KEEPING the +// real provider (children still get a working action context). The record +// page's own set is the only one carrying `approval`, so the capture below +// selects it even if a nested surface mounts a provider of its own. The page +// body itself is orthogonal here — SchemaRenderer is stubbed so the file +// stays about the wiring, not the render tree. +const capturedHandlers: Array Promise>> = []; +vi.mock('@object-ui/react', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + ActionProvider: (props: any) => { + capturedHandlers.push(props.handlers); + return React.createElement(actual.ActionProvider as any, props); + }, + SchemaRenderer: () => null, + }; +}); + +import { MetadataCtx } from '@object-ui/react'; +import { RecordDetailView } from './RecordDetailView'; + +const OBJECT_NAME = 'crm_call'; +const RECORD_ID = 'rec-call-1'; + +const OBJECTS = [ + { + name: OBJECT_NAME, + label: 'Call', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + }, + }, +]; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: [] })), + findOne: vi.fn(async () => ({ id: RECORD_ID, name: 'Intro call' })), + create: vi.fn(async () => ({})), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + } as any; +} + +const METADATA = { + objects: OBJECTS, + pages: [], + loading: false, + error: null, + refresh: async () => {}, + invalidate: () => {}, + ensureType: async () => [], + getItem: async () => null, + getItemsByType: () => [], +} as any; + +function renderDetail() { + return render( + + + {}} + objectNameOverride={OBJECT_NAME} + recordIdOverride={RECORD_ID} + embedded + /> + + , + ); +} + +/** The record page's OWN handler set — the only provider carrying `approval`. */ +function recordPageHandlers() { + return [...capturedHandlers].reverse().find((h) => h && 'approval' in h); +} + +/** Render the view and hand back its ActionProvider handlers, spies cleared. */ +async function mountAndCaptureHandlers() { + renderDetail(); + await waitFor(() => expect(recordPageHandlers()).toBeTruthy()); + const handlers = recordPageHandlers()!; + // Anything the mount itself fetched is not the dispatch under test. + authFetchSpy.mockClear(); + serverActionSpy.mockClear(); + resolveTargetSpy.mockClear(); + modalHandlerSpy.mockClear(); + return handlers; +} + +beforeEach(() => { + cleanup(); + capturedHandlers.length = 0; + authFetchSpy.mockClear(); + serverActionSpy.mockClear(); + modalHandlerSpy.mockClear(); + resolveTargetSpy.mockReset(); + resolveTargetSpy.mockResolvedValue(null); + // Unrelated chrome on this view (approvals, favourites, …) reaches for the + // platform API; in jsdom that is a real socket. Answer it locally so the + // only asynchrony left is the record load the capture waits on. + vi.stubGlobal( + 'fetch', + vi.fn(async () => + new Response(JSON.stringify({ data: [] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ), + ); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("RecordDetailView modal dispatch — CLIENT-SIDE ONLY (objectstack#3959 / objectui#3320)", () => { + it('opens the resolved target client-side and never POSTs to /actions', async () => { + const handlers = await mountAndCaptureHandlers(); + resolveTargetSpy.mockResolvedValue({ content: { name: 'log_call', type: 'utility' } }); + + let r: any; + await act(async () => { + r = await handlers.modal({ name: 'log_call', type: 'modal', target: 'log_call' }); + }); + + expect(resolveTargetSpy).toHaveBeenCalledWith('log_call'); + expect(modalHandlerSpy).toHaveBeenCalledWith({ content: { name: 'log_call', type: 'utility' } }); + expect(serverActionSpy).not.toHaveBeenCalled(); + expect(authFetchSpy).not.toHaveBeenCalled(); + expect(r).toMatchObject({ success: true }); + }); + + // The RecordDetailView-side counterpart of useConsoleActionRuntime.test.tsx's + // case of the same name. Until objectui#3320 this handler fell back to + // `serverActionHandler(action)`, POSTing the modal to /actions — which the + // framework rejects with a 400 (headlessActionTypeError) — while the shared + // runtime already reported the authoring error (objectstack#3959). + it('reports an unresolvable target instead of POSTing to /actions', async () => { + const handlers = await mountAndCaptureHandlers(); + resolveTargetSpy.mockResolvedValue(null); + + let r: any; + await act(async () => { + r = await handlers.modal({ + name: 'log_call', type: 'modal', target: 'log_call', params: { subject: 'Intro' }, + }); + }); + + expect(modalHandlerSpy).not.toHaveBeenCalled(); + expect(serverActionSpy).not.toHaveBeenCalled(); + expect(authFetchSpy).not.toHaveBeenCalled(); + expect(r.success).toBe(false); + // The message must name the action, the dud target, and the way out — + // the same copy the shared runtime reports, so the two surfaces cannot + // drift apart silently again. + expect(String(r.error)).toContain('log_call'); + expect(String(r.error)).toMatch(/type:'script' with params/); + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 1c9eeb48b..fa21e281c 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -831,19 +831,35 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri /** * `type: 'modal'` dispatch — same rule as the shared console runtime (see - * `useConsoleActionRuntime.modalActionHandler`): open `target` as a page (or - * an object form) when it names one, else fall through to the action's - * server-side handler. Registering it as a handler rather than relying on the - * runner's built-in `executeModal` is what gives the record page that server - * fallback, so a modal action bound to `engine.registerAction(...)` completes - * here exactly as it does on a list page. + * `useConsoleActionRuntime.modalActionHandler`): CLIENT-SIDE ONLY. The + * action's `target` names the page (or object form) to open; rendering it is + * the whole of what a modal action does. + * + * [objectstack#3959 / objectui#3320] This used to fall through to + * `serverActionHandler` when the target resolved to neither a page nor an + * object, "so a modal action bound to `engine.registerAction(...)` completes + * here exactly as it does on a list page". It never ran: the framework's + * `headlessActionTypeError` rejects `type: 'modal'` over REST with a 400, + * because a modal has no server dispatch — the fallthrough only converted an + * authoring mistake (a target naming no page) into a confusing round-trip. + * objectstack#3959 removed it from the shared runtime; this copy kept the + * pre-#3959 shape until objectui#3320. An unresolvable target is now + * reported as what it is. To collect input and then run server-side, declare + * `type: 'script'` with `params`: the runner collects the same dialog and + * the handler runs with those values. */ const modalActionHandler = useCallback(async (action: ActionDef) => { const schema = (action as any).modal ?? action.target ?? (action as any).params?.schema; const descriptor = schema != null ? await resolveModalTarget(schema) : null; if (descriptor) return modalHandler(descriptor); - return serverActionHandler(action); - }, [resolveModalTarget, modalHandler, serverActionHandler]); + return { + success: false, + error: + `Action "${action.name}" is type:'modal' but its target ` + + `${schema != null ? `"${String(schema)}" ` : ''}names no page or object to open. ` + + `Point it at a page, or use type:'script' with params to collect input and run a handler.`, + }; + }, [resolveModalTarget, modalHandler]); // ─── Approvals ───────────────────────────────────────────────────── // Since ADR-0019 an approval is a flow node: the flow opens the request,