From 49b11685c3dd6caa09d3f1b11c98a76cf7eb3f9f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 22:33:27 +0000 Subject: [PATCH] fix(data-objectstack): every view write path invalidates the override map (#4363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ObjectStackAdapter` caches two view-shaped reads — `getView` under `view:{object}:{viewId}` and `listViewOverrides` under `view-overrides:{object}`. Of the four write paths that touch view rows, only `updateViewConfig` invalidated the second key, so `createView` / `updateView` / `deleteView` left the batch override map stale for `MetadataCache`'s default 5-minute TTL. The gap does not self-heal: `loadViewOverrides` (app-shell `ObjectView`) treats a resolved map as authoritative and deliberately does not re-probe per view (#3774), so the per-view `getView` fallback that would mask a stale map is by design unreachable, and `listViews` — uncached — answers fresh beside it. All four paths now emit the same ordered pair, per method rather than per branch. `updateView`'s draft half joins its published half (deliberate over-invalidation: both readers enumerate published rows, and a missed invalidation costs the whole TTL while a spare one costs a refetch), and `createView` names the per-view key because `saveItem` is an upsert. Extends the pin suite from #4328 / PR #4366 to assert the full key set for all five call sites; its two sweep pins stay as untouched controls. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../view-overrides-invalidation-4363.md | 13 ++++ packages/data-objectstack/src/index.ts | 40 ++++++++++- .../src/viewCacheInvalidation.pin.test.ts | 72 ++++++++++++++++--- 3 files changed, 112 insertions(+), 13 deletions(-) create mode 100644 .changeset/view-overrides-invalidation-4363.md diff --git a/.changeset/view-overrides-invalidation-4363.md b/.changeset/view-overrides-invalidation-4363.md new file mode 100644 index 000000000..ec98bab7f --- /dev/null +++ b/.changeset/view-overrides-invalidation-4363.md @@ -0,0 +1,13 @@ +--- +'@object-ui/data-objectstack': patch +--- + +Every view write path now invalidates the override map — a created, renamed or deleted view is no longer shadowed by a five-minute-stale batch read + +`ObjectStackAdapter` caches two view-shaped reads: `getView` under `view:{object}:{viewId}`, and `listViewOverrides` under `view-overrides:{object}`. Four write paths touch view rows, and until now exactly one of them — `updateViewConfig` — invalidated the second key. `createView`, `updateView` and `deleteView` invalidated only the per-view key, so the batch override map kept answering from a snapshot taken up to `MetadataCache`'s default 5-minute TTL earlier. + +That gap does not heal itself. `loadViewOverrides` in app-shell's `ObjectView` treats a resolved map as authoritative and deliberately does not re-probe per view — that is objectui#3774's fix, and it is correct, since re-probing reinstates the 404 flurry the batch read exists to remove. So the per-view `getView` fallback that would have masked a stale map is by design unreachable, and the stale map is served in full. Meanwhile `listViews` is uncached and answers fresh, so the view switcher could list a view whose override body came from a map written minutes earlier: the sharpest shape is the rename/pin path (`updateView`), where a user edits a view, returns to the object, and is served the pre-edit override. + +All four paths now emit the same ordered pair — the per-view key, then the object's override map. The rule is uniform per method rather than per branch: `updateView`'s draft half invalidates both keys as its published half does, which is deliberate over-invalidation (both readers enumerate published rows, so a draft write stales neither) chosen because an unnecessary invalidation costs one refetch while a missed one costs the full TTL. `createView` names the per-view key too, because `saveItem` is an upsert and an explicit `spec.name` that already exists overwrites a published row a prior `getView` may hold. + +No signature, no cache key and no read path changed; the only difference is which keys each write drops. The pin suite added by objectui#4328 now asserts the full invalidation key set for all five call sites, with the sweep's two pins kept as untouched controls: `listViews` stays uncached, and no write path names the retired `views:{object}` key. diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 6c94f0133..b05d9b22c 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -2917,8 +2917,16 @@ export class ObjectStackAdapter implements DataSource { * (the cache stores on success only), so a transient failure does not * pin an empty answer for the TTL. * - * Result is cached identically to {@link getView}; saving a view via - * {@link updateViewConfig} invalidates the cache. + * Result is cached identically to {@link getView}, and EVERY view write path + * on this adapter invalidates it — {@link updateViewConfig}, + * {@link createView}, {@link updateView} and {@link deleteView}. For a long + * time only the first did (objectui#4363), which left the other three stale + * for the cache's 5-minute TTL. That gap does not self-heal: the consumer + * (`loadViewOverrides`, app-shell `ObjectView`) treats a RESOLVED map as + * authoritative and deliberately does not re-probe per view — objectui#3774, + * and correct, since re-probing reinstates the 404 flurry the batch read + * exists to remove. So a stale map here is served in full, and the per-view + * {@link getView} fallback that would have masked it never runs. * * @param objectName - Object name (e.g. 'lead') * @returns Map keyed by view name with the persisted override config @@ -3150,6 +3158,10 @@ export class ObjectStackAdapter implements DataSource { * * Generates a snake_case name if `spec.name` is not provided by appending * a short timestamp suffix to the source-name hint. + * + * Invalidates both view-shaped cache keys for the object, exactly as + * {@link updateViewConfig} does — see {@link listViewOverrides} for why the + * batch map is the one that cannot heal itself (objectui#4363). */ async createView( objectName: string, @@ -3179,6 +3191,14 @@ export class ObjectStackAdapter implements DataSource { data: spec?.data || { provider: 'object', object: objectName }, }; const result: any = await this.client.meta.saveItem('view', name, fullSpec); + // Same key set as `updateViewConfig` — this is the other `saveItem('view', …)` + // write, and `saveItem` is an UPSERT: an explicit `spec.name` that already + // exists overwrites the published row, which a prior `getView` may hold. + // (A generated name cannot collide, and a miss is a `Map.delete` on an absent + // key, so the uniform rule costs nothing on the create-a-new-row path.) + this.metadataCache.invalidate?.(`view:${objectName}:${name}`); + // The batch override map gains a row and cannot notice on its own (#4363). + this.metadataCache.invalidate?.(`view-overrides:${objectName}`); if (result && result.item) return result.item; return fullSpec; } @@ -3215,6 +3235,15 @@ export class ObjectStackAdapter implements DataSource { * *published* view (no draft pending) is unchanged — it writes the * published overlay, as before. * + * **Both halves invalidate the same two keys** (objectui#4363): the per-view + * {@link getView} key and the batch {@link listViewOverrides} map. On the + * draft half that is deliberate over-invalidation — both readers enumerate + * PUBLISHED rows, so a draft write stales neither, exactly as was already + * true of the per-view line this joins. The costs are not symmetric: an + * unnecessary invalidation costs one refetch, a missed one costs up to the + * cache's 5-minute TTL of stale overrides, and "which half am I in?" is not + * a question a future edit to this method should have to re-answer. + * * @throws when the view resolves in neither home, or when either read fails * for any other reason (network, permission). Both used to be swallowed * and converted into the bad partial write above; a caller that wants a @@ -3239,6 +3268,7 @@ export class ObjectStackAdapter implements DataSource { const mergedDraft = mergeViewPatch(draft, partial, viewName, objectName); await metaClient.save('view', viewName, mergedDraft, { mode: 'draft' }); this.metadataCache.invalidate?.(`view:${objectName}:${viewName}`); + this.metadataCache.invalidate?.(`view-overrides:${objectName}`); return mergedDraft; } @@ -3269,6 +3299,7 @@ export class ObjectStackAdapter implements DataSource { const merged = mergeViewPatch(current, partial, viewName, objectName); const result: any = await this.client.meta.saveItem('view', viewName, merged); this.metadataCache.invalidate?.(`view:${objectName}:${viewName}`); + this.metadataCache.invalidate?.(`view-overrides:${objectName}`); if (result && result.item) return result.item; return merged; } @@ -3277,6 +3308,10 @@ export class ObjectStackAdapter implements DataSource { * Delete an overlay view (reset to artifact default if one exists, or * remove entirely if it was a user-created view). Routes to * `DELETE /api/v1/meta/view/:name`. + * + * Invalidates both view-shaped keys: the deleted row leaves the batch + * override map too, and a ghost entry there is what the object page would + * keep applying (objectui#4363). */ async deleteView( objectName: string, @@ -3285,6 +3320,7 @@ export class ObjectStackAdapter implements DataSource { await this.connect(); const result: any = await this.client.meta.deleteItem('view', viewName); this.metadataCache.invalidate?.(`view:${objectName}:${viewName}`); + this.metadataCache.invalidate?.(`view-overrides:${objectName}`); return { deleted: !!(result?.deleted ?? result?.reset ?? true) }; } diff --git a/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts b/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts index 3550d0838..829c7916b 100644 --- a/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts +++ b/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts @@ -31,6 +31,23 @@ import { ObjectStackAdapter } from './index'; * request count, which is what makes it a pure dead-code removal rather * than a caching change. (Whether `listViews` SHOULD be cached is a * separate product question, deliberately not settled here.) + * + * ## objectui#4363 — every write path names BOTH keys + * + * Removing the dead key made the surviving asymmetry visible: only + * `updateViewConfig` invalidated `view-overrides:{object}`, so `createView` / + * `updateView` / `deleteView` left the batch map stale for the cache's + * 5-minute TTL. It does not self-heal — `loadViewOverrides` (app-shell + * `ObjectView`) treats a RESOLVED map as authoritative and deliberately does + * not re-probe per view (#3774), so the per-view `getView` fallback that would + * have masked a stale map is by design unreachable. + * + * So the rule these pins now enforce is uniform and per-METHOD, not + * per-branch: **a write to a view row invalidates the per-view key and the + * object's override map.** Four paths (five call sites — `updateView` has a + * draft half and a published half) emit the same ordered pair. The two sweep + * pins are untouched controls: `listViews` stays uncached, and no path names a + * `views:` key. */ interface Harness { @@ -152,36 +169,69 @@ describe('view metadata cache — invalidation names only keys with readers (#37 expect(invalidated).toEqual(['view:account:v1', 'view-overrides:account']); }); - it('updateView (published overlay) invalidates the getView key only', async () => { + it('listViewOverrides reads back under the key the write paths invalidate', async () => { + // The pairing, not the string: every assertion below names + // `view-overrides:account` because THIS is the key the batch reader caches + // under. An invalidation that named anything else would be the `views:` + // mistake again, one rename later. + const { ds, cacheReads } = makeDS({ items: [VIEW] }); + + await ds.listViewOverrides('account'); + + expect(cacheReads).toEqual(['view-overrides:account']); + }); + + it('createView invalidates the per-view key and the override map (#4363)', async () => { + const { ds, invalidated } = makeDS(); + + await ds.createView('account', { name: 'account.mine', object: 'account' }); + + // A created view is a new row in the batch map; nothing else notices. + // `saveItem` is an upsert, so the per-view key is named too — an explicit + // `name` that already exists overwrites a row `getView` may hold cached. + expect(invalidated).toEqual(['view:account:account.mine', 'view-overrides:account']); + }); + + it('updateView (published overlay) invalidates both keys (#4363)', async () => { const { ds, invalidated } = makeDS({ published: { name: 'v1', object: 'account' } }); await ds.updateView('account', 'v1', { label: 'Renamed' }); - expect(invalidated).toEqual(['view:account:v1']); + expect(invalidated).toEqual(['view:account:v1', 'view-overrides:account']); }); - it('updateView (pending draft) invalidates the getView key only', async () => { + it('updateView (pending draft) invalidates both keys (#4363)', async () => { const { ds, invalidated } = makeDS({ draft: VIEW, published: notFound() }); await ds.updateView('account', VIEW.name, { label: 'Renamed' }); - expect(invalidated).toEqual([`view:account:${VIEW.name}`]); + // Deliberate over-invalidation on this half: both readers enumerate + // PUBLISHED rows, so a draft write stales neither — as was already true of + // the per-view line this pairs with. Pinned so the uniform per-method rule + // is a decision on the record, not an oversight the next reader "fixes". + expect(invalidated).toEqual([ + `view:account:${VIEW.name}`, + 'view-overrides:account', + ]); }); - it('deleteView invalidates the getView key only', async () => { + it('deleteView invalidates both keys (#4363)', async () => { const { ds, invalidated } = makeDS(); await ds.deleteView('account', 'v1'); - expect(invalidated).toEqual(['view:account:v1']); + // The deleted row leaves the override map too — a ghost entry there is what + // the object page would keep applying for the rest of the TTL. + expect(invalidated).toEqual(['view:account:v1', 'view-overrides:account']); }); it('no write path invalidates a `views:` key — nothing populates one', async () => { - // createView is the path whose ONLY invalidation was the dead key, so it - // now invalidates nothing. Asserted as "no `views:` key" rather than "no - // invalidation at all": whether it ought to invalidate the override map - // (`listViewOverrides` enumerates the same rows) is a separate question, - // filed on its own card — this pin must not freeze the answer. + // createView is the path whose ONLY invalidation was the dead key. #3778 + // asserted "no `views:` key" here rather than "no invalidation at all", + // deliberately leaving the override-map question to its own card — and + // #4363 answered it: createView now names both live keys (pinned above). + // The ASSERTION is unchanged, which is the point of the slot: the dead key + // stays dead however the live key set grows. const created = makeDS(); await created.ds.createView('account', { name: 'account.mine', object: 'account' });