From d14f9d09f35bfe31e36444747a41d002ae6b3d45 Mon Sep 17 00:00:00 2001 From: Lambda Date: Tue, 2 Jun 2026 15:01:02 +0800 Subject: [PATCH] refactor(onboarding): source picker is single-select primary source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Jiayuan's call after the survey of HDYHAU UX in PLG SaaS (Linear / Vercel / Loom / Notion / Webflow / Stripe / Figma / Cursor / PostHog mostly skip the question entirely; where it's asked the documented default — Fairing / Recast / HockeyStack / Ruler Analytics — is to capture the primary source so channel weights sum to 100% and ROI math is defensible). Modal + StepSource both pivot from multi-select to single-select radio. Server schema is intentionally untouched: `source` stays `string[]` for back-compat with v2 multi-select rows; the client always sends a one-element array. Zero migration, zero data loss. Frontend: - `source-backfill-modal.tsx`: state pivots from a multi-element `source: Source[]` to a single `pickedSlug` derived from `source[0]`; click handler replaces the array instead of toggling. Cards switch to `mode="radio"`, the fieldset gets `role="radiogroup"`, the now-redundant `pendingOther` and `allowToggleOff` opt-in go away — radio mode means no toggle-off, so the original UAT bug ("Other can't be deselected") is structurally impossible. - `step-source.tsx`: drop the `multiSelect` prop so it routes through `step-question.tsx`'s existing radio path (same one StepRole already uses). Picking a second option replaces the first; switching away from Other clears `source_other` so a stale value can't leak. - `icon-option-card.tsx`: revert the `allowToggleOff` plumbing. Tests: - `source-backfill-modal.test.tsx`: drop the multi-select toggle-off assertion; add "picking a second option replaces the first" with explicit radio-role queries. - `step-source.test.tsx`: rewrite multi-select tests as single-select (no more "stacks several picks" / "toggle off" cases); add "switching away from Other clears source_other". Full suite (970 tests) green, typecheck + lint clean. Co-authored-by: multica-agent --- .../components/icon-option-card.tsx | 26 +------ .../onboarding/source-backfill-modal.test.tsx | 51 +++++-------- .../onboarding/source-backfill-modal.tsx | 76 ++++++++----------- .../onboarding/steps/step-source.test.tsx | 53 +++++++------ .../views/onboarding/steps/step-source.tsx | 53 +++++-------- 5 files changed, 101 insertions(+), 158 deletions(-) diff --git a/packages/views/onboarding/components/icon-option-card.tsx b/packages/views/onboarding/components/icon-option-card.tsx index 4f3c6bf54..be8c6da6a 100644 --- a/packages/views/onboarding/components/icon-option-card.tsx +++ b/packages/views/onboarding/components/icon-option-card.tsx @@ -74,7 +74,6 @@ export function IconOtherOptionCard({ onConfirm, placeholder, mode = "radio", - allowToggleOff = false, }: { icon: ReactNode; label: string; @@ -85,34 +84,13 @@ export function IconOtherOptionCard({ onConfirm: () => void; placeholder: string; mode?: "radio" | "checkbox"; - /** - * When true, clicking the card while it is already selected fires - * `onSelect` again so the parent can toggle the pick off (multi-select - * pattern, used by the source-backfill modal). Default `false` - * preserves the original onboarding StepSource behaviour: a re-click - * on the selected Other card is a no-op so users can move focus - * into the text input without deselecting. Clicks that originate - * inside the input never toggle — the input stops their propagation. - */ - allowToggleOff?: boolean; }) { return (
{ - // In `allowToggleOff` mode, clicking the row again deselects. - // Skip when the click originated on the inner text input so - // typing / focusing it doesn't accidentally deselect — clicks - // on the icon, padding, or label area still toggle. - if ( - allowToggleOff && - selected && - e.target instanceof HTMLInputElement - ) { - return; - } - if (allowToggleOff || !selected) onSelect(); + onClick={() => { + if (!selected) onSelect(); }} className={cn( "flex w-full items-center gap-3 rounded-xl border bg-card px-4 py-3 text-left transition-all", diff --git a/packages/views/onboarding/source-backfill-modal.test.tsx b/packages/views/onboarding/source-backfill-modal.test.tsx index c77e0037f..8ef72e0a2 100644 --- a/packages/views/onboarding/source-backfill-modal.test.tsx +++ b/packages/views/onboarding/source-backfill-modal.test.tsx @@ -216,14 +216,10 @@ describe("SourceBackfillModal", () => { expect(await screen.findByText("GitHub")).toBeInTheDocument(); }); - it("Other toggles off on the second click instead of getting stuck", async () => { - // Regression: an earlier version held a parallel `pendingOther` - // flag that flipped to true on every Other click. A second click - // then removed "other" from `source` but the row's onClick guard - // (`if (!selected) onSelect()`) ALSO swallowed re-clicks while - // selected, so the toggle-off never fired. Fixed by dropping - // pendingOther AND opting the modal's Other card into the new - // `allowToggleOff` prop on `IconOtherOptionCard`. + it("picking a second option replaces the first (single-select primary source)", async () => { + // The modal is now a single-select radio. Industry default for + // HDYHAU is to capture the primary acquisition source, so picking + // a second option must replace the first — never accumulate. setUser({ id: "u1", onboarded_at: "2026-01-01T00:00:00Z", @@ -231,37 +227,30 @@ describe("SourceBackfillModal", () => { }); const user = userEvent.setup(); renderModal(); - // Wait for the modal to render. Pick the Other card by its - // checkbox role — `role="checkbox"` + matching `aria-checked` - // stays stable whether the label is showing or the input has - // replaced it. await screen.findByText("Friends or colleagues"); - const checkboxes = screen.getAllByRole("checkbox"); - // 12 options: 11 normal + 1 Other. Other is the last per the - // component's option array. - const other = checkboxes[checkboxes.length - 1]!; - const friends = checkboxes[0]!; + const radios = screen.getAllByRole("radio"); + const friends = radios[0]!; + const search = radios[1]!; - // Tick Other → Submit disabled (no free-text yet) - await user.click(other); - expect(other).toHaveAttribute("aria-checked", "true"); - expect(screen.getByRole("button", { name: "Submit" })).toBeDisabled(); - // Add a real selection so we can test the un-tick path without - // the validator hiding the bug behind the empty-other-text branch. await user.click(friends); - expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled(); - // Un-tick Other → still enabled because friends is still picked. - await user.click(other); - expect(other).toHaveAttribute("aria-checked", "false"); - expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled(); - // Critically: submit and assert "other" is NOT in the payload. + expect(friends).toHaveAttribute("aria-checked", "true"); + expect(search).toHaveAttribute("aria-checked", "false"); + + // Pick a second option — the first must clear and Submit stays + // enabled with exactly one pick in the payload. + await user.click(search); + expect(friends).toHaveAttribute("aria-checked", "false"); + expect(search).toHaveAttribute("aria-checked", "true"); + await user.click(screen.getByRole("button", { name: "Submit" })); await waitFor(() => { expect(mockSaveQuestionnaire).toHaveBeenCalledTimes(1); }); const sent = mockSaveQuestionnaire.mock.calls[0]![0]; - expect(sent.source).toEqual(["friends_colleagues"]); - expect(sent.source).not.toContain("other"); + // Server schema is still `source: string[]` for back-compat with + // v2 rows; the client always sends a single-element array. + expect(sent.source).toEqual(["search"]); + expect(sent.source).not.toContain("friends_colleagues"); }); it("defers the entrance by ~700ms when the user has not opted into reduced motion", async () => { diff --git a/packages/views/onboarding/source-backfill-modal.tsx b/packages/views/onboarding/source-backfill-modal.tsx index 492dc2327..39532aecc 100644 --- a/packages/views/onboarding/source-backfill-modal.tsx +++ b/packages/views/onboarding/source-backfill-modal.tsx @@ -196,56 +196,45 @@ function SourceBackfillDialogBody({ [t], ); - const selected = useMemo( - () => [ - ...answers.source, - ...(!answers.source.includes("other") && answers.source_other - ? ["other"] - : []), - ], - [answers.source, answers.source_other], - ); - + // Single-select: at most one slug in `source` at any time. The server + // schema keeps the array (back-compat with the v2 multi-select shape + // and with existing rows), but the modal UI commits exactly one pick + // — primary-source attribution is the documented industry default for + // HDYHAU prompts (Fairing, Recast, HockeyStack) and gives the team + // clean channel weights without splitting users across N buckets. + const pickedSlug: string | null = answers.source[0] ?? null; const otherOption = options.find((o) => o.isOther) ?? null; - // Single source of truth for "is Other ticked": derive from `source` - // directly, NOT a parallel useState flag. The previous version kept a - // `pendingOther` state set to true on every Other click — which meant - // a second click toggled "other" off in `source` but left - // `pendingOther` stuck at true, so `otherActive` (its OR with - // selected) never flipped back and the card visually stayed selected - // (caught in UAT). - const otherSelected = otherOption - ? selected.includes(otherOption.slug) - : false; + const otherSelected = pickedSlug === otherOption?.slug; const otherFilled = (answers.source_other ?? "").trim().length > 0; - const hasNonOtherSelection = selected.some( - (slug) => slug !== otherOption?.slug, - ); const canSubmit = !busy && - selected.length > 0 && - (hasNonOtherSelection || !otherSelected || otherFilled); + pickedSlug !== null && + // If the user picked Other, gate Submit on having typed something — + // an empty Other selection isn't useful attribution data. + (!otherSelected || otherFilled); const handleSelect = useCallback( (option: QuestionOption) => { if (option.isOther) { - setAnswers((a) => { - const has = a.source.includes("other"); - return has - ? { ...a, source: a.source.filter((s) => s !== "other"), source_other: null } - : { ...a, source: [...a.source, "other"], source_skipped: false }; - }); + const slug = option.slug as Source; + setAnswers((a) => ({ + ...a, + source: [slug], + // Picking Other doesn't carry text from a prior Other pick + // forward: the text input auto-focuses fresh so the user can + // type immediately. A previous text value would be misleading. + source_other: a.source[0] === "other" ? a.source_other : null, + source_skipped: false, + })); return; } const slug = option.slug as Source; - setAnswers((a) => { - const has = a.source.includes(slug); - return { - ...a, - source: has ? a.source.filter((s) => s !== slug) : [...a.source, slug], - source_skipped: false, - }; - }); + setAnswers((a) => ({ + ...a, + source: [slug], + source_other: null, + source_skipped: false, + })); }, [], ); @@ -318,7 +307,7 @@ function SourceBackfillDialogBody({
$.questions.source.question)} className="m-0 grid grid-cols-1 gap-2 p-0 px-6 pt-4 sm:grid-cols-2" > @@ -334,17 +323,16 @@ function SourceBackfillDialogBody({ onOtherChange={handleOtherChange} onConfirm={submit} placeholder={t(($) => $.questions.source.other_placeholder)} - mode="checkbox" - allowToggleOff + mode="radio" /> ) : ( handleSelect(option)} - mode="checkbox" + mode="radio" /> ), )} diff --git a/packages/views/onboarding/steps/step-source.test.tsx b/packages/views/onboarding/steps/step-source.test.tsx index 37a39b0db..447834127 100644 --- a/packages/views/onboarding/steps/step-source.test.tsx +++ b/packages/views/onboarding/steps/step-source.test.tsx @@ -41,54 +41,41 @@ function renderStep(answers: QuestionnaireAnswers = EMPTY) { return { onChange, onAdvance, onSkip, onBack }; } -describe("StepSource (multi-select)", () => { +describe("StepSource (single-select primary source)", () => { beforeEach(() => vi.restoreAllMocks()); - it("clicking a non-Other option appends the slug to the array", async () => { + it("clicking a non-Other option writes a one-element source array", async () => { const user = userEvent.setup(); const { onChange, onAdvance } = renderStep(); - await user.click(screen.getByRole("checkbox", { name: /linkedin/i })); + await user.click(screen.getByRole("radio", { name: /linkedin/i })); expect(onChange).toHaveBeenCalledWith({ source: ["social_linkedin"], + source_other: null, source_skipped: false, }); // A click only records — it must NOT auto-advance. expect(onAdvance).not.toHaveBeenCalled(); }); - it("clicking an already-selected option removes it (toggle)", async () => { + it("picking a second option replaces the first (no stacking)", async () => { const user = userEvent.setup(); const { onChange } = renderStep({ ...EMPTY, source: ["social_linkedin"], }); - await user.click(screen.getByRole("checkbox", { name: /linkedin/i })); + await user.click(screen.getByRole("radio", { name: /twitter/i })); expect(onChange).toHaveBeenCalledWith({ - source: [], + source: ["social_x"], + source_other: null, source_skipped: false, }); }); - it("multi-select stacks several picks", async () => { - const user = userEvent.setup(); - const { onChange } = renderStep({ - ...EMPTY, - source: ["social_linkedin"], - }); - - await user.click(screen.getByRole("checkbox", { name: /twitter/i })); - - expect(onChange).toHaveBeenCalledWith({ - source: ["social_linkedin", "social_x"], - source_skipped: false, - }); - }); - - it("Skip clears the array + other and marks the step skipped, then calls onSkip", async () => { + it("Skip clears source + source_other and marks the step skipped, then calls onSkip", async () => { const user = userEvent.setup(); const { onChange, onSkip } = renderStep(); @@ -102,14 +89,15 @@ describe("StepSource (multi-select)", () => { expect(onSkip).toHaveBeenCalledTimes(1); }); - it("Other: clicking adds 'other' to the array; free-text writes to source_other", async () => { + it("Other: clicking writes `source: ['other']` and lets the user type into source_other", async () => { const user = userEvent.setup(); const { onChange } = renderStep(); - await user.click(screen.getByRole("checkbox", { name: /^other$/i })); + await user.click(screen.getByRole("radio", { name: /^other$/i })); expect(onChange).toHaveBeenCalledWith({ source: ["other"], + source_other: null, source_skipped: false, }); @@ -117,4 +105,21 @@ describe("StepSource (multi-select)", () => { await user.type(input, "x"); expect(onChange).toHaveBeenLastCalledWith({ source_other: "x" }); }); + + it("switching away from Other clears source_other so a stale value can't leak", async () => { + const user = userEvent.setup(); + const { onChange } = renderStep({ + ...EMPTY, + source: ["other"], + source_other: "a podcast", + }); + + await user.click(screen.getByRole("radio", { name: /linkedin/i })); + + expect(onChange).toHaveBeenCalledWith({ + source: ["social_linkedin"], + source_other: null, + source_skipped: false, + }); + }); }); diff --git a/packages/views/onboarding/steps/step-source.tsx b/packages/views/onboarding/steps/step-source.tsx index a9f9e732b..fa9476fff 100644 --- a/packages/views/onboarding/steps/step-source.tsx +++ b/packages/views/onboarding/steps/step-source.tsx @@ -56,41 +56,25 @@ export function StepSource({ { slug: "other", icon: , label: t(($) => $.questions.source.other), isOther: true }, ]; - // Multi-select: source already had several "yes I heard via X" intents - // collapsed into a single radio pick. Letting users tick multiple - // channels keeps attribution honest without making them rank them. - // `other` is a Source enum value so it stacks into the array alongside - // regular picks; the free-text input lives in `source_other`. - const selected: readonly string[] = [ - ...(answers.source ?? []), - ...(!answers.source?.includes("other") && answers.source_other - ? ["other"] - : []), - ]; + // Single-select on the primary acquisition source. The server schema + // keeps `source` as a string array for back-compat with v2 multi- + // select rows, but the UI only ever commits a one-element array — + // primary-source attribution is the documented industry default for + // HDYHAU prompts (Fairing, Recast, HockeyStack) and keeps channel + // weights clean for analytics. + const selected: readonly string[] = answers.source?.[0] ? [answers.source[0]] : []; - const toggle = (slug: string) => { - const current = answers.source ?? []; - if (slug === "other") { - // Toggling Other: add/remove from the array; clear the text when - // removing so a re-tick starts from an empty input. - if (current.includes("other")) { - onChange({ - source: current.filter((s) => s !== "other"), - source_other: null, - }); - } else { - onChange({ - source: [...current, "other"], - source_skipped: false, - }); - } - return; - } + const pick = (slug: string) => { const typed = slug as Source; - const next = current.includes(typed) - ? current.filter((s) => s !== typed) - : [...current, typed]; - onChange({ source: next, source_skipped: false }); + onChange({ + source: [typed], + // Switching off Other clears the free-text input so a stale + // value can't leak into the next pick. Picking Other keeps the + // existing text so the user can finish typing without losing + // their input. + source_other: typed === "other" ? answers.source_other : null, + source_skipped: false, + }); }; return ( @@ -104,14 +88,13 @@ export function StepSource({ otherValue={answers.source_other ?? ""} onOtherChange={(v) => onChange({ source_other: v })} otherPlaceholder={t(($) => $.questions.source.other_placeholder)} - onAnswer={toggle} + onAnswer={pick} onAdvance={onAdvance} onSkip={() => { onChange({ source: [], source_other: null, source_skipped: true }); onSkip(); }} onBack={onBack} - multiSelect /> ); }