fix(visual-builder): handle rejection when discussion highlights has no listener - #652
kirtesh-cstk wants to merge 2 commits into
Conversation
…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 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
There was a problem hiding this comment.
🟡 Changes recommended
The new .catch() chaining is not safely optional-chained and can throw if the post-message manager is undefined, undermining the intended guard behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents dev-time Next.js runtime error overlays caused by unhandled promise rejections when REQUEST_DISCUSSION_HIGHLIGHTS is sent while the Visual Builder Discussions panel (the listener) is not mounted.
Changes:
- Handle
REQUEST_DISCUSSION_HIGHLIGHTSsend rejections in the variant post-message event handler (shared for SSR/CSR paths). - Handle send rejections for the debounced discussion-highlights request triggered from CSLP mutation observation.
- Update the unit test mock contract for
send()to always return a promise and add a regression test asserting rejection handling.
File summaries
| File | Description |
|---|---|
| src/visualBuilder/eventManager/useVariantsPostMessageEvent.ts | Adds rejection handling for REQUEST_DISCUSSION_HIGHLIGHTS when variant changes. |
| src/visualBuilder/eventManager/useRecalculateVariantDataCSLPValues.ts | Adds rejection handling for the debounced highlights request fired from mutation observers. |
| src/visualBuilder/eventManager/test/useVariantsPostMessageEvent.spec.ts | Fixes send() mock to return a promise and adds a regression test for rejection handling. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
hitesh-shetty-cstk
left a comment
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Automated review
What this changes: Two visualBuilderPostMessage.send(REQUEST_DISCUSSION_HIGHLIGHTS) call sites now attach a .catch, so a missing receiver no longer produces an unhandled rejection. In useVariantFieldsPostMessageEvent the send also moves out of the isSSR branches, which both sent the same event. The spec's base send mock now returns a promise, and one regression test is added.
Business impact: The changed handler sits on the variant switching path (get-variant-id), a canvas journey content editors depend on. I found no break in it. The hoisted send fires in both modes exactly as it did before, and the optional chain short-circuits before .catch when visualBuilderPostMessage is undefined, so no new throw can reach the handler. The residual user-visible risk runs the other way: .catch(() => {}) also discards genuine send failures, so if the request fails for any reason other than a missing listener, comment markers stop appearing on the canvas and nothing reports it. That is the first should-fix below.
Security: nothing beyond what the scanners cover. Snyk is clean on this head. The diff adds no payload handling, moves no origin check, and sends nothing new across the iframe boundary.
On the Copilot review: both of its comments claim ?.send(...).catch(...) can throw a TypeError when visualBuilderPostMessage is undefined. That is not correct, and acting on it would churn the PR for nothing. An optional chain short-circuits as a whole expression, so when the base is nullish the rest of the chain is not evaluated and the result is undefined, which means .catch is never accessed. a?.b().c() is safe for nullish a in the same way a?.b.c is. I have replied on both threads.
Flow
sequenceDiagram
participant VB as Visual Builder (parent window)
participant Hook as useVariantFieldsPostMessageEvent
participant Obs as useRecalculateVariantDataCSLPValues
participant PM as visualBuilderPostMessage
VB->>Hook: get-variant-id
Hook->>Hook: setVariant, FieldSchemaMap.clear, variant classes
Hook->>PM: send(request-discussion-highlights)
Obs->>PM: send(request-discussion-highlights), debounced 200ms
Note over Hook,Obs: changed here, both sends now attach .catch
alt Discussions panel mounted
PM->>VB: request delivered
VB-->>PM: highlight payload
else Panel closed, which is the default
PM-->>Hook: reject NO_REQUEST_LISTENER_FOUND
Note over Hook: discarded, and so is every other failure
end
Test changes: The base mock moving from vi.fn() to vi.fn().mockResolvedValue(undefined) is a correction, not a loosening. The real send returns a promise, so the old mock was wrong and the spec passed only on implementations leaking out of earlier tests. No assertion was weakened, nothing was skipped or deleted, and no snapshot was regenerated. Two assertions in the new test are weaker than they read, which is the first nit inline.
Findings: 0 blocker, 2 should fix, 3 nit. All inline.
Reviewer candidates:
- @karancs06 wrote the
REQUEST_DISCUSSION_HIGHLIGHTSmechanism this PR is fixing (commitf03cee9) and has 8 commits across the three changed files. - @csAyushDubey has 7 commits across the changed files, most of them on variant field handling in
useVariantsPostMessageEvent.ts. - @hitesh-shetty-cstk is the top author of the variant handler overall, with 20 commits across these files, and is worth walking through the
get-variant-idchange before merge. Not requested here because it is the account this review posts from.
Not covered: Nothing was executed. Dependencies are not installed in this checkout, so the unit suite, tsc --noEmit and prettier were not run and the numbers in the description are unverified here. @contentstack/advanced-post-message is not vendored, so the send contract, including the claim that it always returns a promise, was read from its call sites in this repo rather than from the package. That the Visual Builder registers the request-discussion-highlights listener only while the Discussions panel is mounted is taken from the description and not checked against the visual builder repo.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| visualBuilderPostMessage | ||
| ?.send( | ||
| VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS | ||
| ) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
should fix .catch(() => {}) discards every rejection, not only the missing-listener one this PR is about. A send that fails for another reason, such as a request timeout or the Visual Builder handler itself throwing, now leaves discussion highlights unrendered with no signal anywhere. getHighlightVariantFieldsStatus, about 40 lines above in this same file, logs instead of discarding.
Narrowing the discard to the expected code keeps the fix and leaves real failures visible:
| visualBuilderPostMessage | |
| ?.send( | |
| VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS | |
| ) | |
| .catch(() => {}); | |
| visualBuilderPostMessage | |
| ?.send( | |
| VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS | |
| ) | |
| .catch((error) => { | |
| if (error?.code !== "NO_REQUEST_LISTENER_FOUND") { | |
| console.error( | |
| "Failed to request discussion highlights:", | |
| error | |
| ); | |
| } | |
| }); |
NO_REQUEST_LISTENER_FOUND currently appears as a bare string in your new test and nowhere else in src. If you take this, a shared constant is worth more than two copies of the literal.
Separately, on the Copilot comment on these same lines: it is incorrect, and the code as written is safe. An optional chain short-circuits as a whole expression, so when visualBuilderPostMessage is nullish the rest of the chain is not evaluated and the result is undefined. .catch is never accessed, and no TypeError is possible. Adding ?. before catch would be harmless, but it would also hide the one case worth knowing about, which is a send that returns a non-promise.
Generated by Claude Code
| visualBuilderPostMessage | ||
| ?.send(VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
should fix Same blanket discard as the other send site, so the same narrowing applies:
| visualBuilderPostMessage | |
| ?.send(VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS) | |
| .catch(() => {}); | |
| visualBuilderPostMessage | |
| ?.send(VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS) | |
| .catch((error) => { | |
| if (error?.code !== "NO_REQUEST_LISTENER_FOUND") { | |
| console.error( | |
| "Failed to request discussion highlights:", | |
| error | |
| ); | |
| } | |
| }); |
A design question that the description partly answers, and I am not certain of the answer myself. src/visualBuilder/utils/visualBuilderPostMessage.ts:10 carries // suppressErrors: true, commented out, and the live preview channel sets that option for real at src/livePreview/eventManager/livePreviewEventManager.ts:11. The description says this cannot be fixed in the post message layer because only the caller knows the receiver is optional. That reasoning holds for a per-call decision, and I have not read the package, so I cannot say whether suppressErrors would suppress the right things or blanket-suppress errors that matter on this channel. Worth a line in the description either way, because the answer decides whether the next optional-receiver send site copies this .catch or gets fixed once at the channel. These two are currently the only .catch(() => {}) sites in src, and the sends in CommentIcon.tsx, FieldToolbar.tsx and HighlightedCommentIcon.tsx are still fire and forget.
The Copilot comment on these lines is incorrect for the same reason as the one on useVariantsPostMessageEvent.ts: an optional chain short-circuits as a whole expression, so .catch is never reached when visualBuilderPostMessage is nullish.
Generated by Claude Code
| expect(catchSpy).toHaveBeenCalled(); | ||
| await expect(rejection).rejects.toMatchObject({ | ||
| code: "NO_REQUEST_LISTENER_FOUND", | ||
| }); |
There was a problem hiding this comment.
nit Both of these assertions are weaker than they read.
expect(catchSpy).toHaveBeenCalled() asserts the mechanism rather than the outcome. try { await send(...) } catch {} and .then(undefined, noop) both fix the reported bug and both fail this test, so it will report a correct refactor as a regression.
await expect(rejection).rejects.toMatchObject({ code: ... }) asserts on the object the test constructed two statements earlier. It holds whatever the hook does with it.
What you actually care about is that no unhandled rejection escapes. Registering an unhandledrejection listener (or process.on("unhandledRejection")) for the duration of the test and asserting it never fires would survive a refactor. If that turns out to be awkward under vitest with jsdom, keeping the spy is defensible, but then one line saying the assertion is deliberately coupled to the .catch shape would save the next person the guess.
Generated by Claude Code
| 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), |
There was a problem hiding this comment.
nit Setting the implementation inside the vi.mock factory works, but it survives only because nothing in this file calls vi.resetAllMocks() and vitest.config.ts sets no mockReset. That is the same fragility the description describes, one level up: the spec still leans on reset semantics instead of stating the contract per test. If mockReset is ever switched on, every test that reaches the get-variant-id handler starts failing on .catch of undefined.
A beforeEach re-establishing it is not order dependent, and it also removes the need for the finally block in the new test to hand-restore the mock:
beforeEach(() => {
(mockVisualBuilderPostMessage.send as any).mockResolvedValue(undefined);
});src/visualBuilder/utils/__test__/getResolvedVariantPermissions.spec.ts mocks this module with a bare send: vi.fn(). It does not reach either changed hook today, so nothing is broken there, but it is the other copy of the same wrong contract.
Generated by Claude Code
| @@ -14,9 +14,12 @@ const VARIANT_UPDATE_DELAY_MS: Readonly<number> = 8000; | |||
| // Coalesce a burst of data-cslp mutations into a single request to the | |||
| // visual editor. | |||
| const requestDiscussionHighlights = debounce(() => { | |||
There was a problem hiding this comment.
nit The regression test covers the useVariantFieldsPostMessageEvent send only. This debounced site got the same fix and has no test, and there is no spec file for this module at all. The two sites exist because the observer path and the SSR path fire independently, so a test here is the one that would catch a future edit dropping the .catch from the path that runs on most CSR page loads. The description notes that jsdom does not fire attribute mutations reliably, which is fair for the observer itself, but requestDiscussionHighlights can be exercised without the observer by invoking it directly and advancing timers.
Generated by Claude Code
…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 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly handles the expected optional-listener rejection without masking other failures, and adds focused regression/unit coverage to prevent recurrence.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
hitesh-shetty-cstk
left a comment
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Automated review
Re-review after new commits (pass 2 of 4). Only the incremental diff from eb3a86d to 19f80f9 was reviewed, so nothing from pass 1 is repeated here.
Journey at risk: variant switching on the canvas (get-variant-id) and audience mode variant highlighting. Neither can break from this commit. The send, its timing and its arguments are unchanged, and only the rejection handler moved.
What this changes: pass 1 flagged that .catch(() => {}) discarded real send failures along with the expected one. This commit answers that. Both call sites now pass ignoreMissingListener(event), a new module at src/visualBuilder/utils/postMessageErrors.ts that returns silently on NO_REQUEST_LISTENER_FOUND and routes everything else to PublicLogger.warn. A unit test covers both branches.
Business impact: none identified. The only behavioural change outside the discarded-failure fix is console output on the consumer's own site, so no content editor flow moves. The pass 1 concern is addressed in substance: a genuine failure to fetch discussion highlights now leaves a trace instead of vanishing.
Security: nothing beyond what the scanners cover. Snyk is clean on this head. The new code reads one property off a rejection value and logs it. It parses no payload, moves no origin check, and sends nothing new across the iframe boundary.
On the dependency contract: I unpacked @contentstack/advanced-post-message@0.0.5 from npm rather than reading it from call sites as I did in pass 1, and two things came out of it.
The PR is right on the point it turns on. The wire value really is NO_REQUEST_LISTENER_FOUND, the receiver really does ack before it looks for a listener (so the panel-closed case rejects with that code and not a timeout), and ERROR_CODES really is absent from the package entry point, which is only EventManager plus ./types. Matching the literal is the right call, and a deep import would not work anyway since the runtime ships as a single bundle with no module at that path.
The part that does not hold is the claim in the description that NO_ACK_RECEIVED, WINDOW_CLOSED and CODE_RETURNED_ERROR arrive as coded objects. They do not. Two of them reject without a code at all, and the third never reaches the sender. That drives both should-fix comments below.
Flow
flowchart TD
A[send request-discussion-highlights] --> B{rejection shape}
B -->|"{ code: NO_REQUEST_LISTENER_FOUND }<br/>panel closed"| C[return silently]:::changed
B -->|"Error, window closed"| D[PublicLogger.warn]:::changed
B -->|"bare string, no ack in 1s"| D
B -->|anything else| D
C --> E[no console output]
D --> F[warning names the event]
classDef changed fill:#fff3cd,stroke:#d39e00
Test changes: the new spec adds coverage and weakens nothing. No assertion was loosened, nothing was skipped or deleted, no snapshot regenerated, and the existing regression test in useVariantsPostMessageEvent.spec.ts still rejects with the shape the library genuinely sends. The problem is the opposite of a bent test: three of its five cases assert against values the dependency never produces, so they pass without exercising a real failure. Detail inline.
Findings: 0 blocker, 2 should fix, 3 nit. All inline.
Reviewer candidates: no reviewer was requested on this pass, since requests are made once when the pull request opens. @karancs06 is already on it and wrote the discussion-highlights mechanism this change sits on, which is the right pair of eyes for the get-variant-id path.
Not covered: nothing was executed. Dependencies are not installed in this checkout, so the unit suite, tsc --noEmit and prettier were not run, and the counts in the description are unverified here. The claim that PublicLogger.warn is inert without a process global is read off the source and the build config rather than observed in a browser, which is why it is filed as a nit to check. That the visual builder registers the listener only while the Discussions panel is mounted is still taken from the description and not checked against the visual builder repository. Copilot moved to approval on this head with no new comments, and the two findings above are not in its scope.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| */ | ||
| export function ignoreMissingListener(event: string): (error: unknown) => void { | ||
| return (error: unknown) => { | ||
| if ((error as { code?: string })?.code === NO_REQUEST_LISTENER_FOUND) { |
There was a problem hiding this comment.
should fix — this recognises only one of the three ways the library reports that the receiver is not there, so the other two land in the warn branch carrying nothing a reader can act on.
I unpacked @contentstack/advanced-post-message@0.0.5 to check. Only the missing-listener response is sent back as a coded object:
this.postMessage.sendResponse({ type, hash, payload: undefined,
error: { code: ERROR_CODES.receiveEvent.noRequestListenerFound, message: ... } })The two timeout paths in EventManager.send do not carry a code at all:
targetWindow.closed
? r.reject(new Error(getErrorMessage(ERROR_MESSAGES.common.windowClosed))) // an Error
: (!hasReceivedAck && budget <= 0
? r.reject(getErrorMessage(ERROR_MESSAGES.sendEvent.noAckReceived)) // a bare string
: undefined)(error as { code?: string })?.code is undefined for both, so both warn. The realistic trigger is the visual builder parent not acking inside the library's 1s budget, or unloading mid-send, which is the same "receiver is not there" condition this helper exists to absorb. The exposure is a transient console warning rather than anything a content editor sees, so it is not urgent, but the helper's own doc comment claims to cover no-ack and closed-window and it does not.
Either match the no-ack case as well (the message is stable: contentstack-adv-post-message: The ACK was not received), or keep the code check as the only rule and narrow the doc comment to say so.
Generated by Claude Code
| 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" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
should fix — these three cases pass, but none of them is a shape a sender can actually receive, so the test gives confidence about failures it never exercises.
From the 0.0.5 bundle: WINDOW_CLOSED reaches the sender as new Error(...) with no code, NO_ACK_RECEIVED as a bare string with no code, and CODE_RETURNED_ERROR is only logged on the receiving side (.catch(e => logger.error(codeReturnedError, e))) and never sent back as a response error, so the sender's promise does not settle at all in that case. { code: "NO_ACK_RECEIVED" } is a value the library never produces.
The handler still does the right thing for all of them, because an uncoded rejection warns. It is the test, and the matching claim in the PR description, that describe a contract the dependency does not have.
| 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.each([ | |
| ["an Error, as the closed-window path rejects", new Error("closed")], | |
| ["a bare string, as the no-ack timeout rejects", "no ACK received"], | |
| ])("warns on %s so a real failure is still visible", (_shape, error) => { | |
| const warn = vi.spyOn(PublicLogger, "warn").mockImplementation(() => {}); | |
| ignoreMissingListener("request-discussion-highlights")(error); | |
| expect(warn).toHaveBeenCalledOnce(); | |
| expect(warn.mock.calls[0][0]).toContain( | |
| "request-discussion-highlights" | |
| ); | |
| }); |
The last case in the file (new Error("boom")) then overlaps the first row here, so it can go.
Generated by Claude Code
| * 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 { |
There was a problem hiding this comment.
nit — event: string drops typing that both call sites already have, since both pass VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS. The workspace rule is to keep postMessage events on the enum, and a string parameter lets a hand-written name in later.
| export function ignoreMissingListener(event: string): (error: unknown) => void { | |
| export function ignoreMissingListener( | |
| event: VisualBuilderPostMessageEvents | |
| ): (error: unknown) => void { |
That needs import { VisualBuilderPostMessageEvents } from "./types/postMessage.types"; at the top, and the two string literals in postMessageErrors.test.ts become enum members.
Generated by Claude Code
| visualBuilderPostMessage | ||
| ?.send( | ||
| VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS | ||
| ) | ||
| .catch( | ||
| ignoreMissingListener( | ||
| VisualBuilderPostMessageEvents.REQUEST_DISCUSSION_HIGHLIGHTS | ||
| ) | ||
| ); |
There was a problem hiding this comment.
nit — the event name is now written twice per call site, so the two sends carry four references that have to stay in sync, and a mismatch would be silent (the send goes to one event, the warning names another).
A wrapper next to ignoreMissingListener that takes the name once would collapse both sites to a single line:
export function sendOptional(event: VisualBuilderPostMessageEvents): void {
visualBuilderPostMessage?.send(event).catch(ignoreMissingListener(event));
}Worth weighing against the import cycle it would add, since postMessageErrors.ts would then pull in visualBuilderPostMessage, which is the module the variants spec mocks wholesale. The PR description says that separation was deliberate, so leaving it as is with the name repeated is a reasonable call too.
Generated by Claude Code
| if ((error as { code?: string })?.code === NO_REQUEST_LISTENER_FOUND) { | ||
| return; | ||
| } | ||
| PublicLogger.warn( |
There was a problem hiding this comment.
nit, pre-existing rather than introduced here, and worth a check rather than a change in this PR.
PublicLogger.warn only reaches the console when process exists:
if (typeof process !== "undefined" && process?.env?.NODE_ENV !== "test") {tsup.config.js defines process.env.PACKAGE_VERSION and the two purge flags, but nothing that makes bare process resolvable, and the dist is cjs and esm only. A consumer whose bundler replaces process.env.NODE_ENV but leaves typeof process as a runtime check, and anyone using the ESM CDN snippet in the README, would evaluate that guard as false and see nothing.
If that holds, the visibility this commit is buying does not reach those consumers, and the silent branch and the warn branch look the same from outside. I did not run it, so please confirm before treating it as a gap.
Generated by Claude Code
There was a problem hiding this comment.
🟢 Approval recommended
The change safely handles an expected rejection case without masking unexpected failures, and it is covered by targeted unit tests that exercise both the helper and the affected send sites.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
What
request-discussion-highlightswas sent without handling the returned promise. The visual builder registers that listener inside its Discussions panel, which is only mounted while the panel is open. With the panel closed, which is the default, the send rejects withNO_REQUEST_LISTENER_FOUNDand nothing handles it.Next.js registers a global
unhandledrejectionhandler in dev and renders whatever it catches as a runtime error overlay. The rejection value is a plain{ code, message }object rather than anError, so the overlay title reads[object Object].Dev only, and nothing breaks. The overlay ships with
next dev, not withnext buildornext start. It shows for any Next 15 or 16 app whenever the Discussions panel is closed.Why this fix
An absent receiver is the expected state for this event, not a failure, so the rejection is handled at the send sites. Only the caller knows the receiver is optional, so this cannot be fixed in the post message layer.
Swallowing every rejection would have hidden real problems, so the handler discriminates.
NO_REQUEST_LISTENER_FOUNDreturns silently, because that is the normal state while the panel is closed. Everything else (NO_ACK_RECEIVED,WINDOW_CLOSED,CODE_RETURNED_ERROR, or a rejection that carries no code at all) is warned throughPublicLogger, which the SDK already uses elsewhere and which is silent underNODE_ENV=test. Nothing is logged in the common case, and a genuine breakage still surfaces.Reproduced against
@contentstack/live-preview-utils4.5.0 with Next.js 16.1.6 and React 19.2.3.Changes
postMessageErrors.ts: newignoreMissingListener(event)helper. It sits in its own module because the spec mocks thevisualBuilderPostMessagemodule wholesale, so keeping it separate means the tests exercise the real function rather than a mock.useVariantsPostMessageEvent.ts: handle the rejection. Both branches of theisSSRcheck sent the same event, so the send is hoisted out of the branch rather than duplicated.useRecalculateVariantDataCSLPValues.ts: handle the rejection on the debounced send.useVariantsPostMessageEvent.spec.ts: add a regression test, and fix the base mock.postMessageErrors.test.ts: cover both sides of the discrimination.Note on the spec mock
The mock was
send: vi.fn(), which returnsundefinedinstead of a promise. Tests passed only because two later tests callmockResolvedValue, andvi.clearAllMocks()does not reset implementations, so every test after them inherited a promise. That made the spec order dependent. The base mock now returns a promise, matching whatsendactually does.Verification
.catchand the regression test fails withexpected "catch" to be called at least once. Change the helper to silence everything and 4 of its 5 tests fail.tsc --noEmiterrors on this branch are the same 81 present on the base branch, none in the changed files.Not included
The error code is matched as a string literal because
@contentstack/advanced-post-messagedoes not re-exportERROR_CODESfrom its entry point, only./types.Rejecting with an
Errorsubclass carrying a.codewould make the overlay name the event instead of printing[object Object]. That would help every consumer and remove the string literal above, but it belongs in the post message package and is tracked separately.The touched files are not prettier clean, but they were already that way on the base branch. Reformatting them would bury this change in unrelated churn, so it is left for a separate pass.
🤖 Generated with Claude Code