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) <noreply@anthropic.com>
This commit is contained in:
@@ -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');
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user