From 4f82be6ec2b95f407d555d917cc1bafb69272f07 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 26 Jun 2026 17:23:03 +0300 Subject: [PATCH] Address PR #215 review: temporary notes hardening Must-fix: - CHANGELOG: add [Unreleased]/Added entry for temporary notes (#201). - temporary-note-cleanup: re-check temporary_expires_at at deletion time so a concurrent "Make permanent" (sets it NULL) between the batch SELECT and the per-row removePage wins the race and the note is not trashed. Add unit tests for the make-permanent and already-trashed race windows. Non-blocking review items: - temporary-note-cleanup: cap the sweep batch (LIMIT 500) so a large backlog is not loaded into memory; remainder drains on the next hourly run. - client: extract duplicated post-toggle cache sync into syncTemporaryExpiresInCache() shared by the header menu and the banner. - Remove the tautological migration spec that mocked the whole Kysely builder. - Tests: cover create() frozen temporaryExpiresAt (workspace override + NULL default fallback + non-temporary skips lookup) and restorePage disarming the timer (temporaryExpiresAt: null). Deferred (forward-looking, non-blocking): extract PageService.computeTemporaryExpiresAt() to dedupe the deadline formula and drop the @InjectKysely from PageTemplateController; replace migration unit test with a real Postgres up/down integration test. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 10 +++ .../page-embed/queries/page-embed-query.ts | 27 ++++++ .../components/header/page-header-menu.tsx | 20 ++--- .../page/components/temporary-note-banner.tsx | 20 ++--- .../core/page/services/page.service.spec.ts | 76 ++++++++++++++++ .../temporary-note-cleanup.service.spec.ts | 50 ++++++++++- .../temporary-note-cleanup.service.ts | 31 +++++++ ...260626T130000-page-temporary-notes.spec.ts | 88 ------------------- .../repos/page/page.repo.restore.spec.ts | 64 ++++++++++++++ 9 files changed, 266 insertions(+), 120 deletions(-) delete mode 100644 apps/server/src/database/migrations/spec/20260626T130000-page-temporary-notes.spec.ts create mode 100644 apps/server/src/database/repos/page/page.repo.restore.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2aa9c9..b491ccab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Temporary notes — auto-move to Trash after a workspace lifetime.** A note can + be marked temporary so it auto-moves to Trash once a configurable workspace + lifetime elapses (default `DEFAULT_TEMPORARY_NOTE_HOURS` = 24h) unless made + permanent first. The deadline is frozen at creation time, so later changes to + the workspace setting never reschedule existing notes; an hourly background + sweep trashes notes past their deadline (children ride along). An open + temporary note shows a banner with a "Make permanent" rescue action; restoring + a note from Trash disarms the timer so it is not immediately re-trashed. + Operators configure the lifetime per workspace. (#201) + - **Persistent AI-chat history as the source of truth + server-side export.** An assistant turn is now persisted to the database step by step: the row is inserted upfront as `streaming` and updated as each agent step finishes, then diff --git a/apps/client/src/features/page-embed/queries/page-embed-query.ts b/apps/client/src/features/page-embed/queries/page-embed-query.ts index 2e037e44..2aaa9b0a 100644 --- a/apps/client/src/features/page-embed/queries/page-embed-query.ts +++ b/apps/client/src/features/page-embed/queries/page-embed-query.ts @@ -8,6 +8,33 @@ import type { ToggleTemplateResponse, ToggleTemporaryResponse, } from "@/features/page-embed/types/page-embed.types"; +import { queryClient } from "@/main.tsx"; + +/** + * After toggling a note's temporary state, mirror the new deadline into the + * shared page cache (keyed by both slugId and id) and refresh the sidebar so the + * menu label, the in-page banner, and the tree icon all reflect the change. + * Centralised here so the header menu and the banner can't drift apart on the + * cache-key plumbing. + */ +export function syncTemporaryExpiresInCache( + page: { id: string; slugId: string }, + temporaryExpiresAt: string | null, +) { + for (const key of [page.slugId, page.id]) { + const cached = queryClient.getQueryData(["pages", key]); + if (cached) { + queryClient.setQueryData(["pages", key], { + ...cached, + temporaryExpiresAt, + }); + } + } + queryClient.invalidateQueries({ + predicate: (item) => + ["sidebar-pages"].includes(item.queryKey[0] as string), + }); +} export function useToggleTemplateMutation() { return useMutation< diff --git a/apps/client/src/features/page/components/header/page-header-menu.tsx b/apps/client/src/features/page/components/header/page-header-menu.tsx index 8de3983a..fae3d898 100644 --- a/apps/client/src/features/page/components/header/page-header-menu.tsx +++ b/apps/client/src/features/page/components/header/page-header-menu.tsx @@ -25,8 +25,10 @@ import { useDisclosure, useHotkeys } from "@mantine/hooks"; import { useClipboard } from "@/hooks/use-clipboard"; import { useParams } from "react-router-dom"; import { usePageQuery } from "@/features/page/queries/page-query.ts"; -import { useToggleTemporaryMutation } from "@/features/page-embed/queries/page-embed-query.ts"; -import { queryClient } from "@/main.tsx"; +import { + useToggleTemporaryMutation, + syncTemporaryExpiresInCache, +} from "@/features/page-embed/queries/page-embed-query.ts"; import { buildPageUrl } from "@/features/page/page.utils.ts"; import { notifications } from "@mantine/notifications"; import { getAppUrl } from "@/lib/config.ts"; @@ -176,19 +178,7 @@ function PageActionMenu({ readOnly }: PageActionMenuProps) { }); // Reflect the new deadline in the page cache so the menu label flips and // any banner updates. The sidebar icon refreshes via its own query. - for (const key of [page.slugId, page.id]) { - const cached = queryClient.getQueryData(["pages", key]); - if (cached) { - queryClient.setQueryData(["pages", key], { - ...cached, - temporaryExpiresAt: res.temporaryExpiresAt, - }); - } - } - queryClient.invalidateQueries({ - predicate: (item) => - ["sidebar-pages"].includes(item.queryKey[0] as string), - }); + syncTemporaryExpiresInCache(page, res.temporaryExpiresAt); notifications.show({ message: next ? t("Note will move to trash unless made permanent") diff --git a/apps/client/src/features/page/components/temporary-note-banner.tsx b/apps/client/src/features/page/components/temporary-note-banner.tsx index 2fd92a3b..0c004abf 100644 --- a/apps/client/src/features/page/components/temporary-note-banner.tsx +++ b/apps/client/src/features/page/components/temporary-note-banner.tsx @@ -3,8 +3,10 @@ import { IconClockHour4 } from "@tabler/icons-react"; import { Trans, useTranslation } from "react-i18next"; import { useTimeAgo } from "@/hooks/use-time-ago.tsx"; import { usePageQuery } from "@/features/page/queries/page-query.ts"; -import { useToggleTemporaryMutation } from "@/features/page-embed/queries/page-embed-query.ts"; -import { queryClient } from "@/main.tsx"; +import { + useToggleTemporaryMutation, + syncTemporaryExpiresInCache, +} from "@/features/page-embed/queries/page-embed-query.ts"; import { useGetSpaceBySlugQuery } from "@/features/space/queries/space-query.ts"; import { useSpaceAbility } from "@/features/space/permissions/use-space-ability.ts"; import { @@ -42,19 +44,7 @@ export function TemporaryNoteBanner({ slugId }: TemporaryNoteBannerProps) { pageId: page.id, temporary: false, }); - for (const key of [page.slugId, page.id]) { - const cached = queryClient.getQueryData(["pages", key]); - if (cached) { - queryClient.setQueryData(["pages", key], { - ...cached, - temporaryExpiresAt: res.temporaryExpiresAt, - }); - } - } - queryClient.invalidateQueries({ - predicate: (item) => - ["sidebar-pages"].includes(item.queryKey[0] as string), - }); + syncTemporaryExpiresInCache(page, res.temporaryExpiresAt); } catch { // mutation surfaces the error via notifications } diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index bb387b31..3d9b3a55 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/apps/server/src/core/page/services/page.service.spec.ts @@ -2,6 +2,7 @@ import { BadRequestException } from '@nestjs/common'; import { PageService } from './page.service'; import { MovePageDto } from '../dto/move-page.dto'; import { Page } from '@docmost/db/types/entity.types'; +import { DEFAULT_TEMPORARY_NOTE_HOURS } from '../constants/temporary-note.constants'; // Direct instantiation with stub deps. The Test.createTestingModule form failed // to resolve the @InjectKysely()/@InjectQueue() tokens at compile(), and this @@ -389,4 +390,79 @@ describe('PageService', () => { }); }); }); + + describe('create() temporary deadline (#201)', () => { + // db stub for the workspaces.temporaryNoteHours lookup: + // selectFrom('workspaces').select(['temporaryNoteHours']).where(...).executeTakeFirst() + const makeDb = (workspaceRow: any) => { + const builder: any = { + selectFrom: jest.fn(() => builder), + select: jest.fn(() => builder), + where: jest.fn(() => builder), + executeTakeFirst: jest.fn().mockResolvedValue(workspaceRow), + }; + return builder; + }; + + const makeGeneralQueue = () => + ({ add: jest.fn().mockReturnValue({ catch: jest.fn() }) }) as any; + + const run = async (dto: any, workspaceRow: any) => { + const pageRepo = { + insertPage: jest.fn().mockResolvedValue({ id: 'p1' }), + }; + const db = makeDb(workspaceRow); + const svc = new PageService( + pageRepo as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // attachmentRepo + db as any, // db + {} as any, // storageService + {} as any, // attachmentQueue + {} as any, // aiQueue + makeGeneralQueue(), // generalQueue + {} as any, // eventEmitter + {} as any, // collaborationGateway + {} as any, // watcherService + {} as any, // transclusionService + ); + // nextPagePosition runs a real db query; stub it out. + jest.spyOn(svc, 'nextPagePosition').mockResolvedValue('a0' as any); + await svc.create('u1', 'w1', dto, undefined); + return { payload: pageRepo.insertPage.mock.calls[0][0], db }; + }; + + afterEach(() => jest.useRealTimers()); + + it('freezes temporaryExpiresAt at now + workspace hours when temporary', async () => { + jest.useFakeTimers().setSystemTime(new Date('2026-06-26T00:00:00.000Z')); + const { payload } = await run( + { title: 't', spaceId: 's1', temporary: true }, + { temporaryNoteHours: 5 }, + ); + expect(payload.temporaryExpiresAt).toEqual( + new Date(Date.now() + 5 * 60 * 60 * 1000), + ); + }); + + it('falls back to DEFAULT_TEMPORARY_NOTE_HOURS when the workspace hours are null', async () => { + jest.useFakeTimers().setSystemTime(new Date('2026-06-26T00:00:00.000Z')); + const { payload } = await run( + { title: 't', spaceId: 's1', temporary: true }, + { temporaryNoteHours: null }, + ); + expect(payload.temporaryExpiresAt).toEqual( + new Date(Date.now() + DEFAULT_TEMPORARY_NOTE_HOURS * 60 * 60 * 1000), + ); + }); + + it('leaves temporaryExpiresAt undefined and skips the workspace lookup for a non-temporary page', async () => { + const { payload, db } = await run( + { title: 't', spaceId: 's1' }, + { temporaryNoteHours: 5 }, + ); + expect(payload.temporaryExpiresAt).toBeUndefined(); + expect(db.selectFrom).not.toHaveBeenCalled(); + }); + }); }); diff --git a/apps/server/src/core/page/services/spec/temporary-note-cleanup.service.spec.ts b/apps/server/src/core/page/services/spec/temporary-note-cleanup.service.spec.ts index 1cb8d9dc..66ac3586 100644 --- a/apps/server/src/core/page/services/spec/temporary-note-cleanup.service.spec.ts +++ b/apps/server/src/core/page/services/spec/temporary-note-cleanup.service.spec.ts @@ -3,10 +3,17 @@ import { TemporaryNoteCleanupService } from '../temporary-note-cleanup.service'; /** * Chainable Kysely stub that records every `.where(...)` call so the test can * assert the sweep only selects armed, expired, not-yet-trashed notes. The - * terminal `.execute()` resolves the configured expired rows. + * terminal `.execute()` resolves the configured expired rows (the batch SELECT); + * `.executeTakeFirst()` resolves the per-row deadline re-read done just before + * each `removePage`. By default the re-read reports the note as still armed and + * still expired (epoch deadline < now), so the sweep proceeds to delete it; + * tests override `reReadFirst` to simulate a concurrent "Make permanent". */ function makeDbStub(expiredRows: any[]) { const whereCalls: any[][] = []; + const reReadFirst = jest + .fn() + .mockResolvedValue({ temporaryExpiresAt: new Date(0), deletedAt: null }); const builder: any = { selectFrom: jest.fn(() => builder), select: jest.fn(() => builder), @@ -14,9 +21,11 @@ function makeDbStub(expiredRows: any[]) { whereCalls.push(args); return builder; }), + limit: jest.fn(() => builder), execute: jest.fn().mockResolvedValue(expiredRows), + executeTakeFirst: reReadFirst, }; - return { builder, whereCalls }; + return { builder, whereCalls, reReadFirst }; } describe('TemporaryNoteCleanupService.sweepExpiredTemporaryNotes', () => { @@ -38,6 +47,9 @@ describe('TemporaryNoteCleanupService.sweepExpiredTemporaryNotes', () => { expect(ops).toEqual(['is not', '<', 'is']); // last operand is the trash filter -> null expect(whereCalls[2][2]).toBeNull(); + // The batch SELECT is capped so a large backlog is not pulled at once. + expect(builder.limit).toHaveBeenCalledTimes(1); + expect(builder.limit.mock.calls[0][0]).toBeGreaterThan(0); }); it('soft-deletes each expired note via removePage, attributed to its creator', async () => { @@ -77,6 +89,40 @@ describe('TemporaryNoteCleanupService.sweepExpiredTemporaryNotes', () => { expect(pageRepo.removePage).toHaveBeenNthCalledWith(2, 'good', 'u2', 'w1'); }); + it('does NOT trash a note made permanent in the race window', async () => { + // The batch SELECT saw the note as expired, but before its turn in the loop + // the user clicked "Make permanent" (temporary_expires_at -> null). The + // deadline re-read must catch this and skip the delete so the keep wins. + const expired = [{ id: 'p1', creatorId: 'u1', workspaceId: 'w1' }]; + const { builder, reReadFirst } = makeDbStub(expired); + reReadFirst.mockResolvedValueOnce({ + temporaryExpiresAt: null, + deletedAt: null, + }); + const pageRepo = { removePage: jest.fn() } as any; + const service = new TemporaryNoteCleanupService(builder, pageRepo); + + await service.sweepExpiredTemporaryNotes(); + + expect(reReadFirst).toHaveBeenCalledTimes(1); + expect(pageRepo.removePage).not.toHaveBeenCalled(); + }); + + it('skips a note already trashed since the batch SELECT', async () => { + const expired = [{ id: 'p1', creatorId: 'u1', workspaceId: 'w1' }]; + const { builder, reReadFirst } = makeDbStub(expired); + reReadFirst.mockResolvedValueOnce({ + temporaryExpiresAt: new Date(0), + deletedAt: new Date(), + }); + const pageRepo = { removePage: jest.fn() } as any; + const service = new TemporaryNoteCleanupService(builder, pageRepo); + + await service.sweepExpiredTemporaryNotes(); + + expect(pageRepo.removePage).not.toHaveBeenCalled(); + }); + it('does nothing when no notes are expired', async () => { const { builder } = makeDbStub([]); const pageRepo = { removePage: jest.fn() } as any; diff --git a/apps/server/src/core/page/services/temporary-note-cleanup.service.ts b/apps/server/src/core/page/services/temporary-note-cleanup.service.ts index 0cd4b55d..d08275ed 100644 --- a/apps/server/src/core/page/services/temporary-note-cleanup.service.ts +++ b/apps/server/src/core/page/services/temporary-note-cleanup.service.ts @@ -14,6 +14,11 @@ import { PageRepo } from '@docmost/db/repos/page/page.repo'; export class TemporaryNoteCleanupService { private readonly logger = new Logger(TemporaryNoteCleanupService.name); + // Cap a single sweep so a large backlog (e.g. many notes created during + // downtime under a short lifetime) is not loaded into memory at once. The + // remainder is drained on the next hourly run; sub-hour overshoot is fine. + private static readonly SWEEP_BATCH_LIMIT = 500; + constructor( @InjectKysely() private readonly db: KyselyDB, private readonly pageRepo: PageRepo, @@ -32,11 +37,37 @@ export class TemporaryNoteCleanupService { .where('temporaryExpiresAt', 'is not', null) .where('temporaryExpiresAt', '<', now) .where('deletedAt', 'is', null) // not already in trash + .limit(TemporaryNoteCleanupService.SWEEP_BATCH_LIMIT) .execute(); let trashed = 0; for (const page of expired) { try { + // Re-check the deadline at deletion time. The SELECT above is not + // transactional, so a user may click "Make permanent" + // (toggleTemporary sets temporary_expires_at = null) in the window + // between the SELECT and this per-row removePage. removePage deletes + // by id with only a `deletedAt IS NULL` filter and never re-reads the + // deadline, so without this guard a concurrently-kept note would + // still be trashed. Re-read the row and skip it unless it is still + // armed AND still expired, so a concurrent make-permanent wins. + const current = await this.db + .selectFrom('pages') + .select(['temporaryExpiresAt', 'deletedAt']) + .where('id', '=', page.id) + .executeTakeFirst(); + + if ( + !current || + current.deletedAt !== null || + current.temporaryExpiresAt === null || + new Date(current.temporaryExpiresAt) >= now + ) { + // Made permanent, already trashed, or no longer expired since the + // SELECT — leave it alone. + continue; + } + // Reuse the exact soft-delete path: recursive over children, removes // shares in a transaction, and emits PAGE_SOFT_DELETED (tree // invalidation + watcher notifications). Attribute the automatic diff --git a/apps/server/src/database/migrations/spec/20260626T130000-page-temporary-notes.spec.ts b/apps/server/src/database/migrations/spec/20260626T130000-page-temporary-notes.spec.ts deleted file mode 100644 index 9be98e0b..00000000 --- a/apps/server/src/database/migrations/spec/20260626T130000-page-temporary-notes.spec.ts +++ /dev/null @@ -1,88 +0,0 @@ -// Mock the `sql` tagged template so the migration's partial-index statement is -// recorded without a real database. Keep `Kysely` (type-only) intact. -const sqlCalls: string[] = []; -jest.mock('kysely', () => ({ - sql: (strings: TemplateStringsArray) => { - sqlCalls.push(strings.join('{}')); - return { execute: jest.fn().mockResolvedValue(undefined) }; - }, -})); - -import { - up, - down, -} from '../20260626T130000-page-temporary-notes'; - -/** - * Chainable Kysely schema stub: each builder method returns `this` and records - * (method, firstArg) so the test can assert the columns/index the migration - * touches. `addColumn` runs its column-builder callback to exercise it. - */ -function makeSchemaStub() { - const calls: Array<[string, any]> = []; - const colBuilder: any = new Proxy( - {}, - { get: () => () => colBuilder }, - ); - const builder: any = { - schema: {} as any, - alterTable(name: string) { - calls.push(['alterTable', name]); - return builder; - }, - addColumn(name: string, _type: any, cb?: (c: any) => any) { - calls.push(['addColumn', name]); - if (cb) cb(colBuilder); - return builder; - }, - dropColumn(name: string) { - calls.push(['dropColumn', name]); - return builder; - }, - dropIndex(name: string) { - calls.push(['dropIndex', name]); - return builder; - }, - execute: jest.fn().mockResolvedValue(undefined), - }; - builder.schema = builder; - return { db: builder, calls }; -} - -describe('migration 20260626T130000-page-temporary-notes', () => { - beforeEach(() => { - sqlCalls.length = 0; - }); - - it('up adds both columns and creates the partial cleanup index', async () => { - const { db, calls } = makeSchemaStub(); - await up(db); - - const added = calls.filter((c) => c[0] === 'addColumn').map((c) => c[1]); - expect(added).toContain('temporary_expires_at'); - expect(added).toContain('temporary_note_hours'); - - const altered = calls.filter((c) => c[0] === 'alterTable').map((c) => c[1]); - expect(altered).toContain('pages'); - expect(altered).toContain('workspaces'); - - // The partial index is created via the raw sql tag. - expect(sqlCalls.join(' ')).toContain('pages_temporary_expires_at_idx'); - expect(sqlCalls.join(' ')).toContain('temporary_expires_at IS NOT NULL'); - expect(sqlCalls.join(' ')).toContain('deleted_at IS NULL'); - }); - - it('down reverses both columns and drops the index', async () => { - const { db, calls } = makeSchemaStub(); - await down(db); - - const dropped = calls.filter((c) => c[0] === 'dropColumn').map((c) => c[1]); - expect(dropped).toContain('temporary_expires_at'); - expect(dropped).toContain('temporary_note_hours'); - - const droppedIdx = calls - .filter((c) => c[0] === 'dropIndex') - .map((c) => c[1]); - expect(droppedIdx).toContain('pages_temporary_expires_at_idx'); - }); -}); diff --git a/apps/server/src/database/repos/page/page.repo.restore.spec.ts b/apps/server/src/database/repos/page/page.repo.restore.spec.ts new file mode 100644 index 00000000..72f6fbe8 --- /dev/null +++ b/apps/server/src/database/repos/page/page.repo.restore.spec.ts @@ -0,0 +1,64 @@ +import { PageRepo } from './page.repo'; + +/** + * Regression guard for #201: restorePage must disarm the temporary-note death + * timer by setting `temporaryExpiresAt = null` alongside the un-delete fields. + * Otherwise a restored note whose frozen deadline already passed would be + * re-trashed by the very next cleanup sweep. There is no real DB here — a + * chainable Kysely proxy records every `.set(...)` payload so we can assert the + * single restore UPDATE clears the deadline. + */ +function makeRestoreDbStub(opts: { + pageToRestore: any; + descendants: any[]; +}) { + const setCalls: any[] = []; + const proxy: any = new Proxy(function () {}, { + get(_t, prop) { + if (prop === 'then') return undefined; + if (prop === 'set') + return (payload: any) => { + setCalls.push(payload); + return proxy; + }; + if (prop === 'executeTakeFirst') + return () => Promise.resolve(opts.pageToRestore); + if (prop === 'execute') return () => Promise.resolve(opts.descendants); + if (prop === 'withRecursive') + return (_name: string, cb: any) => { + // Exercise the recursive CTE builder against the proxy without a DB. + try { + cb(proxy); + } catch { + // builder shape only; ignore + } + return proxy; + }; + return () => proxy; + }, + }); + return { proxy, setCalls }; +} + +describe('PageRepo.restorePage temporary-timer disarm (#201)', () => { + it('clears temporaryExpiresAt together with the un-delete fields', async () => { + const { proxy, setCalls } = makeRestoreDbStub({ + // No parent => the deleted-parent lookup and detach branch are skipped, so + // the only UPDATE is the bulk restore we assert on. + pageToRestore: { id: 'p1', parentPageId: null, spaceId: 's1' }, + descendants: [{ id: 'p1' }], + }); + const eventEmitter = { emit: jest.fn() } as any; + + const repo = new PageRepo(proxy, {} as any, eventEmitter); + + await repo.restorePage('p1', 'w1'); + + expect(setCalls).toHaveLength(1); + expect(setCalls[0]).toEqual({ + deletedById: null, + deletedAt: null, + temporaryExpiresAt: null, + }); + }); +});