diff --git a/.changeset/activity-map-single-reading-5878.md b/.changeset/activity-map-single-reading-5878.md new file mode 100644 index 0000000000..b04790a791 --- /dev/null +++ b/.changeset/activity-map-single-reading-5878.md @@ -0,0 +1,33 @@ +--- +'@object-ui/app-shell': minor +'@object-ui/plugin-detail': minor +--- + +The console record page and the `record:activity` block read ONE `sys_activity` +type table, so a scheduled meeting no longer appears on one and vanishes on the +other. + +`RecordDetailView`'s `sys_activity` merge carried a hand-written copy of the +table that `record:activity` exports as `ACTIVITY_TYPE_TO_FEED_TYPE`. Neither +file imported the other and nothing compared them, so the two could drift +silently — and they had. objectui#5840 added `scheduled` -> `event` to the +exported table, because a shipped producer (HotCRM's `schedule_meeting`) writes +that value and the row was being dropped before any filter ran. The copy here +was left untouched, so the same row rendered on a hand-authored record page and +was dropped on the console record page: same record, same row, two answers. + +`RecordDetailView` now imports the exported table, and the copy is gone. The +table is re-exported from `@object-ui/plugin-detail`'s entry point, which is +what makes a single reading possible at all — it was previously reachable only +from inside the plugin. No module enters the eager closure: the module holding +it was already pulled in by the renderer beside it. + +Adding an activity type is now one edit, in one place, that both surfaces see. +A re-fork is caught rather than merely discouraged: the new pins spy on the +shared object and inject a member into it at runtime, so a private copy holding +today's members exactly — the failure this change removes — fails, where a +value comparison would pass on the defect. + +Only the table converges. The row-to-`FeedItem` construction around it is still +written twice, so an unmapped activity type is still dropped silently on the +console surface where the block warns once; that mirror is filed separately. diff --git a/packages/app-shell/src/views/RecordDetailView.activityMapIdentity-5878.test.tsx b/packages/app-shell/src/views/RecordDetailView.activityMapIdentity-5878.test.tsx new file mode 100644 index 0000000000..5dd5b4df1e --- /dev/null +++ b/packages/app-shell/src/views/RecordDetailView.activityMapIdentity-5878.test.tsx @@ -0,0 +1,326 @@ +/** + * 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#5878 — the console record page's `sys_activity` merge reads + * `record:activity`'s table, instead of hand-copying it. + * + * `RecordDetailView` carried a local `activityTypeToFeed` literal holding the + * same eleven entries as `@object-ui/plugin-detail`'s exported + * `ACTIVITY_TYPE_TO_FEED_TYPE`. Neither file imported the other and no test + * compared them, so the two could drift with nothing going red — and + * objectui#5840 drifted them: it added `scheduled` -> `event` to the renderer's + * table because a shipped producer (HotCRM's `schedule_meeting`) writes that + * value, and left the copy here untouched. The same `sys_activity` row then + * rendered on a hand-authored record page and vanished on the console one. + * + * ## Why the load-bearing pin is IDENTITY, not membership + * + * Every behavioural assertion below is also satisfied by restoring a private + * literal that happens to hold today's members — i.e. by exactly the re-fork + * this change removed, which would pass ON the defect. So two pins decide the + * convergence, and neither can be satisfied by a copy: + * + * 1. a getter spy on the shared object: the merge's answer for `scheduled` is + * recorded only if it read THAT object's property; + * 2. a member injected into the shared object at runtime: a private copy + * cannot have it, so the seeded row is dropped and the leg goes red. + * + * (2) is deliberately mechanism-independent — it survives any change in how + * Vitest implements accessor spies — and it is also the card's thesis stated + * executably: ONE edit to the shared table must reach BOTH surfaces. + * + * The identity spy is real here because the specifier is identical on both + * sides. `vitest.config.mts` aliases `@object-ui/plugin-detail` to + * `packages/plugin-detail/src`, and both this file and `RecordDetailView` + * import through that specifier, so ESM hands them one module object. (No + * `dist` is involved, so no rebuild stands between an edit and this run.) + * + * ## The legs that do NOT discriminate, and why they are still here + * + * `describe('the previously mapped kinds are unchanged')` stays GREEN under + * both ablation legs by construction — a member-identical private copy answers + * those exactly as the shared table does. They are regression controls, not + * pins: if they move, the convergence took the mapping's membership with it + * and the pins above would be reporting on rubble. + * + * Same for the "the row rendered at all" assertion inside the identity legs: + * it is a LIVE control on the same read as the subject, so a harness that + * stopped delivering rows fails as a broken probe rather than reporting the + * consumer forked. + * + * ## Out of scope, deliberately + * + * Only the TABLE is shared. The row -> `FeedItem` construction around it is + * still written twice (this merge builds the item inline; the block calls + * `activityRowToFeedItem`), so the console surface still drops an unmapped + * type SILENTLY where the block warns once. That is the same class of mirror + * one level up, and it is not this card's fence. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import { MetadataCtx } from '@object-ui/react'; +import type { FeedItemType } from '@object-ui/types'; +import { ACTIVITY_TYPE_TO_FEED_TYPE } from '@object-ui/plugin-detail'; + +vi.mock('@object-ui/auth', () => ({ + useAuth: () => ({ user: { id: 'u1', name: 'Ada', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => vi.fn(), +})); + +vi.mock('@object-ui/collaboration', () => ({ + useRecordPresence: () => ({ viewers: [], others: [] }), + 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(), + }), +})); + +// Orthogonal chrome — stubbed so the only asynchrony in this file is the feed. +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: () => {} }), +})); + +import { RecordDetailView } from './RecordDetailView'; + +const OBJECT_NAME = 'crm_account'; +const RECORD_ID = 'rec-1'; + +/** + * A mutable-typed VIEW of the shared table — the same object, not a copy. + * The export is `Readonly<...>` at the type level only; `vi.spyOn` and the + * injected-member leg both need to redefine a property on it. + */ +const sharedTable = ACTIVITY_TYPE_TO_FEED_TYPE as Record; + +const OBJECTS = [ + { + name: OBJECT_NAME, + label: 'Account', + managedBy: 'platform', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + }, + }, +]; + +/** Body text names the activity type, so a failure message says which row moved. */ +const summaryFor = (type: string) => `activity row of type ${type}`; + +const activityRow = (type: string, index: number) => ({ + id: `a-${index}`, + type, + actor_name: 'Grace', + summary: summaryFor(type), + timestamp: `2026-01-0${index + 1}T00:00:00.000Z`, +}); + +/** + * A fake backend that answers `sys_activity` with the seeded rows and every + * other read with nothing — the same filter shape the view queries with, so a + * row can only arrive through the merge under test. + */ +function makeDataSource(activityRows: Array>) { + return { + find: vi.fn((objectName: string) => + Promise.resolve({ data: objectName === 'sys_activity' ? activityRows : [] }), + ), + create: vi.fn(async (_o: string, row: any) => row), + findOne: vi.fn(async (_o: string, recordId: string) => ({ + id: recordId, + name: `Record ${recordId}`, + })), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + } as any; +} + +function makeMetadata() { + return { + objects: OBJECTS, + pages: [], + loading: false, + error: null, + refresh: async () => {}, + invalidate: () => {}, + ensureType: async () => [], + getItem: async () => null, + getItemsByType: () => [], + } as any; +} + +function tree(dataSource: any) { + return ( + + + {}} + objectNameOverride={OBJECT_NAME} + recordIdOverride={RECORD_ID} + embedded + /> + + + ); +} + +/** Render the console record page over the seeded `sys_activity` rows. */ +const renderWith = (rows: Array>) => render(tree(makeDataSource(rows))); + +beforeEach(() => { + cleanup(); + // Unrelated chrome (approvals, favourites…) reaches for the platform API. + vi.stubGlobal( + 'fetch', + vi.fn(async () => + new Response(JSON.stringify({ data: [] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ), + ); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('the console record page renders a `scheduled` row (objectui#5878)', () => { + it('SUBJECT — a `scheduled` activity reaches this surface, like the block', async () => { + // The user-visible half of the divergence: HotCRM's `schedule_meeting` + // writes `scheduled`, objectui#5840 mapped it to `event` for the block, and + // until this convergence the console page dropped the very same row. + renderWith([activityRow('updated', 0), activityRow('scheduled', 1)]); + + expect(await screen.findByText(summaryFor('scheduled'))).toBeTruthy(); + // Live control on the same read — an ordinary row still arrives, so a + // missing `scheduled` above would mean "dropped", not "harness silent". + expect(screen.getByText(summaryFor('updated'))).toBeTruthy(); + }); + + it("the answer comes from plugin-detail's table, not a value that matches it", () => { + // Not a pin — a statement of where the reading lives, so the assertion + // above cannot be "fixed" by editing a second literal. + expect(ACTIVITY_TYPE_TO_FEED_TYPE.scheduled).toBe('event'); + }); +}); + +describe('the merge consults the SHARED object — the pins a copy cannot pass', () => { + it('IDENTITY (spy) — reading `scheduled` goes through the exported table', async () => { + // The real value is taken FROM the object rather than written here: an + // accessor spy on a data property drops the original, and re-typing + // `'event'` at this line would make the pin carry its own little copy of + // the thing it exists to stop. + const real = sharedTable.scheduled; + const spy = vi + .spyOn(sharedTable, 'scheduled', 'get') + .mockImplementation(() => real); + + try { + renderWith([activityRow('scheduled', 0)]); + // Live control first: the row rendered, so the merge definitely ran. + expect(await screen.findByText(summaryFor('scheduled'))).toBeTruthy(); + // SUBJECT: it ran through THIS object. A member-identical private copy + // leaves this empty while every value assertion above still passes. + expect( + spy.mock.calls.length, + 'the merge never read ACTIVITY_TYPE_TO_FEED_TYPE.scheduled — it is reading a copy', + ).toBeGreaterThan(0); + } finally { + spy.mockRestore(); + } + }); + + it('IDENTITY (injected member) — one edit to the shared table reaches this surface', async () => { + // The card's thesis, executable and independent of any spy mechanism: a + // type that exists ONLY on the shared object must render here. A private + // copy — however faithful to today's members — cannot contain it. + const PROBE_TYPE = 'os_probe_5878_not_a_real_activity_type'; + expect(Object.prototype.hasOwnProperty.call(sharedTable, PROBE_TYPE)).toBe(false); + + Object.defineProperty(sharedTable, PROBE_TYPE, { + value: 'event' satisfies FeedItemType, + configurable: true, + enumerable: true, + writable: true, + }); + + try { + renderWith([activityRow('updated', 0), activityRow(PROBE_TYPE, 1)]); + expect(await screen.findByText(summaryFor(PROBE_TYPE))).toBeTruthy(); + // Live control: an ordinary row on the same read. + expect(screen.getByText(summaryFor('updated'))).toBeTruthy(); + } finally { + Reflect.deleteProperty(sharedTable, PROBE_TYPE); + } + expect(Object.prototype.hasOwnProperty.call(sharedTable, PROBE_TYPE)).toBe(false); + }); +}); + +describe('the previously mapped kinds are unchanged — regression controls', () => { + // Green under BOTH ablation legs by construction: a member-identical private + // copy answers these exactly as the shared table does. They are here to catch + // a convergence that moved membership, not to decide the convergence. + it('every pre-#5840 entry still maps where it did', () => { + expect(ACTIVITY_TYPE_TO_FEED_TYPE).toMatchObject({ + created: 'field_change', + updated: 'field_change', + deleted: 'field_change', + assigned: 'field_change', + shared: 'field_change', + system: 'system', + completed: 'task', + }); + }); + + it('the deliberate exclusions are still excluded, and still PRESENT as keys', () => { + // Present-but-undefined is the distinction the block draws between "a + // decision" and "an unmapped producer"; losing the keys would turn four + // decisions into four unknowns. + for (const type of ['commented', 'mentioned', 'login', 'logout']) { + expect(Object.prototype.hasOwnProperty.call(ACTIVITY_TYPE_TO_FEED_TYPE, type), type).toBe(true); + expect(ACTIVITY_TYPE_TO_FEED_TYPE[type], type).toBeUndefined(); + } + }); + + it('the console surface still shows a mapped row and still drops the rest', async () => { + renderWith([ + activityRow('updated', 0), + activityRow('commented', 1), + activityRow('zzz_not_an_activity_type', 2), + ]); + + expect(await screen.findByText(summaryFor('updated'))).toBeTruthy(); + // `commented` is a deliberate exclusion (its content comes from + // `sys_comment`); the third is a type nothing maps. + await waitFor(() => { + expect(screen.queryByText(summaryFor('commented'))).toBeNull(); + expect(screen.queryByText(summaryFor('zzz_not_an_activity_type'))).toBeNull(); + }); + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index e2a8c03f3f..5ca8c1781f 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -10,7 +10,7 @@ import { useState, useEffect, useCallback, useMemo, useRef } from 'react'; import { useParams, useNavigate, useLocation, useSearchParams, Link } from 'react-router-dom'; -import { RecordChatterPanel, InlineEditSaveBar, buildDefaultPageSchema, deriveFieldGroupDetailSections, extractMentions, resolveTitleField, useRecordEditable } from '@object-ui/plugin-detail'; +import { ACTIVITY_TYPE_TO_FEED_TYPE, RecordChatterPanel, InlineEditSaveBar, buildDefaultPageSchema, deriveFieldGroupDetailSections, extractMentions, resolveTitleField, useRecordEditable } from '@object-ui/plugin-detail'; import { Empty, EmptyTitle, EmptyDescription } from '@object-ui/components'; import { useAuth, createAuthenticatedFetch } from '@object-ui/auth'; import { usePermissions } from '@object-ui/permissions'; @@ -1543,31 +1543,19 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri // true), so this surface gives us a Salesforce-style "what happened // on this record" feed without any per-app glue. // - // We map sys_activity.type to FeedItemType so the existing icon / - // colour map in RecordActivityTimeline keeps working: - // created/updated/deleted/system → 'field_change' - // assigned/shared → 'field_change' - // completed → 'task' - // commented/mentioned → 'comment' (but skipped — we - // already load these from - // sys_comment to get reactions - // and threading) + // The sys_activity.type -> FeedItemType reading is `record:activity`'s + // exported `ACTIVITY_TYPE_TO_FEED_TYPE` (@object-ui/plugin-detail), NOT a + // copy of it (objectui#5878). This merge and that block read the SAME rows + // of the SAME system table, so two tables can only ever be a bug: until + // this import, objectui#5840's `scheduled` -> `event` entry existed in the + // renderer and not here, and a scheduled meeting therefore rendered on a + // hand-authored record page and vanished on the console record page. The + // shared table's docblock carries which types map where and why + // `commented` / `mentioned` / `login` / `logout` deliberately map to + // nothing; do not restate it here, and never re-declare the table. // // sys_activity is system-owned so a 404 ("table not provisioned", // older schemas without activities) is silently tolerated. - const activityTypeToFeed: Record = { - created: 'field_change', - updated: 'field_change', - deleted: 'field_change', - assigned: 'field_change', - shared: 'field_change', - system: 'system', - completed: 'task', - commented: undefined, - mentioned: undefined, - login: undefined, - logout: undefined, - }; if (activitiesEnabled) inFlight.push(dataSource.find('sys_activity', { $filter: { object_name: objectName, record_id: pureRecordId }, $orderby: { timestamp: 'asc' }, @@ -1577,7 +1565,7 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri if (!res?.data?.length) return; const mapped: FeedItem[] = []; for (const row of res.data) { - const feedType = activityTypeToFeed[row.type]; + const feedType = ACTIVITY_TYPE_TO_FEED_TYPE[row.type]; if (!feedType) continue; // Prefer the explicit `timestamp` column, but tolerate older // rows where the driver leaked the literal "NOW()" — fall diff --git a/packages/plugin-detail/src/index.tsx b/packages/plugin-detail/src/index.tsx index 3d32ae9132..998efe5d1e 100644 --- a/packages/plugin-detail/src/index.tsx +++ b/packages/plugin-detail/src/index.tsx @@ -45,6 +45,29 @@ export { RecordReferenceRailRenderer, RecordAlertRenderer, }; +/** + * The `sys_activity.type` -> `FeedItem.type` reading, made importable + * (objectui#5878). + * + * It was already exported from `renderers/recordActivityFeed`, but not from + * this barrel — so `app-shell`'s `RecordDetailView`, which merges the SAME + * `sys_activity` rows into the console record page's feed, carried a + * hand-written copy of the table instead. Nothing failed when the two + * disagreed, and objectui#5840 made them disagree: `scheduled` -> `event` + * landed here and not there, so one row rendered on a hand-authored record + * page and vanished on the console one. + * + * Publishing it is what lets there be ONE reading. Adding a member is now a + * single edit at `ACTIVITY_TYPE_TO_FEED_TYPE`; re-forking it is caught by + * `RecordDetailView.activityMapIdentity-5878.test.tsx`, which spies on THIS + * object rather than comparing values (a member-identical private copy passes + * a value check, which is the whole failure mode being closed). + * + * No new module enters the eager closure: `renderers/record-activity`, already + * imported above, pulls `recordActivityFeed` in. + */ +export { ACTIVITY_TYPE_TO_FEED_TYPE } from './renderers/recordActivityFeed'; + export { RecordDetailDrawer, deriveRecordPageHref } from './RecordDetailDrawer'; export type { RecordDetailDrawerProps } from './RecordDetailDrawer'; export {