fix(git-sync): unwedge per-page conflicts, preserve callout types, flush collab on disconnect
Addresses QA findings on PR #119 (issues #235/#236). SYNC-WEDGE (HIGH): one same-line conflict on one page froze sync for the WHOLE space in both directions forever. The pull's docmost->main merge left the vault mid-merge, so every later cycle's isMergeInProgress() check returned skipped:"merge-in-progress" and skipped the entire space with no recovery. - pull.ts now COMMITS a conflicting merge with markers in place (commitMerge): cleanly-merged pages land, the conflicted page carries its markers on main and is isolated by the existing push-side conflict-marker skip (markers never reach Docmost), and the next cycle is no longer wedged. conflictedPaths is surfaced. - cycle.ts now RECOVERS a vault left mid-merge by a prior/pre-fix cycle: it aborts the stale merge (merge --abort, hard-reset fallback) and continues, instead of skipping the space forever. - git.ts: listUnmergedPaths / commitMerge / abortMerge / resetHardToHead. CALLOUT TYPE FIDELITY: git-sync's CALLOUT_TYPES was missing "note" and "default" (editor-canonical types), so [!note]/[!default] callouts flattened to [!info] on every round-trip. Aligned the list with @docmost/editor-ext getValidCalloutType. LOSS-ON-FAST-CLOSE: editing a page then closing the tab inside the collab debounce window (~3-18s) lost the edit, because with unloadImmediately:false Hocuspocus does not flush the debounced onStoreDocument on the last-client disconnect. PersistenceExtension.onDisconnect now flushes the pending store (debouncer.executeNow) on the last disconnect only, with no redundant write. DUPLICATION re-verify (#1): the schema-default merge-key normalization is intact; faithful toYdoc-based reproduction shows callout + rich content resync with 0 ops and no growth/strip across cycles -> the re-report was leftover vault data, not a live regression. Locked with a callout regression spec. Tests: git-sync 688 pass (incl. real-VaultGit wedge-recovery integration); server git-sync+collaboration 285 pass; new callout merge/fidelity + onDisconnect-flush specs. tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -66,6 +66,10 @@ function makeGit(merge: { ok: boolean; conflict: boolean; output?: string } = {
|
||||
order.push('merge');
|
||||
return { ok: merge.ok, conflict: merge.conflict, output: merge.output ?? '' };
|
||||
}),
|
||||
listUnmergedPaths: vi.fn(async () => ['Conflicted.md']),
|
||||
commitMerge: vi.fn(async (subject: string) => {
|
||||
order.push(`commitMerge:${subject}`);
|
||||
}),
|
||||
};
|
||||
return {
|
||||
git,
|
||||
@@ -403,7 +407,11 @@ describe('applyPullActions — commit subject reflects ACTUAL counts', () => {
|
||||
});
|
||||
|
||||
describe('applyPullActions — merge result is surfaced, not swallowed', () => {
|
||||
it('returns conflict:true on a conflicting merge (no auto-resolve)', async () => {
|
||||
it('COMMITS a conflicting merge with markers (no wedge) and surfaces conflictedPaths', async () => {
|
||||
// Regression for the WEDGE bug (QA #119): a conflicting docmost -> main merge
|
||||
// must NOT be left mid-merge (which wedged the whole space). It is committed
|
||||
// WITH markers so the rest of the space keeps syncing; the conflicted page is
|
||||
// surfaced in `conflictedPaths` and isolated by the push side.
|
||||
const { client } = makeClient();
|
||||
const g = makeGit({ ok: false, conflict: true, output: 'CONFLICT' });
|
||||
const fs = makeFs();
|
||||
@@ -415,6 +423,10 @@ describe('applyPullActions — merge result is surfaced, not swallowed', () => {
|
||||
);
|
||||
expect(res.merge.conflict).toBe(true);
|
||||
expect(res.merge.ok).toBe(false);
|
||||
// The merge was COMMITTED (vault no longer mid-merge) and the bad page named.
|
||||
expect(g.git.commitMerge).toHaveBeenCalledTimes(1);
|
||||
expect(res.conflictedPaths).toEqual(['Conflicted.md']);
|
||||
expect(g.order.some((o) => o.startsWith('commitMerge:'))).toBe(true);
|
||||
});
|
||||
|
||||
it('returns ok:false conflict:false on a non-conflict merge failure', async () => {
|
||||
|
||||
@@ -129,7 +129,7 @@ describe("runCycle against a REAL VaultGit (integration)", () => {
|
||||
expect(onDisk).toContain("new-id");
|
||||
});
|
||||
|
||||
it("an unresolved merge short-circuits before any client call", async () => {
|
||||
it("RECOVERS a vault left mid-merge instead of wedging the whole space", async () => {
|
||||
if (!available) return;
|
||||
|
||||
dir = await mkdtemp(join(tmpdir(), "docmost-cycle-merge-"));
|
||||
@@ -149,8 +149,9 @@ describe("runCycle against a REAL VaultGit (integration)", () => {
|
||||
await writeFile(join(dir, "C.md"), "main-side\n", "utf8");
|
||||
await git.stageAll();
|
||||
await git.commit("main edit", { authorName: "h", authorEmail: "h@l" });
|
||||
// Start a conflicting merge and leave it unresolved.
|
||||
// Start a conflicting merge and leave it unresolved (the wedged state).
|
||||
await execFileAsync("git", ["-C", dir, "merge", "docmost"]).catch(() => {});
|
||||
expect(await git.isMergeInProgress()).toBe(true);
|
||||
|
||||
const client = makeEmptyClientFake();
|
||||
const res = await runCycle({
|
||||
@@ -162,8 +163,25 @@ describe("runCycle against a REAL VaultGit (integration)", () => {
|
||||
log: () => undefined,
|
||||
});
|
||||
|
||||
expect(res).toEqual({ ran: false, skipped: "merge-in-progress" });
|
||||
expect(client.listSpaceTree).not.toHaveBeenCalled();
|
||||
expect(client.createPage).not.toHaveBeenCalled();
|
||||
// WEDGE FIX: the cycle does NOT skip forever — it aborts the stale merge and
|
||||
// RUNS the full pull/push. The space is no longer frozen.
|
||||
expect(res.ran).toBe(true);
|
||||
expect(client.listSpaceTree).toHaveBeenCalled();
|
||||
// And crucially, the vault is NOT left mid-merge afterward (the re-merge of a
|
||||
// genuinely conflicting page is committed-with-markers, not wedged), so the
|
||||
// next cycle can run too.
|
||||
expect(await git.isMergeInProgress()).toBe(false);
|
||||
|
||||
// A SECOND cycle also runs cleanly (proves the wedge is gone for good).
|
||||
const res2 = await runCycle({
|
||||
spaceId: "space-1",
|
||||
client: client as any,
|
||||
vault: git,
|
||||
settings: makeSettings(dir),
|
||||
fs: nodeFs,
|
||||
log: () => undefined,
|
||||
});
|
||||
expect(res2.ran).toBe(true);
|
||||
expect(await git.isMergeInProgress()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -24,6 +24,10 @@ function fakeVault(overrides: Record<string, any> = {}) {
|
||||
stageAll: rec("stageAll"),
|
||||
commit: rec("commit", { committed: false }),
|
||||
merge: rec("merge", { ok: true, conflict: false, output: "" }),
|
||||
listUnmergedPaths: vi.fn(async () => [] as string[]),
|
||||
commitMerge: rec("commitMerge"),
|
||||
abortMerge: rec("abortMerge"),
|
||||
resetHardToHead: rec("resetHardToHead"),
|
||||
readRef: vi.fn(async () => null),
|
||||
revParse: vi.fn(async () => "0000000000000000000000000000000000000000"),
|
||||
diffNameStatus: vi.fn(async () => [] as any[]),
|
||||
@@ -64,16 +68,47 @@ function baseDeps(vault: any, over: Partial<RunCycleDeps> = {}): RunCycleDeps {
|
||||
}
|
||||
|
||||
describe("runCycle (composition)", () => {
|
||||
it("short-circuits with skipped:'merge-in-progress' and runs no pull/push", async () => {
|
||||
const vault = fakeVault({ isMergeInProgress: vi.fn(async () => true) });
|
||||
it("RECOVERS from a vault left mid-merge: aborts the stale merge and continues (no wedge)", async () => {
|
||||
// Regression for the WEDGE bug (QA #119): a vault left mid-merge by a prior
|
||||
// cycle used to skip the WHOLE space forever. Now the cycle aborts the stale
|
||||
// merge and proceeds so the space self-heals.
|
||||
let midMerge = true;
|
||||
const vault = fakeVault({
|
||||
// mid-merge until `abortMerge` clears it (then the cycle continues).
|
||||
isMergeInProgress: vi.fn(async () => midMerge),
|
||||
abortMerge: vi.fn(async () => {
|
||||
midMerge = false;
|
||||
}),
|
||||
});
|
||||
const deps = baseDeps(vault);
|
||||
|
||||
const res = await runCycle(deps);
|
||||
|
||||
expect(res).toEqual({ ran: false, skipped: "merge-in-progress" });
|
||||
// Never advanced to the pull (listSpaceTree) or push.
|
||||
expect(deps.client.listSpaceTree).not.toHaveBeenCalled();
|
||||
expect(vault.order).not.toContain("checkout:docmost");
|
||||
// The stale merge was aborted and the cycle RAN (no permanent wedge).
|
||||
expect(vault.abortMerge).toHaveBeenCalledTimes(1);
|
||||
expect(res.ran).toBe(true);
|
||||
expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1);
|
||||
expect(vault.order).toContain("checkout:docmost");
|
||||
});
|
||||
|
||||
it("hard-resets when 'merge --abort' cannot clear a stray unmerged index", async () => {
|
||||
// abortMerge does NOT clear it (no MERGE_HEAD but stray unmerged entries);
|
||||
// the cycle falls back to a hard reset, then proceeds.
|
||||
let midMerge = true;
|
||||
const vault = fakeVault({
|
||||
isMergeInProgress: vi.fn(async () => midMerge),
|
||||
abortMerge: vi.fn(async () => undefined), // leaves it mid-merge
|
||||
resetHardToHead: vi.fn(async () => {
|
||||
midMerge = false;
|
||||
}),
|
||||
});
|
||||
const deps = baseDeps(vault);
|
||||
|
||||
const res = await runCycle(deps);
|
||||
|
||||
expect(vault.abortMerge).toHaveBeenCalledTimes(1);
|
||||
expect(vault.resetHardToHead).toHaveBeenCalledTimes(1);
|
||||
expect(res.ran).toBe(true);
|
||||
});
|
||||
|
||||
it("stages ensureRepo -> ensureBranch(docmost,main) -> checkout(docmost) BEFORE pulling", async () => {
|
||||
|
||||
@@ -65,9 +65,21 @@ describe('clampCalloutType', () => {
|
||||
expect(clampCalloutType('success')).toBe('success');
|
||||
});
|
||||
|
||||
it('falls back to "info" for unknown types', () => {
|
||||
expect(clampCalloutType('note')).toBe('info');
|
||||
it('PRESERVES every editor-canonical type (note/default no longer flattened)', () => {
|
||||
// Regression for the QA "callout type -> [!info]" fidelity loss: `note` and
|
||||
// `default` are valid editor callout types and must survive the git
|
||||
// round-trip, not collapse to `info`.
|
||||
expect(clampCalloutType('note')).toBe('note');
|
||||
expect(clampCalloutType('default')).toBe('default');
|
||||
expect(clampCalloutType('info')).toBe('info');
|
||||
expect(clampCalloutType('warning')).toBe('warning');
|
||||
expect(clampCalloutType('danger')).toBe('danger');
|
||||
expect(clampCalloutType('success')).toBe('success');
|
||||
});
|
||||
|
||||
it('falls back to "info" for genuinely unknown types', () => {
|
||||
expect(clampCalloutType('tip')).toBe('info');
|
||||
expect(clampCalloutType('banana')).toBe('info');
|
||||
});
|
||||
|
||||
it('falls back to "info" for empty string and null', () => {
|
||||
|
||||
Reference in New Issue
Block a user