From 9128284aae8918193559e8ce33977ce255daa13a Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Fri, 6 Nov 2026 14:10:48 +0000 Subject: [PATCH 1/4] fix(browser-sdk): notify feedback callback once on score-only close --- .changeset/friendly-feedback-close.md | 5 + packages/browser-sdk/src/client.ts | 3 +- packages/browser-sdk/src/feedback/feedback.ts | 18 +- packages/browser-sdk/src/feedback/ui/index.ts | 7 +- packages/browser-sdk/src/feedback/ui/types.ts | 13 + .../ui/withFeedbackSubmissionCallback.ts | 71 +++++ packages/browser-sdk/src/ui/Dialog.tsx | 4 + .../test/e2e/feedback-widget.browser.spec.ts | 243 +++++++++++++++++- .../test/feedbackSubmissionCallback.test.ts | 203 +++++++++++++++ 9 files changed, 541 insertions(+), 26 deletions(-) create mode 100644 .changeset/friendly-feedback-close.md create mode 100644 packages/browser-sdk/src/feedback/ui/withFeedbackSubmissionCallback.ts create mode 100644 packages/browser-sdk/test/feedbackSubmissionCallback.test.ts diff --git a/.changeset/friendly-feedback-close.md b/.changeset/friendly-feedback-close.md new file mode 100644 index 000000000..ca97c40d7 --- /dev/null +++ b/.changeset/friendly-feedback-close.md @@ -0,0 +1,5 @@ +--- +"@reflag/browser-sdk": patch +--- + +Call `onAfterSubmit` once per feedback form, including when a user saves a score and closes without pressing Send. Closing after Send does not repeat the callback, and closing during a save waits for it to finish. Close-only callbacks include the latest saved score and an empty comment, never an unsubmitted comment draft. Applies to both automated surveys and manually requested feedback. diff --git a/packages/browser-sdk/src/client.ts b/packages/browser-sdk/src/client.ts index 4f63bbbef..2897107b5 100644 --- a/packages/browser-sdk/src/client.ts +++ b/packages/browser-sdk/src/client.ts @@ -1186,6 +1186,7 @@ export class ReflagClient { openWithCommentVisible: options.openWithCommentVisible, onClose: options.onClose, onDismiss: options.onDismiss, + onAfterSubmit: options.onAfterSubmit, onScoreSubmit: async (data) => { const res = await this.feedback({ ...feedbackData, @@ -1204,8 +1205,6 @@ export class ReflagClient { ...feedbackData, ...data, }); - - options.onAfterSubmit?.(data); }, }); }, 1); diff --git a/packages/browser-sdk/src/feedback/feedback.ts b/packages/browser-sdk/src/feedback/feedback.ts index 1a68eeed8..1bc40683a 100644 --- a/packages/browser-sdk/src/feedback/feedback.ts +++ b/packages/browser-sdk/src/feedback/feedback.ts @@ -12,11 +12,7 @@ import { } from "./prompts"; import * as feedbackLib from "./ui"; import { DEFAULT_POSITION } from "./ui"; -import { - FeedbackSubmission, - FeedbackTranslations, - OpenFeedbackFormOptions, -} from "./ui/types"; +import { FeedbackTranslations, OpenFeedbackFormOptions } from "./ui/types"; const INITIAL_FETCH_RETRY_DELAYS_MS = [0, 5000]; @@ -59,17 +55,6 @@ export type RequestFeedbackData = Omit< */ companyId?: string; - /** - * Allows you to handle a copy of the already submitted - * feedback. - * - * This can be used for side effects, such as storing a - * copy of the feedback in your own application or CRM. - * - * @param {Object} data - */ - onAfterSubmit?: (data: FeedbackSubmission) => void; - /** * Flag key. */ @@ -436,7 +421,6 @@ export class AutoFeedback { }, onSubmit: async (data) => { await replyCallback(data); - options.onAfterSubmit?.(data); }, onDismiss: () => replyCallback(null), position: this.position, diff --git a/packages/browser-sdk/src/feedback/ui/index.ts b/packages/browser-sdk/src/feedback/ui/index.ts index cb6307f44..2076f67e6 100644 --- a/packages/browser-sdk/src/feedback/ui/index.ts +++ b/packages/browser-sdk/src/feedback/ui/index.ts @@ -4,6 +4,7 @@ import { feedbackContainerId, propagatedEvents } from "../../ui/constants"; import { Position } from "../../ui/types"; import { FeedbackDialog } from "./FeedbackDialog"; import { OpenFeedbackFormOptions } from "./types"; +import { withFeedbackSubmissionCallback } from "./withFeedbackSubmissionCallback"; export const DEFAULT_POSITION: Position = { type: "DIALOG", @@ -79,7 +80,11 @@ export function openFeedbackForm(options: OpenFeedbackFormOptions): void { openInstances++; render( - h(FeedbackDialog, { ...options, position, key: openInstances.toString() }), + h(FeedbackDialog, { + ...withFeedbackSubmissionCallback(options), + position, + key: openInstances.toString(), + }), shadowRoot, ); } diff --git a/packages/browser-sdk/src/feedback/ui/types.ts b/packages/browser-sdk/src/feedback/ui/types.ts index 79d7d5912..5d381800d 100644 --- a/packages/browser-sdk/src/feedback/ui/types.ts +++ b/packages/browser-sdk/src/feedback/ui/types.ts @@ -41,6 +41,19 @@ export interface OpenFeedbackFormOptions { openWithCommentVisible?: boolean; onSubmit: (data: FeedbackSubmission) => Promise | void; + + /** + * Handle a copy of submitted feedback, for example to send it to analytics + * or your own CRM. Called once per form: after submitting the form, or when + * it closes with a saved score without a form submission. + * + * On close, includes the latest saved score and an empty comment. Comment + * text that was not submitted is never included. If a save is in flight, + * the callback waits for it to finish. Closing without feedback does not + * call the callback. + */ + onAfterSubmit?: (data: FeedbackSubmission) => void; + onScoreSubmit?: ( data: FeedbackScoreSubmission, ) => Promise; diff --git a/packages/browser-sdk/src/feedback/ui/withFeedbackSubmissionCallback.ts b/packages/browser-sdk/src/feedback/ui/withFeedbackSubmissionCallback.ts new file mode 100644 index 000000000..9b120532b --- /dev/null +++ b/packages/browser-sdk/src/feedback/ui/withFeedbackSubmissionCallback.ts @@ -0,0 +1,71 @@ +import { FeedbackSubmission, OpenFeedbackFormOptions } from "./types"; + +/** Share the once-per-form callback lifecycle between manual and automated feedback. */ +export function withFeedbackSubmissionCallback( + options: OpenFeedbackFormOptions, +): OpenFeedbackFormOptions { + const { onAfterSubmit, onClose, onScoreSubmit, onSubmit } = options; + if (!onAfterSubmit) return options; + + let closed = false; + let notified = false; + let pendingSubmissions = 0; + let scoreSubmission = 0; + let savedScoreSubmission = 0; + let feedback: FeedbackSubmission | undefined; + + const notify = () => { + if (notified || !feedback) return; + // Set this before calling user code, including callbacks that throw or close the form. + notified = true; + onAfterSubmit(feedback); + }; + + const notifyOnClose = () => { + if (closed && pendingSubmissions === 0) notify(); + }; + + return { + ...options, + onScoreSubmit: onScoreSubmit + ? async (data) => { + const submission = ++scoreSubmission; + pendingSubmissions++; + try { + const result = await onScoreSubmit(data); + if (result.feedbackId && submission > savedScoreSubmission) { + savedScoreSubmission = submission; + feedback = { + ...data, + feedbackId: result.feedbackId, + comment: "", + }; + } + return result; + } finally { + pendingSubmissions--; + notifyOnClose(); + } + } + : undefined, + onSubmit: async (data) => { + pendingSubmissions++; + try { + await onSubmit(data); + feedback = data; + notify(); + } finally { + pendingSubmissions--; + notifyOnClose(); + } + }, + onClose: () => { + closed = true; + try { + notifyOnClose(); + } finally { + onClose?.(); + } + }, + }; +} diff --git a/packages/browser-sdk/src/ui/Dialog.tsx b/packages/browser-sdk/src/ui/Dialog.tsx index 784c55c4f..28da75904 100644 --- a/packages/browser-sdk/src/ui/Dialog.tsx +++ b/packages/browser-sdk/src/ui/Dialog.tsx @@ -239,6 +239,10 @@ export const Dialog: FunctionComponent = ({ ref={setDiagRef} class={classes} style={anchor ? floatingStyles : unanchoredPosition} + onCancel={(event) => { + event.preventDefault(); + dismiss(); + }} > {children && {children}} diff --git a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts index 6183b7037..6d5e62178 100644 --- a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts +++ b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts @@ -4,7 +4,10 @@ import { expect, Locator, Page, test } from "@playwright/test"; import { InitOptions } from "../../src/client"; import { DEFAULT_TRANSLATIONS } from "../../src/feedback/ui/config/defaultTranslations"; -import { FeedbackTranslations } from "../../src/feedback/ui/types"; +import { + FeedbackSubmission, + FeedbackTranslations, +} from "../../src/feedback/ui/types"; import { feedbackContainerId, propagatedEvents } from "../../src/ui/constants"; const KEY = randomUUID(); @@ -16,6 +19,7 @@ const WINDOW_HEIGHT = 720; declare global { interface Window { eventsFired: Record; + feedbackSubmissions: FeedbackSubmission[]; } } @@ -26,6 +30,7 @@ function pick(options: T[]): T { async function getOpenedWidgetContainer( page: Page, initOptions: Omit = {}, + source: "widget" | "prompt" = "widget", ) { await page.goto("http://localhost:8001/test/e2e/empty.html"); @@ -48,12 +53,53 @@ async function getOpenedWidgetContainer( await page.evaluate(` ;(async () => { const { ReflagClient } = await import("/dist/reflag-browser-sdk.mjs"); - const reflag = new ReflagClient({publishableKey: "${KEY}", user: {id: "foo"}, company: {id: "bar"}, ...${JSON.stringify(initOptions ?? {})}}); - await reflag.initialize(); - await reflag.requestFeedback({ - flagKey: "flag1", - title: "baz", + const initOptions = ${JSON.stringify(initOptions)}; + if (initOptions.feedback?.ui?.position?.type === "POPOVER") { + const anchor = document.createElement("button"); + anchor.id = "feedback-anchor"; + anchor.textContent = "Give feedback"; + document.body.appendChild(anchor); + initOptions.feedback.ui.position.anchor = anchor; + } + window.feedbackSubmissions = []; + const onAfterSubmit = (data) => window.feedbackSubmissions.push(data); + let onMessage; + const reflag = new ReflagClient({ + publishableKey: "${KEY}", + user: {id: "foo"}, + company: {id: "bar"}, + ...initOptions, + feedback: { + ...initOptions.feedback, + autoFeedbackHandler: (_prompt, handlers) => { + handlers.openFeedbackForm({ onAfterSubmit }); + }, + }, + eventSourceFactory: () => ({ + addEventListener: (type, handler) => { + if (type === "message") onMessage = handler; + }, + close: () => {}, + }), }); + await reflag.initialize(); + if ("${source}" === "prompt") { + onMessage(new MessageEvent("message", { + data: JSON.stringify({ + question: "baz", + showAfter: Date.now() - 1000, + showBefore: Date.now() + 60000, + promptId: "prompt1", + featureId: "feature1", + }), + })); + } else { + reflag.requestFeedback({ + flagKey: "flag1", + title: "baz", + onAfterSubmit, + }); + } })() `); @@ -496,6 +542,191 @@ test("Closes the dialog shortly after submitting", async ({ page }) => { await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); }); +for (const source of ["widget", "prompt"] as const) { + test.describe(`${source} feedback submission callback`, () => { + test.beforeEach(async ({ page }) => { + await page.route(`${API_HOST}/feedback`, async (route) => { + await route.fulfill({ + status: 200, + body: JSON.stringify({ feedbackId: "123" }), + contentType: "application/json", + }); + }); + }); + + test("notifies once on close with the latest saved score, excluding draft comments", async ({ + page, + }) => { + const requests: object[] = []; + page.on("request", (request) => { + if (request.url() === `${API_HOST}/feedback`) { + requests.push(request.postDataJSON()); + } + }); + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 2); + await setScore(container, 5); + await setComment(container, "This comment was not submitted"); + await expect( + container.locator(".form-expanded-content button"), + ).toBeEnabled(); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); + + await container.locator(".dialog .close").click(); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([ + { question: "baz", score: 5, feedbackId: "123", comment: "" }, + ]); + expect(requests).toHaveLength(2); // Closing must not submit again. + }); + + test("does not notify when closed without feedback", async ({ page }) => { + const container = await getOpenedWidgetContainer(page, {}, source); + await container.locator(".dialog .close").click(); + await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); + }); + + test("notifies on modal Escape with a saved score", async ({ page }) => { + const container = await getOpenedWidgetContainer( + page, + { feedback: { ui: { position: { type: "MODAL" } } } }, + source, + ); + await setScore(container, 4); + await expect( + container.locator(".form-expanded-content button"), + ).toBeEnabled(); + await page.keyboard.press("Escape"); + await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([ + { question: "baz", score: 4, feedbackId: "123", comment: "" }, + ]); + }); + + for (const dismiss of ["Escape", "outside click", "anchor removal"]) { + test(`notifies once when a popover closes via ${dismiss}`, async ({ + page, + }) => { + const container = await getOpenedWidgetContainer( + page, + { feedback: { ui: { position: { type: "POPOVER", anchor: null } } } }, + source, + ); + await setScore(container, 4); + await expect( + container.locator(".form-expanded-content button"), + ).toBeEnabled(); + if (dismiss === "Escape") { + await page.keyboard.press("Escape"); + } else if (dismiss === "outside click") { + await page.mouse.click(1000, 500); + } else { + await page + .locator("#feedback-anchor") + .evaluate((anchor) => anchor.remove()); + } + await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([ + { question: "baz", score: 4, feedbackId: "123", comment: "" }, + ]); + }); + } + + for (const comment of ["", "Useful feature!"]) { + test(`notifies once on Send with ${comment ? "a comment" : "no comment"}, not again on auto-close`, async ({ + page, + }) => { + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await setComment(container, comment); + await submitForm(container); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([{ question: "baz", score: 4, feedbackId: "123", comment }]); + await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); + expect( + await page.evaluate(() => window.feedbackSubmissions), + ).toHaveLength(1); + }); + } + + test("waits for an in-flight score save after closing", async ({ + page, + }) => { + let finishSave!: () => void; + const pendingSave = new Promise((resolve) => { + finishSave = resolve; + }); + let saving = false; + await page.route(`${API_HOST}/feedback`, async (route) => { + saving = true; + await pendingSave; + await route.fulfill({ + status: 200, + body: JSON.stringify({ feedbackId: "123" }), + contentType: "application/json", + }); + }); + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await expect.poll(() => saving).toBe(true); + await container.locator(".dialog .close").click(); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); + + finishSave(); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([ + { question: "baz", score: 4, feedbackId: "123", comment: "" }, + ]); + }); + + test("closing during Send waits and notifies once with the submitted comment", async ({ + page, + }) => { + let finishSave!: () => void; + const pendingSave = new Promise((resolve) => { + finishSave = resolve; + }); + let savingComment = false; + await page.route(`${API_HOST}/feedback`, async (route) => { + if (route.request().postDataJSON().comment) { + savingComment = true; + await pendingSave; + } + await route.fulfill({ + status: 200, + body: JSON.stringify({ feedbackId: "123" }), + contentType: "application/json", + }); + }); + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await setComment(container, "Useful feature!"); + await submitForm(container); + await expect.poll(() => savingComment).toBe(true); + await container.locator(".dialog .close").click(); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); + + finishSave(); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([ + { + question: "baz", + score: 4, + feedbackId: "123", + comment: "Useful feature!", + }, + ]); + }); + }); +} + test("Blocks event propagation to the containing document", async ({ page, }) => { diff --git a/packages/browser-sdk/test/feedbackSubmissionCallback.test.ts b/packages/browser-sdk/test/feedbackSubmissionCallback.test.ts new file mode 100644 index 000000000..5d764a3ce --- /dev/null +++ b/packages/browser-sdk/test/feedbackSubmissionCallback.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, test, vi } from "vitest"; + +import { + FeedbackSubmission, + OnScoreSubmitResult, + OpenFeedbackFormOptions, +} from "../src/feedback/ui/types"; +import { withFeedbackSubmissionCallback } from "../src/feedback/ui/withFeedbackSubmissionCallback"; +import { deferred } from "./deferred"; + +const score = { question: "How was it?", score: 4 }; +const feedbackId = "feedback-123"; +const savedScore = { ...score, feedbackId, comment: "" }; +const submittedFeedback: FeedbackSubmission = { + ...savedScore, + comment: "Very useful!", +}; + +function setup(overrides: Partial = {}) { + const onAfterSubmit = vi.fn(); + const onClose = vi.fn(); + const onSubmit = vi.fn().mockResolvedValue(undefined); + const onScoreSubmit = vi.fn().mockResolvedValue({ feedbackId }); + const handlers = withFeedbackSubmissionCallback({ + key: "test", + onAfterSubmit, + onClose, + onSubmit, + onScoreSubmit, + ...overrides, + }); + return { handlers, onAfterSubmit, onClose, onSubmit, onScoreSubmit }; +} + +describe("feedback submission callback", () => { + test("does not notify when closing without feedback", () => { + const { handlers, onAfterSubmit, onClose } = setup(); + handlers.onClose!(); + expect(onAfterSubmit).not.toHaveBeenCalled(); + expect(onClose).toHaveBeenCalledOnce(); + }); + + test("notifies once on close with the latest saved score and an empty comment", async () => { + const { handlers, onAfterSubmit, onScoreSubmit } = setup(); + await handlers.onScoreSubmit!(score); + await handlers.onScoreSubmit!({ ...score, feedbackId, score: 5 }); + expect(onAfterSubmit).not.toHaveBeenCalled(); + + handlers.onClose!(); + handlers.onClose!(); + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith({ + ...savedScore, + score: 5, + }); + expect(onScoreSubmit).toHaveBeenCalledTimes(2); + }); + + test("notifies after Send, not again when the form closes", async () => { + const { handlers, onAfterSubmit, onSubmit } = setup(); + await handlers.onScoreSubmit!(score); + await handlers.onSubmit(submittedFeedback); + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(submittedFeedback); + + handlers.onClose!(); + handlers.onClose!(); + expect(onAfterSubmit).toHaveBeenCalledOnce(); + expect(onSubmit).toHaveBeenCalledExactlyOnceWith(submittedFeedback); + }); + + test("notifies after Send with an empty comment too", async () => { + const { handlers, onAfterSubmit } = setup(); + await handlers.onSubmit(savedScore); + handlers.onClose!(); + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); + }); + + test("waits for an in-flight score save when closing", async () => { + const pending = deferred(); + const { handlers, onAfterSubmit } = setup({ + onScoreSubmit: () => pending.promise, + }); + const saving = handlers.onScoreSubmit!(score); + handlers.onClose!(); + expect(onAfterSubmit).not.toHaveBeenCalled(); + + pending.resolve({ feedbackId }); + await saving; + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); + }); + + test("waits for all score saves and keeps the latest selection even with out-of-order responses", async () => { + const first = deferred(); + const second = deferred(); + const onScoreSubmit = vi + .fn() + .mockReturnValueOnce(first.promise) + .mockReturnValueOnce(second.promise); + const { handlers, onAfterSubmit } = setup({ onScoreSubmit }); + const savingFirst = handlers.onScoreSubmit!(score); + const savingSecond = handlers.onScoreSubmit!({ ...score, score: 5 }); + handlers.onClose!(); + + second.resolve({ feedbackId }); + await savingSecond; + expect(onAfterSubmit).not.toHaveBeenCalled(); + first.resolve({ feedbackId }); + await savingFirst; + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith({ + ...savedScore, + score: 5, + }); + }); + + test("waits for an in-flight Send rather than notifying with just the score", async () => { + const pending = deferred(); + const { handlers, onAfterSubmit } = setup({ + onSubmit: () => pending.promise, + }); + await handlers.onScoreSubmit!(score); + const saving = handlers.onSubmit(submittedFeedback); + handlers.onClose!(); + expect(onAfterSubmit).not.toHaveBeenCalled(); + + pending.resolve(); + await saving; + handlers.onClose!(); + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(submittedFeedback); + }); + + test("does not notify for a failed score save", async () => { + const { handlers, onAfterSubmit } = setup({ + onScoreSubmit: vi.fn().mockRejectedValue(new Error("Save failed")), + }); + const saving = handlers.onScoreSubmit!(score); + handlers.onClose!(); + await expect(saving).rejects.toThrow("Save failed"); + expect(onAfterSubmit).not.toHaveBeenCalled(); + }); + + test("does not notify if the score handler did not save feedback", async () => { + const { handlers, onAfterSubmit } = setup({ + // The manual feedback handler returns an undefined ID on an unsuccessful response. + onScoreSubmit: vi.fn().mockResolvedValue({ feedbackId: undefined }), + }); + await handlers.onScoreSubmit!(score); + handlers.onClose!(); + expect(onAfterSubmit).not.toHaveBeenCalled(); + }); + + test("falls back to the saved score if Send fails after closing", async () => { + const pending = deferred(); + const { handlers, onAfterSubmit } = setup({ + onSubmit: async () => { + await pending.promise; + throw new Error("Send failed"); + }, + }); + await handlers.onScoreSubmit!(score); + const saving = handlers.onSubmit(submittedFeedback); + handlers.onClose!(); + pending.resolve(); + + await expect(saving).rejects.toThrow("Send failed"); + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); + }); + + test("retains a saved score if a subsequent score save fails", async () => { + const onScoreSubmit = vi + .fn() + .mockResolvedValueOnce({ feedbackId }) + .mockRejectedValueOnce(new Error("Save failed")); + const { handlers, onAfterSubmit } = setup({ onScoreSubmit }); + await handlers.onScoreSubmit!(score); + await expect( + handlers.onScoreSubmit!({ ...score, score: 5 }), + ).rejects.toThrow("Save failed"); + handlers.onClose!(); + expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); + }); + + test("does not call a throwing callback again or skip the user's close handler", async () => { + const onAfterSubmit = vi.fn(() => { + throw new Error("Analytics failed"); + }); + const { handlers, onClose } = setup({ onAfterSubmit }); + await handlers.onScoreSubmit!(score); + expect(() => handlers.onClose!()).toThrow("Analytics failed"); + expect(onClose).toHaveBeenCalledOnce(); + handlers.onClose!(); + expect(onAfterSubmit).toHaveBeenCalledOnce(); + }); + + test("each new form has an independent callback lifecycle", async () => { + const first = setup(); + const second = setup(); + await first.handlers.onScoreSubmit!(score); + await second.handlers.onScoreSubmit!(score); + first.handlers.onClose!(); + second.handlers.onClose!(); + expect(first.onAfterSubmit).toHaveBeenCalledOnce(); + expect(second.onAfterSubmit).toHaveBeenCalledOnce(); + }); +}); From ea8b17f28729f55537b1014bd7ac73a31f72de6c Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Fri, 6 Nov 2026 14:27:08 +0000 Subject: [PATCH 2/4] fix(browser-sdk): preserve feedback callbacks on replacement and failed Send --- .changeset/friendly-feedback-close.md | 2 +- packages/browser-sdk/src/client.ts | 6 +- packages/browser-sdk/src/feedback/feedback.ts | 5 +- .../src/feedback/ui/FeedbackDialog.tsx | 16 +- packages/browser-sdk/src/feedback/ui/types.ts | 3 +- .../test/e2e/feedback-widget.browser.spec.ts | 188 ++++++++++++++---- 6 files changed, 172 insertions(+), 48 deletions(-) diff --git a/.changeset/friendly-feedback-close.md b/.changeset/friendly-feedback-close.md index ca97c40d7..b017c3ca2 100644 --- a/.changeset/friendly-feedback-close.md +++ b/.changeset/friendly-feedback-close.md @@ -2,4 +2,4 @@ "@reflag/browser-sdk": patch --- -Call `onAfterSubmit` once per feedback form, including when a user saves a score and closes without pressing Send. Closing after Send does not repeat the callback, and closing during a save waits for it to finish. Close-only callbacks include the latest saved score and an empty comment, never an unsubmitted comment draft. Applies to both automated surveys and manually requested feedback. +Call `onAfterSubmit` once per feedback form, including when a user saves a score and closes without pressing Send. Closing after Send does not repeat the callback, and closing during a save waits for it to finish. Close-only callbacks include the latest saved score and an empty comment, never an unsubmitted comment draft. Replacing a form also closes the previous response, and a failed Send preserves the callback for retry or score-only close. Applies to both automated surveys and manually requested feedback. diff --git a/packages/browser-sdk/src/client.ts b/packages/browser-sdk/src/client.ts index 2897107b5..b7ec57c5a 100644 --- a/packages/browser-sdk/src/client.ts +++ b/packages/browser-sdk/src/client.ts @@ -1200,11 +1200,13 @@ export class ReflagClient { return { feedbackId: undefined }; }, onSubmit: async (data) => { - // Default onSubmit handler - await this.feedback({ + const res = await this.feedback({ ...feedbackData, ...data, }); + if (!res?.ok) { + throw new Error("Couldn't submit feedback. Please try again."); + } }, }); }, 1); diff --git a/packages/browser-sdk/src/feedback/feedback.ts b/packages/browser-sdk/src/feedback/feedback.ts index 1bc40683a..7f501ee0a 100644 --- a/packages/browser-sdk/src/feedback/feedback.ts +++ b/packages/browser-sdk/src/feedback/feedback.ts @@ -420,7 +420,10 @@ export class AutoFeedback { return { feedbackId: res.feedbackId }; }, onSubmit: async (data) => { - await replyCallback(data); + const res = await replyCallback(data); + if (!res?.feedbackId) { + throw new Error("Couldn't submit feedback. Please try again."); + } }, onDismiss: () => replyCallback(null), position: this.position, diff --git a/packages/browser-sdk/src/feedback/ui/FeedbackDialog.tsx b/packages/browser-sdk/src/feedback/ui/FeedbackDialog.tsx index d5a834064..9f02fedfb 100644 --- a/packages/browser-sdk/src/feedback/ui/FeedbackDialog.tsx +++ b/packages/browser-sdk/src/feedback/ui/FeedbackDialog.tsx @@ -1,5 +1,5 @@ import { Fragment, FunctionComponent, h } from "preact"; -import { useCallback, useState } from "preact/hooks"; +import { useCallback, useLayoutEffect, useRef, useState } from "preact/hooks"; import { feedbackContainerId } from "../../ui/constants"; import { Dialog, useDialog } from "../../ui/Dialog"; @@ -40,7 +40,19 @@ export const FeedbackDialog: FunctionComponent = ({ "idle" | "submitting" | "submitted" >("idle"); - const { isOpen, close } = useDialog({ onClose, initialValue: true }); + const closed = useRef(false); + const notifyClose = useCallback(() => { + if (closed.current) return; + closed.current = true; + onClose?.(); + }, [onClose]); + + // Replacing a form unmounts it. Notify on that path too, but not again after an explicit close. + useLayoutEffect(() => notifyClose, [notifyClose]); + const { isOpen, close } = useDialog({ + onClose: notifyClose, + initialValue: true, + }); const autoClose = useTimer({ enabled: position.type === "DIALOG", diff --git a/packages/browser-sdk/src/feedback/ui/types.ts b/packages/browser-sdk/src/feedback/ui/types.ts index 5d381800d..64589f641 100644 --- a/packages/browser-sdk/src/feedback/ui/types.ts +++ b/packages/browser-sdk/src/feedback/ui/types.ts @@ -45,7 +45,8 @@ export interface OpenFeedbackFormOptions { /** * Handle a copy of submitted feedback, for example to send it to analytics * or your own CRM. Called once per form: after submitting the form, or when - * it closes with a saved score without a form submission. + * it closes with a saved score without a form submission. Replacing the + * form with another feedback form also counts as closing it. * * On close, includes the latest saved score and an empty comment. Comment * text that was not submitted is never included. If a save is in flight, diff --git a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts index 6d5e62178..9ea81b39c 100644 --- a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts +++ b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts @@ -9,6 +9,7 @@ import { FeedbackTranslations, } from "../../src/feedback/ui/types"; import { feedbackContainerId, propagatedEvents } from "../../src/ui/constants"; +import { deferred } from "../deferred"; const KEY = randomUUID(); const API_HOST = `https://front.reflag.com`; @@ -20,6 +21,8 @@ declare global { interface Window { eventsFired: Record; feedbackSubmissions: FeedbackSubmission[]; + feedbackCloses: number; + requestTestFeedback: () => void; } } @@ -49,28 +52,36 @@ async function getOpenedWidgetContainer( }); }); - // Golden path requests - await page.evaluate(` - ;(async () => { - const { ReflagClient } = await import("/dist/reflag-browser-sdk.mjs"); - const initOptions = ${JSON.stringify(initOptions)}; - if (initOptions.feedback?.ui?.position?.type === "POPOVER") { + await page.evaluate( + async ({ key, optionsJson, feedbackSource }) => { + const modulePath = "/dist/reflag-browser-sdk.mjs"; + const { ReflagClient } = (await import( + modulePath + )) as typeof import("../../src/client"); + const options = JSON.parse(optionsJson) as Omit< + InitOptions, + "publishableKey" + >; + const position = options.feedback?.ui?.position; + if (position?.type === "POPOVER") { const anchor = document.createElement("button"); anchor.id = "feedback-anchor"; anchor.textContent = "Give feedback"; document.body.appendChild(anchor); - initOptions.feedback.ui.position.anchor = anchor; + position.anchor = anchor; } window.feedbackSubmissions = []; - const onAfterSubmit = (data) => window.feedbackSubmissions.push(data); - let onMessage; + window.feedbackCloses = 0; + const onAfterSubmit = (data: FeedbackSubmission) => + window.feedbackSubmissions.push(data); + let onMessage: ((event: MessageEvent) => void) | undefined; const reflag = new ReflagClient({ - publishableKey: "${KEY}", - user: {id: "foo"}, - company: {id: "bar"}, - ...initOptions, + publishableKey: key, + user: { id: "foo" }, + company: { id: "bar" }, + ...options, feedback: { - ...initOptions.feedback, + ...options.feedback, autoFeedbackHandler: (_prompt, handlers) => { handlers.openFeedbackForm({ onAfterSubmit }); }, @@ -79,29 +90,45 @@ async function getOpenedWidgetContainer( addEventListener: (type, handler) => { if (type === "message") onMessage = handler; }, - close: () => {}, + close: () => { + onMessage = undefined; + }, }), }); - await reflag.initialize(); - if ("${source}" === "prompt") { - onMessage(new MessageEvent("message", { - data: JSON.stringify({ - question: "baz", - showAfter: Date.now() - 1000, - showBefore: Date.now() + 60000, - promptId: "prompt1", - featureId: "feature1", - }), - })); - } else { + window.requestTestFeedback = () => reflag.requestFeedback({ flagKey: "flag1", title: "baz", onAfterSubmit, + onClose: () => { + window.feedbackCloses++; + }, }); + await reflag.initialize(); + if (feedbackSource === "prompt") { + if (!onMessage) + throw new Error("Feedback SSE listener was not registered"); + onMessage( + new MessageEvent("message", { + data: JSON.stringify({ + question: "baz", + showAfter: Date.now() - 1000, + showBefore: Date.now() + 60000, + promptId: "prompt1", + featureId: "feature1", + }), + }), + ); + } else { + window.requestTestFeedback(); } - })() - `); + }, + { + key: KEY, + optionsJson: JSON.stringify(initOptions), + feedbackSource: source, + }, + ); return page.locator(`#${feedbackContainerId}`); } @@ -657,14 +684,11 @@ for (const source of ["widget", "prompt"] as const) { test("waits for an in-flight score save after closing", async ({ page, }) => { - let finishSave!: () => void; - const pendingSave = new Promise((resolve) => { - finishSave = resolve; - }); + const pendingSave = deferred(); let saving = false; await page.route(`${API_HOST}/feedback`, async (route) => { saving = true; - await pendingSave; + await pendingSave.promise; await route.fulfill({ status: 200, body: JSON.stringify({ feedbackId: "123" }), @@ -677,7 +701,7 @@ for (const source of ["widget", "prompt"] as const) { await container.locator(".dialog .close").click(); expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); - finishSave(); + pendingSave.resolve(); await expect .poll(() => page.evaluate(() => window.feedbackSubmissions)) .toEqual([ @@ -685,18 +709,100 @@ for (const source of ["widget", "prompt"] as const) { ]); }); - test("closing during Send waits and notifies once with the submitted comment", async ({ + test("notifies when a saved response is replaced by a new form", async ({ page, }) => { - let finishSave!: () => void; - const pendingSave = new Promise((resolve) => { - finishSave = resolve; + let feedbackId = 0; + await page.route(`${API_HOST}/feedback`, async (route) => { + await route.fulfill({ json: { feedbackId: String(++feedbackId) } }); + }); + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await expect( + container.locator(".form-expanded-content button"), + ).toBeEnabled(); + + await page.evaluate(() => window.requestTestFeedback()); + await expect( + container.locator(".form-expanded-content button"), + ).toBeDisabled(); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([{ question: "baz", score: 4, feedbackId: "1", comment: "" }]); + + await setScore(container, 5); + await expect( + container.locator(".form-expanded-content button"), + ).toBeEnabled(); + await container.locator(".dialog .close").click(); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([ + { question: "baz", score: 4, feedbackId: "1", comment: "" }, + { question: "baz", score: 5, feedbackId: "2", comment: "" }, + ]); + + await page.evaluate(() => window.requestTestFeedback()); + await expect( + container.locator(".form-expanded-content button"), + ).toBeDisabled(); + expect(await page.evaluate(() => window.feedbackCloses)).toBe( + source === "widget" ? 2 : 1, + ); + expect( + await page.evaluate(() => window.feedbackSubmissions), + ).toHaveLength(2); + }); + + for (const action of ["close", "retry"] as const) { + test(`failed Send does not consume the callback before ${action}`, async ({ + page, + }) => { + let failCommentSave = true; + await page.route(`${API_HOST}/feedback`, async (route) => { + if (route.request().postDataJSON().comment && failCommentSave) { + await route.fulfill({ + status: 500, + json: { message: "Save failed" }, + }); + } else { + await route.fulfill({ json: { feedbackId: "123" } }); + } + }); + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await setComment(container, "Useful feature!"); + await submitForm(container); + await expect(container.locator(".error")).toBeVisible(); + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual( + [], + ); + + if (action === "retry") { + failCommentSave = false; + await submitForm(container); + await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); + } else { + await container.locator(".dialog .close").click(); + } + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([ + { + question: "baz", + score: 4, + feedbackId: "123", + comment: action === "retry" ? "Useful feature!" : "", + }, + ]); }); + } + + test("closing during Send waits and notifies once with the submitted comment", async ({ + page, + }) => { + const pendingSave = deferred(); let savingComment = false; await page.route(`${API_HOST}/feedback`, async (route) => { if (route.request().postDataJSON().comment) { savingComment = true; - await pendingSave; + await pendingSave.promise; } await route.fulfill({ status: 200, @@ -712,7 +818,7 @@ for (const source of ["widget", "prompt"] as const) { await container.locator(".dialog .close").click(); expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); - finishSave(); + pendingSave.resolve(); await expect .poll(() => page.evaluate(() => window.feedbackSubmissions)) .toEqual([ From f4e2a6e0564863197a76ba8bad19cf68d5385902 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Fri, 6 Nov 2026 16:20:18 +0000 Subject: [PATCH 3/4] test(browser-sdk): cover pending score saves on form replacement --- .../test/e2e/feedback-widget.browser.spec.ts | 67 ++++++++++++------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts index 9ea81b39c..bee5c791c 100644 --- a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts +++ b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts @@ -681,33 +681,50 @@ for (const source of ["widget", "prompt"] as const) { }); } - test("waits for an in-flight score save after closing", async ({ - page, - }) => { - const pendingSave = deferred(); - let saving = false; - await page.route(`${API_HOST}/feedback`, async (route) => { - saving = true; - await pendingSave.promise; - await route.fulfill({ - status: 200, - body: JSON.stringify({ feedbackId: "123" }), - contentType: "application/json", + for (const action of ["closing", "replacement"] as const) { + test(`waits for an in-flight score save after ${action}`, async ({ + page, + }) => { + const pendingSave = deferred(); + let saving = false; + await page.route(`${API_HOST}/feedback`, async (route) => { + saving = true; + await pendingSave.promise; + await route.fulfill({ json: { feedbackId: "123" } }); }); - }); - const container = await getOpenedWidgetContainer(page, {}, source); - await setScore(container, 4); - await expect.poll(() => saving).toBe(true); - await container.locator(".dialog .close").click(); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await expect.poll(() => saving).toBe(true); + if (action === "replacement") { + await page.evaluate(() => window.requestTestFeedback()); + await expect( + container.locator("#reflag-feedback-score-4"), + ).not.toBeChecked(); + } else { + await container.locator(".dialog .close").click(); + } + expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual( + [], + ); - pendingSave.resolve(); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([ - { question: "baz", score: 4, feedbackId: "123", comment: "" }, - ]); - }); + pendingSave.resolve(); + await expect + .poll(() => page.evaluate(() => window.feedbackSubmissions)) + .toEqual([ + { question: "baz", score: 4, feedbackId: "123", comment: "" }, + ]); + + if (action === "replacement") { + await expect( + container.locator("#reflag-feedback-score-4"), + ).not.toBeChecked(); + await container.locator(".dialog .close").click(); + expect( + await page.evaluate(() => window.feedbackSubmissions), + ).toHaveLength(1); + } + }); + } test("notifies when a saved response is replaced by a new form", async ({ page, From 13c58e2626a5d178d86afd38a6174ec3144c7e64 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Sat, 7 Nov 2026 08:50:52 +0000 Subject: [PATCH 4/4] refactor(browser-sdk): narrow feedback callback change and regressions --- .changeset/friendly-feedback-close.md | 2 +- packages/browser-sdk/src/client.ts | 5 +- packages/browser-sdk/src/feedback/feedback.ts | 5 +- .../test/e2e/feedback-widget.browser.spec.ts | 379 ++++-------------- .../test/feedbackSubmissionCallback.test.ts | 203 ---------- 5 files changed, 75 insertions(+), 519 deletions(-) delete mode 100644 packages/browser-sdk/test/feedbackSubmissionCallback.test.ts diff --git a/.changeset/friendly-feedback-close.md b/.changeset/friendly-feedback-close.md index b017c3ca2..b6af99c7d 100644 --- a/.changeset/friendly-feedback-close.md +++ b/.changeset/friendly-feedback-close.md @@ -2,4 +2,4 @@ "@reflag/browser-sdk": patch --- -Call `onAfterSubmit` once per feedback form, including when a user saves a score and closes without pressing Send. Closing after Send does not repeat the callback, and closing during a save waits for it to finish. Close-only callbacks include the latest saved score and an empty comment, never an unsubmitted comment draft. Replacing a form also closes the previous response, and a failed Send preserves the callback for retry or score-only close. Applies to both automated surveys and manually requested feedback. +Call `onAfterSubmit` once per feedback form, including when a user saves a score and closes without pressing Send. Closing after Send does not repeat the callback, and closing during a save waits for it to finish. Close-only callbacks include the latest saved score and an empty comment, never an unsubmitted comment draft. Replacing a form also closes the previous response. Applies to both automated surveys and manually requested feedback. diff --git a/packages/browser-sdk/src/client.ts b/packages/browser-sdk/src/client.ts index b7ec57c5a..a24200a0b 100644 --- a/packages/browser-sdk/src/client.ts +++ b/packages/browser-sdk/src/client.ts @@ -1200,13 +1200,10 @@ export class ReflagClient { return { feedbackId: undefined }; }, onSubmit: async (data) => { - const res = await this.feedback({ + await this.feedback({ ...feedbackData, ...data, }); - if (!res?.ok) { - throw new Error("Couldn't submit feedback. Please try again."); - } }, }); }, 1); diff --git a/packages/browser-sdk/src/feedback/feedback.ts b/packages/browser-sdk/src/feedback/feedback.ts index 7f501ee0a..1bc40683a 100644 --- a/packages/browser-sdk/src/feedback/feedback.ts +++ b/packages/browser-sdk/src/feedback/feedback.ts @@ -420,10 +420,7 @@ export class AutoFeedback { return { feedbackId: res.feedbackId }; }, onSubmit: async (data) => { - const res = await replyCallback(data); - if (!res?.feedbackId) { - throw new Error("Couldn't submit feedback. Please try again."); - } + await replyCallback(data); }, onDismiss: () => replyCallback(null), position: this.position, diff --git a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts index bee5c791c..e24581d95 100644 --- a/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts +++ b/packages/browser-sdk/test/e2e/feedback-widget.browser.spec.ts @@ -21,7 +21,6 @@ declare global { interface Window { eventsFired: Record; feedbackSubmissions: FeedbackSubmission[]; - feedbackCloses: number; requestTestFeedback: () => void; } } @@ -52,83 +51,37 @@ async function getOpenedWidgetContainer( }); }); - await page.evaluate( - async ({ key, optionsJson, feedbackSource }) => { - const modulePath = "/dist/reflag-browser-sdk.mjs"; - const { ReflagClient } = (await import( - modulePath - )) as typeof import("../../src/client"); - const options = JSON.parse(optionsJson) as Omit< - InitOptions, - "publishableKey" - >; - const position = options.feedback?.ui?.position; - if (position?.type === "POPOVER") { - const anchor = document.createElement("button"); - anchor.id = "feedback-anchor"; - anchor.textContent = "Give feedback"; - document.body.appendChild(anchor); - position.anchor = anchor; - } + // Golden path requests + await page.evaluate(` + ;(async () => { + const { ReflagClient } = await import("/dist/reflag-browser-sdk.mjs"); + const options = ${JSON.stringify(initOptions)}; window.feedbackSubmissions = []; - window.feedbackCloses = 0; - const onAfterSubmit = (data: FeedbackSubmission) => - window.feedbackSubmissions.push(data); - let onMessage: ((event: MessageEvent) => void) | undefined; + const onAfterSubmit = (data) => window.feedbackSubmissions.push(data); + let onMessage; const reflag = new ReflagClient({ - publishableKey: key, - user: { id: "foo" }, - company: { id: "bar" }, - ...options, + publishableKey: "${KEY}", user: {id: "foo"}, company: {id: "bar"}, ...options, feedback: { ...options.feedback, - autoFeedbackHandler: (_prompt, handlers) => { - handlers.openFeedbackForm({ onAfterSubmit }); - }, + autoFeedbackHandler: (_prompt, handlers) => handlers.openFeedbackForm({ onAfterSubmit }), }, eventSourceFactory: () => ({ - addEventListener: (type, handler) => { - if (type === "message") onMessage = handler; - }, - close: () => { - onMessage = undefined; - }, + addEventListener: (type, handler) => { if (type === "message") onMessage = handler; }, + close: () => {}, }), }); - window.requestTestFeedback = () => - reflag.requestFeedback({ - flagKey: "flag1", - title: "baz", - onAfterSubmit, - onClose: () => { - window.feedbackCloses++; - }, - }); await reflag.initialize(); - if (feedbackSource === "prompt") { - if (!onMessage) - throw new Error("Feedback SSE listener was not registered"); - onMessage( - new MessageEvent("message", { - data: JSON.stringify({ - question: "baz", - showAfter: Date.now() - 1000, - showBefore: Date.now() + 60000, - promptId: "prompt1", - featureId: "feature1", - }), - }), - ); + window.requestTestFeedback = () => reflag.requestFeedback({ flagKey: "flag1", title: "baz", onAfterSubmit }); + if ("${source}" === "prompt") { + onMessage({ data: JSON.stringify({ + question: "baz", promptId: "prompt1", featureId: "feature1", + showAfter: Date.now() - 1000, showBefore: Date.now() + 60000, + }) }); } else { window.requestTestFeedback(); } - }, - { - key: KEY, - optionsJson: JSON.stringify(initOptions), - feedbackSource: source, - }, - ); + })() + `); return page.locator(`#${feedbackContainerId}`); } @@ -571,281 +524,93 @@ test("Closes the dialog shortly after submitting", async ({ page }) => { for (const source of ["widget", "prompt"] as const) { test.describe(`${source} feedback submission callback`, () => { + const saved = { question: "baz", score: 4, feedbackId: "123", comment: "" }; + const submissions = (page: Page) => + page.evaluate(() => window.feedbackSubmissions); + test.beforeEach(async ({ page }) => { - await page.route(`${API_HOST}/feedback`, async (route) => { - await route.fulfill({ - status: 200, - body: JSON.stringify({ feedbackId: "123" }), - contentType: "application/json", - }); - }); + await page.route(`${API_HOST}/feedback`, (route) => + route.fulfill({ json: { feedbackId: "123" } }), + ); }); - test("notifies once on close with the latest saved score, excluding draft comments", async ({ + test("reports the latest saved score on close, without a draft comment", async ({ page, }) => { - const requests: object[] = []; - page.on("request", (request) => { - if (request.url() === `${API_HOST}/feedback`) { - requests.push(request.postDataJSON()); - } - }); - const container = await getOpenedWidgetContainer(page, {}, source); - await setScore(container, 2); - await setScore(container, 5); - await setComment(container, "This comment was not submitted"); - await expect( - container.locator(".form-expanded-content button"), - ).toBeEnabled(); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); - - await container.locator(".dialog .close").click(); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([ - { question: "baz", score: 5, feedbackId: "123", comment: "" }, - ]); - expect(requests).toHaveLength(2); // Closing must not submit again. - }); - - test("does not notify when closed without feedback", async ({ page }) => { - const container = await getOpenedWidgetContainer(page, {}, source); - await container.locator(".dialog .close").click(); - await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); - }); - - test("notifies on modal Escape with a saved score", async ({ page }) => { const container = await getOpenedWidgetContainer( page, { feedback: { ui: { position: { type: "MODAL" } } } }, source, ); + await setScore(container, 2); await setScore(container, 4); + await setComment(container, "Unsubmitted draft"); await expect( container.locator(".form-expanded-content button"), ).toBeEnabled(); + expect(await submissions(page)).toEqual([]); await page.keyboard.press("Escape"); - await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([ - { question: "baz", score: 4, feedbackId: "123", comment: "" }, - ]); + await expect.poll(() => submissions(page)).toEqual([saved]); }); - for (const dismiss of ["Escape", "outside click", "anchor removal"]) { - test(`notifies once when a popover closes via ${dismiss}`, async ({ - page, - }) => { - const container = await getOpenedWidgetContainer( - page, - { feedback: { ui: { position: { type: "POPOVER", anchor: null } } } }, - source, - ); - await setScore(container, 4); - await expect( - container.locator(".form-expanded-content button"), - ).toBeEnabled(); - if (dismiss === "Escape") { - await page.keyboard.press("Escape"); - } else if (dismiss === "outside click") { - await page.mouse.click(1000, 500); - } else { - await page - .locator("#feedback-anchor") - .evaluate((anchor) => anchor.remove()); - } - await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([ - { question: "baz", score: 4, feedbackId: "123", comment: "" }, - ]); - }); - } - - for (const comment of ["", "Useful feature!"]) { - test(`notifies once on Send with ${comment ? "a comment" : "no comment"}, not again on auto-close`, async ({ - page, - }) => { - const container = await getOpenedWidgetContainer(page, {}, source); - await setScore(container, 4); - await setComment(container, comment); - await submitForm(container); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([{ question: "baz", score: 4, feedbackId: "123", comment }]); - await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); - expect( - await page.evaluate(() => window.feedbackSubmissions), - ).toHaveLength(1); - }); - } - - for (const action of ["closing", "replacement"] as const) { - test(`waits for an in-flight score save after ${action}`, async ({ - page, - }) => { - const pendingSave = deferred(); - let saving = false; - await page.route(`${API_HOST}/feedback`, async (route) => { - saving = true; - await pendingSave.promise; - await route.fulfill({ json: { feedbackId: "123" } }); - }); - const container = await getOpenedWidgetContainer(page, {}, source); - await setScore(container, 4); - await expect.poll(() => saving).toBe(true); - if (action === "replacement") { - await page.evaluate(() => window.requestTestFeedback()); - await expect( - container.locator("#reflag-feedback-score-4"), - ).not.toBeChecked(); - } else { - await container.locator(".dialog .close").click(); - } - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual( - [], - ); - - pendingSave.resolve(); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([ - { question: "baz", score: 4, feedbackId: "123", comment: "" }, - ]); - - if (action === "replacement") { - await expect( - container.locator("#reflag-feedback-score-4"), - ).not.toBeChecked(); - await container.locator(".dialog .close").click(); - expect( - await page.evaluate(() => window.feedbackSubmissions), - ).toHaveLength(1); - } - }); - } + test("reports Send once, not again on auto-close", async ({ page }) => { + const container = await getOpenedWidgetContainer(page, {}, source); + await setScore(container, 4); + await setComment(container, "Useful!"); + await submitForm(container); + const expected = [{ ...saved, comment: "Useful!" }]; + await expect.poll(() => submissions(page)).toEqual(expected); + await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); + expect(await submissions(page)).toEqual(expected); + }); - test("notifies when a saved response is replaced by a new form", async ({ + test("closing during Send waits for the submitted comment", async ({ page, }) => { - let feedbackId = 0; + const pending = deferred(); + let saving = false; await page.route(`${API_HOST}/feedback`, async (route) => { - await route.fulfill({ json: { feedbackId: String(++feedbackId) } }); + if (route.request().postDataJSON().comment) { + saving = true; + await pending.promise; + } + await route.fulfill({ json: { feedbackId: "123" } }); }); const container = await getOpenedWidgetContainer(page, {}, source); await setScore(container, 4); - await expect( - container.locator(".form-expanded-content button"), - ).toBeEnabled(); - - await page.evaluate(() => window.requestTestFeedback()); - await expect( - container.locator(".form-expanded-content button"), - ).toBeDisabled(); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([{ question: "baz", score: 4, feedbackId: "1", comment: "" }]); - - await setScore(container, 5); - await expect( - container.locator(".form-expanded-content button"), - ).toBeEnabled(); + await setComment(container, "Useful!"); + await submitForm(container); + await expect.poll(() => saving).toBe(true); await container.locator(".dialog .close").click(); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([ - { question: "baz", score: 4, feedbackId: "1", comment: "" }, - { question: "baz", score: 5, feedbackId: "2", comment: "" }, - ]); - - await page.evaluate(() => window.requestTestFeedback()); - await expect( - container.locator(".form-expanded-content button"), - ).toBeDisabled(); - expect(await page.evaluate(() => window.feedbackCloses)).toBe( - source === "widget" ? 2 : 1, - ); - expect( - await page.evaluate(() => window.feedbackSubmissions), - ).toHaveLength(2); + expect(await submissions(page)).toEqual([]); + pending.resolve(); + await expect + .poll(() => submissions(page)) + .toEqual([{ ...saved, comment: "Useful!" }]); }); - for (const action of ["close", "retry"] as const) { - test(`failed Send does not consume the callback before ${action}`, async ({ - page, - }) => { - let failCommentSave = true; - await page.route(`${API_HOST}/feedback`, async (route) => { - if (route.request().postDataJSON().comment && failCommentSave) { - await route.fulfill({ - status: 500, - json: { message: "Save failed" }, - }); - } else { - await route.fulfill({ json: { feedbackId: "123" } }); - } - }); - const container = await getOpenedWidgetContainer(page, {}, source); - await setScore(container, 4); - await setComment(container, "Useful feature!"); - await submitForm(container); - await expect(container.locator(".error")).toBeVisible(); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual( - [], - ); - - if (action === "retry") { - failCommentSave = false; - await submitForm(container); - await expect.poll(() => isFeedbackDialogOpen(page)).toBe(false); - } else { - await container.locator(".dialog .close").click(); - } - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([ - { - question: "baz", - score: 4, - feedbackId: "123", - comment: action === "retry" ? "Useful feature!" : "", - }, - ]); - }); - } - - test("closing during Send waits and notifies once with the submitted comment", async ({ + test("replacement waits for a pending score save and does not notify for the empty new form", async ({ page, }) => { - const pendingSave = deferred(); - let savingComment = false; + const pending = deferred(); + let saving = false; await page.route(`${API_HOST}/feedback`, async (route) => { - if (route.request().postDataJSON().comment) { - savingComment = true; - await pendingSave.promise; - } - await route.fulfill({ - status: 200, - body: JSON.stringify({ feedbackId: "123" }), - contentType: "application/json", - }); + saving = true; + await pending.promise; + await route.fulfill({ json: { feedbackId: "123" } }); }); const container = await getOpenedWidgetContainer(page, {}, source); await setScore(container, 4); - await setComment(container, "Useful feature!"); - await submitForm(container); - await expect.poll(() => savingComment).toBe(true); + await expect.poll(() => saving).toBe(true); + await page.evaluate(() => window.requestTestFeedback()); + await expect( + container.locator("#reflag-feedback-score-4"), + ).not.toBeChecked(); + expect(await submissions(page)).toEqual([]); + pending.resolve(); + await expect.poll(() => submissions(page)).toEqual([saved]); await container.locator(".dialog .close").click(); - expect(await page.evaluate(() => window.feedbackSubmissions)).toEqual([]); - - pendingSave.resolve(); - await expect - .poll(() => page.evaluate(() => window.feedbackSubmissions)) - .toEqual([ - { - question: "baz", - score: 4, - feedbackId: "123", - comment: "Useful feature!", - }, - ]); + expect(await submissions(page)).toEqual([saved]); }); }); } diff --git a/packages/browser-sdk/test/feedbackSubmissionCallback.test.ts b/packages/browser-sdk/test/feedbackSubmissionCallback.test.ts deleted file mode 100644 index 5d764a3ce..000000000 --- a/packages/browser-sdk/test/feedbackSubmissionCallback.test.ts +++ /dev/null @@ -1,203 +0,0 @@ -import { describe, expect, test, vi } from "vitest"; - -import { - FeedbackSubmission, - OnScoreSubmitResult, - OpenFeedbackFormOptions, -} from "../src/feedback/ui/types"; -import { withFeedbackSubmissionCallback } from "../src/feedback/ui/withFeedbackSubmissionCallback"; -import { deferred } from "./deferred"; - -const score = { question: "How was it?", score: 4 }; -const feedbackId = "feedback-123"; -const savedScore = { ...score, feedbackId, comment: "" }; -const submittedFeedback: FeedbackSubmission = { - ...savedScore, - comment: "Very useful!", -}; - -function setup(overrides: Partial = {}) { - const onAfterSubmit = vi.fn(); - const onClose = vi.fn(); - const onSubmit = vi.fn().mockResolvedValue(undefined); - const onScoreSubmit = vi.fn().mockResolvedValue({ feedbackId }); - const handlers = withFeedbackSubmissionCallback({ - key: "test", - onAfterSubmit, - onClose, - onSubmit, - onScoreSubmit, - ...overrides, - }); - return { handlers, onAfterSubmit, onClose, onSubmit, onScoreSubmit }; -} - -describe("feedback submission callback", () => { - test("does not notify when closing without feedback", () => { - const { handlers, onAfterSubmit, onClose } = setup(); - handlers.onClose!(); - expect(onAfterSubmit).not.toHaveBeenCalled(); - expect(onClose).toHaveBeenCalledOnce(); - }); - - test("notifies once on close with the latest saved score and an empty comment", async () => { - const { handlers, onAfterSubmit, onScoreSubmit } = setup(); - await handlers.onScoreSubmit!(score); - await handlers.onScoreSubmit!({ ...score, feedbackId, score: 5 }); - expect(onAfterSubmit).not.toHaveBeenCalled(); - - handlers.onClose!(); - handlers.onClose!(); - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith({ - ...savedScore, - score: 5, - }); - expect(onScoreSubmit).toHaveBeenCalledTimes(2); - }); - - test("notifies after Send, not again when the form closes", async () => { - const { handlers, onAfterSubmit, onSubmit } = setup(); - await handlers.onScoreSubmit!(score); - await handlers.onSubmit(submittedFeedback); - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(submittedFeedback); - - handlers.onClose!(); - handlers.onClose!(); - expect(onAfterSubmit).toHaveBeenCalledOnce(); - expect(onSubmit).toHaveBeenCalledExactlyOnceWith(submittedFeedback); - }); - - test("notifies after Send with an empty comment too", async () => { - const { handlers, onAfterSubmit } = setup(); - await handlers.onSubmit(savedScore); - handlers.onClose!(); - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); - }); - - test("waits for an in-flight score save when closing", async () => { - const pending = deferred(); - const { handlers, onAfterSubmit } = setup({ - onScoreSubmit: () => pending.promise, - }); - const saving = handlers.onScoreSubmit!(score); - handlers.onClose!(); - expect(onAfterSubmit).not.toHaveBeenCalled(); - - pending.resolve({ feedbackId }); - await saving; - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); - }); - - test("waits for all score saves and keeps the latest selection even with out-of-order responses", async () => { - const first = deferred(); - const second = deferred(); - const onScoreSubmit = vi - .fn() - .mockReturnValueOnce(first.promise) - .mockReturnValueOnce(second.promise); - const { handlers, onAfterSubmit } = setup({ onScoreSubmit }); - const savingFirst = handlers.onScoreSubmit!(score); - const savingSecond = handlers.onScoreSubmit!({ ...score, score: 5 }); - handlers.onClose!(); - - second.resolve({ feedbackId }); - await savingSecond; - expect(onAfterSubmit).not.toHaveBeenCalled(); - first.resolve({ feedbackId }); - await savingFirst; - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith({ - ...savedScore, - score: 5, - }); - }); - - test("waits for an in-flight Send rather than notifying with just the score", async () => { - const pending = deferred(); - const { handlers, onAfterSubmit } = setup({ - onSubmit: () => pending.promise, - }); - await handlers.onScoreSubmit!(score); - const saving = handlers.onSubmit(submittedFeedback); - handlers.onClose!(); - expect(onAfterSubmit).not.toHaveBeenCalled(); - - pending.resolve(); - await saving; - handlers.onClose!(); - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(submittedFeedback); - }); - - test("does not notify for a failed score save", async () => { - const { handlers, onAfterSubmit } = setup({ - onScoreSubmit: vi.fn().mockRejectedValue(new Error("Save failed")), - }); - const saving = handlers.onScoreSubmit!(score); - handlers.onClose!(); - await expect(saving).rejects.toThrow("Save failed"); - expect(onAfterSubmit).not.toHaveBeenCalled(); - }); - - test("does not notify if the score handler did not save feedback", async () => { - const { handlers, onAfterSubmit } = setup({ - // The manual feedback handler returns an undefined ID on an unsuccessful response. - onScoreSubmit: vi.fn().mockResolvedValue({ feedbackId: undefined }), - }); - await handlers.onScoreSubmit!(score); - handlers.onClose!(); - expect(onAfterSubmit).not.toHaveBeenCalled(); - }); - - test("falls back to the saved score if Send fails after closing", async () => { - const pending = deferred(); - const { handlers, onAfterSubmit } = setup({ - onSubmit: async () => { - await pending.promise; - throw new Error("Send failed"); - }, - }); - await handlers.onScoreSubmit!(score); - const saving = handlers.onSubmit(submittedFeedback); - handlers.onClose!(); - pending.resolve(); - - await expect(saving).rejects.toThrow("Send failed"); - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); - }); - - test("retains a saved score if a subsequent score save fails", async () => { - const onScoreSubmit = vi - .fn() - .mockResolvedValueOnce({ feedbackId }) - .mockRejectedValueOnce(new Error("Save failed")); - const { handlers, onAfterSubmit } = setup({ onScoreSubmit }); - await handlers.onScoreSubmit!(score); - await expect( - handlers.onScoreSubmit!({ ...score, score: 5 }), - ).rejects.toThrow("Save failed"); - handlers.onClose!(); - expect(onAfterSubmit).toHaveBeenCalledExactlyOnceWith(savedScore); - }); - - test("does not call a throwing callback again or skip the user's close handler", async () => { - const onAfterSubmit = vi.fn(() => { - throw new Error("Analytics failed"); - }); - const { handlers, onClose } = setup({ onAfterSubmit }); - await handlers.onScoreSubmit!(score); - expect(() => handlers.onClose!()).toThrow("Analytics failed"); - expect(onClose).toHaveBeenCalledOnce(); - handlers.onClose!(); - expect(onAfterSubmit).toHaveBeenCalledOnce(); - }); - - test("each new form has an independent callback lifecycle", async () => { - const first = setup(); - const second = setup(); - await first.handlers.onScoreSubmit!(score); - await second.handlers.onScoreSubmit!(score); - first.handlers.onClose!(); - second.handlers.onClose!(); - expect(first.onAfterSubmit).toHaveBeenCalledOnce(); - expect(second.onAfterSubmit).toHaveBeenCalledOnce(); - }); -});