From 729023893c723992814f762ac0daee1606ea421c Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 03:48:01 +0300 Subject: [PATCH] fix(git-sync): never trash a page whose pageId still exists in the tree (cross-cycle move) + browser e2e MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/server/test/git-sync-browser-e2e.cjs | 102 ++++++++++++++++++ packages/git-sync/src/engine/push.ts | 39 ++++++- .../test/compute-push-actions.test.ts | 38 +++++++ 3 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 apps/server/test/git-sync-browser-e2e.cjs diff --git a/apps/server/test/git-sync-browser-e2e.cjs b/apps/server/test/git-sync-browser-e2e.cjs new file mode 100644 index 00000000..35cce07f --- /dev/null +++ b/apps/server/test/git-sync-browser-e2e.cjs @@ -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); }); diff --git a/packages/git-sync/src/engine/push.ts b/packages/git-sync/src/engine/push.ts index 4750b2bc..96e9c5c3 100644 --- a/packages/git-sync/src/engine/push.ts +++ b/packages/git-sync/src/engine/push.ts @@ -200,6 +200,15 @@ export interface PushActionsInput { * pass a plain lookup. */ 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; } /** @@ -228,7 +237,7 @@ export interface PushActionsInput { * (`C` copy is treated the same as `R` for recording purposes.) */ export function computePushActions(input: PushActionsInput): PushActions { - const { changes, metaAt } = input; + const { changes, metaAt, currentPageIds } = input; const actions: PushActions = { creates: [], updates: [], @@ -343,6 +352,16 @@ export function computePushActions(input: PushActionsInput): PushActions { status: "D", 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) { actions.deletes.push({ pageId }); } else { @@ -978,6 +997,7 @@ export interface PushDeps { | "showFileAtRef" | "updateRef" | "fastForwardBranch" + | "listTrackedFiles" >; /** Build a real client — called ONLY on `--apply`, never on dry-run. */ makeClient: (settings: Settings) => ApplyPushDeps["client"]; @@ -1149,7 +1169,22 @@ export async function runPush( const metaAt = (path: string, side: MetaSide): DocmostMdMeta | 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 | undefined; + if (changes.some((c) => c.status === "D")) { + currentPageIds = new Set(); + 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 = { creates: actions.creates.length, updates: actions.updates.length, diff --git a/packages/git-sync/test/compute-push-actions.test.ts b/packages/git-sync/test/compute-push-actions.test.ts index 22b74a55..85a27ad8 100644 --- a/packages/git-sync/test/compute-push-actions.test.ts +++ b/packages/git-sync/test/compute-push-actions.test.ts @@ -280,3 +280,41 @@ describe('computePushActions — ghost-move coalescing (data-loss guard)', () => 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([]); + }); +});