From 8740317bc345314102dbd5eae8817698572fd30b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 17:24:19 +0000 Subject: [PATCH] fix(plugin-detail): render author-extended sys_activity types through a defined fallback `activityRowToFeedItem` returned `null` for any `sys_activity.type` outside `ACTIVITY_TYPE_TO_FEED_TYPE`, so a stored row with an author-extended type was queryable and invisible. Under the 2026-08-24 direction-4 ruling on objectstack#11507 that column is author-extensible, so such a value is legitimate data and dropping it reproduces the objectui#5840 failure by a different route. It now renders through `UNMAPPED_ACTIVITY_FEED_TYPE`, still announced once per distinct type. The pin that replaces the set-equality check objectui#5840 removed is two-directional and says so in its docblock: the map must cover every BUILT-IN type (superset), and an unknown type must reach the feed through the fallback. Set equality is not asserted in either spelling. Part of #5969 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L --- ...ecord-activity-open-vocabulary-fallback.md | 31 +++ .../__tests__/recordActivityFeed.test.ts | 218 +++++++++++++----- .../src/renderers/recordActivityFeed.ts | 110 ++++++--- 3 files changed, 273 insertions(+), 86 deletions(-) create mode 100644 .changeset/record-activity-open-vocabulary-fallback.md diff --git a/.changeset/record-activity-open-vocabulary-fallback.md b/.changeset/record-activity-open-vocabulary-fallback.md new file mode 100644 index 0000000000..f1e300a948 --- /dev/null +++ b/.changeset/record-activity-open-vocabulary-fallback.md @@ -0,0 +1,31 @@ +--- +'@object-ui/plugin-detail': patch +--- + +`record:activity` renders an author-extended `sys_activity.type` instead of dropping it + +**Behaviour change.** `activityRowToFeedItem` used to return `null` for any +`sys_activity.type` outside `ACTIVITY_TYPE_TO_FEED_TYPE`: the row was stored, +queryable and invisible, with only a console warning to say so. It now renders +through a defined fallback presentation (`UNMAPPED_ACTIVITY_FEED_TYPE`, the +generic `system` feed type), still announced once per distinct type. + +This follows the maintainer ruling of 2026-08-24 on objectstack#11507, +direction 4: `sys_activity.type` is **author-extensible**. Every field on +`sys_activity` is `readonly: true` and objectql's `validateRecord` skips +readonly fields on both write branches, and ADR-0052 §5b.2 forwards an author's +`activityMilestones[].type` into the column verbatim — so a value the platform +never declared is legitimate stored data, not a mistake, and dropping it +reproduces the objectui#5840 failure for every author who writes one. + +The types this map deliberately excludes (`commented` / `mentioned` / `login` / +`logout` → `undefined`) are unchanged: those are decisions, not gaps, and they +still return `null` silently. `ACTIVITY_TYPE_TO_FEED_TYPE` itself is unchanged, +so the copy `RecordDetailView` reads is unaffected. + +The pin that goes with it replaces the set-equality check objectui#5840 removed, +and is deliberately two-directional: the map must cover every **built-in** type +(superset — a new built-in turns it red), and an unknown type must reach the +feed through the fallback (never dropped, never crashing). Set equality is not +asserted in either spelling, and the docblock says so, because pinning to the +closed declaration is what made #5840 drop stored rows. diff --git a/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts b/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts index f8cdfe05b2..3de8a02001 100644 --- a/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts +++ b/packages/plugin-detail/src/renderers/__tests__/recordActivityFeed.test.ts @@ -30,6 +30,7 @@ import { normalizeLimit, resetUnknownActivityTypeWarnings, resetUnrecognisedFeedTypeWarnings, + UNMAPPED_ACTIVITY_FEED_TYPE, } from '../recordActivityFeed'; const item = (over: Partial & Pick): FeedItem => ({ @@ -39,76 +40,112 @@ const item = (over: Partial & Pick): FeedItem }); /** - * The two vocabularies this map has to cover (objectui#5840). + * The platform's BUILT-IN `sys_activity.type` set. * - * 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. + * ## Provenance — read this before editing the list + * + * Source: objectstack `packages/plugins/plugin-audit/src/objects/sys-activity.object.ts`, + * the `type: Field.select([...])` declaration. Read at objectstack commit + * `91b1342` ("declare sys_activity.type 'scheduled' and record its writer", + * objectstack#11522) on 2026-08-24. That file is the ONLY declaration of this + * vocabulary in the platform — `@objectstack/spec` declares `FeedItemType`, the + * vocabulary this map's VALUES come from, and never the keys. + * + * ## Why it is a hand census and not a live read + * + * There is no live source for it in reach: `@objectstack/plugin-audit` is a + * server plugin, and the packages this repo depends on (`@objectstack/spec`, + * `client`, `formula`, `lint`) do not carry the declaration. Reading it live + * would mean a UI package taking a dependency on a services plugin. + * + * ⚠️ So this list CAN go stale, and measurably has: `scheduled` was declared + * upstream on 2026-08-24 and the previous census still had it filed as + * "undeclared but written". Two things keep that from being a data loss rather + * than a labelling one, and both are load-bearing: + * + * 1. the fallback below — an undeclared or newly-declared value RENDERS, so a + * stale census costs a specific icon, never a vanished row; and + * 2. objectstack#11807 — the platform-side ask to publish this vocabulary + * where a UI package can read it, which is what would let this list be + * deleted. + * + * ⛔ Do not derive this list from `ACTIVITY_TYPE_TO_FEED_TYPE`. A pin that reads + * its expectation out of the thing it is pinning cannot fail. */ -const DECLARED_UPSTREAM_TYPES = [ - 'assigned', 'commented', 'completed', 'created', 'deleted', - 'login', 'logout', 'mentioned', 'shared', 'system', 'updated', +const PLATFORM_BUILTIN_ACTIVITY_TYPES = [ + 'assigned', 'commented', 'completed', 'created', 'deleted', 'login', + 'logout', 'mentioned', 'scheduled', '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. + * A value no built-in declares — what an author extends the column with. * - * 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. + * `sys_activity.type` is author-extensible (objectstack#11507, ruled direction + * 4 on 2026-08-24): every field on the object is `readonly: true`, objectql's + * `validateRecord` skips readonly fields on both write branches, and ADR-0052 + * §5b.2 forwards an author's `activityMilestones[].type` into the column + * verbatim. So a value like this is STORED, queryable, and legitimate. */ -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', - ], -]; +const AUTHOR_EXTENDED_TYPE = 'crm_contract_signed'; describe('sys_activity row → FeedItem', () => { - 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(), - ); + /** + * SUPERSET, one direction only: **the map covers every built-in**. + * + * In words, because the direction is easy to get backwards: every value the + * platform DECLARES must have an entry in `ACTIVITY_TYPE_TO_FEED_TYPE` — a + * feed type, or a deliberate `undefined` exclusion. A new built-in with no + * entry turns this red, which is what forces the map to keep up. + * + * ⛔ The converse — every map key must be declared upstream — is NOT asserted, + * and neither is set equality in any spelling. Under the objectstack#11507 + * direction-4 ruling (2026-08-24) `sys_activity.type` is author-extensible, + * so map keys outside the declaration are legitimate by construction. + * objectui#5840 removed the old equality pin because pinning to the closed + * declaration meant dropping stored rows; ⛔ do not put it back. What replaces + * it is this leg plus the fallback leg below — either alone is worse than + * neither: a superset pin on its own re-creates the closed-vocabulary + * failure slowly, and a fallback on its own lets the map fall behind. + */ + it('covers every BUILT-IN type — superset, not equality (objectstack#11507)', () => { + for (const type of PLATFORM_BUILTIN_ACTIVITY_TYPES) { + expect( + Object.prototype.hasOwnProperty.call(ACTIVITY_TYPE_TO_FEED_TYPE, type), + `'${type}' is declared by plugin-audit's sys_activity.type and has no entry in ` + + 'ACTIVITY_TYPE_TO_FEED_TYPE. Every built-in needs a decision here: a feed ' + + 'type, or an explicit `undefined` that says the exclusion was meant.', + ).toBe(true); + } 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(); - }, - ); + it('does NOT require the map to be contained by the declaration', () => { + // The other direction, stated as an assertion rather than as a comment so + // that "re-add the equality check" has to delete a passing test to happen. + // An author-extended value has no entry and still renders (see the fallback + // leg), and a mapped key that upstream later drops is not an error here. + const declared = new Set(PLATFORM_BUILTIN_ACTIVITY_TYPES); + expect(declared.has(AUTHOR_EXTENDED_TYPE)).toBe(false); + expect(Object.prototype.hasOwnProperty.call( + ACTIVITY_TYPE_TO_FEED_TYPE, AUTHOR_EXTENDED_TYPE, + )).toBe(false); + expect(activityRowToFeedItem({ id: 'ext', type: AUTHOR_EXTENDED_TYPE }, 'System')) + .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. + * Regression control on the map's OWN table, not on the platform's. + * + * This is `toEqual` over the map and it is deliberately not the pin the + * ruling forbids: it compares the map to the readings this repo recorded, so + * a future edit that RE-POINTS or drops an existing type has to say so here. + * It says nothing about which values the platform declares — that is the + * superset leg above, and only that leg moves when upstream moves. */ - it('leaves every previously-mapped type pointing where it did', () => { - expect({ ...ACTIVITY_TYPE_TO_FEED_TYPE, scheduled: undefined }).toEqual({ + it('leaves every mapped type pointing where it did', () => { + expect({ ...ACTIVITY_TYPE_TO_FEED_TYPE }).toEqual({ created: 'field_change', updated: 'field_change', deleted: 'field_change', @@ -116,11 +153,11 @@ describe('sys_activity row → FeedItem', () => { shared: 'field_change', system: 'system', completed: 'task', + scheduled: 'event', commented: undefined, mentioned: undefined, login: undefined, logout: undefined, - scheduled: undefined, }); }); @@ -178,6 +215,16 @@ describe('sys_activity row → FeedItem', () => { * 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. + * + * ⚠️ objectui#5969 changed what that unknown-type leg asserts, and the reason + * the sentence above survives is that the objection it records still stands. + * Under the objectstack#11507 direction-4 ruling an unknown value now renders + * through a defined FALLBACK rather than being dropped — but a catch-all is + * still not a substitute for reading a type and mapping it, which is why the + * superset leg exists and why `scheduled` keeps its own `event` presentation + * here instead of landing in the bucket. The pair still discriminates; what it + * discriminates between is now "mapped on purpose" and "shown pending a + * decision", rather than "mapped on purpose" and "invisible". */ describe('a scheduled activity reaches the feed (objectui#5840)', () => { afterEach(() => { @@ -244,20 +291,71 @@ describe('a scheduled activity reaches the feed (objectui#5840)', () => { expect(applyFeedConfig([scheduledItem], { types: ['comment'] }, 50).items).toEqual([]); }); - it('still DROPS a type nothing maps — the fix is an addition, not a catch-all', () => { + /** + * FALLBACK leg of the objectstack#11507 pin — the second half of the ruling. + * + * The observable is POSITIVE, not "did not throw": the row is present, it + * carries its own summary and actor, and its feed type is the declared + * fallback. "Unknown types do not crash" would also be true of a feed that + * drops every row, which is exactly what this replaces. + * + * The counter-probe rides in the same run: a built-in with its own reading + * still gets THAT reading, not the bucket. Without it, a map replaced + * wholesale by the fallback would read as green here. + */ + it('RENDERS an author-extended type through the fallback presentation', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const mapped = activityRowToFeedItem( + { + id: 'ext-1', + type: AUTHOR_EXTENDED_TYPE, + summary: 'Contract countersigned', + timestamp: '2026-05-06T07:08:09.000Z', + actor_name: 'Grace', + }, + 'System', + ); + // Present, and carrying the row — not a husk, and not `null`. + expect(mapped).not.toBeNull(); + expect(mapped).toMatchObject({ + id: 'ext-1', + type: UNMAPPED_ACTIVITY_FEED_TYPE, + actor: 'Grace', + body: 'Contract countersigned', + createdAt: '2026-05-06T07:08:09.000Z', + }); + + // COUNTER-PROBE — a built-in keeps its own presentation, so "everything + // renders" cannot be reached by pointing the whole map at the fallback. + expect(activityRowToFeedItem({ id: 'b1', type: 'created' }, 'System')?.type) + .toBe('field_change'); + expect(activityRowToFeedItem({ id: 'b2', type: 'scheduled' }, 'System')?.type) + .toBe('event'); + // ...and the fallback is DISTINGUISHABLE from the one it was compared with. + expect(UNMAPPED_ACTIVITY_FEED_TYPE).not.toBe('field_change'); + + // Shown, but not silently: the missing decision is still announced once. + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain(AUTHOR_EXTENDED_TYPE); + }); + + it('renders a row whose type is missing entirely rather than dropping it', () => { + // A stored row with no `type` is still a stored row. Same posture: visible + // through the fallback, announced once. 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'); + expect(activityRowToFeedItem({ id: 'z2', summary: 'no type' }, 'System')) + .toMatchObject({ id: 'z2', type: UNMAPPED_ACTIVITY_FEED_TYPE, body: 'no type' }); + expect(warn).toHaveBeenCalledTimes(1); }); 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(activityRowToFeedItem({ id: `z${i}`, type: 'teleported' }, 'System')) + .not.toBeNull(); } expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('teleported'); }); it('stays SILENT for the types it deliberately drops', () => { diff --git a/packages/plugin-detail/src/renderers/recordActivityFeed.ts b/packages/plugin-detail/src/renderers/recordActivityFeed.ts index 6df0f4e297..185076921d 100644 --- a/packages/plugin-detail/src/renderers/recordActivityFeed.ts +++ b/packages/plugin-detail/src/renderers/recordActivityFeed.ts @@ -92,14 +92,35 @@ export const DEFAULT_ACTIVITY_LIMIT = 20; * `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. + * ## The vocabulary is OPEN — ruled. Do NOT restore set-equality. + * + * Maintainer ruling of 2026-08-24 on objectstack#11507, **direction 4**: + * `sys_activity.type` is AUTHOR-EXTENSIBLE. The declared select options are the + * platform's BUILT-IN set, not the column's domain, and the two facts above are + * why — readonly fields are never validated on write, and ADR-0052 §5b.2 + * forwards an author's `activityMilestones[].type` into the column verbatim. + * + * So the relation this table stands in is a SUPERSET, in one direction only: + * + * - **map ⊇ built-ins** — every value the platform declares has an entry here, + * either a feed type or a deliberate `undefined` exclusion. A new built-in + * with no entry is a gap, and objectui#5969's pin turns red for it. + * - **NOT built-ins ⊇ map**, and NOT set-equality in either spelling. An + * author-extended value legitimately exists outside the declaration, so a + * pin that required the two sets to match would be false by construction. + * + * ⛔ A reader who finds no equality check here is looking at a DECISION, not an + * omission: objectui#5840 removed the old set-equality pin on purpose, because + * pinning to the closed declaration meant dropping stored rows, and + * objectui#5969 replaced it with the two-directional pin under this ruling. + * + * (`scheduled` — the value #5840 was about — has since been declared upstream + * by objectstack#11522, superseding objectstack#11424. That is precisely the + * drift a hand-maintained equality pin cannot survive, and the reason the pin + * is a superset assertion plus a fallback rather than a census of two groups.) + * + * A value outside this table is NO LONGER DROPPED: it renders through + * {@link UNMAPPED_ACTIVITY_FEED_TYPE}. See {@link activityRowToFeedItem}. */ export const ACTIVITY_TYPE_TO_FEED_TYPE: Readonly> = { created: 'field_change', @@ -116,6 +137,30 @@ export const ACTIVITY_TYPE_TO_FEED_TYPE: Readonly(); /** - * Say out loud that a row was dropped for having a type nothing maps. + * Say out loud that a row reached the timeline through the generic fallback. * * 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. + * ignore the channel. It fires only for a value outside the table entirely. + * + * Since objectui#5969 that value is RENDERED rather than dropped + * ({@link UNMAPPED_ACTIVITY_FEED_TYPE}), so the diagnostic no longer reports + * lost data — it reports a MISSING DECISION, which is the thing that is still + * wrong. The row is visible; what it is missing is the specific icon and colour + * a mapped type gets. That is the channel by which an author-extended value + * becomes a mapping somebody made on purpose. */ function warnUnknownActivityType(type: string): void { warnOnce(warnedUnknownActivityTypes, [type], () => - `[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.'); + `[record:activity] rendered a sys_activity row with type "${type}" through the ` + + `generic "${UNMAPPED_ACTIVITY_FEED_TYPE}" presentation: no feed item type is ` + + 'mapped for it. `sys_activity.type` is author-extensible (objectstack#11507, ' + + 'ruled 2026-08-24) and is not validated on write, so a producer can store a ' + + 'value the platform never declared — the row is shown rather than dropped. ' + + 'Map it in ACTIVITY_TYPE_TO_FEED_TYPE (@object-ui/plugin-detail) to give it ' + + 'its own presentation.'); } /** Test seam: forget which unknown types have already been warned about. */ @@ -333,20 +384,27 @@ export function resetUnknownActivityTypeWarnings(): void { * 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. + * Three outcomes, and the difference between the last two is the whole of the + * objectstack#11507 direction-4 ruling (2026-08-24): + * + * - a type mapped to a feed type renders with THAT presentation; + * - a type the table maps to `undefined` is a DELIBERATE exclusion — `null`, + * quietly, because a warning about a decision teaches authors to ignore the + * channel; + * - a type the table does not contain at all is an AUTHOR-EXTENDED value under + * the ruled open vocabulary. It renders through + * {@link UNMAPPED_ACTIVITY_FEED_TYPE} and says so once. It used to return + * `null` here, which made every extended value invisible — the same outcome + * objectui#5840 was filed for, reached by a different route. */ export function activityRowToFeedItem( row: SysActivityRow, systemActorLabel: string, ): FeedItem | null { 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]; + const known = Object.prototype.hasOwnProperty.call(ACTIVITY_TYPE_TO_FEED_TYPE, rawType); + if (!known) warnUnknownActivityType(rawType); + const feedType = known ? ACTIVITY_TYPE_TO_FEED_TYPE[rawType] : UNMAPPED_ACTIVITY_FEED_TYPE; if (!feedType) return null; return { id: row.id as string | number,