From 537e0e42c336c03effe33fd796df6390c35cf624 Mon Sep 17 00:00:00 2001 From: vorotamoroz Date: Tue, 25 Aug 2026 12:43:53 +0000 Subject: [PATCH] Fix premature settings evaluation at startup --- .../features/ModuleObsidianSettingTab.ts | 5 +- .../ModuleObsidianSettingTab.unit.spec.ts | 91 ++++++++++++++++++ ...iveSyncSettingTab.declarative.unit.spec.ts | 64 ++++++++++--- .../ObsidianLiveSyncSettingTab.ts | 35 +++---- test/e2e-obsidian/scripts/settings-ui.ts | 93 ++++++++++++++----- updates.md | 6 ++ 6 files changed, 232 insertions(+), 62 deletions(-) create mode 100644 src/modules/features/ModuleObsidianSettingTab.unit.spec.ts diff --git a/src/modules/features/ModuleObsidianSettingTab.ts b/src/modules/features/ModuleObsidianSettingTab.ts index 09a82824..727eee81 100644 --- a/src/modules/features/ModuleObsidianSettingTab.ts +++ b/src/modules/features/ModuleObsidianSettingTab.ts @@ -8,8 +8,9 @@ import { openObsidianSettings } from "@/common/obsidianSettings.ts"; export class ModuleObsidianSettingDialogue extends AbstractObsidianModule { settingTab!: ObsidianLiveSyncSettingTab; - _everyOnloadStart(): Promise { + _everyOnloadAfterLoadSettings(): Promise { this.settingTab = new ObsidianLiveSyncSettingTab(this.app, this.plugin); + this.settingTab.reloadAllSettings(true); this.plugin.addSettingTab(this.settingTab); eventHub.onEvent(EVENT_REQUEST_OPEN_SETTINGS, () => this.openSetting()); @@ -24,6 +25,6 @@ export class ModuleObsidianSettingDialogue extends AbstractObsidianModule { return `${"appId" in this.app ? this.app.appId : ""}`; } override onBindFunction(core: LiveSyncCore, services: typeof core.services): void { - services.appLifecycle.onInitialise.addHandler(this._everyOnloadStart.bind(this)); + services.appLifecycle.onSettingLoaded.addHandler(this._everyOnloadAfterLoadSettings.bind(this)); } } diff --git a/src/modules/features/ModuleObsidianSettingTab.unit.spec.ts b/src/modules/features/ModuleObsidianSettingTab.unit.spec.ts new file mode 100644 index 00000000..b1840ea0 --- /dev/null +++ b/src/modules/features/ModuleObsidianSettingTab.unit.spec.ts @@ -0,0 +1,91 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const settingTabState = vi.hoisted(() => ({ + callOrder: [] as string[], + reloadAllSettings: vi.fn<(skipUpdate?: boolean) => void>(), +})); + +const eventHubState = vi.hoisted(() => ({ + onEvent: vi.fn(), +})); + +vi.mock("./SettingDialogue/ObsidianLiveSyncSettingTab.ts", () => ({ + ObsidianLiveSyncSettingTab: class ObsidianLiveSyncSettingTab { + reloadAllSettings(skipUpdate?: boolean) { + settingTabState.callOrder.push(`reload:${String(skipUpdate)}`); + settingTabState.reloadAllSettings(skipUpdate); + } + }, +})); + +vi.mock("@/common/events.ts", () => ({ + EVENT_REQUEST_OPEN_SETTINGS: "request-open-settings", + eventHub: eventHubState, +})); + +import { ModuleObsidianSettingDialogue } from "./ModuleObsidianSettingTab.ts"; + +function createModuleHarness() { + let initialisationHandler: (() => Promise) | undefined; + let settingsLoadedHandler: (() => Promise) | undefined; + const plugin = { + app: {}, + addSettingTab: vi.fn(() => settingTabState.callOrder.push("add-setting-tab")), + }; + const services = { + appLifecycle: { + onInitialise: { + addHandler: vi.fn((handler: () => Promise) => { + initialisationHandler = handler; + }), + }, + onSettingLoaded: { + addHandler: vi.fn((handler: () => Promise) => { + settingsLoadedHandler = handler; + }), + }, + }, + }; + const module = Object.assign(Object.create(ModuleObsidianSettingDialogue.prototype), { + plugin, + core: { services }, + }) as ModuleObsidianSettingDialogue; + + module.onBindFunction(module.core as never, services as never); + + return { + initialisationHandler: () => initialisationHandler, + module, + plugin, + services, + settingsLoadedHandler: () => settingsLoadedHandler, + }; +} + +describe("ModuleObsidianSettingDialogue startup lifecycle", () => { + beforeEach(() => { + settingTabState.callOrder.length = 0; + settingTabState.reloadAllSettings.mockClear(); + eventHubState.onEvent.mockClear(); + }); + + it("registers the setting tab after persisted settings have loaded", () => { + const { initialisationHandler, services, settingsLoadedHandler } = createModuleHarness(); + + expect(services.appLifecycle.onInitialise.addHandler).not.toHaveBeenCalled(); + expect(services.appLifecycle.onSettingLoaded.addHandler).toHaveBeenCalledOnce(); + expect(initialisationHandler()).toBeUndefined(); + expect(settingsLoadedHandler()).toBeTypeOf("function"); + }); + + it("seeds the setting editor without requesting a render before registration", async () => { + const { initialisationHandler, settingsLoadedHandler } = createModuleHarness(); + const handler = settingsLoadedHandler() ?? initialisationHandler(); + + expect(handler).toBeTypeOf("function"); + await handler!(); + + expect(settingTabState.reloadAllSettings).toHaveBeenCalledWith(true); + expect(settingTabState.callOrder).toEqual(["reload:true", "add-setting-tab"]); + }); +}); diff --git a/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.declarative.unit.spec.ts b/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.declarative.unit.spec.ts index bac9214e..d45582bb 100644 --- a/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.declarative.unit.spec.ts +++ b/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.declarative.unit.spec.ts @@ -145,22 +145,36 @@ function findPage(tab: ObsidianLiveSyncSettingTab, name: string): SettingDefinit return page; } -function createSettingsTab(): ObsidianLiveSyncSettingTab { - const plugin = { - app: {}, - core: { - settings: { ...DEFAULT_SETTINGS, useAdvancedMode: true }, - confirm: { - askInPopup: vi.fn(), +type SettingsTabOptions = { + activeReplicatorGetter?: () => { syncStatus: "CONNECTED" | "PAUSED" } | undefined; + replicationStatus?: "CLOSED" | "CONNECTED" | "PAUSED"; +}; + +function createSettingsTab(options: SettingsTabOptions = {}): ObsidianLiveSyncSettingTab { + const core = { + settings: { ...DEFAULT_SETTINGS, useAdvancedMode: true }, + confirm: { + askInPopup: vi.fn(), + }, + services: { + setting: { + getDeviceAndVaultName: vi.fn(() => ""), + saveSettingData: vi.fn(async () => undefined), }, - services: { - setting: { - getDeviceAndVaultName: vi.fn(() => ""), - saveSettingData: vi.fn(async () => undefined), + replicator: { + replicationStatics: { + value: { syncStatus: options.replicationStatus ?? "CLOSED" }, }, }, }, }; + Object.defineProperty(core, "replicator", { + get: options.activeReplicatorGetter ?? (() => undefined), + }); + const plugin = { + app: {}, + core, + }; const tab = new ObsidianLiveSyncSettingTab({} as never, plugin as never); Object.assign(tab, { _editingSettings: { ...DEFAULT_SETTINGS, useAdvancedMode: true }, @@ -181,8 +195,27 @@ beforeEach(() => { }); describe("ObsidianLiveSyncSettingTab native page lifecycle", () => { - it("keeps Quick Setup first while synchronisation is inactive and separates synchronisation pages from it", () => { + it("builds definitions before database readiness without requesting the active replicator", () => { + const activeReplicatorGetter = vi.fn(() => { + throw new Error("The active replicator is not ready"); + }); + const tab = createSettingsTab({ activeReplicatorGetter }); + + expect(() => tab.getSettingDefinitions()).not.toThrow(); + expect(activeReplicatorGetter).not.toHaveBeenCalled(); + }); + + it("keeps Quick Setup first while LiveSync is not configured, regardless of transient replication status", () => { + const tab = createSettingsTab({ replicationStatus: "CONNECTED" }); + tab.editingSettings.isConfigured = false; + const definitions = tab.getSettingDefinitions().filter(isGroup); + + expect(definitions[0]?.heading).toBe("🧙‍♂️ Quick Setup"); + }); + + it("keeps Quick Setup first while LiveSync is not configured and separates synchronisation pages from it", () => { const tab = createSettingsTab(); + tab.editingSettings.isConfigured = false; const definitions = tab.getSettingDefinitions().filter(isGroup); expect(definitions.slice(0, 3).map(itemLabel)).toEqual([ @@ -192,14 +225,15 @@ describe("ObsidianLiveSyncSettingTab native page lifecycle", () => { ]); }); - it("keeps the synchronisation group first and orders General Settings before Quick Setup while synchronisation is active", () => { + it("keeps the synchronisation group first for a configured device with automatic triggers disabled", () => { const tab = createSettingsTab(); - tab.editingSettings.liveSync = true; + tab.editingSettings.isConfigured = true; const definitions = tab.getSettingDefinitions().filter(isGroup); - expect(definitions.slice(0, 3).map(itemLabel)).toEqual([ + expect(definitions.slice(0, 4).map(itemLabel)).toEqual([ "🔄 Synchronisation", "⚙️ General Settings", + "📲 Set up other devices", "🧙‍♂️ Quick Setup", ]); }); diff --git a/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.ts b/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.ts index 1f564928..38c1be92 100644 --- a/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.ts +++ b/src/modules/features/SettingDialogue/ObsidianLiveSyncSettingTab.ts @@ -40,7 +40,6 @@ import { eventHub, } from "@/common/events.ts"; import { - enableOnly, // findAttrFromParent, // getLevelStr, setLevelClass, @@ -587,19 +586,8 @@ export class ObsidianLiveSyncSettingTab extends PluginSettingTab { "encrypt", ]); } - isAnySyncEnabled() { - if (this.isConfiguredAs("isConfigured", false)) return false; - if (this.isConfiguredAs("liveSync", true)) return true; - if (this.isConfiguredAs("periodicReplication", true)) return true; - if (this.isConfiguredAs("syncOnFileOpen", true)) return true; - if (this.isConfiguredAs("syncOnSave", true)) return true; - if (this.isConfiguredAs("syncOnEditorSave", true)) return true; - if (this.isConfiguredAs("syncOnStart", true)) return true; - if (this.isConfiguredAs("syncAfterMerge", true)) return true; - if (this.isConfiguredAs("syncOnFileOpen", true)) return true; - if (this.core?.replicator?.syncStatus == "CONNECTED") return true; - if (this.core?.replicator?.syncStatus == "PAUSED") return true; - return false; + isLiveSyncConfigured() { + return this.isConfiguredAs("isConfigured", true); } private supportsDeclarativeSettings(): boolean { @@ -905,13 +893,20 @@ export class ObsidianLiveSyncSettingTab extends PluginSettingTab { getPage("help"), getPage("change-log"), ]); - const laterGroups = [setupOtherDevices, maintenance, extraFeatures, advancedSettings, helpAndInformation]; + const laterGroups = [maintenance, extraFeatures, advancedSettings, helpAndInformation]; const pendingInitialisation = this.createRebuildRequiredAction(); - if (this.isAnySyncEnabled()) { - return [pendingInitialisation, synchronisation, generalSettings, quickSetup, ...laterGroups]; + if (this.isLiveSyncConfigured()) { + return [ + pendingInitialisation, + synchronisation, + generalSettings, + setupOtherDevices, + quickSetup, + ...laterGroups, + ]; } - return [pendingInitialisation, quickSetup, synchronisation, generalSettings, ...laterGroups]; + return [pendingInitialisation, quickSetup, synchronisation, generalSettings, setupOtherDevices, ...laterGroups]; } private beginRenderScope(refresh: () => void): Component { @@ -936,8 +931,6 @@ export class ObsidianLiveSyncSettingTab extends PluginSettingTab { this.controlledElementFunc.length = 0; } - enableOnlySyncDisabled = enableOnly(() => !this.isAnySyncEnabled()); - onlyOnP2POrCouchDB = () => ({ visibility: @@ -1184,7 +1177,7 @@ export class ObsidianLiveSyncSettingTab extends PluginSettingTab { void yieldNextAnimationFrame().then(() => { if (this.selectedScreen == "") { - if (this.isAnySyncEnabled()) { + if (this.isLiveSyncConfigured()) { changeDisplay("20"); } else { changeDisplay("110"); diff --git a/test/e2e-obsidian/scripts/settings-ui.ts b/test/e2e-obsidian/scripts/settings-ui.ts index 091533f7..31de3061 100644 --- a/test/e2e-obsidian/scripts/settings-ui.ts +++ b/test/e2e-obsidian/scripts/settings-ui.ts @@ -14,7 +14,7 @@ import { withObsidianPage, } from "../runner/ui.ts"; import { createTemporaryVault } from "../runner/vault.ts"; -import type { Locator } from "playwright"; +import type { Locator, Page } from "playwright"; const uiTimeoutMs = Number(process.env.E2E_OBSIDIAN_SETTINGS_TIMEOUT_MS ?? 10000); const settingsOnly = process.env.E2E_OBSIDIAN_SETTINGS_ONLY === "true"; @@ -34,6 +34,11 @@ type LiveSyncTestPlugin = { applySettings: () => Promise; isP2P: boolean; }) => Promise; + settingTab?: { + editingSettings: { isConfigured: boolean }; + initialSettings?: { isConfigured: boolean }; + requestCatalogueRefresh(): void; + }; }[]; settings: { handleFilenameCaseSensitive: boolean; @@ -84,17 +89,14 @@ const settingsPageNames = [ "Change Log", ] as const; -async function assertDeclarativeLandingOrder(root: Locator): Promise { +async function assertDeclarativeLandingOrder(root: Locator, configured: boolean): Promise { + const synchronisation = ["Synchronisation", "Remote Configuration", "Sync Settings"]; + const generalSettings = ["General Settings", "Appearance", "Logging", "Extra menus"]; + const setup = configured + ? [...synchronisation, ...generalSettings, "📲 Set up other devices", "Quick Setup"] + : ["Quick Setup", ...synchronisation, ...generalSettings, "📲 Set up other devices"]; const labels = [ - "Quick Setup", - "Synchronisation", - "Remote Configuration", - "Sync Settings", - "General Settings", - "Appearance", - "Logging", - "Extra menus", - "📲 Set up other devices", + ...setup, "Maintenance and recovery", "Maintenance", "Hatch", @@ -131,10 +133,31 @@ async function assertDeclarativeLandingOrder(root: Locator): Promise { }, labels); } -async function scrollDeclarativeLandingToTop(root: Locator): Promise { - const quickSetupHeading = root.locator(".setting-item-heading").filter({ hasText: "Quick Setup" }).first(); - await quickSetupHeading.waitFor({ state: "visible", timeout: uiTimeoutMs }); - await quickSetupHeading.scrollIntoViewIfNeeded(); +async function scrollDeclarativeLandingToTop(root: Locator, configured: boolean): Promise { + const firstHeading = root + .locator(".setting-item-heading") + .filter({ hasText: configured ? "Synchronisation" : "Quick Setup" }) + .first(); + await firstHeading.waitFor({ state: "visible", timeout: uiTimeoutMs }); + await firstHeading.scrollIntoViewIfNeeded(); +} + +async function setConfiguredStateForLandingInspection(page: Page, configured: boolean): Promise { + await page.evaluate((nextConfigured) => { + const plugin = (globalThis as ObsidianTestGlobal).app?.plugins?.plugins["obsidian-livesync"]; + if (plugin === undefined) throw new Error("Self-hosted LiveSync is unavailable"); + const settingDialogue = plugin.core.modules.find( + (module) => module.constructor.name === "ModuleObsidianSettingDialogue" + ); + if (settingDialogue?.settingTab === undefined) { + throw new Error("The Self-hosted LiveSync setting tab is unavailable"); + } + settingDialogue.settingTab.editingSettings.isConfigured = nextConfigured; + if (settingDialogue.settingTab.initialSettings !== undefined) { + settingDialogue.settingTab.initialSettings.isConfigured = nextConfigured; + } + settingDialogue.settingTab.requestCatalogueRefresh(); + }, configured); } async function captureDeclarativeMobileLanding(): Promise { @@ -148,8 +171,8 @@ async function captureDeclarativeMobileLanding(): Promise { return undefined; } await settingsNavigator.returnToCatalogue(); - await scrollDeclarativeLandingToTop(settingsNavigator.dialogue); - await assertDeclarativeLandingOrder(settingsNavigator.dialogue); + await scrollDeclarativeLandingToTop(settingsNavigator.dialogue, true); + await assertDeclarativeLandingOrder(settingsNavigator.dialogue, true); const remoteConfiguration = settingsNavigator.dialogue .locator(".setting-item-name") .filter({ hasText: "Remote Configuration" }) @@ -437,8 +460,8 @@ async function verifyConfigDoctorFollowsCompatibilityReview(): Promise { }); } -async function verifyEffectiveSettings(): Promise { - await withObsidianPage(obsidianRemoteDebuggingPort(), async (page) => { +async function verifyEffectiveSettings(): Promise<"declarative" | "imperative"> { + return await withObsidianPage(obsidianRemoteDebuggingPort(), async (page) => { const sleepPreferences = await page.evaluate(() => { const plugin = (globalThis as ObsidianTestGlobal).app?.plugins?.plugins["obsidian-livesync"]; if (plugin === undefined) throw new Error("Self-hosted LiveSync is unavailable"); @@ -462,6 +485,12 @@ async function verifyEffectiveSettings(): Promise { } let settingsNavigator = await openLiveSyncSettings(page, uiTimeoutMs); + if (settingsNavigator.renderer === "imperative") { + await settingsNavigator.dialogue.screenshot({ + ...settingsScreenshotOptions, + path: `${diagnosticsDirectory}/settings-imperative-landing.png`, + }); + } for (const hiddenPage of ["Selector", "Customisation sync", "Advanced", "Power users", "Patches"]) { if (await settingsNavigator.isPageListed(hiddenPage)) { throw new Error(`${hiddenPage} was visible before its feature level was enabled.`); @@ -568,12 +597,22 @@ async function verifyEffectiveSettings(): Promise { if (settingsNavigator.renderer === "declarative") { await settingsNavigator.returnToCatalogue(); - await scrollDeclarativeLandingToTop(settingsNavigator.dialogue); + await scrollDeclarativeLandingToTop(settingsNavigator.dialogue, true); await settingsNavigator.dialogue.screenshot({ ...settingsScreenshotOptions, path: `${diagnosticsDirectory}/settings-declarative-landing.png`, }); - await assertDeclarativeLandingOrder(settingsNavigator.dialogue); + await assertDeclarativeLandingOrder(settingsNavigator.dialogue, true); + await setConfiguredStateForLandingInspection(page, false); + await scrollDeclarativeLandingToTop(settingsNavigator.dialogue, false); + await assertDeclarativeLandingOrder(settingsNavigator.dialogue, false); + await settingsNavigator.dialogue.screenshot({ + ...settingsScreenshotOptions, + path: `${diagnosticsDirectory}/settings-declarative-landing-unconfigured.png`, + }); + await setConfiguredStateForLandingInspection(page, true); + await scrollDeclarativeLandingToTop(settingsNavigator.dialogue, true); + await assertDeclarativeLandingOrder(settingsNavigator.dialogue, true); const rerunOnboarding = settingsNavigator.dialogue .locator(".setting-item-name") .filter({ hasText: "Rerun Onboarding Wizard" }) @@ -647,7 +686,9 @@ async function verifyEffectiveSettings(): Promise { } } + const renderer = settingsNavigator.renderer; await settingsNavigator.close(); + return renderer; }); } @@ -690,7 +731,11 @@ async function verifyPendingSettingsInitialisationFlow(): Promise<{ choice: stri has: settingsNavigator.page.getByText("Changes need to be applied!", { exact: true }), }); await applySetting.waitFor({ state: "visible", timeout: uiTimeoutMs }); - await applySetting.getByRole("button", { name: "Apply", exact: true }).click({ timeout: uiTimeoutMs }); + if (settingsNavigator.renderer === "declarative") { + await applySetting.click({ timeout: uiTimeoutMs }); + } else { + await applySetting.getByRole("button", { name: "Apply", exact: true }).click({ timeout: uiTimeoutMs }); + } const choiceDialogue = await waitForVisibleObsidianDialogue( settingsNavigator.page, @@ -798,10 +843,10 @@ async function main(): Promise { await verifyCompatibilityReview(); await verifyConfigDoctorFollowsCompatibilityReview(); } - await verifyEffectiveSettings(); + const settingsRenderer = await verifyEffectiveSettings(); const initialisation = await verifyPendingSettingsInitialisationFlow(); const p2pInitialisation = await captureP2PSettingsInitialisationDialogue(); - const mobileLanding = await captureDeclarativeMobileLanding(); + const mobileLanding = settingsRenderer === "declarative" ? await captureDeclarativeMobileLanding() : undefined; if (mobileLanding) console.log(`Declarative mobile settings landing page: ${mobileLanding}`); console.log( `Pending-settings initialisation screenshots: ${initialisation.choice}, ${initialisation.fallback}, ${p2pInitialisation}` diff --git a/updates.md b/updates.md index 16692989..32ddfb58 100644 --- a/updates.md +++ b/updates.md @@ -12,6 +12,12 @@ Earlier releases remain available in the 1.0 release history, the 1.0 preview hi ## Unreleased +### Interface and translation + +#### Fixed + +- Obsidian 1.13 settings discovery now waits until persisted settings have loaded and no longer queries the active replicator before database initialisation, preventing a spurious start-up warning. + ## 1.0.19 25th August, 2026