diff --git a/.changeset/record-activity-scheduled-event-5840.md b/.changeset/record-activity-scheduled-event-5840.md new file mode 100644 index 0000000000..011e1447b6 --- /dev/null +++ b/.changeset/record-activity-scheduled-event-5840.md @@ -0,0 +1,17 @@ +--- +"@object-ui/plugin-detail": patch +--- + +`record:activity` no longer discards scheduled activities. `sys_activity` rows with +`type: "scheduled"` now map to the `event` feed kind instead of being dropped before any +filter runs, so a not-yet-held meeting reaches the timeline and `types: ['event']` stops +being a permanently empty tab. Held meetings are unchanged: `completed` still maps to +`task` and still hides unless `showCompleted` is set, which is what keeps an upcoming +meeting visible by default while a finished one is not. + +The unknown-type default is unchanged and deliberately so — a row whose type nothing maps +is still dropped rather than bucketed into `system`, because rendering an unmeasured kind +as something it is not would be new wrong data rather than recovered data. What changes is +that the drop is no longer silent: an unmapped type now logs one `console.warn` naming it, +once per type. Types the map knows and deliberately excludes (`commented`, `mentioned`, +`login`, `logout`) stay silent, since those are decisions rather than gaps. diff --git a/content/docs/plugins/plugin-detail.mdx b/content/docs/plugins/plugin-detail.mdx index 55cdb841b5..d717f29e20 100644 --- a/content/docs/plugins/plugin-detail.mdx +++ b/content/docs/plugins/plugin-detail.mdx @@ -196,12 +196,28 @@ Three sources, in precedence order: rows per page. This is what makes it work on a hand-authored record page with no host feeding it. -`sys_activity` rows map to feed items the same way the console's record page -maps them, so both surfaces agree about what a row is: +`sys_activity` rows map to feed items like this: `created` / `updated` / `deleted` / `assigned` / `shared` → `field_change`, -`completed` → `task`, `system` → `system`. `commented` / `mentioned` are -skipped (their content lives in `sys_comment`), and `login` / `logout` are -account events, not record activity. +`completed` → `task`, `scheduled` → `event`, `system` → `system`. +`commented` / `mentioned` are skipped (their content lives in `sys_comment`), +and `login` / `logout` are account events, not record activity. + +`scheduled` and `completed` are the two branches the same producer writes for a +meeting, and they land at different default visibility on purpose: a held +meeting is `completed` → `task`, hidden unless `showCompleted`; a not-yet-held +meeting is `scheduled` → `event`, shown, because an upcoming meeting is the part +of a timeline you still act on. + +A row whose `type` is in neither list is dropped and logs one `console.warn` +naming the type — `sys_activity.type` is not validated on write, so a producer +can store a value the platform never declared, and a silently missing row is +indistinguishable from no activity at all. + + + The console's own record page maps these rows with a second, hand-written copy + of the table, so the two surfaces agree on everything except `scheduled`, + which that page still drops. Tracked in `objectui#5878`. + ### Which inputs do what diff --git a/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts b/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts index 991be4ada6..d9b544ed3c 100644 --- a/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts +++ b/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts @@ -15,7 +15,7 @@ * green while that was true. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { FeedFilterMode as SpecFilterMode, FeedItemType as SpecFeedItemType } from '@objectstack/spec/data'; import type { FeedItem } from '@object-ui/types'; import { @@ -28,6 +28,7 @@ import { normalizeFeedTypes, normalizeFilterMode, normalizeLimit, + resetUnknownActivityTypeWarnings, } from '../recordActivityFeed'; const item = (over: Partial & Pick): FeedItem => ({ @@ -36,21 +37,92 @@ const item = (over: Partial & Pick): FeedItem ...over, }); +/** + * The two vocabularies this map has to cover (objectui#5840). + * + * They are LITERALS on purpose, in both groups, for the reason plugin-audit's + * own `sys-activity-type-vocabulary.test.ts` gives: a pin that read its + * expectation out of the thing it is pinning cannot fail. The cost is that a + * human redoes the census when either group moves, which is the point. + */ + +/** + * plugin-audit's declared `sys_activity.type` select options + * (`sys-activity.object.ts`). A new option added upstream should show up here + * as a DECISION rather than as a row that silently renders nothing. + */ +const DECLARED_UPSTREAM_TYPES = [ + 'assigned', 'commented', 'completed', 'created', 'deleted', + 'login', 'logout', 'mentioned', 'shared', 'system', 'updated', +] as const; + +/** + * Values a shipped producer measurably WRITES while being undeclared upstream. + * + * This group exists because the declaration is not a contract: every field on + * `sys_activity` is `readonly: true` and objectql's `validateRecord` skips + * readonly fields, so an undeclared value is stored silently. The second + * element names the producer — add the producer before adding the row. + * + * Whether the upstream enum should absorb these is a platform ruling, not this + * block's; until it is made, rendering them is what stops a stored row from + * being invisible. + */ +const UNDECLARED_BUT_WRITTEN_TYPES: ReadonlyArray = [ + [ + 'scheduled', + 'hotcrm/src/actions/global.actions.ts — schedule_meeting: ' + + "type: EVENT_STATUS === 'held' ? 'completed' : 'scheduled'; registered for " + + 'crm_lead / crm_contact / crm_account / crm_opportunity / crm_case', + ], +]; + describe('sys_activity row → FeedItem', () => { - it('maps every activity type the platform writes, and only to spec feed types', () => { - // The keys are `sys_activity.type`'s select options (plugin-audit - // sys-activity.object.ts). Pinned as a set so a new activity type added - // upstream shows up here as a decision rather than silently rendering - // nothing. - expect(Object.keys(ACTIVITY_TYPE_TO_FEED_TYPE).sort()).toEqual([ - 'assigned', 'commented', 'completed', 'created', 'deleted', - 'login', 'logout', 'mentioned', 'shared', 'system', 'updated', - ]); + it('covers the declared vocabulary AND the values producers actually write', () => { + expect(Object.keys(ACTIVITY_TYPE_TO_FEED_TYPE).sort()).toEqual( + [...DECLARED_UPSTREAM_TYPES, ...UNDECLARED_BUT_WRITTEN_TYPES.map(([t]) => t)].sort(), + ); for (const mapped of Object.values(ACTIVITY_TYPE_TO_FEED_TYPE)) { if (mapped) expect(SpecFeedItemType.options).toContain(mapped); } }); + it.each(UNDECLARED_BUT_WRITTEN_TYPES)( + 'renders %s — it is stored by a real producer, so dropping it loses data', + (type, writer) => { + expect( + activityRowToFeedItem({ id: 'x', type }, 'System'), + `'${type}' must keep reaching the feed: it is written by ${writer}. ` + + 'It is absent from plugin-audit\'s declared options and lands anyway, ' + + 'because readonly fields are never validated on write — so the enum ' + + 'cannot be used as the list of what this map has to handle.', + ).not.toBeNull(); + }, + ); + + /** + * Regression control for the #5840 change: the entries that existed before + * `scheduled` was added still resolve exactly as they did. Written as the + * whole table rather than as "not broken" so a future edit that RE-points an + * existing type has to say so here. + */ + it('leaves every previously-mapped type pointing where it did', () => { + expect({ ...ACTIVITY_TYPE_TO_FEED_TYPE, scheduled: undefined }).toEqual({ + 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, + scheduled: undefined, + }); + }); + it('drops the rows that are not record activity', () => { for (const type of ['commented', 'mentioned', 'login', 'logout']) { expect(activityRowToFeedItem({ id: '1', type }, 'System')).toBeNull(); @@ -95,6 +167,109 @@ describe('sys_activity row → FeedItem', () => { }); }); +/** + * objectui#5840 — a `scheduled` meeting reaches the timeline, and the fix is an + * ADDITION rather than a loosening. + * + * Both directions are asserted on purpose. "`scheduled` now renders" alone + * would stay green if the map had been replaced by a catch-all bucket, which is + * the wrong fix (an unmeasured type rendering as `system` is new wrong data, + * not recovered data). So the unknown-type leg below is not decoration: it is + * what makes the pair discriminate between the fix that was made and the fix + * that was rejected. + */ +describe('a scheduled activity reaches the feed (objectui#5840)', () => { + afterEach(() => { + vi.restoreAllMocks(); + resetUnknownActivityTypeWarnings(); + }); + + it('maps a scheduled meeting row onto an `event` feed item, carrying its ADR-0052 pointer', () => { + const mapped = activityRowToFeedItem( + { + id: 'act-9', + type: 'scheduled', + summary: 'Discovery call (30 min)', + timestamp: '2026-04-01T09:00:00.000Z', + actor_name: 'Grace', + source_object: 'crm_event', + source_id: 'evt-3', + }, + 'System', + ); + expect(mapped).toEqual({ + id: 'act-9', + type: 'event', + actor: 'Grace', + actorAvatarUrl: undefined, + body: 'Discovery call (30 min)', + createdAt: '2026-04-01T09:00:00.000Z', + sourceObject: 'crm_event', + sourceId: 'evt-3', + }); + }); + + it('produces a feed type the spec actually declares, so `types` can name it', () => { + // The other half of #5840's complaint: `event` was a declared FeedItemType + // that nothing could produce, so `types: ['event']` was a permanently empty + // tab. It is reachable now. + expect(SpecFeedItemType.options).toContain('event'); + expect(ACTIVITY_TYPE_TO_FEED_TYPE.scheduled).toBe('event'); + }); + + const scheduledItem: FeedItem = item({ id: 'e1', type: 'event' }); + + it('survives the default filters — an upcoming meeting is not "completed"', () => { + // Reaching activityRowToFeedItem is not enough: the row is only visible if + // it also survives the pipeline every page runs. `showCompleted` defaults + // to false, and a scheduled meeting must NOT be caught by it — that is the + // whole difference from the held branch of the same producer. + expect(applyFeedConfig([scheduledItem], {}, 50).items.map((i) => i.id)).toEqual(['e1']); + }); + + it('survives `types: [\'event\']`, the filter an author writes to show meetings', () => { + expect(applyFeedConfig([scheduledItem], { types: ['event'] }, 50).items.map((i) => i.id)) + .toEqual(['e1']); + }); + + it('survives unifiedTimeline:false — a meeting is not a field change', () => { + expect(applyFeedConfig([scheduledItem], { unifiedTimeline: false }, 50).items.map((i) => i.id)) + .toEqual(['e1']); + }); + + it('is excluded when the author asks for other kinds, like any other item', () => { + // Control for the three legs above: they pass because `event` is genuinely + // carried through the pipeline, not because the pipeline stopped filtering. + expect(applyFeedConfig([scheduledItem], { types: ['comment'] }, 50).items).toEqual([]); + }); + + it('still DROPS a type nothing maps — the fix is an addition, not a catch-all', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(activityRowToFeedItem({ id: 'z', type: 'teleported' }, 'System')).toBeNull(); + expect(activityRowToFeedItem({ id: 'z2' }, 'System')).toBeNull(); + expect(warn).toHaveBeenCalledTimes(2); + expect(String(warn.mock.calls[0][0])).toContain('teleported'); + }); + + it('warns once per unknown type, not once per row', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + for (let i = 0; i < 5; i += 1) { + activityRowToFeedItem({ id: `z${i}`, type: 'teleported' }, 'System'); + } + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('stays SILENT for the types it deliberately drops', () => { + // A warning about a decision teaches authors to ignore the channel. Only a + // value outside the table entirely is a missing decision. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + for (const type of ['commented', 'mentioned', 'login', 'logout']) { + expect(activityRowToFeedItem({ id: '1', type }, 'System')).toBeNull(); + } + expect(warn).not.toHaveBeenCalled(); + }); +}); + describe('input normalisation reads its vocabulary from the spec', () => { it('accepts every FeedFilterMode the spec declares, and only those', () => { for (const mode of SpecFilterMode.options) { diff --git a/packages/plugin-detail/src/renderers/recordActivityFeed.ts b/packages/plugin-detail/src/renderers/recordActivityFeed.ts index 05eb047d89..f50c71c405 100644 --- a/packages/plugin-detail/src/renderers/recordActivityFeed.ts +++ b/packages/plugin-detail/src/renderers/recordActivityFeed.ts @@ -41,19 +41,65 @@ export const DEFAULT_ACTIVITY_LIMIT = 20; /** * `sys_activity.type` → `FeedItem.type`. * - * Identical to the map `RecordDetailView` uses when it merges `sys_activity` - * into the discussion feed, deliberately: one table, one reading. Two renderers - * that disagree about what a `created` row IS would put the same record's - * history under two different icons depending on which block an author reached - * for, so the richer `record_create` / `record_delete` / `sharing` feed types - * the spec offers are NOT used here — adopting them is a change to that shared - * map, not to this block. + * Meant to be one reading with the map `RecordDetailView` uses when it merges + * `sys_activity` into the discussion feed. Two renderers that disagree about what a `created` row IS would put + * the same record's history under two different icons depending on which block + * an author reached for, so the richer `record_create` / `record_delete` / + * `sharing` feed types the spec offers are NOT used here — adopting them is a + * change to that shared map, not to this block. + * + * ⚠️ That copy is a hand-written literal in `app-shell`, not an import of this + * one, and nothing fails when the two disagree — so the `scheduled` entry below + * is currently present here and absent there. Tracked as objectui#5878; fixing + * it means the second copy reading this export, which is a different package's + * surface than this card's. * * `commented` / `mentioned` map to nothing because their content lives in * `sys_comment` (with reactions and threading attached) — a host that has both * merges the comment rows, and the block on its own has no comment write path * to pair them with. `login` / `logout` are account events, not record * activity. + * + * ## Two vocabularies, not one (objectui#5840) + * + * The keys were originally set-equal to plugin-audit's declared + * `sys_activity.type` select options, and the test pinned exactly that. They no + * longer are, because those options are **not** what the column stores: + * + * - Every field on `sys_activity` is `readonly: true`, and objectql's + * `validateRecord` skips readonly fields on both the insert and the update + * branch. The eleven-value enum is therefore documentation, not a contract — + * an undeclared value is written silently (measured upstream by + * plugin-audit's own `sys-activity-type-vocabulary.test.ts`). + * - The platform itself forwards author-declared values into the column: + * ADR-0052 §5b.2 `activityMilestones[].type` is applied verbatim by + * plugin-audit's `audit-writers.ts` (`if (milestone.type) activityType = + * milestone.type`). That is how `completed` is produced, and it is a general + * door, not a special case. + * + * So `scheduled` is a value that is written, stored and queryable while being + * undeclared upstream. Dropping it was not a decision this map made; it was the + * absence of one. Its producer is HotCRM's `schedule_meeting` action + * (`src/actions/global.actions.ts` — `type: EVENT_STATUS === 'held' ? + * 'completed' : 'scheduled'`), registered for `crm_lead`, `crm_contact`, + * `crm_account`, `crm_opportunity` and `crm_case`: the held branch reached the + * timeline, the scheduled branch never did. + * + * `scheduled` → `event` is the semantic pairing and it is what makes the + * declared `event` feed type reachable at all. Note the two branches of that + * one producer now land at different DEFAULT visibility, which is the intended + * reading: a held meeting is `completed` → `task`, hidden unless + * `showCompleted`; a not-yet-held meeting is `scheduled` → `event`, shown, + * because an upcoming meeting is the part of a timeline you still act on. + * + * Whether the upstream enum should GAIN `scheduled` is a platform ruling, not + * this block's to make — filed as objectstack#11424. Until it is ruled, the + * honest statement of this table is the one the test now pins: the upstream + * declaration PLUS the values a shipped producer measurably writes. + * + * Values outside BOTH groups are still dropped — see + * {@link activityRowToFeedItem}, which now says so out loud instead of + * silently. */ export const ACTIVITY_TYPE_TO_FEED_TYPE: Readonly> = { created: 'field_change', @@ -63,6 +109,7 @@ export const ACTIVITY_TYPE_TO_FEED_TYPE: Readonly(); + +/** + * Say out loud that a row was dropped for having a type nothing maps. + * + * Deliberately NOT fired for a type this map knows and deliberately drops + * (`commented` / `mentioned` / `login` / `logout` → `undefined`): those are + * decisions, and a warning about a decision is noise that teaches authors to + * ignore the channel. It fires only for a value outside the table entirely — + * which is the objectui#5840 failure mode: written, stored, invisible, no + * diagnostic anywhere. + */ +function warnUnknownActivityType(type: string): void { + if (warnedUnknownActivityTypes.has(type)) return; + warnedUnknownActivityTypes.add(type); + console.warn( + `[record:activity] dropped a sys_activity row with type "${type}": no feed ` + + 'item type is mapped for it, so it cannot appear on any timeline whatever ' + + 'the page authors. `sys_activity.type` is not validated on write (every ' + + 'field on that object is readonly), so a producer can store a value the ' + + 'platform never declared. Map it in ACTIVITY_TYPE_TO_FEED_TYPE ' + + '(@object-ui/plugin-detail) if it is record activity.', + ); +} + +/** Test seam: forget which unknown types have already been warned about. */ +export function resetUnknownActivityTypeWarnings(): void { + warnedUnknownActivityTypes.clear(); +} + /** * One `sys_activity` row → one {@link FeedItem}, or `null` when the row is not * record activity (see {@link ACTIVITY_TYPE_TO_FEED_TYPE}). + * + * Two different `null`s, and the difference is the point: a type the table + * maps to `undefined` is a deliberate exclusion and returns quietly; a type the + * table does not contain at all is an unmapped producer and says so once. */ export function activityRowToFeedItem( row: SysActivityRow, systemActorLabel: string, ): FeedItem | null { - const feedType = ACTIVITY_TYPE_TO_FEED_TYPE[String(row?.type)]; + const rawType = String(row?.type); + if (!Object.prototype.hasOwnProperty.call(ACTIVITY_TYPE_TO_FEED_TYPE, rawType)) { + warnUnknownActivityType(rawType); + return null; + } + const feedType = ACTIVITY_TYPE_TO_FEED_TYPE[rawType]; if (!feedType) return null; return { id: row.id as string | number,