From a3efc9b9637e9d2ee42ac85b30f62f86cd4c98d7 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Thu, 22 Oct 2026 19:31:21 +0200 Subject: [PATCH] fix(tabs): keep the case-default GET read-only, forget a stale id outside the rollback Merge-time fix for #676 (review nits 4 and 5). sessionCreated forgot a stale case default inside the try whose catch rolls the registration back; it now does so after the try, once the session is committed, so the cleanup can never drop a committed session from the live map. caseDefaultGroup (behind GET /api/cases/:name/default-group) no longer deletes a stale id as a side effect: it reports Ungrouped and leaves the value for the next creation to forget. The two tests that expected the read to clear it now expect it to write nothing; the creation test still pins the cleanup. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/api-reference.md | 3 ++- src/tab-layout-service.ts | 24 +++++++++---------- src/web/routes/case-routes.ts | 3 ++- test/case-default-group.test.ts | 6 +++-- test/routes/case-default-group-routes.test.ts | 5 ++-- 5 files changed, 23 insertions(+), 18 deletions(-) diff --git a/docs/api-reference.md b/docs/api-reference.md index 4837a376..99a42de1 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -783,7 +783,8 @@ non-string) is a `400 INVALID_INPUT`. Requests without the field are unchanged. - `GET /api/v1/cases/:name/default-group` → `{ caseName, groupId, groups }`: `groupId` is a group id from the caller's own tab layout or `null` (Ungrouped), `groups` the caller's current named groups (`{ id, name }`, in layout order). A stored id whose group was - deleted reads back as `null` and is forgotten. + deleted reads back as `null` (the read changes nothing; the next session created for the + case forgets it). - `PUT /api/v1/cases/:name/default-group` with `{ "groupId": string | null }` saves it. An id not in the caller's layout is a `400 INVALID_INPUT` that leaves the old value in place. diff --git a/src/tab-layout-service.ts b/src/tab-layout-service.ts index a6c781d6..a809a80a 100644 --- a/src/tab-layout-service.ts +++ b/src/tab-layout-service.ts @@ -584,6 +584,7 @@ export class TabLayoutService { const caseDefault = caseName ? this.deps.caseDefaults?.getCaseDefaultGroup(owner, caseName) : undefined; let staleCaseDefault = false; const rollback = register?.(); + let layout: TabLayout; try { const groupId = placement.tabGroupId; const adjust = @@ -600,33 +601,32 @@ export class TabLayoutService { return follow ? placed : pinCreatedChild(placed, sessionId, metadata); } : undefined; - const layout = await this.getUnlocked(owner, adjust); - // A default whose group was deleted is ignored above and forgotten here, - // only once the creation's own write has landed. - if (staleCaseDefault && caseName) this.deps.caseDefaults?.deleteCaseDefaultGroup(owner, caseName); - return layout; + layout = await this.getUnlocked(owner, adjust); } catch (error) { if (typeof rollback === 'function') rollback(); throw error; } + // A default whose group was deleted is ignored above and forgotten here, only once the + // creation's own write has landed, and outside the rollback: the session is committed by now, + // and nothing this cleanup does may drop it from the live map. + if (staleCaseDefault && caseName) this.deps.caseDefaults?.deleteCaseDefaultGroup(owner, caseName); + return layout; }); } /** * A case's default session group and the owner's named groups to choose from. - * A stored id that is no longer one of this owner's groups is forgotten and - * reported as `null` (Ungrouped), which is also where creation would put it. + * A stored id that is no longer one of this owner's groups is reported as + * `null` (Ungrouped), which is also where creation would put it. Read-only: + * the next creation for the case forgets the stale id. */ async caseDefaultGroup(owner: string, caseName: string): Promise { return this.withOwner(owner, async () => { const { authoritative } = await this.prepareUnlocked(owner); const groups = authoritative.groups.map((group) => ({ id: group.id, name: group.name })); const stored = this.deps.caseDefaults?.getCaseDefaultGroup(owner, caseName); - if (typeof stored === 'string' && !groups.some((group) => group.id === stored)) { - this.deps.caseDefaults?.deleteCaseDefaultGroup(owner, caseName); - return { groupId: null, groups }; - } - return { groupId: typeof stored === 'string' ? stored : null, groups }; + const known = typeof stored === 'string' && groups.some((group) => group.id === stored); + return { groupId: known ? stored : null, groups }; }); } diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index 6d5c7c42..8a679b91 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -1717,7 +1717,8 @@ export function registerCaseRoutes( * The tab group new sessions of this case land in when the create request names * no group and no parent places them. Scoped to the caller's own tab layout: the * options are their groups, and a stored id that is no longer one of them reads - * as Ungrouped (null) and is forgotten. Not admin-gated in multi-user mode: the + * as Ungrouped (null); the read writes nothing (the next creation for the case + * forgets it). Not admin-gated in multi-user mode: the * value is per owner (like the layout it points into), never server-wide. */ app.get('/api/cases/:name/default-group', async (req, reply) => { diff --git a/test/case-default-group.test.ts b/test/case-default-group.test.ts index 5b2c79b3..8a8c8c3c 100644 --- a/test/case-default-group.test.ts +++ b/test/case-default-group.test.ts @@ -299,7 +299,7 @@ describe('TabLayoutService case default placement', () => { }); describe('TabLayoutService case default reads and writes', () => { - it('reports the selection with the owner group options, clearing a stale id', async () => { + it('reports the selection with the owner group options, and a stale id as Ungrouped without writing', async () => { const h = createHarness({ layouts: { alice: grouped() }, live: facts() }); expect(await h.service.caseDefaultGroup('alice', 'app')).toEqual({ groupId: null, @@ -310,7 +310,9 @@ describe('TabLayoutService case default reads and writes', () => { }); h.defaults.alice = { app: 'gone' }; expect((await h.service.caseDefaultGroup('alice', 'app')).groupId).toBeNull(); - expect(h.caseDefaults.getCaseDefaultGroup('alice', 'app')).toBeUndefined(); + // A read never writes: the next creation for the case is what forgets the stale id. + expect(h.caseDefaults.deleteCaseDefaultGroup).not.toHaveBeenCalled(); + expect(h.caseDefaults.getCaseDefaultGroup('alice', 'app')).toBe('gone'); }); it('saves a known group or null, and refuses an id that is not in the owner layout', async () => { diff --git a/test/routes/case-default-group-routes.test.ts b/test/routes/case-default-group-routes.test.ts index 07ba2945..e34f4147 100644 --- a/test/routes/case-default-group-routes.test.ts +++ b/test/routes/case-default-group-routes.test.ts @@ -343,10 +343,11 @@ describe('case default group: GET/PUT /api/cases/:name/default-group', () => { expect(data.groups[1]).toEqual({ id: 'g-ops', name: 'Operations' }); }); - it('a deleted group reads back as Ungrouped and is cleared', async () => { + it('a deleted group reads back as Ungrouped, and the read changes nothing', async () => { world.defaults[OWNER] = { [CASE]: 'g-gone' }; expect((await get()).json().data.groupId).toBeNull(); - expect(CASE in world.defaults[OWNER]).toBe(false); + // GET is read-only; the next session created for the case forgets the stale id. + expect(world.defaults[OWNER][CASE]).toBe('g-gone'); }); it.each([