diff --git a/packages/obsidian-plugin-kit/README.md b/packages/obsidian-plugin-kit/README.md index 7a0ec14..02c0bdd 100644 --- a/packages/obsidian-plugin-kit/README.md +++ b/packages/obsidian-plugin-kit/README.md @@ -78,7 +78,7 @@ notifications.show("conflict", { notifications.dispose(); ``` -`KeyedNoticeManager` updates one visible Notice per application-defined key and restarts its expiry on every update. Dispose the manager during plug-in unload. +`KeyedNoticeManager` updates one visible Notice per application-defined key and restarts its expiry on every update. After click dismissal, the next update creates a fresh Notice rather than reviving the acknowledged one. The manager retains its own message root and does not read `Notice.messageEl` or deprecated `Notice.noticeEl`. Dispose the manager during plug-in unload. ```ts import { KeyedNoticeManager } from "@vrtmrz/obsidian-plugin-kit/notice"; diff --git a/packages/obsidian-plugin-kit/docs/usage-guide.md b/packages/obsidian-plugin-kit/docs/usage-guide.md index 76f03c8..bf0e4df 100644 --- a/packages/obsidian-plugin-kit/docs/usage-guide.md +++ b/packages/obsidian-plugin-kit/docs/usage-guide.md @@ -419,7 +419,9 @@ notices.hide("sync"); notices.dispose(); ``` -Use a separate manager for each owning application scope. `hideAll()` clears visible Notices while leaving the manager reusable. `dispose()` clears them and permanently ends the manager lifecycle, so call it during plug-in unload. +Use a separate manager for each owning application scope. `hideAll()` clears visible Notices while leaving the manager reusable. `dispose()` clears them and permanently ends the manager lifecycle, so call it during plug-in unload. After click dismissal, the next update creates a fresh Notice rather than reviving the acknowledged one, even while Obsidian is completing its hide transition. + +The manager retains the DOM root it supplies to Obsidian and does not read `Notice.messageEl` or deprecated `Notice.noticeEl`. This entry-point compatibility does not lower the package-wide `obsidian >=1.8.7` peer dependency; other exported entry points retain their own host requirements until they are reviewed separately. Use `KeyedNoticeGroupManager` when several messages or actions belong to one operation and should not stack as separate Notices. Each group key owns one Notice, and each item key owns one insertion-ordered row within it: diff --git a/packages/obsidian-plugin-kit/src/notice.test.ts b/packages/obsidian-plugin-kit/src/notice.test.ts index 3d1b09a..cc72dde 100644 --- a/packages/obsidian-plugin-kit/src/notice.test.ts +++ b/packages/obsidian-plugin-kit/src/notice.test.ts @@ -1,6 +1,15 @@ // @vitest-environment jsdom -import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; type TestElementInfo = | string @@ -65,44 +74,60 @@ afterAll(() => { }); interface NoticeMockInstance { - messageEl: HTMLElement; + container: HTMLElement; duration: number; hidden: boolean; + setMessageCalls: number; hide(): void; } -const noticeState = vi.hoisted(() => ({ instances: [] as unknown[] })); +interface ModernNoticeMockInstance extends NoticeMockInstance { + messageEl: HTMLElement; +} + +const noticeState = vi.hoisted(() => ({ + instances: [] as unknown[], + legacy: false, +})); vi.mock("obsidian", () => { class Notice { - messageEl = document.createElement("div"); + readonly container = document.createElement("div"); hidden = false; + setMessageCalls = 0; constructor( message: string | DocumentFragment, readonly duration = 4_000, ) { + if (!noticeState.legacy) { + Object.defineProperty(this, "messageEl", { + enumerable: true, + value: this.container, + }); + } this.setMessage(message); - this.messageEl.addEventListener("click", () => { + this.container.addEventListener("click", () => { // Obsidian begins a hide transition immediately, while the Notice DOM // can remain connected until that transition completes. this.hidden = true; - this.messageEl.style.display = "none"; + this.container.style.display = "none"; }); - document.body.append(this.messageEl); + document.body.append(this.container); noticeState.instances.push(this); } setMessage(message: string | DocumentFragment): this { - this.messageEl.replaceChildren(); - if (typeof message === "string") this.messageEl.textContent = message; - else this.messageEl.append(message); + this.setMessageCalls += 1; + this.container.replaceChildren(); + if (typeof message === "string") this.container.textContent = message; + else this.container.append(message); return this; } hide(): void { this.hidden = true; - this.messageEl.remove(); + this.container.remove(); } } @@ -118,18 +143,28 @@ import { afterEach(() => { vi.useRealTimers(); noticeState.instances.length = 0; + noticeState.legacy = false; document.body.replaceChildren(); }); +function keyedNoticeRoot(notice: NoticeMockInstance): HTMLElement { + const root = notice.container.querySelector( + ".vpk-keyed-notice", + ); + if (root === null) throw new Error("Keyed Notice root was not rendered"); + return root; +} + describe("KeyedNoticeManager", () => { it("updates one Notice per key and restarts its expiry", async () => { vi.useFakeTimers(); const manager = new KeyedNoticeManager({ defaultDurationMs: 500 }); const first = manager.show("scan", "Scanning 1"); - const notice = noticeState.instances[0] as NoticeMockInstance; + const notice = noticeState.instances[0] as ModernNoticeMockInstance; expect(notice.duration).toBe(0); - expect(notice.messageEl.classList.contains("vpk-keyed-notice")).toBe(true); + const root = keyedNoticeRoot(notice); + expect(root.parentElement).toBe(notice.messageEl); await vi.advanceTimersByTimeAsync(400); const updated = manager.show("scan", "Scanning 2"); @@ -167,7 +202,8 @@ describe("KeyedNoticeManager", () => { expect(second).not.toBe(first); expect(noticeState.instances).toHaveLength(2); expect( - (noticeState.instances[1] as NoticeMockInstance).messageEl.textContent, + (noticeState.instances[1] as ModernNoticeMockInstance).messageEl + .textContent, ).toBe("Second"); }); @@ -201,6 +237,134 @@ describe("KeyedNoticeManager", () => { }); }); +describe("KeyedNoticeManager with a legacy Notice host", () => { + beforeEach(() => { + noticeState.legacy = true; + }); + + it("shows content without host element properties", () => { + const manager = new KeyedNoticeManager({ defaultDurationMs: false }); + + const returned = manager.show("legacy", "Visible on older Obsidian"); + const notice = noticeState.instances[0] as NoticeMockInstance; + + expect(returned).toBe(notice); + expect("messageEl" in notice).toBe(false); + expect("noticeEl" in notice).toBe(false); + expect(keyedNoticeRoot(notice).textContent).toBe( + "Visible on older Obsidian", + ); + }); + + it("updates the existing Notice without replacing its retained root", () => { + const manager = new KeyedNoticeManager({ defaultDurationMs: false }); + const first = manager.show("sync", "Scanning 1"); + const notice = noticeState.instances[0] as NoticeMockInstance; + const root = keyedNoticeRoot(notice); + + const updated = manager.show("sync", "Scanning 2"); + + expect(updated).toBe(first); + expect(noticeState.instances).toHaveLength(1); + expect(keyedNoticeRoot(notice)).toBe(root); + expect(root.textContent).toBe("Scanning 2"); + expect(notice.setMessageCalls).toBe(1); + }); + + it("uses the retained root lifecycle for has", () => { + const manager = new KeyedNoticeManager({ defaultDurationMs: false }); + manager.show("sync", "Connected"); + const notice = noticeState.instances[0] as NoticeMockInstance; + const root = keyedNoticeRoot(notice); + + expect(manager.has("sync")).toBe(true); + notice.container.remove(); + expect(root.isConnected).toBe(false); + expect(manager.has("sync")).toBe(false); + expect(notice.hidden).toBe(true); + expect(manager.hide("sync")).toBe(false); + }); + + it("restarts expiry after an update", async () => { + vi.useFakeTimers(); + const manager = new KeyedNoticeManager({ defaultDurationMs: 500 }); + const first = manager.show("sync", "Scanning 1"); + const notice = noticeState.instances[0] as NoticeMockInstance; + await vi.advanceTimersByTimeAsync(400); + + const updated = manager.show("sync", "Scanning 2"); + await vi.advanceTimersByTimeAsync(499); + + expect(updated).toBe(first); + expect(notice.hidden).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(notice.hidden).toBe(true); + expect(manager.has("sync")).toBe(false); + }); + + it("replaces a user-dismissed Notice on the next update", () => { + const manager = new KeyedNoticeManager({ defaultDurationMs: false }); + const first = manager.show("sync", "First"); + const firstNotice = noticeState.instances[0] as NoticeMockInstance; + const firstRoot = keyedNoticeRoot(firstNotice); + + firstRoot.click(); + expect(firstRoot.isConnected).toBe(true); + + const second = manager.show("sync", "Second"); + const secondNotice = noticeState.instances[1] as NoticeMockInstance; + + expect(second).not.toBe(first); + expect(noticeState.instances).toHaveLength(2); + expect(firstNotice.hidden).toBe(true); + expect(firstRoot.isConnected).toBe(false); + expect(keyedNoticeRoot(secondNotice).textContent).toBe("Second"); + }); + + it("removes retained entries through hide and dispose", () => { + const manager = new KeyedNoticeManager({ defaultDurationMs: false }); + manager.show("first", "First"); + const firstNotice = noticeState.instances[0] as NoticeMockInstance; + const firstRoot = keyedNoticeRoot(firstNotice); + + expect(manager.hide("first")).toBe(true); + expect(firstRoot.isConnected).toBe(false); + expect(manager.has("first")).toBe(false); + expect(manager.hide("first")).toBe(false); + + manager.show("second", "Second"); + const secondNotice = noticeState.instances[1] as NoticeMockInstance; + const secondRoot = keyedNoticeRoot(secondNotice); + manager.dispose(); + + expect(manager.isDisposed).toBe(true); + expect(secondNotice.hidden).toBe(true); + expect(secondRoot.isConnected).toBe(false); + expect(manager.has("second")).toBe(false); + }); + + it("renders string and consumed DocumentFragment messages", () => { + const manager = new KeyedNoticeManager({ defaultDurationMs: false }); + const first = manager.show("content", "Plain text"); + const notice = noticeState.instances[0] as NoticeMockInstance; + const root = keyedNoticeRoot(notice); + expect(root.textContent).toBe("Plain text"); + + const fragment = document.createDocumentFragment(); + const strong = document.createElement("strong"); + strong.textContent = "Fragment content"; + fragment.append(strong); + + const updated = manager.show("content", fragment); + + expect(updated).toBe(first); + expect(root.firstElementChild).toBe(strong); + expect(root.textContent).toBe("Fragment content"); + expect(fragment.childNodes).toHaveLength(0); + expect(notice.setMessageCalls).toBe(1); + }); +}); + describe("KeyedNoticeGroupManager", () => { it("keeps named messages in one Notice and separates them into rows", () => { const manager = new KeyedNoticeGroupManager(); @@ -317,7 +481,8 @@ describe("KeyedNoticeGroupManager", () => { .querySelector(".vpk-keyed-notice-group__message") ?.click(); expect( - (noticeState.instances[0] as NoticeMockInstance).messageEl.isConnected, + (noticeState.instances[0] as ModernNoticeMockInstance).messageEl + .isConnected, ).toBe(true); const second = manager.setItem("settings", "gamma", { @@ -327,7 +492,8 @@ describe("KeyedNoticeGroupManager", () => { expect(second).not.toBe(first); expect(noticeState.instances).toHaveLength(2); expect( - (noticeState.instances[1] as NoticeMockInstance).messageEl.textContent, + (noticeState.instances[1] as ModernNoticeMockInstance).messageEl + .textContent, ).toBe("Gamma changed"); }); @@ -377,7 +543,7 @@ describe("ObsidianUiNotifications", () => { }); notifications.show("sync", { message: "One" }); - const notice = noticeState.instances[0] as NoticeMockInstance; + const notice = noticeState.instances[0] as ModernNoticeMockInstance; await vi.advanceTimersByTimeAsync(400); notifications.show("sync", { message: "Two" }); diff --git a/packages/obsidian-plugin-kit/src/notice.ts b/packages/obsidian-plugin-kit/src/notice.ts index f96f2ae..30478a2 100644 --- a/packages/obsidian-plugin-kit/src/notice.ts +++ b/packages/obsidian-plugin-kit/src/notice.ts @@ -49,7 +49,9 @@ export interface FinishKeyedNoticeGroupOptions { interface NoticeEntry { notice: Notice; + root: HTMLElement; hideTimer: ReturnType | undefined; + dismissed: boolean; } function duration(value: number | false, name: string): number | false { @@ -61,12 +63,39 @@ function duration(value: number | false, name: string): number | false { return value; } -function noticeIsConnected(notice: Notice): boolean { - const messageEl = notice.messageEl as HTMLElement & { - isShown?: () => boolean; +function createNoticeEntry(message: KeyedNoticeMessage): NoticeEntry { + const root = createDiv({ cls: "vpk-keyed-notice" }); + root.replaceChildren(message); + const fragment = document.createDocumentFragment(); + fragment.append(root); + const entry: NoticeEntry = { + notice: new Notice(fragment, 0), + root, + hideTimer: undefined, + dismissed: false, }; - if (!messageEl.isConnected) return false; - return messageEl.isShown?.() ?? true; + // Obsidian can leave a dismissed Notice connected during its hide + // transition. Capture the click before that transition starts so an update + // cannot revive the acknowledged Notice. + root.addEventListener( + "click", + () => { + entry.dismissed = true; + }, + { capture: true }, + ); + return entry; +} + +function noticeEntryIsActive(entry: NoticeEntry): boolean { + return !entry.dismissed && entry.root.isConnected; +} + +function renderNoticeMessage( + entry: NoticeEntry, + message: KeyedNoticeMessage, +): void { + entry.root.replaceChildren(message); } /** @@ -74,6 +103,9 @@ function noticeIsConnected(notice: Notice): boolean { * * @remarks * Reusing a key updates the existing visible Notice and restarts its expiry. + * After a Notice is dismissed, the next update creates a fresh Notice even + * while the host is still completing its hide transition. The manager retains + * the DOM root it supplies to Obsidian and does not read host Notice elements. * Call {@link dispose} from the owning plug-in's unload lifecycle. A disposed * manager cannot show more Notices. */ @@ -101,7 +133,7 @@ export class KeyedNoticeManager { * @param key - Non-empty identifier scoped to this manager instance. * @param message - Text or fragment passed to Obsidian's Notice API. * @param options - Optional expiry override for this update. - * @returns The active Obsidian Notice. The same instance is returned while a keyed Notice remains connected. + * @returns The active Obsidian Notice. The same instance is returned while a keyed Notice remains active. */ show( key: string, @@ -116,19 +148,18 @@ export class KeyedNoticeManager { "durationMs", ); let entry = this.entries.get(key); - if (entry !== undefined && !noticeIsConnected(entry.notice)) { + if (entry !== undefined && !noticeEntryIsActive(entry)) { this.clearTimer(entry); this.entries.delete(key); + entry.notice.hide(); entry = undefined; } if (entry === undefined) { - const notice = new Notice(message, 0); - notice.messageEl.classList.add("vpk-keyed-notice"); - entry = { notice, hideTimer: undefined }; + entry = createNoticeEntry(message); this.entries.set(key, entry); } else { - entry.notice.setMessage(message); + renderNoticeMessage(entry, message); } this.clearTimer(entry); @@ -142,13 +173,14 @@ export class KeyedNoticeManager { return entry.notice; } - /** Returns whether the manager currently owns a connected Notice for a key. */ + /** Returns whether the manager currently owns an active Notice for a key. */ has(key: string): boolean { const entry = this.entries.get(key); if (entry === undefined) return false; - if (noticeIsConnected(entry.notice)) return true; + if (noticeEntryIsActive(entry)) return true; this.clearTimer(entry); this.entries.delete(key); + entry.notice.hide(); return false; } diff --git a/test/e2e-obsidian/scripts/notices.ts b/test/e2e-obsidian/scripts/notices.ts index 011742f..c23c452 100644 --- a/test/e2e-obsidian/scripts/notices.ts +++ b/test/e2e-obsidian/scripts/notices.ts @@ -44,6 +44,33 @@ async function main(): Promise { .waitFor({ state: "hidden", timeout: 5_000 }); }); + await executeHarnessStory(session, "notice-show"); + await withObsidianPage(port, async (page) => { + const notice = page + .locator(".vpk-keyed-notice") + .filter({ hasText: "Scanning Vault: 1", visible: true }); + if ((await notice.count()) !== 1) + throw new Error("Expected one visible keyed Notice before dismissal"); + await notice.evaluate((element) => { + element.setAttribute("data-vpk-e2e-instance", "dismissed"); + }); + await notice.click(); + await notice.waitFor({ state: "hidden", timeout: 5_000 }); + }); + + await executeHarnessStory(session, "notice-update"); + await withObsidianPage(port, async (page) => { + const notice = page + .locator(".vpk-keyed-notice") + .filter({ hasText: "Scanning Vault: 2", visible: true }); + if ((await notice.count()) !== 1) + throw new Error("Expected one fresh keyed Notice after dismissal"); + if ((await notice.getAttribute("data-vpk-e2e-instance")) !== null) { + throw new Error("Keyed Notice update revived the dismissed DOM root"); + } + await notice.waitFor({ state: "hidden", timeout: 5_000 }); + }); + await executeHarnessStory(session, "notice-group-start"); await withObsidianPage(port, async (page) => { const notice = page.locator(".notice:has(.vpk-keyed-notice-group)");