From eb3a86ddf1c9b8c2382d345c0d3c17f61c1d6a01 Mon Sep 17 00:00:00 2001 From: Kirtesh Suthar Date: Thu, 17 Sep 2026 12:06:43 +0530 Subject: [PATCH 1/5] fix(visual-builder): handle rejection when discussion highlights has no listener The visual builder registers the request-discussion-highlights listener inside DiscussionPage, which Venus only mounts while the Discussions panel is open. With the panel closed, which is the default, the send rejects with NO_REQUEST_LISTENER_FOUND and nothing handles it. Next.js dev registers a global unhandledrejection handler and renders the plain {code, message} rejection as a runtime error overlay titled "[object Object]". Handle the rejection at both send sites, since an absent receiver is the expected state for this event rather than a failure. Both branches of the isSSR check sent the same event, so the send is hoisted out of the branch instead of being duplicated. The spec mocked send as vi.fn(), which returns undefined rather than a promise. Tests only passed because two later tests set mockResolvedValue and vi.clearAllMocks does not reset implementations, so every test after them inherited a promise. The base mock now matches the real contract. Co-Authored-By: Claude --- .../useVariantsPostMessageEvent.spec.ts | 38 ++++++++++++++++++- .../useRecalculateVariantDataCSLPValues.ts | 9 +++-- .../useVariantsPostMessageEvent.ts | 18 +++++---- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts index 0ccd3941..22c3b5fb 100644 --- a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts +++ b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts @@ -48,7 +48,9 @@ vi.mock("../../../visualBuilder/utils/visualBuilderPostMessage", () => { return { default: { on: vi.fn(), - send: vi.fn(), + // send always returns a promise; tests that assert on rejection + // handling need the mock to match that contract. + send: vi.fn().mockResolvedValue(undefined), }, }; }); @@ -728,4 +730,38 @@ describe("useVariantFieldsPostMessageEvent SSR handling", () => { ); expect(updateVariantClasses).toHaveBeenCalled(); }); + + // The visual builder registers this listener only while the + // Discussions panel is open, so the send rejects on most page loads. Left + // unhandled it reaches Next.js's unhandledrejection hook and renders a + // "[object Object]" runtime error overlay in dev. + it("attaches a rejection handler to the send", async () => { + useVariantFieldsPostMessageEvent({ isSSR: true }); + const call = mockVisualBuilderPostMessage.on.mock.calls.find( + (call: any[]) => + call[0] === VisualBuilderPostMessageEvents.GET_VARIANT_ID + ); + const handler = call ? call[1] : null; + + const rejection = Promise.reject({ + code: "NO_REQUEST_LISTENER_FOUND", + message: + 'No request listener found for event "request-discussion-highlights"', + }); + const catchSpy = vi.spyOn(rejection, "catch"); + (mockVisualBuilderPostMessage.send as any).mockReturnValue(rejection); + + try { + handler!({ data: { variant: "variant-123" } }); + expect(catchSpy).toHaveBeenCalled(); + await expect(rejection).rejects.toMatchObject({ + code: "NO_REQUEST_LISTENER_FOUND", + }); + } finally { + catchSpy.mockRestore(); + (mockVisualBuilderPostMessage.send as any).mockResolvedValue( + undefined + ); + } + }); }); diff --git a/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts b/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts index d44dd5ed..2827b041 100644 --- a/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts +++ b/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts @@ -14,9 +14,12 @@ const VARIANT_UPDATE_DELAY_MS: Readonly = 8000; // Coalesce a burst of data-cslp mutations into a single request to the // visual editor. const requestDiscussionHighlights = debounce(() => { - visualBuilderPostMessage?.send( - VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS - ); + // Receiver is optional: the visual builder only listens while the + // Discussions panel is open, so a missing listener is expected and its + // rejection must not surface as an unhandled one. + visualBuilderPostMessage + ?.send(VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS) + .catch(() => {}); }, 200); type OnAudienceModeVariantPatchUpdate = { diff --git a/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts b/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts index 33f98443..75a81112 100644 --- a/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts +++ b/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts @@ -167,17 +167,19 @@ export function useVariantFieldsPostMessageEvent({ isSSR }: { isSSR: boolean }): if (selectedVariant) { addVariantFieldClass(selectedVariant); } - // SSR DOM is final; observer never fires, request directly. - visualBuilderPostMessage?.send( - VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS - ); } else { - // CSR: observer in updateVariantClasses requests on settle. + // CSR: observer in updateVariantClasses also requests on settle. updateVariantClasses(); - visualBuilderPostMessage?.send( - VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS - ); } + // Sent in both modes: SSR has no observer to fire it later. The + // receiver is optional — VB listens only while the Discussions + // panel is open — so the rejection must be handled here or it + // surfaces as an unhandled one. + visualBuilderPostMessage + ?.send( + VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS + ) + .catch(() => {}); } ); visualBuilderPostMessage?.on( From 19f80f9a69bd53ded3bf579d96ec6e36ad162904 Mon Sep 17 00:00:00 2001 From: Kirtesh Suthar Date: Thu, 17 Sep 2026 17:03:40 +0530 Subject: [PATCH 2/5] refactor(visual-builder): warn on real send failures, stay silent on a missing listener Swallowing every rejection hid genuine problems. NO_ACK_RECEIVED, WINDOW_CLOSED and CODE_RETURNED_ERROR would have been discarded as quietly as the expected missing listener. ignoreMissingListener returns silently only for NO_REQUEST_LISTENER_FOUND, which is the normal state while the Discussions panel is closed, and warns through PublicLogger for anything else. PublicLogger is already used across the SDK and is silent under NODE_ENV=test, so this adds no console noise in the common case. The helper lives in its own module because the spec mocks the visualBuilderPostMessage module wholesale; keeping it separate means tests exercise the real function rather than a mock. Co-Authored-By: Claude --- .../useRecalculateVariantDataCSLPValues.ts | 12 ++++-- .../useVariantsPostMessageEvent.ts | 12 ++++-- .../utils/__test__/postMessageErrors.test.ts | 43 +++++++++++++++++++ src/visualBuilder/utils/postMessageErrors.ts | 23 ++++++++++ 4 files changed, 82 insertions(+), 8 deletions(-) create mode 100644 src/visualBuilder/utils/__test__/postMessageErrors.test.ts create mode 100644 src/visualBuilder/utils/postMessageErrors.ts diff --git a/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts b/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts index 2827b041..c8810df0 100644 --- a/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts +++ b/src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts @@ -6,6 +6,7 @@ import { visualBuilderStyles } from "../visualBuilder.style"; import { isValidCslp } from "../../cslp/cslpdata"; import { setHighlightVariantFields } from "./useVariantsPostMessageEvent"; import visualBuilderPostMessage from "../utils/visualBuilderPostMessage"; +import { ignoreMissingListener } from "../utils/postMessageErrors"; import { VisualBuilderPostMessageEvents } from "../utils/types/postMessage.types"; import { debounce } from "lodash-es"; @@ -14,12 +15,15 @@ const VARIANT_UPDATE_DELAY_MS: Readonly = 8000; // Coalesce a burst of data-cslp mutations into a single request to the // visual editor. const requestDiscussionHighlights = debounce(() => { - // Receiver is optional: the visual builder only listens while the - // Discussions panel is open, so a missing listener is expected and its - // rejection must not surface as an unhandled one. + // The visual builder listens only while the Discussions panel is open, so + // an absent receiver is expected here rather than a failure. visualBuilderPostMessage ?.send(VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS) - .catch(() => {}); + .catch( + ignoreMissingListener( + VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS + ) + ); }, 200); type OnAudienceModeVariantPatchUpdate = { diff --git a/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts b/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts index 75a81112..75d7f19e 100644 --- a/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts +++ b/src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts @@ -1,6 +1,7 @@ import { VisualBuilder } from ".."; import { visualBuilderStyles } from "../visualBuilder.style"; import visualBuilderPostMessage from "../utils/visualBuilderPostMessage"; +import { ignoreMissingListener } from "../utils/postMessageErrors"; import { VisualBuilderPostMessageEvents } from "../utils/types/postMessage.types"; import { FieldSchemaMap } from "../utils/fieldSchemaMap"; import { updateVariantClasses } from "./useRecalculateVariantDataCSLPValues"; @@ -172,14 +173,17 @@ export function useVariantFieldsPostMessageEvent({ isSSR }: { isSSR: boolean }): updateVariantClasses(); } // Sent in both modes: SSR has no observer to fire it later. The - // receiver is optional — VB listens only while the Discussions - // panel is open — so the rejection must be handled here or it - // surfaces as an unhandled one. + // visual builder listens only while the Discussions panel is open, + // so an absent receiver is expected here rather than a failure. visualBuilderPostMessage ?.send( VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS ) - .catch(() => {}); + .catch( + ignoreMissingListener( + VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS + ) + ); } ); visualBuilderPostMessage?.on( diff --git a/src/visualBuilder/utils/__test__/postMessageErrors.test.ts b/src/visualBuilder/utils/__test__/postMessageErrors.test.ts new file mode 100644 index 00000000..dbaa2dbc --- /dev/null +++ b/src/visualBuilder/utils/__test__/postMessageErrors.test.ts @@ -0,0 +1,43 @@ +import { vi, describe, it, expect, beforeEach } from "vitest"; +import { ignoreMissingListener } from "../postMessageErrors"; +import { PublicLogger } from "../../../logger/logger"; + +describe("ignoreMissingListener", () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it("stays silent when the receiver is simply not mounted", () => { + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); + + ignoreMissingListener("request-discussion-highlights")({ + code: "NO_REQUEST_LISTENER_FOUND", + message: 'No request listener found for event "x"', + }); + + expect(warn).not.toHaveBeenCalled(); + }); + + it.each([ + "NO_ACK_RECEIVED", + "WINDOW_CLOSED", + "CODE_RETURNED_ERROR", + ])("warns on %s so a real failure is still visible", (code) => { + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); + + ignoreMissingListener("request-discussion-highlights")({ code }); + + expect(warn).toHaveBeenCalledOnce(); + expect(warn.mock.calls[0][0]).toContain( + "request-discussion-highlights" + ); + }); + + it("warns when the rejection is not a coded object", () => { + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); + + ignoreMissingListener("some-event")(new Error("boom")); + + expect(warn).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/visualBuilder/utils/postMessageErrors.ts b/src/visualBuilder/utils/postMessageErrors.ts new file mode 100644 index 00000000..3c864e8a --- /dev/null +++ b/src/visualBuilder/utils/postMessageErrors.ts @@ -0,0 +1,23 @@ +import { PublicLogger } from "../../logger/logger"; + +// adv-post-message does not export ERROR_CODES from its entry point, so the +// wire value is matched directly. +const NO_REQUEST_LISTENER_FOUND = "NO_REQUEST_LISTENER_FOUND"; + +/** + * Rejection handler for sends whose receiver is only mounted some of the time. + * A missing listener is the expected state and stays silent; every other + * failure (no ack, closed window, a throwing receiver) is warned about so a + * real breakage stays visible. + */ +export function ignoreMissingListener(event: string): (error: unknown) => void { + return (error: unknown) => { + if ((error as { code?: string })?.code === NO_REQUEST_LISTENER_FOUND) { + return; + } + PublicLogger.warn( + `Failed to send "${event}" to the visual builder`, + error + ); + }; +} From b78d2662f563f2261b02eea9a6b7088fc2485ee5 Mon Sep 17 00:00:00 2001 From: Kirtesh Suthar Date: Mon, 21 Sep 2026 11:39:27 +0530 Subject: [PATCH 3/5] fix(visual-builder): correct the documented rejection contract and cover the observer send Review found that the helper's doc comment and its tests described rejection shapes the library does not produce. Verified against @contentstack/advanced-post-message 0.0.5: only the missing-listener reply carries a code. The closed-window path rejects with an uncoded Error, the no-ack timeout rejects with a bare string, and CODE_RETURNED_ERROR is logged on the receiving side and never sent back, so the sender never sees it. The handler already did the right thing for all of these, since an uncoded rejection warns. Only the doc comment and the tests were wrong, so the code check stays as the single rule and the comment now says so. Matching the no-ack case too would mean matching on a message string, and a receiver that never acks is worth a warning anyway. Also from review: - type the event parameter as VisualBuilderPostMessageEvents instead of string - restate the send mock contract in beforeEach so the spec no longer depends on implementations leaking between tests, and drop the hand-restore - add a spec for the debounced observer send, which had no coverage. The suite stubs MutationObserver globally in vitest.setup.ts, so the spec captures the observer callback and drives it directly. Co-Authored-By: Claude --- ...seRecalculateVariantDataCSLPValues.spec.ts | 97 +++++++++++++++++++ .../useVariantsPostMessageEvent.spec.ts | 30 +++--- .../utils/__test__/postMessageErrors.test.ts | 34 ++++--- src/visualBuilder/utils/postMessageErrors.ts | 13 ++- 4 files changed, 144 insertions(+), 30 deletions(-) create mode 100644 src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts diff --git a/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts b/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts new file mode 100644 index 00000000..9ac7a9e9 --- /dev/null +++ b/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts @@ -0,0 +1,97 @@ +import { vi, describe, it, expect, beforeEach, afterEach } from "vitest"; + +// Identity debounce so the observer's request fires within the test. +vi.mock("lodash-es", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, debounce: vi.fn((fn: any) => fn) }; +}); + +vi.mock("../../../visualBuilder/utils/visualBuilderPostMessage", () => ({ + default: { on: vi.fn(), send: vi.fn().mockResolvedValue(undefined) }, +})); + +vi.mock("../../../visualBuilder", () => ({ + VisualBuilder: { + VisualBuilderGlobalState: { + value: { variant: null, highlightVariantFields: false }, + }, + }, +})); + +import { updateVariantClasses } from "../useRecalculateVariantDataCSLPValues"; +import { VisualBuilderPostMessageEvents } from "../../utils/types/postMessage.types"; +import visualBuilderPostMessage from "../../../visualBuilder/utils/visualBuilderPostMessage"; +import * as cslpdata from "../../../cslp/cslpdata"; + +const send = (visualBuilderPostMessage as any).send; + +// vitest.setup.ts swaps MutationObserver for a stub that drops the callback, so +// the real observer never fires here. Capture the callback and drive it instead. +let observerCallbacks: MutationCallback[] = []; +const realMutationObserver = global.MutationObserver; + +class CapturingMutationObserver { + observe = vi.fn(); + disconnect = vi.fn(); + takeRecords = vi.fn((): MutationRecord[] => []); + constructor(callback: MutationCallback) { + observerCallbacks.push(callback); + } +} + +const attributeMutation = { + type: "attributes", + attributeName: "data-cslp", + addedNodes: [] as unknown as NodeList, +} as unknown as MutationRecord; + +describe("requestDiscussionHighlights via the CSLP mutation observer", () => { + beforeEach(() => { + vi.clearAllMocks(); + observerCallbacks = []; + global.MutationObserver = + CapturingMutationObserver as unknown as typeof MutationObserver; + send.mockResolvedValue(undefined); + vi.spyOn(cslpdata, "isValidCslp").mockReturnValue(true); + document.body.innerHTML = `

hi

`; + }); + + afterEach(() => { + global.MutationObserver = realMutationObserver; + document.body.innerHTML = ""; + vi.restoreAllMocks(); + }); + + function fireMutation() { + updateVariantClasses(); + expect(observerCallbacks).toHaveLength(1); + observerCallbacks[0]([attributeMutation], {} as MutationObserver); + } + + it("requests discussion highlights when a data-cslp attribute changes", () => { + fireMutation(); + + expect(send).toHaveBeenCalledWith( + VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS + ); + }); + + // The bug this guards: the Discussions panel is closed on most CSR page + // loads, so this send rejects nearly every time. Left unhandled it reaches + // the Next.js dev overlay. + it("attaches a rejection handler to that send", async () => { + const rejection = Promise.reject({ + code: "NO_REQUEST_LISTENER_FOUND", + message: 'No request listener found for event "x"', + }); + const catchSpy = vi.spyOn(rejection, "catch"); + send.mockReturnValue(rejection); + + fireMutation(); + + expect(catchSpy).toHaveBeenCalled(); + await expect(rejection).rejects.toMatchObject({ + code: "NO_REQUEST_LISTENER_FOUND", + }); + }); +}); diff --git a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts index 22c3b5fb..3e55452b 100644 --- a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts +++ b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts @@ -155,7 +155,8 @@ describe("useVariantFieldsPostMessageEvent", () => { // Reset mocks vi.clearAllMocks(); - + (mockVisualBuilderPostMessage.send as any).mockResolvedValue(undefined); + // Mock isValidCslp to return true for test data (after clearAllMocks) vi.spyOn(cslpdata, "isValidCslp").mockReturnValue(true); }); @@ -615,6 +616,9 @@ describe("useVariantFieldsPostMessageEvent SSR handling", () => { beforeEach(() => { document.querySelectorAll = mockQuerySelectorAll; vi.clearAllMocks(); + // Restate the contract rather than inherit it: send always returns a + // promise, and a test below swaps in a rejecting one. + (mockVisualBuilderPostMessage.send as any).mockResolvedValue(undefined); }); afterEach(() => { @@ -748,20 +752,20 @@ describe("useVariantFieldsPostMessageEvent SSR handling", () => { message: 'No request listener found for event "request-discussion-highlights"', }); + // Deliberately coupled to the `.catch` shape. Asserting "no unhandled + // rejection escapes" would survive a refactor to try/catch, but neither + // a process `unhandledRejection` listener nor the jsdom window event + // fires reliably under vitest here: the same assertion passes with the + // fix removed, so it proves nothing. Rewriting this to catch a + // try/catch refactor means fixing that detection first. const catchSpy = vi.spyOn(rejection, "catch"); (mockVisualBuilderPostMessage.send as any).mockReturnValue(rejection); - try { - handler!({ data: { variant: "variant-123" } }); - expect(catchSpy).toHaveBeenCalled(); - await expect(rejection).rejects.toMatchObject({ - code: "NO_REQUEST_LISTENER_FOUND", - }); - } finally { - catchSpy.mockRestore(); - (mockVisualBuilderPostMessage.send as any).mockResolvedValue( - undefined - ); - } + handler!({ data: { variant: "variant-123" } }); + + expect(catchSpy).toHaveBeenCalled(); + await expect(rejection).rejects.toMatchObject({ + code: "NO_REQUEST_LISTENER_FOUND", + }); }); }); diff --git a/src/visualBuilder/utils/__test__/postMessageErrors.test.ts b/src/visualBuilder/utils/__test__/postMessageErrors.test.ts index dbaa2dbc..87850fbc 100644 --- a/src/visualBuilder/utils/__test__/postMessageErrors.test.ts +++ b/src/visualBuilder/utils/__test__/postMessageErrors.test.ts @@ -1,42 +1,50 @@ import { vi, describe, it, expect, beforeEach } from "vitest"; import { ignoreMissingListener } from "../postMessageErrors"; +import { VisualBuilderPostMessageEvents } from "../types/postMessage.types"; import { PublicLogger } from "../../../logger/logger"; +const EVENT = VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS; + describe("ignoreMissingListener", () => { beforeEach(() => { vi.restoreAllMocks(); }); - it("stays silent when the receiver is simply not mounted", () => { + // The only rejection adv-post-message sends back with a code: the receiver + // acked, looked for a listener and found none. + it("stays silent when the receiver answers that nobody is listening", () => { const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); - ignoreMissingListener("request-discussion-highlights")({ + ignoreMissingListener(EVENT)({ code: "NO_REQUEST_LISTENER_FOUND", - message: 'No request listener found for event "x"', + message: + 'No request listener found for event "request-discussion-highlights"', }); expect(warn).not.toHaveBeenCalled(); }); + // The shapes the library's own timeout paths reject with. Neither carries a + // code, so both must fall through to the warning. it.each([ - "NO_ACK_RECEIVED", - "WINDOW_CLOSED", - "CODE_RETURNED_ERROR", - ])("warns on %s so a real failure is still visible", (code) => { + ["an Error, as the closed-window path rejects", new Error("closed")], + [ + "a bare string, as the no-ack timeout rejects", + "contentstack-adv-post-message: The ACK was not received", + ], + ])("warns on %s so a real failure stays visible", (_shape, error) => { const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); - ignoreMissingListener("request-discussion-highlights")({ code }); + ignoreMissingListener(EVENT)(error); expect(warn).toHaveBeenCalledOnce(); - expect(warn.mock.calls[0][0]).toContain( - "request-discussion-highlights" - ); + expect(warn.mock.calls[0][0]).toContain(EVENT); }); - it("warns when the rejection is not a coded object", () => { + it("warns on a coded rejection that is not the missing listener", () => { const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); - ignoreMissingListener("some-event")(new Error("boom")); + ignoreMissingListener(EVENT)({ code: "SOMETHING_ELSE" }); expect(warn).toHaveBeenCalledOnce(); }); diff --git a/src/visualBuilder/utils/postMessageErrors.ts b/src/visualBuilder/utils/postMessageErrors.ts index 3c864e8a..6c10b5cd 100644 --- a/src/visualBuilder/utils/postMessageErrors.ts +++ b/src/visualBuilder/utils/postMessageErrors.ts @@ -1,4 +1,5 @@ import { PublicLogger } from "../../logger/logger"; +import { VisualBuilderPostMessageEvents } from "./types/postMessage.types"; // adv-post-message does not export ERROR_CODES from its entry point, so the // wire value is matched directly. @@ -6,11 +7,15 @@ const NO_REQUEST_LISTENER_FOUND = "NO_REQUEST_LISTENER_FOUND"; /** * Rejection handler for sends whose receiver is only mounted some of the time. - * A missing listener is the expected state and stays silent; every other - * failure (no ack, closed window, a throwing receiver) is warned about so a - * real breakage stays visible. + * + * Only the missing-listener reply carries a `code`; the library's other + * rejections are an uncoded `Error` (closed window) or a bare string (no ack), + * so they fall through to the warning. That is deliberate: a receiver that + * never acks is not the same as one that answered "nobody is listening". */ -export function ignoreMissingListener(event: string): (error: unknown) => void { +export function ignoreMissingListener( + event: VisualBuilderPostMessageEvents +): (error: unknown) => void { return (error: unknown) => { if ((error as { code?: string })?.code === NO_REQUEST_LISTENER_FOUND) { return; From dc4d8bf3f2c102ca30479773435d95584a881e56 Mon Sep 17 00:00:00 2001 From: Kirtesh Suthar Date: Mon, 21 Sep 2026 11:58:44 +0530 Subject: [PATCH 4/5] test(visual-builder): bind both send sites to the discriminating handler Review pointed out that asserting `.catch` was called proves a handler was attached, not which one: swapping ignoreMissingListener for `() => {}` kept the tests green. Asserting PublicLogger.warn stays silent on the missing-listener code does not close that gap either, since a no-op catch is also silent. Verified by mutation: with `() => {}` at both call sites, the silent-case assertions still pass. What fails is a rejection the helper is supposed to warn about, so each spec now has that case, using the uncoded string the library's no-ack timeout actually rejects with. Also from review: import VisualBuilderPostMessageEvents as a type in postMessageErrors.ts, since it is only used in a type position. Co-Authored-By: Claude --- ...seRecalculateVariantDataCSLPValues.spec.ts | 27 +++++++++++++-- .../useVariantsPostMessageEvent.spec.ts | 34 +++++++++++++++++-- src/visualBuilder/utils/postMessageErrors.ts | 2 +- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts b/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts index 9ac7a9e9..67436578 100644 --- a/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts +++ b/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts @@ -22,6 +22,7 @@ import { updateVariantClasses } from "../useRecalculateVariantDataCSLPValues"; import { VisualBuilderPostMessageEvents } from "../../utils/types/postMessage.types"; import visualBuilderPostMessage from "../../../visualBuilder/utils/visualBuilderPostMessage"; import * as cslpdata from "../../../cslp/cslpdata"; +import { PublicLogger } from "../../../logger/logger"; const send = (visualBuilderPostMessage as any).send; @@ -85,13 +86,35 @@ describe("requestDiscussionHighlights via the CSLP mutation observer", () => { message: 'No request listener found for event "x"', }); const catchSpy = vi.spyOn(rejection, "catch"); + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); send.mockReturnValue(rejection); fireMutation(); - - expect(catchSpy).toHaveBeenCalled(); await expect(rejection).rejects.toMatchObject({ code: "NO_REQUEST_LISTENER_FOUND", }); + + expect(catchSpy).toHaveBeenCalled(); + expect(warn).not.toHaveBeenCalled(); + }); + + // postMessageErrors is not mocked here, so the real helper runs. This is + // the case that ties the call site to it: a bare `.catch(() => {})` would + // pass every assertion above but fail this one. + it("warns through the helper when the send fails for another reason", async () => { + // The shape the library's no-ack timeout rejects with: no code. + const rejection = Promise.reject( + "contentstack-adv-post-message: The ACK was not received" + ); + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); + send.mockReturnValue(rejection); + + fireMutation(); + await expect(rejection).rejects.toBeTruthy(); + + expect(warn).toHaveBeenCalledOnce(); + expect(warn.mock.calls[0][0]).toContain( + VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS + ); }); }); diff --git a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts index 3e55452b..eef0dd2d 100644 --- a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts +++ b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts @@ -39,6 +39,7 @@ import visualBuilderPostMessage from "../../../visualBuilder/utils/visualBuilder import { EventManager } from "@contentstack/advanced-post-message"; import { updateVariantClasses } from "../../../visualBuilder/eventManager/useRecalculateVariantDataCSLPValues"; import * as cslpdata from "../../../cslp/cslpdata"; +import { PublicLogger } from "../../../logger/logger"; const mockVisualBuilderPostMessage = visualBuilderPostMessage as MockedObject; @@ -759,13 +760,42 @@ describe("useVariantFieldsPostMessageEvent SSR handling", () => { // fix removed, so it proves nothing. Rewriting this to catch a // try/catch refactor means fixing that detection first. const catchSpy = vi.spyOn(rejection, "catch"); + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); (mockVisualBuilderPostMessage.send as any).mockReturnValue(rejection); handler!({ data: { variant: "variant-123" } }); - - expect(catchSpy).toHaveBeenCalled(); await expect(rejection).rejects.toMatchObject({ code: "NO_REQUEST_LISTENER_FOUND", }); + + expect(catchSpy).toHaveBeenCalled(); + expect(warn).not.toHaveBeenCalled(); + }); + + // postMessageErrors is not mocked here, so the real helper runs. This is + // the case that ties the call site to it: a bare `.catch(() => {})` would + // pass every assertion above but fail this one. + it("warns through the helper when the send fails for another reason", async () => { + useVariantFieldsPostMessageEvent({ isSSR: true }); + const call = mockVisualBuilderPostMessage.on.mock.calls.find( + (call: any[]) => + call[0] === VisualBuilderPostMessageEvents.GET_VARIANT_ID + ); + const handler = call ? call[1] : null; + + // The shape the library's no-ack timeout rejects with: no code. + const rejection = Promise.reject( + "contentstack-adv-post-message: The ACK was not received" + ); + const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); + (mockVisualBuilderPostMessage.send as any).mockReturnValue(rejection); + + handler!({ data: { variant: "variant-123" } }); + await expect(rejection).rejects.toBeTruthy(); + + expect(warn).toHaveBeenCalledOnce(); + expect(warn.mock.calls[0][0]).toContain( + VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS + ); }); }); diff --git a/src/visualBuilder/utils/postMessageErrors.ts b/src/visualBuilder/utils/postMessageErrors.ts index 6c10b5cd..bf141ab5 100644 --- a/src/visualBuilder/utils/postMessageErrors.ts +++ b/src/visualBuilder/utils/postMessageErrors.ts @@ -1,5 +1,5 @@ import { PublicLogger } from "../../logger/logger"; -import { VisualBuilderPostMessageEvents } from "./types/postMessage.types"; +import type { VisualBuilderPostMessageEvents } from "./types/postMessage.types"; // adv-post-message does not export ERROR_CODES from its entry point, so the // wire value is matched directly. From de91b09de7d3f8d47252f9aa5fcfe1c6ada7b278 Mon Sep 17 00:00:00 2001 From: Kirtesh Suthar Date: Mon, 21 Sep 2026 12:14:20 +0530 Subject: [PATCH 5/5] test(visual-builder): pin the rejection value in the warning, and tidy spy lifecycles Review found that every warn-branch assertion read only the message the helper builds from the event name, so dropping the error argument from PublicLogger.warn kept all of them green while the warning lost the part that says what failed. All four sites now assert the second argument. Verified by mutation: removing that argument fails all four. Also from review: - capture MutationObserver in beforeEach rather than at module scope. The setup file installs its stub in beforeAll, so the module-level read took jsdom's native implementation and afterEach restored the wrong one. - restore spies in the SSR block's afterEach. clearAllMocks leaves the implementation in place, so PublicLogger.warn stayed stubbed for the rest of the file once these tests had run. - assert the rejected value instead of toBeTruthy, which held regardless of what the code did. Co-Authored-By: Claude --- .../useRecalculateVariantDataCSLPValues.spec.ts | 15 ++++++++++++--- .../__test__/useVariantsPostMessageEvent.spec.ts | 8 +++++++- .../utils/__test__/postMessageErrors.test.ts | 3 +++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts b/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts index 67436578..8364f7c3 100644 --- a/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts +++ b/src/visualBuilder/eventManager/__test__/useRecalculateVariantDataCSLPValues.spec.ts @@ -29,7 +29,10 @@ const send = (visualBuilderPostMessage as any).send; // vitest.setup.ts swaps MutationObserver for a stub that drops the callback, so // the real observer never fires here. Capture the callback and drive it instead. let observerCallbacks: MutationCallback[] = []; -const realMutationObserver = global.MutationObserver; +// Captured in beforeEach, not at module scope: vitest.setup.ts installs its +// stub in beforeAll, so a module-level read would grab jsdom's native one and +// afterEach would restore the wrong implementation. +let installedMutationObserver: typeof MutationObserver; class CapturingMutationObserver { observe = vi.fn(); @@ -50,6 +53,7 @@ describe("requestDiscussionHighlights via the CSLP mutation observer", () => { beforeEach(() => { vi.clearAllMocks(); observerCallbacks = []; + installedMutationObserver = global.MutationObserver; global.MutationObserver = CapturingMutationObserver as unknown as typeof MutationObserver; send.mockResolvedValue(undefined); @@ -58,7 +62,7 @@ describe("requestDiscussionHighlights via the CSLP mutation observer", () => { }); afterEach(() => { - global.MutationObserver = realMutationObserver; + global.MutationObserver = installedMutationObserver; document.body.innerHTML = ""; vi.restoreAllMocks(); }); @@ -110,11 +114,16 @@ describe("requestDiscussionHighlights via the CSLP mutation observer", () => { send.mockReturnValue(rejection); fireMutation(); - await expect(rejection).rejects.toBeTruthy(); + await expect(rejection).rejects.toBe( + "contentstack-adv-post-message: The ACK was not received" + ); expect(warn).toHaveBeenCalledOnce(); expect(warn.mock.calls[0][0]).toContain( VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS ); + expect(warn.mock.calls[0][1]).toBe( + "contentstack-adv-post-message: The ACK was not received" + ); }); }); diff --git a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts index eef0dd2d..efba4531 100644 --- a/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts +++ b/src/visualBuilder/eventManager/__test__/useVariantsPostMessageEvent.spec.ts @@ -624,6 +624,7 @@ describe("useVariantFieldsPostMessageEvent SSR handling", () => { afterEach(() => { document.querySelectorAll = originalQuerySelectorAll; + vi.restoreAllMocks(); }); it("should call addVariantFieldClass directly when isSSR is true and variant is provided", () => { @@ -791,11 +792,16 @@ describe("useVariantFieldsPostMessageEvent SSR handling", () => { (mockVisualBuilderPostMessage.send as any).mockReturnValue(rejection); handler!({ data: { variant: "variant-123" } }); - await expect(rejection).rejects.toBeTruthy(); + await expect(rejection).rejects.toBe( + "contentstack-adv-post-message: The ACK was not received" + ); expect(warn).toHaveBeenCalledOnce(); expect(warn.mock.calls[0][0]).toContain( VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS ); + expect(warn.mock.calls[0][1]).toBe( + "contentstack-adv-post-message: The ACK was not received" + ); }); }); diff --git a/src/visualBuilder/utils/__test__/postMessageErrors.test.ts b/src/visualBuilder/utils/__test__/postMessageErrors.test.ts index 87850fbc..fc1291a3 100644 --- a/src/visualBuilder/utils/__test__/postMessageErrors.test.ts +++ b/src/visualBuilder/utils/__test__/postMessageErrors.test.ts @@ -39,6 +39,9 @@ describe("ignoreMissingListener", () => { expect(warn).toHaveBeenCalledOnce(); expect(warn.mock.calls[0][0]).toContain(EVENT); + // The rejection itself is the reason this branch exists; without this + // the warn call could drop it and every assertion here would hold. + expect(warn.mock.calls[0][1]).toBe(error); }); it("warns on a coded rejection that is not the missing listener", () => {