From 6b1a3ff64651d95171559f2e8602e8e5c41eb296 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 19:23:23 +0000 Subject: [PATCH] fix(plugin-detail): name the actor of an activity row that carries only actor_id (objectui#12067) On @objectstack/* 17.7.0 the audit writer fills sys_activity.actor_id and never actor_name, so record:history read every such entry as "Unknown user" and the activity feed (record:activity, and the console record page's merged feed) read it as "System". The three sys_activity reads now pass $expand: ['actor_id'], so the engine batch-loads the page's sys_user rows in the same request. One reader, activityActorName, names the actor: the actor_name snapshot when present, else the expanded actor_id's name, else the existing fallback. A bare id (a user the viewer may not read) is never shown. Claude-Session: https://claude.ai/code/session_01CGZy1BGCjdN5cXqL9cnvB8 Co-authored-by: Claude --- .changeset/12067-history-actor-id.md | 6 + content/docs/plugins/plugin-detail.mdx | 8 + ...dDetailView.activityActorId-12067.test.tsx | 176 ++++++++++++++++++ .../app-shell/src/views/RecordDetailView.tsx | 5 + .../record-history.actorId-12067.test.tsx | 127 +++++++++++++ .../recordActivityFeed.actorId-12067.test.tsx | 89 +++++++++ .../src/renderers/record-activity.tsx | 3 + .../src/renderers/record-history.tsx | 7 +- .../src/renderers/recordActivityFeed.ts | 35 +++- 9 files changed, 454 insertions(+), 2 deletions(-) create mode 100644 .changeset/12067-history-actor-id.md create mode 100644 packages/app-shell/src/views/RecordDetailView.activityActorId-12067.test.tsx create mode 100644 packages/plugin-detail/src/renderers/__tests__/record-history.actorId-12067.test.tsx create mode 100644 packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.actorId-12067.test.tsx diff --git a/.changeset/12067-history-actor-id.md b/.changeset/12067-history-actor-id.md new file mode 100644 index 0000000000..7d4d51c055 --- /dev/null +++ b/.changeset/12067-history-actor-id.md @@ -0,0 +1,6 @@ +--- +'@object-ui/plugin-detail': patch +'@object-ui/app-shell': patch +--- + +A record's History tab and activity feed name the user behind an activity row that carries `actor_id` and no `actor_name` (objectui#12067). On `@objectstack/*` 17.7.0 the audit writer fills only `actor_id`, so `record:history` showed every such entry as "Unknown user", and the `record:activity` block and the console record page's activity feed showed the change as made by "System". Their `sys_activity` reads now expand `actor_id`, which brings each user's record back with the page in the same request, and the entry shows that user's `name`. `actor_name`, when present, still wins: it is the name recorded when the action happened. A row with neither, or whose user the viewer may not read, keeps the existing fallback, and a raw user id is never shown as a name. diff --git a/content/docs/plugins/plugin-detail.mdx b/content/docs/plugins/plugin-detail.mdx index 12f9c27726..1c3f1af242 100644 --- a/content/docs/plugins/plugin-detail.mdx +++ b/content/docs/plugins/plugin-detail.mdx @@ -260,6 +260,14 @@ lost data — the row is on the timeline; what it lacks is the icon and colour a mapped type gets. Give it one by adding the type to `ACTIVITY_TYPE_TO_FEED_TYPE` in `@object-ui/plugin-detail`. +Each item names who acted. The row's `actor_name`, the name recorded when the +action happened, wins when it is present. A row without one is named by its +`actor_id`, a lookup to `sys_user`: the block's read expands it, so each user's +record comes back with the page in the same request and the item shows that +user's `name`. A row with neither, or whose user the viewer may not read, shows +"System". `record:history` names the entries of its own `sys_activity` read the +same way, and falls back to "Unknown user". + The rows that really are dropped are the four the map lists as non-activity — `commented` / `mentioned` / `login` / `logout` — and they go silently on purpose: each is a decision already taken, and a warning about a decision diff --git a/packages/app-shell/src/views/RecordDetailView.activityActorId-12067.test.tsx b/packages/app-shell/src/views/RecordDetailView.activityActorId-12067.test.tsx new file mode 100644 index 0000000000..ed8507a044 --- /dev/null +++ b/packages/app-shell/src/views/RecordDetailView.activityActorId-12067.test.tsx @@ -0,0 +1,176 @@ +/** + * 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#12067 — the console record page's activity feed names the user + * behind a `sys_activity` row that carries `actor_id` and no `actor_name`. + * + * On `@objectstack/*` 17.7.0 the audit writer fills `actor_id` (a lookup to + * `sys_user`) and never `actor_name` (objectstack#22510). The page builds its + * feed items with `activityRowToFeedItem`, which read `actor_name` alone, so + * every such row read "System". The constructor now names the expanded + * `actor_id`, and this page's `sys_activity` read asks for that expansion. + * + * The data source answers the way objectql does: a read that passes + * `$expand: ['actor_id']` gets the user's record in place of the id, and a + * read without it gets the bare id, so the pin goes red if the page stops + * asking. + */ + +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'; + +vi.mock('@object-ui/auth', async (importOriginal) => ({ + ...(await importOriginal>()), + useAuth: () => ({ user: { id: 'u-me', name: 'Viewer', image: null }, activeOrganization: null }), + createAuthenticatedFetch: () => vi.fn(), +})); + +vi.mock('@object-ui/collaboration', async (importOriginal) => ({ + ...(await importOriginal>()), + 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(), + }), +})); + +// 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_lead'; +const RECORD_ID = 'rec-1'; +const ADA = { id: 'u-ada', name: 'Ada Lovelace' }; + +const ACTIVITY_ROWS = [ + { + id: 'a-1', + type: 'updated', + summary: 'Stage changed', + timestamp: '2026-01-02T00:00:00.000Z', + actor_id: 'u-ada', + }, +]; + +function makeDataSource() { + return { + find: vi.fn((objectName: string, params: { $expand?: string[] } = {}) => + Promise.resolve({ + data: + objectName === 'sys_activity' + ? ACTIVITY_ROWS.map((r) => + params.$expand?.includes('actor_id') ? { ...r, actor_id: ADA } : { ...r }, + ) + : [], + }), + ), + 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; +} + +const OBJECTS = [ + { + name: OBJECT_NAME, + label: 'Lead', + managedBy: 'platform', + fields: { + id: { type: 'text', label: 'Id' }, + name: { type: 'text', label: 'Name' }, + }, + }, +]; + +function makeMetadata() { + return { + objects: OBJECTS, + pages: [], + loading: false, + error: null, + refresh: async () => {}, + invalidate: () => {}, + ensureType: async () => [], + getItem: async () => null, + getItemsByType: () => [], + } as any; +} + +beforeEach(() => { + cleanup(); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + // 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 record page feed names the actor of an id-only activity row (objectui#12067)', () => { + it('reads `sys_activity` with `actor_id` expanded and shows the user, not "System"', async () => { + const dataSource = makeDataSource(); + render( + + + {}} + objectNameOverride={OBJECT_NAME} + recordIdOverride={RECORD_ID} + embedded + /> + + , + ); + + expect(await screen.findByText('Stage changed')).toBeTruthy(); + expect(screen.getByText('Ada Lovelace')).toBeTruthy(); + expect(screen.queryByText('System')).toBeNull(); + + await waitFor(() => { + const reads = dataSource.find.mock.calls.filter((c: unknown[]) => c[0] === 'sys_activity'); + expect(reads).toHaveLength(1); + expect(reads[0][1].$expand).toEqual(['actor_id']); + }); + }); +}); diff --git a/packages/app-shell/src/views/RecordDetailView.tsx b/packages/app-shell/src/views/RecordDetailView.tsx index 515b7e6998..82c83eb15b 100644 --- a/packages/app-shell/src/views/RecordDetailView.tsx +++ b/packages/app-shell/src/views/RecordDetailView.tsx @@ -1970,6 +1970,11 @@ export function RecordDetailView({ dataSource, objects, onEdit, objectNameOverri $filter: { object_name: objectName, record_id: pureRecordId }, $orderby: { timestamp: 'asc' }, $top: 200, + // The actor's record in place of its id, so a row with no `actor_name` + // names its user instead of reading "System" (objectui#12067). The + // shared constructor reads it; the `record:activity` block's own read + // passes the same expand. + $expand: ['actor_id'], }) .then((res: any) => { recordRefusal('activity', false); diff --git a/packages/plugin-detail/src/renderers/__tests__/record-history.actorId-12067.test.tsx b/packages/plugin-detail/src/renderers/__tests__/record-history.actorId-12067.test.tsx new file mode 100644 index 0000000000..56264c2152 --- /dev/null +++ b/packages/plugin-detail/src/renderers/__tests__/record-history.actorId-12067.test.tsx @@ -0,0 +1,127 @@ +/** + * 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. + */ + +/** + * `record:history` names the user behind a row that carries `actor_id` and no + * `actor_name` (objectui#12067). + * + * On `@objectstack/*` 17.7.0 the audit writer fills `sys_activity.actor_id` (a + * lookup to `sys_user`) and never `actor_name` (objectstack#22510 fixes the + * writer from then on). The self-fetch mapped the entry's user from + * `actor_name` alone, so every such row read "Unknown user". + * + * The data source below answers the way objectql does: rows store the bare + * user id, and a read that passes `$expand: ['actor_id']` gets each user's + * record in place of the id, or keeps the bare id for a user the viewer may + * not read. A read without the expand gets bare ids only, so the name pins go + * red when the renderer stops asking for it. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { RecordContextProvider } from '@object-ui/react'; +import { RecordHistoryRenderer } from '../record-history'; + +const UNKNOWN_USER = 'Unknown user'; + +/** The `sys_user` rows this viewer may read. */ +const USERS: Record = { + 'u-ada': { id: 'u-ada', name: 'Ada Lovelace' }, + 'u-grace': { id: 'u-grace', name: 'Grace Hopper' }, +}; + +type Row = Record; + +const row = (id: string, extra: Row): Row => ({ + id, + type: 'updated', + summary: `summary of ${id}`, + timestamp: '2026-01-02T00:00:00.000Z', + ...extra, +}); + +/** One `find()` that expands `actor_id` the way the engine does, or does not. */ +function makeDataSource(rows: Row[]) { + return { + find: vi.fn(async (_object: string, params: { $expand?: string[] } = {}) => { + const expand = params.$expand?.includes('actor_id') ?? false; + return { + data: rows.map((r) => { + const id = r.actor_id; + if (!expand || typeof id !== 'string') return { ...r }; + return { ...r, actor_id: USERS[id] ?? id }; + }), + }; + }), + } as any; +} + +function mount(rows: Row[], schema: Record = {}) { + const dataSource = makeDataSource(rows); + render( + + + , + ); + return dataSource; +} + +beforeEach(() => { + cleanup(); +}); + +describe('record:history names the actor of a row with only `actor_id` (objectui#12067)', () => { + it("a row with only `actor_id` renders that user's name", async () => { + mount([row('h-1', { actor_id: 'u-ada' })]); + expect(await screen.findByText('Ada Lovelace')).toBeTruthy(); + expect(screen.queryByText(UNKNOWN_USER)).toBeNull(); + }); + + it('a row with both renders `actor_name`, the snapshot taken at the time of the action', async () => { + mount([row('h-1', { actor_id: 'u-ada', actor_name: 'Ada (at the time)' })]); + expect(await screen.findByText('Ada (at the time)')).toBeTruthy(); + expect(screen.queryByText('Ada Lovelace')).toBeNull(); + }); + + it('a row with neither renders the existing fallback, and so does an id the viewer may not read', async () => { + mount( + [ + row('h-1', {}), + // The engine keeps the bare id when the user cannot be read. + row('h-2', { actor_id: 'u-not-readable' }), + ], + { unknownUserText: 'Someone' }, + ); + await screen.findByText('summary of h-1'); + expect(screen.getAllByText('Someone')).toHaveLength(2); + // A raw id is never shown as the actor's name. + expect(screen.queryByText('u-not-readable')).toBeNull(); + }); + + it('a page of N id-only rows issues one read, not one per row', async () => { + const rows = Array.from({ length: 6 }, (_, i) => + row(`h-${i}`, { actor_id: i % 2 === 0 ? 'u-ada' : 'u-grace' }), + ); + const dataSource = mount(rows); + + expect(await screen.findAllByText('Ada Lovelace')).toHaveLength(3); + expect(screen.getAllByText('Grace Hopper')).toHaveLength(3); + await waitFor(() => expect(dataSource.find).toHaveBeenCalled()); + // The names arrive on the page's own read: one `sys_activity` find that + // expands `actor_id`, and no `sys_user` read at all. + const objects = dataSource.find.mock.calls.map((c: unknown[]) => c[0]); + expect(objects).toEqual(['sys_activity']); + expect(dataSource.find.mock.calls[0][1].$expand).toEqual(['actor_id']); + }); +}); diff --git a/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.actorId-12067.test.tsx b/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.actorId-12067.test.tsx new file mode 100644 index 0000000000..c3d0cfe64a --- /dev/null +++ b/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.actorId-12067.test.tsx @@ -0,0 +1,89 @@ +/** + * 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 activity feed names the user behind a row that carries `actor_id` and no + * `actor_name` (objectui#12067), instead of calling that user "System". + * + * Measured on this tree before the fix: `activityRowToFeedItem` mapped + * `actor: row.actor_name ?? systemActorLabel`, so the 17.7.0 row (`actor_id` + * set, no `actor_name`, objectstack#22510) read "System", the label for a + * change no person made. The `record:activity` block's self-fetch and the + * console record page's merged feed both build their items with it. + * + * The rule is `activityActorName`, the one `record:history` uses: the + * `actor_name` snapshot, else the expanded `actor_id`'s `name`, else the + * existing fallback. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { RecordContextProvider } from '@object-ui/react'; +import { activityRowToFeedItem } from '../recordActivityFeed'; +import { RecordActivityRenderer } from '../record-activity'; + +const ADA = { id: 'u-ada', name: 'Ada Lovelace' }; + +const base = { id: 'a-1', type: 'updated', summary: 'Stage changed' }; + +beforeEach(() => { + cleanup(); +}); + +describe('activityRowToFeedItem — who acted (objectui#12067)', () => { + it("a row with only an expanded `actor_id` names that user", () => { + expect(activityRowToFeedItem({ ...base, actor_id: ADA }, 'System')?.actor).toBe('Ada Lovelace'); + }); + + it('a row with both keeps `actor_name`, the snapshot taken at the time of the action', () => { + expect( + activityRowToFeedItem({ ...base, actor_name: 'Ada (at the time)', actor_id: ADA }, 'System')?.actor, + ).toBe('Ada (at the time)'); + }); + + it('a row with neither, or with an id the engine could not expand, keeps the existing fallback', () => { + expect(activityRowToFeedItem({ ...base }, 'System')?.actor).toBe('System'); + expect(activityRowToFeedItem({ ...base, actor_name: ' ' }, 'System')?.actor).toBe('System'); + // A bare id is what the engine leaves when the viewer may not read the user. + expect(activityRowToFeedItem({ ...base, actor_id: 'u-hidden' }, 'System')?.actor).toBe('System'); + }); +}); + +describe('record:activity self-fetch expands `actor_id` (objectui#12067)', () => { + it('names the actor of id-only rows from the page read, with no read per row', async () => { + const rows = [ + { ...base, id: 'a-1', timestamp: '2026-01-02T00:00:00.000Z', actor_id: 'u-ada' }, + { ...base, id: 'a-2', timestamp: '2026-01-03T00:00:00.000Z', actor_id: 'u-ada' }, + ]; + const dataSource = { + find: vi.fn(async (_object: string, params: { $expand?: string[] } = {}) => ({ + data: rows.map((r) => + params.$expand?.includes('actor_id') ? { ...r, actor_id: ADA } : { ...r }, + ), + })), + } as any; + + render( + + + , + ); + + expect(await screen.findAllByText('Ada Lovelace')).toHaveLength(2); + expect(screen.queryByText('System')).toBeNull(); + await waitFor(() => expect(dataSource.find).toHaveBeenCalled()); + expect(dataSource.find.mock.calls.map((c: unknown[]) => c[0])).toEqual(['sys_activity']); + expect(dataSource.find.mock.calls[0][1].$expand).toEqual(['actor_id']); + }); +}); diff --git a/packages/plugin-detail/src/renderers/record-activity.tsx b/packages/plugin-detail/src/renderers/record-activity.tsx index 0f5cbdfdf6..6a0a39846e 100644 --- a/packages/plugin-detail/src/renderers/record-activity.tsx +++ b/packages/plugin-detail/src/renderers/record-activity.tsx @@ -213,6 +213,9 @@ export const RecordActivityRenderer: React.FC = ({ $orderby: { timestamp: 'desc' }, // One row past the window so "Load more" can be offered honestly. $top: Math.max(1, pageSize) + 1, + // The actor's record in place of its id, for a row with no + // `actor_name` (objectui#12067). See `activityActorName`. + $expand: ['actor_id'], }), ) .then((res: any) => { diff --git a/packages/plugin-detail/src/renderers/record-history.tsx b/packages/plugin-detail/src/renderers/record-history.tsx index baa1324e45..8d5e6edc10 100644 --- a/packages/plugin-detail/src/renderers/record-history.tsx +++ b/packages/plugin-detail/src/renderers/record-history.tsx @@ -21,6 +21,7 @@ import React from 'react'; import { useRecordContext } from '@object-ui/react'; import { HistoryTimeline, type HistoryEntry } from '../HistoryTimeline'; +import { activityActorName } from './recordActivityFeed'; const splitDesigner = (props: Record) => { const { 'data-obj-id': id, 'data-obj-type': type, style, ...rest } = props || {}; @@ -172,6 +173,10 @@ export const RecordHistoryRenderer: React.FC = ({ $filter: { object_name: objectName, record_id: recordId }, $orderby: { timestamp: 'desc' }, $top: limit, + // Name the actor on this one read (objectui#12067): the engine + // batch-loads the page's `sys_user` rows and puts each record in place + // of its id. `activityActorName` reads it; never a request per row. + $expand: ['actor_id'], }), ) .then((res: any) => { @@ -192,7 +197,7 @@ export const RecordHistoryRenderer: React.FC = ({ id: r.id, created_at: when, action: r.type, - user_name: r.actor_name ?? null, + user_name: activityActorName(r), user_avatar: r.actor_avatar_url ?? null, summary: r.summary ?? null, } as HistoryEntry; diff --git a/packages/plugin-detail/src/renderers/recordActivityFeed.ts b/packages/plugin-detail/src/renderers/recordActivityFeed.ts index 91c9fdb914..e649c13c6d 100644 --- a/packages/plugin-detail/src/renderers/recordActivityFeed.ts +++ b/packages/plugin-detail/src/renderers/recordActivityFeed.ts @@ -741,6 +741,37 @@ export function resetUnknownActivityTypeWarnings(): void { warnedUnknownActivityTypes.clear(); } +/** + * The name of whoever acted on a `sys_activity` row, or `null` when the row + * names nobody this viewer can name (objectui#12067). One reading for every + * surface that shows these rows: `record:history`, and through + * {@link activityRowToFeedItem} the `record:activity` block and the console + * record page's merged feed. + * + * 1. `actor_name`, when it is non-blank. It is the snapshot the writer took at + * the time of the action, so it wins even over the user's current name. + * 2. `actor_id`, a lookup to `sys_user`. A read that passes + * `$expand: ['actor_id']` gets the user's record in place of the id, and + * that record's `name` (`sys_user`'s declared name field) names the actor. + * The engine expands every row of a page in one batched read. This is the + * row the `@objectstack/*` 17.7.0 writer leaves: `actor_id` set and no + * `actor_name` (objectstack#22510 fills `actor_name` from then on). + * 3. `null`, so the caller shows its existing fallback. That covers a row + * with neither, and an `actor_id` the engine kept as the bare id because + * this viewer may not read that user or the user is gone. A raw id is + * never shown as a name. + */ +export function activityActorName(row: SysActivityRow): string | null { + const snapshot = row?.actor_name; + if (typeof snapshot === 'string' && snapshot.trim()) return snapshot.trim(); + const actor = row?.actor_id; + if (actor && typeof actor === 'object') { + const name = (actor as { name?: unknown }).name; + if (typeof name === 'string' && name.trim()) return name.trim(); + } + return null; +} + /** * One `sys_activity` row → one {@link FeedItem}, or `null` when the row is not * record activity (see {@link ACTIVITY_TYPE_TO_FEED_TYPE}). @@ -770,7 +801,9 @@ export function activityRowToFeedItem( return { id: row.id as string | number, type: feedType, - actor: row.actor_name ?? systemActorLabel, + // `systemActorLabel` only when no person can be named (objectui#12067): + // a row a user made used to read "System" whenever `actor_name` was empty. + actor: activityActorName(row) ?? systemActorLabel, actorAvatarUrl: row.actor_avatar_url ?? undefined, body: row.summary ?? '', createdAt: activityTimestamp(row),