fix(git-sync): propagate nested details open; drop dead delete-cap wiring; cover lost-lock abort + lose-prone atom round-trips
Addresses review 1863 (delta) on PR #119. MUST-FIX: - detailsToHtml (the raw-HTML path used for a details nested inside columns/spanned cells) now emits `<details${open}>`, mirroring the top-level case, so `open` no longer silently drops every round trip. - Remove the dead `resolveApplyClient` delete-cap hook from the engine `runCycle`: the orchestrator stopped passing it, so the hook + its dry-run pass were inert. Deletes are soft (Trash) + always logged and engine convergence is the guard, so no cap is re-added — just the dead wiring removed. TEST COVERAGE: - space-lock: heartbeat refresh CAS-miss (eval -> 0) and Redis-error (eval throws) both abort the in-flight fn's signal. - cycle: a pre-aborted signal (and an abort during the pull read) throws before the push apply / first destructive phase. - converter: htmlEmbed source VALUE + height survive; encode/decode UTF-8 symmetry and '' -> ''; footnote definition body + ref/def id match; transclusionReference both ids survive; fix the bad transclusionSource fixture (wrong `pageId` attr + empty content -> schema `id` + a block child); nested details `open` parity test. - orchestrator: autoMergeConflicts:true reaches engine settings; default false on a missing settings row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -54,6 +54,12 @@ interface BuildOptions {
|
||||
debounceMs?: number;
|
||||
/** A hook applied to the fake vault so a test can override its behaviour. */
|
||||
vaultOverrides?: Record<string, unknown>;
|
||||
/**
|
||||
* The row `buildSettings` reads for the per-space `autoMergeConflicts` flag
|
||||
* (`executeTakeFirst`). Default: the SAFE off value. Pass `undefined` to model
|
||||
* a missing row (no space / no settings).
|
||||
*/
|
||||
settingsRow?: { autoMergeConflicts: boolean } | undefined;
|
||||
}
|
||||
|
||||
interface Built {
|
||||
@@ -78,6 +84,10 @@ function build(opts: BuildOptions = {}): Built {
|
||||
debounceMs = 2000,
|
||||
vaultOverrides = {},
|
||||
} = opts;
|
||||
// Distinguish "key omitted" (default off row) from "key present but undefined"
|
||||
// (a deliberately MISSING settings row).
|
||||
const settingsRow =
|
||||
'settingsRow' in opts ? opts.settingsRow : { autoMergeConflicts: false };
|
||||
// Distinguish "key omitted" (default to a valid id) from "key present but
|
||||
// undefined" (the no-service-user test deliberately sets it undefined).
|
||||
const serviceUserId = 'serviceUserId' in opts ? opts.serviceUserId : 'svc-user';
|
||||
@@ -135,7 +145,7 @@ function build(opts: BuildOptions = {}): Built {
|
||||
const builder: any = {
|
||||
select: () => builder,
|
||||
where: () => builder,
|
||||
executeTakeFirst: async () => ({ autoMergeConflicts: false }),
|
||||
executeTakeFirst: async () => settingsRow,
|
||||
execute: async () => [],
|
||||
};
|
||||
return { selectFrom: () => builder };
|
||||
@@ -290,6 +300,20 @@ describe('GitSyncOrchestrator', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('threads autoMergeConflicts:true from the space settings row into the engine settings', async () => {
|
||||
const built = build({ settingsRow: { autoMergeConflicts: true } });
|
||||
await built.orchestrator.runOnce('space-1', 'ws-1');
|
||||
const [deps] = runCycleMock.mock.calls[0];
|
||||
expect(deps.settings.autoMergeConflicts).toBe(true);
|
||||
});
|
||||
|
||||
it('defaults autoMergeConflicts to false when the settings row is missing', async () => {
|
||||
const built = build({ settingsRow: undefined });
|
||||
await built.orchestrator.runOnce('space-1', 'ws-1');
|
||||
const [deps] = runCycleMock.mock.calls[0];
|
||||
expect(deps.settings.autoMergeConflicts).toBe(false);
|
||||
});
|
||||
|
||||
it("surfaces the engine's skipped status (e.g. merge-in-progress) verbatim", async () => {
|
||||
const built = build();
|
||||
runCycleMock.mockResolvedValue({ ran: false, skipped: 'merge-in-progress' });
|
||||
|
||||
@@ -187,6 +187,82 @@ describe('SpaceLockService', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// The lost-lock guard: a heartbeat refresh that cannot CONFIRM we still own the
|
||||
// lock (CAS miss, res !== 1) OR that throws (Redis error) aborts the supplied
|
||||
// controller so the in-flight protected fn stops instead of writing blind after
|
||||
// a possible lock takeover. `withSpaceLock` threads that signal into `fn`.
|
||||
describe('abort-on-lost-lock', () => {
|
||||
it('aborts the in-flight fn when the heartbeat refresh CAS-MISSES (eval -> 0)', async () => {
|
||||
jest.useFakeTimers();
|
||||
try {
|
||||
const { service, redis } = build();
|
||||
let release!: () => void;
|
||||
const gate = new Promise<void>((resolve) => {
|
||||
release = resolve;
|
||||
});
|
||||
let captured: AbortSignal | undefined;
|
||||
|
||||
const run = service.withSpaceLock('space-1', async (signal) => {
|
||||
captured = signal;
|
||||
await gate;
|
||||
return 'done';
|
||||
});
|
||||
// Let acquire resolve and the setInterval register.
|
||||
await flushMicrotasks();
|
||||
expect(captured).toBeDefined();
|
||||
expect(captured!.aborted).toBe(false);
|
||||
|
||||
// The refresh CAS-misses: the key no longer holds our instanceId.
|
||||
redis.eval.mockResolvedValue(0);
|
||||
jest.advanceTimersByTime(Math.floor(GIT_SYNC_LOCK_TTL_MS / 3));
|
||||
await flushMicrotasks();
|
||||
|
||||
// The lost lock aborted the protected fn's signal.
|
||||
expect(captured!.aborted).toBe(true);
|
||||
|
||||
release();
|
||||
await flushMicrotasks();
|
||||
await expect(run).resolves.toBe('done');
|
||||
} finally {
|
||||
jest.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it('aborts the in-flight fn when the heartbeat refresh THROWS (Redis error)', async () => {
|
||||
jest.useFakeTimers();
|
||||
try {
|
||||
const { service, redis } = build();
|
||||
let release!: () => void;
|
||||
const gate = new Promise<void>((resolve) => {
|
||||
release = resolve;
|
||||
});
|
||||
let captured: AbortSignal | undefined;
|
||||
|
||||
const run = service.withSpaceLock('space-1', async (signal) => {
|
||||
captured = signal;
|
||||
await gate;
|
||||
return 'done';
|
||||
});
|
||||
await flushMicrotasks();
|
||||
expect(captured!.aborted).toBe(false);
|
||||
|
||||
// The refresh eval rejects (Redis down). release() in finally must still
|
||||
// resolve, so only reject the NEXT (heartbeat) call, then go back to OK.
|
||||
redis.eval.mockRejectedValueOnce(new Error('redis down'));
|
||||
jest.advanceTimersByTime(Math.floor(GIT_SYNC_LOCK_TTL_MS / 3));
|
||||
await flushMicrotasks();
|
||||
|
||||
expect(captured!.aborted).toBe(true);
|
||||
|
||||
release();
|
||||
await flushMicrotasks();
|
||||
await expect(run).resolves.toBe('done');
|
||||
} finally {
|
||||
jest.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// Silence the warn logger if a refresh/release path ever logs (defensive).
|
||||
|
||||
Reference in New Issue
Block a user