fix(git-sync): never trash a page whose pageId still exists in the tree (cross-cycle move) + browser e2e
Follow-up to 4376c5a6, found by a real BROWSER e2e (the flow the in-diff fix
missed). When the layout reshuffle's two halves land in SEPARATE sync cycles, the
later cycle's diff has only the DELETE of the old path — the matching add was
already pushed — so in-diff D+A coalescing can't see it, and the live page was
still trashed.
Robust fix on the identity invariant the reviewer (and the user) called out: a
page EXISTS iff its pageId is in the vault, regardless of filename. runPush now
collects the pageIds present at ANY path in the current `main` tree and passes
them to computePushActions; a deleted file whose pageId is still tracked
elsewhere is a MOVE, never a deletion. (Built only when the diff has deletes.)
Adds apps/server/test/git-sync-browser-e2e.cjs — a Playwright test that drives the
REAL Docmost web UI: log in, create several untitled pages, type a title, sync,
assert NOTHING is trashed. Reproduced the data loss before this fix; 5/5 green and
stable after. Engine suite 600 green (+2 computePushActions cases:
pageId-still-present -> skip; pageId-gone -> real delete).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
102
apps/server/test/git-sync-browser-e2e.cjs
Normal file
102
apps/server/test/git-sync-browser-e2e.cjs
Normal file
@@ -0,0 +1,102 @@
|
|||||||
|
#!/usr/bin/env node
|
||||||
|
/*
|
||||||
|
* git-sync BROWSER e2e — drives the real Docmost web UI with Playwright to
|
||||||
|
* reproduce the exact user flow that previously caused data loss: pages created
|
||||||
|
* in the browser start UNTITLED (all collapse to the `_` vault filename); typing
|
||||||
|
* a title reshuffles that collision and used to TRASH another live page. This
|
||||||
|
* test creates several pages via the UI, titles one, runs a sync, and asserts
|
||||||
|
* NOTHING was moved to Trash.
|
||||||
|
*
|
||||||
|
* Setup: needs Playwright + a Chromium build. The project should add
|
||||||
|
* `@playwright/test` as a devDep (`pnpm dlx playwright install chromium`). This
|
||||||
|
* script resolves playwright-core + the chromium binary from env so it can run
|
||||||
|
* against an already-installed browser:
|
||||||
|
* PW_CORE=/path/to/node_modules/playwright-core
|
||||||
|
* PW_CHROME=/path/to/chrome
|
||||||
|
* and the live stand env (SERVER/SPACE_ID/EMAIL/PASSWORD/DB_CONTAINER) like the
|
||||||
|
* shell e2e suites.
|
||||||
|
*/
|
||||||
|
const { execSync } = require('node:child_process');
|
||||||
|
|
||||||
|
const SERVER = process.env.SERVER || 'http://localhost:3000';
|
||||||
|
const WEB = process.env.WEB || 'http://localhost:5173';
|
||||||
|
const SPACE_ID = process.env.SPACE_ID || '019ef1f7-437b-7ae9-9306-809a1729f085';
|
||||||
|
const SPACE_SLUG = process.env.SPACE_SLUG || 'general';
|
||||||
|
const EMAIL = process.env.EMAIL || 'admin@test.local';
|
||||||
|
const PASSWORD = process.env.PASSWORD || 'Test12345!';
|
||||||
|
const DB = process.env.DB_CONTAINER || 'gitmost-db';
|
||||||
|
const PW_CORE = process.env.PW_CORE || '/home/claude/pw/node_modules/playwright-core';
|
||||||
|
const PW_CHROME = process.env.PW_CHROME ||
|
||||||
|
'/home/claude/.cache/ms-playwright/chromium-1148/chrome-linux/chrome';
|
||||||
|
|
||||||
|
const { chromium } = require(PW_CORE);
|
||||||
|
const psql = (q) =>
|
||||||
|
execSync(`docker exec ${DB} psql -U docmost -d docmost -tAc "${q}"`, { encoding: 'utf8' }).trim();
|
||||||
|
const trashedCount = () =>
|
||||||
|
Number(psql(`select count(*) from pages where space_id='${SPACE_ID}' and deleted_at is not null`));
|
||||||
|
let cookie = '';
|
||||||
|
const login = () => {
|
||||||
|
const out = execSync(
|
||||||
|
`curl -s -i -X POST ${SERVER}/api/auth/login -H 'Content-Type: application/json' -d '{"email":"${EMAIL}","password":"${PASSWORD}"}'`,
|
||||||
|
{ encoding: 'utf8' });
|
||||||
|
cookie = (out.match(/authToken=([^;]+)/) || [])[1] || '';
|
||||||
|
};
|
||||||
|
const sync = () => execSync(
|
||||||
|
`curl -s -b 'authToken=${cookie}' -X POST ${SERVER}/api/git-sync/trigger -H 'Content-Type: application/json' -d '{"spaceId":"${SPACE_ID}"}'`,
|
||||||
|
{ encoding: 'utf8' });
|
||||||
|
|
||||||
|
let pass = 0, fail = 0;
|
||||||
|
const ok = (m) => { console.log(' \x1b[32mPASS\x1b[0m ' + m); pass++; };
|
||||||
|
const bad = (m) => { console.log(' \x1b[31mFAIL\x1b[0m ' + m); fail++; };
|
||||||
|
|
||||||
|
(async () => {
|
||||||
|
login();
|
||||||
|
const trashBefore = trashedCount();
|
||||||
|
const browser = await chromium.launch({ executablePath: PW_CHROME, args: ['--no-sandbox'] });
|
||||||
|
const page = await browser.newPage();
|
||||||
|
try {
|
||||||
|
// --- log in through the UI ---
|
||||||
|
await page.goto(`${WEB}/login`, { waitUntil: 'networkidle' });
|
||||||
|
await page.getByPlaceholder('email@example.com').fill(EMAIL);
|
||||||
|
await page.getByPlaceholder(/password/i).fill(PASSWORD);
|
||||||
|
await page.getByRole('button', { name: /sign in|log in|login|войти/i }).click();
|
||||||
|
await page.waitForTimeout(2000);
|
||||||
|
ok('logged in via the browser');
|
||||||
|
|
||||||
|
// --- create several UNTITLED pages via the UI (the bug trigger) ---
|
||||||
|
await page.goto(`${WEB}/s/${SPACE_SLUG}`, { waitUntil: 'networkidle' });
|
||||||
|
await page.waitForTimeout(1200);
|
||||||
|
const createdUrls = [];
|
||||||
|
for (let i = 0; i < 3; i++) {
|
||||||
|
await page.getByRole('button', { name: 'Create page' }).first().click();
|
||||||
|
await page.waitForTimeout(1500);
|
||||||
|
createdUrls.push(page.url());
|
||||||
|
sync(); // each create fires a real git-sync cycle
|
||||||
|
}
|
||||||
|
ok('created 3 untitled pages through the UI');
|
||||||
|
|
||||||
|
// --- type a title into the page currently open (retitle == the trigger) ---
|
||||||
|
const titleEditor = page.locator('.tiptap.ProseMirror').first();
|
||||||
|
await titleEditor.click();
|
||||||
|
await page.keyboard.type('Заголовок через браузер');
|
||||||
|
await page.waitForTimeout(1500); // debounced save
|
||||||
|
sync(); sync();
|
||||||
|
ok('typed a title into one page');
|
||||||
|
|
||||||
|
// --- THE assertion: nothing got trashed by the reshuffle ---
|
||||||
|
const trashAfter = trashedCount();
|
||||||
|
if (trashAfter === trashBefore) ok(`no page trashed by the untitled+retitle flow (trash stayed ${trashBefore})`);
|
||||||
|
else bad(`a page was TRASHED by the browser flow (trash ${trashBefore} -> ${trashAfter}) — DATA LOSS`);
|
||||||
|
|
||||||
|
// the titled page must still be live
|
||||||
|
const titled = Number(psql(`select count(*) from pages where space_id='${SPACE_ID}' and title='Заголовок через браузер' and deleted_at is null`));
|
||||||
|
if (titled === 1) ok('the titled page is live'); else bad('the titled page is not live');
|
||||||
|
} finally {
|
||||||
|
await browser.close();
|
||||||
|
// cleanup: hard-delete the pages this run created (titled + the untitled ones from this run)
|
||||||
|
psql(`delete from pages where space_id='${SPACE_ID}' and (title='Заголовок через браузер' or (title='' and created_at > now() - interval '5 minutes'))`);
|
||||||
|
sync();
|
||||||
|
}
|
||||||
|
console.log(`\nRESULTS: ${pass} passed, ${fail} failed`);
|
||||||
|
process.exit(fail === 0 ? 0 : 1);
|
||||||
|
})().catch((e) => { console.error(e); process.exit(2); });
|
||||||
@@ -200,6 +200,15 @@ export interface PushActionsInput {
|
|||||||
* pass a plain lookup.
|
* pass a plain lookup.
|
||||||
*/
|
*/
|
||||||
metaAt: (path: string, side: MetaSide) => DocmostMdMeta | null;
|
metaAt: (path: string, side: MetaSide) => DocmostMdMeta | null;
|
||||||
|
/**
|
||||||
|
* The pageIds present at ANY path in the current `main` tree (optional). When
|
||||||
|
* given, a deleted file whose pageId still lives somewhere in the tree is NOT
|
||||||
|
* a deletion but a MOVE — guards against trashing a live page when a layout
|
||||||
|
* reshuffle relocated its file (possibly across two cycles, so the matching
|
||||||
|
* add isn't in THIS diff). When omitted, only the in-diff D+A/M coalescing
|
||||||
|
* applies.
|
||||||
|
*/
|
||||||
|
currentPageIds?: Set<string>;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -228,7 +237,7 @@ export interface PushActionsInput {
|
|||||||
* (`C` copy is treated the same as `R` for recording purposes.)
|
* (`C` copy is treated the same as `R` for recording purposes.)
|
||||||
*/
|
*/
|
||||||
export function computePushActions(input: PushActionsInput): PushActions {
|
export function computePushActions(input: PushActionsInput): PushActions {
|
||||||
const { changes, metaAt } = input;
|
const { changes, metaAt, currentPageIds } = input;
|
||||||
const actions: PushActions = {
|
const actions: PushActions = {
|
||||||
creates: [],
|
creates: [],
|
||||||
updates: [],
|
updates: [],
|
||||||
@@ -343,6 +352,16 @@ export function computePushActions(input: PushActionsInput): PushActions {
|
|||||||
status: "D",
|
status: "D",
|
||||||
reason: "ghost-move (re-added at a new path) — not a deletion",
|
reason: "ghost-move (re-added at a new path) — not a deletion",
|
||||||
});
|
});
|
||||||
|
} else if (pageId && currentPageIds?.has(pageId)) {
|
||||||
|
// The pageId still EXISTS elsewhere in the current tree: the file moved
|
||||||
|
// (a layout reshuffle whose matching add was in an earlier cycle, so it
|
||||||
|
// is not in this diff). A live page must never be trashed because its
|
||||||
|
// FILENAME changed — identity is the pageId (⭐ data-loss guard).
|
||||||
|
actions.skipped.push({
|
||||||
|
path: change.path,
|
||||||
|
status: "D",
|
||||||
|
reason: "pageId still present in the tree (moved) — not a deletion",
|
||||||
|
});
|
||||||
} else if (pageId) {
|
} else if (pageId) {
|
||||||
actions.deletes.push({ pageId });
|
actions.deletes.push({ pageId });
|
||||||
} else {
|
} else {
|
||||||
@@ -978,6 +997,7 @@ export interface PushDeps {
|
|||||||
| "showFileAtRef"
|
| "showFileAtRef"
|
||||||
| "updateRef"
|
| "updateRef"
|
||||||
| "fastForwardBranch"
|
| "fastForwardBranch"
|
||||||
|
| "listTrackedFiles"
|
||||||
>;
|
>;
|
||||||
/** Build a real client — called ONLY on `--apply`, never on dry-run. */
|
/** Build a real client — called ONLY on `--apply`, never on dry-run. */
|
||||||
makeClient: (settings: Settings) => ApplyPushDeps["client"];
|
makeClient: (settings: Settings) => ApplyPushDeps["client"];
|
||||||
@@ -1149,7 +1169,22 @@ export async function runPush(
|
|||||||
const metaAt = (path: string, side: MetaSide): DocmostMdMeta | null =>
|
const metaAt = (path: string, side: MetaSide): DocmostMdMeta | null =>
|
||||||
metaTable.get(`${path}|${side}`) ?? null;
|
metaTable.get(`${path}|${side}`) ?? null;
|
||||||
|
|
||||||
const actions = computePushActions({ changes, metaAt });
|
// The set of pageIds that STILL EXIST somewhere in the current `main` tree.
|
||||||
|
// Identity is the pageId, NOT the filename: a file vanishing from one path
|
||||||
|
// while the SAME pageId lives at another path is a MOVE (often a layout
|
||||||
|
// reshuffle of `_`-fallback names, whose two halves can even land in separate
|
||||||
|
// cycles), never a deletion. Built only when the diff contains deletes — the
|
||||||
|
// guard's whole job is to stop a phantom delete from trashing a live page.
|
||||||
|
let currentPageIds: Set<string> | undefined;
|
||||||
|
if (changes.some((c) => c.status === "D")) {
|
||||||
|
currentPageIds = new Set<string>();
|
||||||
|
for (const relPath of await git.listTrackedFiles("*.md")) {
|
||||||
|
const pid = (await readMetaCurrent(deps, relPath))?.pageId;
|
||||||
|
if (pid) currentPageIds.add(pid);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
const actions = computePushActions({ changes, metaAt, currentPageIds });
|
||||||
const planned = {
|
const planned = {
|
||||||
creates: actions.creates.length,
|
creates: actions.creates.length,
|
||||||
updates: actions.updates.length,
|
updates: actions.updates.length,
|
||||||
|
|||||||
@@ -280,3 +280,41 @@ describe('computePushActions — ghost-move coalescing (data-loss guard)', () =>
|
|||||||
expect(actions.renamesMoves).toEqual([]);
|
expect(actions.renamesMoves).toEqual([]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('computePushActions — currentPageIds guard (cross-cycle move)', () => {
|
||||||
|
it('a D whose pageId still exists in the tree (no matching A in THIS diff) is NOT deleted', () => {
|
||||||
|
// The move happened across cycles: the new file landed earlier, so this diff
|
||||||
|
// only has the old path D. The pageId still lives in the tree -> not a delete.
|
||||||
|
const changes: DiffEntry[] = [{ status: 'D', path: '_ ~old.md' }];
|
||||||
|
const metaAt = metaTable({
|
||||||
|
'_ ~old.md|prev': meta({ pageId: 'pX', title: '', spaceId: 'sp1' }),
|
||||||
|
});
|
||||||
|
const actions = computePushActions({
|
||||||
|
changes,
|
||||||
|
metaAt,
|
||||||
|
currentPageIds: new Set(['pX']), // pX is still tracked somewhere on main
|
||||||
|
});
|
||||||
|
expect(actions.deletes).toEqual([]);
|
||||||
|
expect(actions.skipped).toEqual([
|
||||||
|
{
|
||||||
|
path: '_ ~old.md',
|
||||||
|
status: 'D',
|
||||||
|
reason: 'pageId still present in the tree (moved) — not a deletion',
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('a D whose pageId is GONE from the tree is a real delete', () => {
|
||||||
|
const changes: DiffEntry[] = [{ status: 'D', path: 'Removed.md' }];
|
||||||
|
const metaAt = metaTable({
|
||||||
|
'Removed.md|prev': meta({ pageId: 'pY', title: 'Removed', spaceId: 'sp1' }),
|
||||||
|
});
|
||||||
|
const actions = computePushActions({
|
||||||
|
changes,
|
||||||
|
metaAt,
|
||||||
|
currentPageIds: new Set(['pOther']), // pY is NOT present -> genuinely deleted
|
||||||
|
});
|
||||||
|
expect(actions.deletes).toEqual([{ pageId: 'pY' }]);
|
||||||
|
expect(actions.skipped).toEqual([]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user