fix(#248 F1): remove dead intentional-clear escape hatch from empty-guard
Round-1 review F1 (maintainer decision: variant A). The store-side
empty-guard's `context?.intentionalClear === true` branch was dead:
`intentionalClear` is never set in production (connection context is
{user, actor, aiChatId}); it appeared only in the guard and a hand-injected
spec, so the guard already blocked empty-over-non-empty unconditionally.
- persistence.extension.ts: drop the dead branch; the guard now simply
skips empty-over-non-empty, full stop. Reference issue #251 (real
intentional-clear UX) in the comment where the branch was.
- persistence-store.spec.ts: remove the misleading "persists an intentional
clear" escape-hatch test (false coverage — green only because the flag was
injected by hand). Real guard tests (empty-over-empty allowed,
empty-over-non-empty blocked, etc.) kept.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -210,8 +210,8 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot'
|
||||
// but not the STORE path, so an empty doc (a client/agent glitch, a bad
|
||||
// merge, an emptying transclusion) was written straight over the page and the
|
||||
// content was wiped silently. The store-side empty-guard now skips the write
|
||||
// when the incoming doc is empty and the stored page is non-empty, unless an
|
||||
// explicit intentional-clear signal is present.
|
||||
// when the incoming doc is empty and the stored page is non-empty. A real
|
||||
// intentional-clear UX is tracked separately in issue #251.
|
||||
it('does NOT overwrite non-empty content with a momentarily-empty live doc (persist-6)', async () => {
|
||||
const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] };
|
||||
const document = ydocFor(emptyDoc);
|
||||
@@ -229,29 +229,6 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot'
|
||||
expect(historyQueue.add).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// persist-6 — a legitimate clear is NOT broken: with the explicit
|
||||
// intentional-clear signal, emptying a non-empty page still persists.
|
||||
it('persists an intentional clear of a non-empty page (persist-6 escape hatch)', async () => {
|
||||
const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] };
|
||||
const document = ydocFor(emptyDoc);
|
||||
pageRepo.findById.mockResolvedValue({
|
||||
...persistedHumanPage('IGNORED'),
|
||||
content: doc('IMPORTANT RICH CONTENT'),
|
||||
});
|
||||
|
||||
await ext.onStoreDocument({
|
||||
documentName: `page.${PAGE_ID}`,
|
||||
document,
|
||||
context: {
|
||||
user: { id: USER_ID, name: 'Alice' },
|
||||
actor: 'user',
|
||||
intentionalClear: true,
|
||||
},
|
||||
} as any);
|
||||
|
||||
expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// persist-6 — a brand-new / already-empty page is unaffected: an empty store
|
||||
// over empty stored content is not blocked (it short-circuits as unchanged).
|
||||
it('does not block an empty store over an already-empty page (persist-6)', async () => {
|
||||
|
||||
@@ -217,21 +217,23 @@ export class PersistenceExtension implements Extension {
|
||||
// when the live doc isEmpty); the STORE path did not, so an empty
|
||||
// serialization was written straight over the page, wiping it
|
||||
// silently. Skip the write when the incoming doc is an empty
|
||||
// paragraph doc AND the stored page is non-empty — unless the writer
|
||||
// sends an explicit intentional-clear signal (a deliberate
|
||||
// select-all + delete), the one case where emptying is the user's
|
||||
// intent. New/empty pages are unaffected (stored content is already
|
||||
// empty), and an unchanged doc was already short-circuited above.
|
||||
const intentionalClear = context?.intentionalClear === true;
|
||||
// paragraph doc AND the stored page is non-empty. New/empty pages are
|
||||
// unaffected (stored content is already empty), and an unchanged doc
|
||||
// was already short-circuited above.
|
||||
//
|
||||
// This unconditionally blocks empty-over-non-empty: a deliberate
|
||||
// select-all + delete is currently indistinguishable from a glitch at
|
||||
// this layer, so data-loss prevention wins. A real intentional-clear
|
||||
// UX (a distinct signal threaded from the client) is tracked in issue
|
||||
// #251; do not re-add an escape hatch here without that signal.
|
||||
if (
|
||||
!intentionalClear &&
|
||||
isEmptyParagraphDoc(tiptapJson as any) &&
|
||||
page.content &&
|
||||
!isEmptyParagraphDoc(page.content as any)
|
||||
) {
|
||||
this.logger.warn(
|
||||
`Skipping store for ${pageId}: empty live doc would overwrite ` +
|
||||
`non-empty persisted content (no intentional-clear signal)`,
|
||||
`non-empty persisted content`,
|
||||
);
|
||||
page = null;
|
||||
return;
|
||||
|
||||
Reference in New Issue
Block a user