From 84be444689b3b52bb5315cf930f4a884f357ec09 Mon Sep 17 00:00:00 2001 From: vorotamoroz Date: Fri, 4 Sep 2026 05:15:45 +0000 Subject: [PATCH] Simplify conflict scheduling and lifecycle subscriptions --- package-lock.json | 8 +- package.json | 2 +- .../ConflictResolveModal.ts | 39 +++--- .../conflictResolution/checker.ts | 28 +--- .../conflictResolution.unit.spec.ts | 120 ++++++++++++++++++ .../interactiveConflictResolution/index.ts | 21 +-- test/e2e-obsidian/runner/ui.ts | 35 +++-- .../scripts/conflict-dialog-policy.ts | 28 +++- 8 files changed, 206 insertions(+), 75 deletions(-) diff --git a/package-lock.json b/package-lock.json index f57d9273..d51fca01 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,7 @@ "markdown-it": "^14.2.0", "minimatch": "^10.2.5", "obsidian": "^1.13.1", - "octagonal-wheels": "^0.1.53", + "octagonal-wheels": "^0.1.54", "qrcode-generator": "^1.4.4", "xxhash-wasm-102": "npm:xxhash-wasm@^1.0.2" }, @@ -9682,9 +9682,9 @@ "license": "MIT" }, "node_modules/octagonal-wheels": { - "version": "0.1.53", - "resolved": "https://registry.npmjs.org/octagonal-wheels/-/octagonal-wheels-0.1.53.tgz", - "integrity": "sha512-4NJsb96Sk6rJXhrTyjAY5GRIoWMFJsFp56b5ba9fV8/87ys2HCjMo0hior4F8k4ma72pLTJT7i6DvuihHxSsMA==", + "version": "0.1.54", + "resolved": "https://registry.npmjs.org/octagonal-wheels/-/octagonal-wheels-0.1.54.tgz", + "integrity": "sha512-Je3ancYhjKX7UY2K19T/qTjG8C9nK8YVrACr5naIf78mN4bbjQkYyWmlj+ooifV/moWVsQrp4fEWz/7mv6It3A==", "license": "MIT", "dependencies": { "idb": "^8.0.3" diff --git a/package.json b/package.json index 945dd729..5d73791e 100644 --- a/package.json +++ b/package.json @@ -186,7 +186,7 @@ "markdown-it": "^14.2.0", "minimatch": "^10.2.5", "obsidian": "^1.13.1", - "octagonal-wheels": "^0.1.53", + "octagonal-wheels": "^0.1.54", "qrcode-generator": "^1.4.4", "xxhash-wasm-102": "npm:xxhash-wasm@^1.0.2" }, diff --git a/src/modules/features/InteractiveConflictResolving/ConflictResolveModal.ts b/src/modules/features/InteractiveConflictResolving/ConflictResolveModal.ts index bd72a303..a5b22068 100644 --- a/src/modules/features/InteractiveConflictResolving/ConflictResolveModal.ts +++ b/src/modules/features/InteractiveConflictResolving/ConflictResolveModal.ts @@ -34,8 +34,7 @@ export class ConflictResolveModal extends Modal { readOnly: boolean = false; localName: string = "Base"; remoteName: string = "Conflicted"; - offConflictCancelled?: ReturnType; - offPluginUnloaded?: ReturnType; + private eventSubscriptions?: AbortController; currentDiffIndex = -1; diffView!: HTMLDivElement; diffNavIndicator!: HTMLSpanElement; @@ -112,23 +111,31 @@ export class ConflictResolveModal extends Modal { override onOpen() { const { contentEl } = this; - this.offConflictCancelled?.(); - this.offConflictCancelled = undefined; - this.offPluginUnloaded?.(); - this.offPluginUnloaded = eventHub.onceEvent(EVENT_PLUGIN_UNLOADED, () => { - this.sendResponse(CANCELLED); - }); + this.eventSubscriptions?.abort(); + const eventSubscriptions = new AbortController(); + this.eventSubscriptions = eventSubscriptions; + eventHub.onceEvent( + EVENT_PLUGIN_UNLOADED, + () => { + this.sendResponse(CANCELLED); + }, + { signal: eventSubscriptions.signal } + ); if (!this.readOnly) { // Cancel an older dialogue for this path before subscribing this // instance. Emitting after subscription would close the replacement // itself; the instance-owned result promise then completes the older // caller even when it only begins waiting after this event. eventHub.emitEvent(EVENT_CONFLICT_CANCELLED, this.filename); - this.offConflictCancelled = eventHub.onEvent(EVENT_CONFLICT_CANCELLED, (path) => { - if (path === this.filename) { - this.sendResponse(CANCELLED); - } - }); + eventHub.onEvent( + EVENT_CONFLICT_CANCELLED, + (path) => { + if (path === this.filename) { + this.sendResponse(CANCELLED); + } + }, + { signal: eventSubscriptions.signal } + ); } this.titleEl.setText(this.title); contentEl.empty(); @@ -219,10 +226,8 @@ export class ConflictResolveModal extends Modal { override onClose() { const { contentEl } = this; contentEl.empty(); - this.offConflictCancelled?.(); - this.offConflictCancelled = undefined; - this.offPluginUnloaded?.(); - this.offPluginUnloaded = undefined; + this.eventSubscriptions?.abort(); + this.eventSubscriptions = undefined; if (this.consumed) { return; } diff --git a/src/serviceFeatures/conflictResolution/checker.ts b/src/serviceFeatures/conflictResolution/checker.ts index cbd6867b..9882761d 100644 --- a/src/serviceFeatures/conflictResolution/checker.ts +++ b/src/serviceFeatures/conflictResolution/checker.ts @@ -28,12 +28,9 @@ export interface ConflictCheckingHandlers { readonly ensureAllProcessed: () => Promise; } -/** - * Create conflict-checking handlers and retain both queue processors privately. - * The returned operations do not expose queue state to a host or consumer. - */ +/** Create conflict-checking handlers while retaining scheduling state privately. */ export function createConflictCheckingHandlers(dependencies: ConflictCheckingDependencies): ConflictCheckingHandlers { - const conflictResolveQueue = new QueueProcessor( + const conflictQueue = new QueueProcessor( async (filenames: FilePathWithPrefix[]) => { const filename = filenames[0]; return await dependencies.conflict.resolve(filename); @@ -47,28 +44,13 @@ export function createConflictCheckingHandlers(dependencies: ConflictCheckingDep concurrentLimit: 10, delay: 0, keepResultUntilDownstreamConnected: false, + totalRemainingReactiveSource: dependencies.conflictProcessQueueCount, } ).replaceEnqueueProcessor((queue, newEntity) => { const newQueue = [...queue].filter((entry) => entry != newEntity); return [...newQueue, newEntity]; }); - const conflictCheckQueue = new QueueProcessor( - (files: FilePathWithPrefix[]) => { - const filename = files[0]; - return Promise.resolve([filename]); - }, - { - suspended: false, - batchSize: 1, - concurrentLimit: 10, - delay: 0, - keepResultUntilDownstreamConnected: true, - pipeTo: conflictResolveQueue, - totalRemainingReactiveSource: dependencies.conflictProcessQueueCount, - } - ); - const queueCheckForIfOpen = async (file: FilePathWithPrefix): Promise => { const path = file; if (dependencies.currentSettings().checkConflictOnlyOnOpen) { @@ -90,11 +72,11 @@ export function createConflictCheckingHandlers(dependencies: ConflictCheckingDep // The conflict should be resolved by the newer entry. await dependencies.conflict.resolveByNewest(file); } else { - conflictCheckQueue.enqueue(file); + conflictQueue.enqueue(file); } }; - const ensureAllProcessed = (): Promise => conflictResolveQueue.waitForAllProcessed(); + const ensureAllProcessed = (): Promise => conflictQueue.waitForAllProcessed(); return { queueCheckForIfOpen, diff --git a/src/serviceFeatures/conflictResolution/conflictResolution.unit.spec.ts b/src/serviceFeatures/conflictResolution/conflictResolution.unit.spec.ts index d6178e0b..0a92cf17 100644 --- a/src/serviceFeatures/conflictResolution/conflictResolution.unit.spec.ts +++ b/src/serviceFeatures/conflictResolution/conflictResolution.unit.spec.ts @@ -472,6 +472,126 @@ describe("conflict resolution serviceFeature", () => { ); }); + it("limits concurrent conflict checks and reports queued and active work", async () => { + const paths = Array.from({ length: 11 }, (_, index) => `concurrent-${index}.md` as FilePathWithPrefix); + const harness = createHarness(); + const finishByPath = new Map void>(); + harness.tryAutoMerge.mockImplementation( + async (path: FilePathWithPrefix) => + await new Promise<{ ok: typeof NOT_CONFLICTED }>((resolve) => { + finishByPath.set(path, () => resolve({ ok: NOT_CONFLICTED })); + }) + ); + + await Promise.all(paths.map(async (path) => await harness.conflict.queueCheckFor(path))); + await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledTimes(10)); + + expect(harness.conflict.conflictProcessQueueCount.value).toBe(11); + let allProcessed = false; + const completion = harness.conflict.ensureAllProcessed().then((result) => { + allProcessed = true; + return result; + }); + await Promise.resolve(); + expect(allProcessed).toBe(false); + + const finishFirst = finishByPath.get(paths[0]); + finishByPath.delete(paths[0]); + finishFirst?.(); + await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledTimes(11)); + for (const finish of finishByPath.values()) finish(); + + await expect(completion).resolves.toBe(true); + expect(harness.conflict.conflictProcessQueueCount.value).toBe(0); + }); + + it("replaces an older same-path check while every resolver slot is occupied", async () => { + const occupiedPaths = Array.from({ length: 10 }, (_, index) => `occupied-${index}.md` as FilePathWithPrefix); + const repeatedPath = "waiting-replacement.md" as FilePathWithPrefix; + const harness = createHarness(); + const finishers: Array<{ path: FilePathWithPrefix; finish: () => void }> = []; + harness.tryAutoMerge.mockImplementation( + async (path: FilePathWithPrefix) => + await new Promise<{ ok: typeof NOT_CONFLICTED }>((resolve) => { + finishers.push({ path, finish: () => resolve({ ok: NOT_CONFLICTED }) }); + }) + ); + + await Promise.all(occupiedPaths.map(async (path) => await harness.conflict.queueCheckFor(path))); + await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledTimes(10)); + + await harness.conflict.queueCheckFor(repeatedPath); + await harness.conflict.queueCheckFor(repeatedPath); + for (const { finish } of finishers.filter(({ path }) => path !== repeatedPath)) finish(); + + await vi.waitFor(() => expect(harness.conflict.conflictProcessQueueCount.value).toBe(1)); + expect(harness.tryAutoMerge.mock.calls.filter(([path]) => path === repeatedPath)).toHaveLength(1); + + finishers.find(({ path }) => path === repeatedPath)?.finish(); + await expect(harness.conflict.ensureAllProcessed()).resolves.toBe(true); + }); + + it("waits for a conflict check requeued by an automatic merge", async () => { + const path = "requeued-automatic-merge.md" as FilePathWithPrefix; + const harness = createHarness({ settings: { syncAfterMerge: false } }); + let finishFirst!: (result: { ok: typeof AUTO_MERGED }) => void; + harness.tryAutoMerge + .mockImplementationOnce( + async () => + await new Promise<{ ok: typeof AUTO_MERGED }>((resolve) => { + finishFirst = resolve; + }) + ) + .mockResolvedValueOnce({ ok: NOT_CONFLICTED }); + + await harness.conflict.queueCheckFor(path); + await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledOnce()); + const completion = harness.conflict.ensureAllProcessed(); + + finishFirst({ ok: AUTO_MERGED }); + + await expect(completion).resolves.toBe(true); + expect(harness.tryAutoMerge).toHaveBeenCalledTimes(2); + expect(harness.conflict.conflictProcessQueueCount.value).toBe(0); + }); + + it("drains repeated manual resolutions for a file with more than two conflicting versions", async () => { + const path = "requeued-manual-merge.md" as FilePathWithPrefix; + const harness = createHarness({ settings: { syncAfterMerge: false } }); + const firstPair = { + leftRev: "3-current", + rightRev: "2-second", + leftLeaf: leaf("3-current", "Current\n", 3), + rightLeaf: leaf("2-second", "Second\n", 2), + }; + const remainingPair = { + leftRev: "4-merged", + rightRev: "2-third", + leftLeaf: leaf("4-merged", "Merged\n", 4), + rightLeaf: leaf("2-third", "Third\n", 2), + }; + harness.tryAutoMerge + .mockResolvedValueOnce(firstPair) + .mockResolvedValueOnce(remainingPair) + .mockResolvedValueOnce({ ok: NOT_CONFLICTED }); + const resolvePair = vi.fn(async (filename: FilePathWithPrefix) => { + await harness.conflict.queueCheckFor(filename); + return false; + }); + const unregister = harness.conflict.resolveByUserInteraction.addHandler(resolvePair); + + try { + await harness.conflict.queueCheckFor(path); + + await expect(harness.conflict.ensureAllProcessed()).resolves.toBe(true); + expect(harness.tryAutoMerge).toHaveBeenCalledTimes(3); + expect(resolvePair).toHaveBeenCalledTimes(2); + expect(harness.conflict.conflictProcessQueueCount.value).toBe(0); + } finally { + unregister(); + } + }); + it("honours optional conflict handlers before entering the check queue", async () => { const path = "optional.md" as FilePathWithPrefix; const harness = createHarness(); diff --git a/src/serviceFeatures/interactiveConflictResolution/index.ts b/src/serviceFeatures/interactiveConflictResolution/index.ts index 0fac2b38..a3b75e6c 100644 --- a/src/serviceFeatures/interactiveConflictResolution/index.ts +++ b/src/serviceFeatures/interactiveConflictResolution/index.ts @@ -61,22 +61,23 @@ export function useInteractiveConflictResolutionFeature( }); services.appLifecycle.getUnresolvedMessages.addHandler(operations.getActiveConflictMessages); services.conflict.resolveByUserInteraction.addHandler(operations.resolveByUserInteraction); - const offConflictCancelled = services.context.events.onEvent(EVENT_CONFLICT_CANCELLED, (filename) => { - operations.invalidateWaitingResolution(filename); - fireAndForget(() => operations.refreshConflictState(filename)); - }); - let featureDisposed = false; + const eventSubscriptions = new AbortController(); const dispose = () => { - if (featureDisposed) return; - featureDisposed = true; // Stop the refresh listener before cancellation so that unloading does // not start a database read which can race with database disposal. - offConflictCancelled(); + eventSubscriptions.abort(); operations.dispose(); }; - const offPluginUnloaded = services.context.events.onceEvent(EVENT_PLUGIN_UNLOADED, dispose); + services.context.events.onEvent( + EVENT_CONFLICT_CANCELLED, + (filename) => { + operations.invalidateWaitingResolution(filename); + fireAndForget(() => operations.refreshConflictState(filename)); + }, + { signal: eventSubscriptions.signal } + ); + services.context.events.onceEvent(EVENT_PLUGIN_UNLOADED, dispose, { signal: eventSubscriptions.signal }); services.appLifecycle.onUnload.addHandler(() => { - offPluginUnloaded(); dispose(); return Promise.resolve(true); }); diff --git a/test/e2e-obsidian/runner/ui.ts b/test/e2e-obsidian/runner/ui.ts index 1f84ceff..ebd6f12b 100644 --- a/test/e2e-obsidian/runner/ui.ts +++ b/test/e2e-obsidian/runner/ui.ts @@ -287,19 +287,28 @@ export async function captureObsidianElement( await mkdir(dirname(screenshotPath), { recursive: true }); await withObsidianPage(port, async (page) => { - try { - const element = await resolveElement(page); - await element.waitFor({ state: "visible", timeout: timeoutMs }); - await element.screenshot({ - path: screenshotPath, - animations: "disabled", - style: ".notice-container { visibility: hidden !important; }", - }); - } catch (error) { - const failurePath = screenshotPath.replace(/\.png$/u, ".failure.png"); - await page.screenshot({ path: failurePath, fullPage: true }); - console.error(`UI element failure screenshot: ${failurePath}`); - throw error; + for (let attempt = 0; attempt < 3; attempt++) { + try { + const element = await resolveElement(page); + await element.waitFor({ state: "visible", timeout: timeoutMs }); + await element.screenshot({ + path: screenshotPath, + animations: "disabled", + style: ".notice-container { visibility: hidden !important; }", + }); + return; + } catch (error) { + const detachedDuringCapture = + error instanceof Error && error.message.includes("not attached to the DOM"); + if (detachedDuringCapture && attempt < 2) { + await page.waitForTimeout(50); + continue; + } + const failurePath = screenshotPath.replace(/\.png$/u, ".failure.png"); + await page.screenshot({ path: failurePath, fullPage: true }); + console.error(`UI element failure screenshot: ${failurePath}`); + throw error; + } } }); diff --git a/test/e2e-obsidian/scripts/conflict-dialog-policy.ts b/test/e2e-obsidian/scripts/conflict-dialog-policy.ts index 63b81215..ed53222a 100644 --- a/test/e2e-obsidian/scripts/conflict-dialog-policy.ts +++ b/test/e2e-obsidian/scripts/conflict-dialog-policy.ts @@ -423,7 +423,17 @@ async function main(): Promise { state: "visible", timeout: uiTimeoutMs, }); - const actionButtonBounds = await modal.locator(".conflict-action-button").evaluateAll((buttons) => + }); + const firstDialogueScreenshot = await captureObsidianElement( + session.remoteDebuggingPort, + "conflict-dialog-three-versions.png", + (page) => conflictDialogue(page).locator(".modal").first() + ); + await withObsidianPage(session.remoteDebuggingPort, async (page) => { + const modal = conflictDialogue(page); + const actionButtons = modal.locator(".conflict-action-button"); + await actionButtons.nth(3).waitFor({ state: "visible", timeout: uiTimeoutMs }); + const actionButtonBounds = await actionButtons.evaluateAll((buttons) => buttons.map((button) => { const bounds = button.getBoundingClientRect(); return { top: bounds.top, bottom: bounds.bottom }; @@ -435,16 +445,20 @@ async function main(): Promise { (bounds, index) => index > 0 && bounds.top < actionButtonBounds[index - 1].bottom ) ) { + const buttonDetails = await modal.locator("button").evaluateAll((buttons) => + buttons.map((button) => ({ + text: button.textContent, + className: button.className, + })) + ); throw new Error( - `Conflict action buttons are not stacked vertically: ${JSON.stringify(actionButtonBounds)}` + `Conflict action buttons are not stacked vertically: ${JSON.stringify({ + actionButtonBounds, + buttonDetails, + })}` ); } }); - const firstDialogueScreenshot = await captureObsidianElement( - session.remoteDebuggingPort, - "conflict-dialog-three-versions.png", - (page) => conflictDialogue(page).locator(".modal").first() - ); await withObsidianPage(session.remoteDebuggingPort, async (page) => { const modal = conflictDialogue(page); await modal.getByRole("button", { name: "Concat both", exact: true }).click({ timeout: uiTimeoutMs });