From b17f18295e8b32e9a083645b05745d728e69cd76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 12:11:17 +0200 Subject: [PATCH] fix(onboarding): let the getting-started list go when its rows are done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things, all in the same pair. The reader that orders the list from the answers shipped without the proof that it reads correctly: every existing case took the early return, so `checklistOrderFromNeeds` and the two new arms were never executed by a test. They are covered now — medication first for somebody who takes one daily, the named connection ahead of the untouched rows, profile always first, and no answer combination that drops or duplicates a row. A first-result task that was offered and then passed left the setup reading as permanently unfinished, because the check asked only whether the offered task had produced anything. A deliberate pass is an answer. And the published state is additive — every record carries the field — so nine pending steps and empty answers must read as "never entered the flow" rather than as an unfinished one, or every account that predates the flow would look half set up. The list itself now follows the design: for a record that ran the flow it goes when its rows are done or the person hides it, and the five-reading rule stays only for records that never entered. --- .../onboarding/__tests__/checklist.test.ts | 246 ++++++++++++++++++ src/lib/onboarding/__tests__/needs.test.ts | 79 +++++- src/lib/onboarding/checklist.ts | 44 ++-- src/lib/onboarding/needs.ts | 35 ++- 4 files changed, 383 insertions(+), 21 deletions(-) diff --git a/src/lib/onboarding/__tests__/checklist.test.ts b/src/lib/onboarding/__tests__/checklist.test.ts index c765877f8..1a807a769 100644 --- a/src/lib/onboarding/__tests__/checklist.test.ts +++ b/src/lib/onboarding/__tests__/checklist.test.ts @@ -2,13 +2,38 @@ import { describe, expect, it } from "vitest"; import { buildChecklist, + checklistOrderFromNeeds, checklistProgress, + CHECKLIST_ITEM_IDS, isProfileComplete, shouldShowChecklist, trendHintFor, visibleChecklist, type ChecklistItemId, } from "../checklist"; +import { + defaultOnboardingSteps, + emptyOnboardingNeeds, + type OnboardingNeeds, + type OnboardingStateDto, +} from "../needs"; + +/** A setup state the flow really produced: Q1 answered, confirm reached. */ +function onboardingState( + overrides: Partial = {}, + needs: Partial = {}, +): OnboardingStateDto { + return { + steps: defaultOnboardingSteps().map((step) => ({ + ...step, + status: "done" as const, + })), + needs: { ...emptyOnboardingNeeds(), recordTarget: "me", ...needs }, + completedAt: "2026-09-10T08:00:00.000Z", + firstResult: null, + ...overrides, + }; +} const completeProfile = { heightCm: 175, @@ -247,3 +272,224 @@ describe("trendHintFor", () => { expect(trendHintFor(99)).toEqual({ kind: "hidden" }); }); }); + +describe("checklistOrderFromNeeds", () => { + function order(needs: Partial) { + return checklistOrderFromNeeds({ ...emptyOnboardingNeeds(), ...needs }); + } + + it("keeps profile first whatever the answers say", () => { + for (const needs of [ + {}, + { medication: "yes" as const }, + { sources: ["withings" as const] }, + { areas: ["sleep" as const] }, + ]) { + expect(order(needs)[0]).toBe("profile"); + } + }); + + it("puts medication second for somebody who takes one on a schedule", () => { + expect(order({ medication: "yes" })[1]).toBe("medication"); + expect(order({ medication: "sometimes" })[1]).toBe("medication"); + expect(order({ medication: "no" })[1]).not.toBe("medication"); + }); + + it("promotes the data source only for a source that is a connection", () => { + expect(order({ sources: ["withings"] }).indexOf("dataSource")).toBe(1); + // Typing readings in and uploading a file are not connections, so they + // say nothing about that row. + expect(order({ sources: ["manual", "file"] }).indexOf("dataSource")).toBe( + 3, + ); + }); + + it("promotes the reading row once an area was chosen", () => { + expect(order({ areas: ["blood-pressure"] })[1]).toBe("measurement"); + }); + + it("orders medication ahead of the data source when both are answered", () => { + const ids = order({ medication: "yes", sources: ["oura"] }); + expect(ids.indexOf("medication")).toBeLessThan(ids.indexOf("dataSource")); + }); + + it("never drops or duplicates a row, for any answer combination", () => { + const combinations: Partial[] = [ + {}, + { medication: "yes" }, + { sources: ["manual"] }, + { sources: ["fitbit"] }, + { areas: ["mood", "labs"] }, + { medication: "sometimes", sources: ["nightscout"], areas: ["glucose"] }, + ]; + for (const needs of combinations) { + const ids = order(needs); + expect([...ids].sort()).toEqual([...CHECKLIST_ITEM_IDS].sort()); + } + }); +}); + +describe("buildChecklist, ordered by the setup answers", () => { + it("keeps the fixed order for a record that never entered the flow", () => { + expect(buildChecklist(inputs()).map((i) => i.id)).toEqual([ + ...CHECKLIST_ITEM_IDS, + ]); + }); + + it("keeps the fixed order while the answers are still being given", () => { + const items = buildChecklist( + inputs({ + onboarding: onboardingState( + { completedAt: null }, + { medication: "yes" }, + ), + }), + ); + // Re-ordering the dashboard under somebody mid-question would be movement + // they did not ask for. + expect(items.map((i) => i.id)).toEqual([...CHECKLIST_ITEM_IDS]); + }); + + it("orders the rows from the answers once the flow is confirmed", () => { + const items = buildChecklist( + inputs({ + onboarding: onboardingState( + {}, + { medication: "yes", sources: ["withings"] }, + ), + }), + ); + expect(items.map((i) => i.id).slice(0, 3)).toEqual([ + "profile", + "medication", + "dataSource", + ]); + expect([...items.map((i) => i.id)].sort()).toEqual( + [...CHECKLIST_ITEM_IDS].sort(), + ); + }); + + it("carries each row's own state through the reordering", () => { + const items = buildChecklist( + inputs({ + medicationCount: 2, + dismissedIds: new Set(["insights"]), + onboarding: onboardingState({}, { medication: "yes" }), + }), + ); + expect(items.find((i) => i.id === "medication")?.done).toBe(true); + expect(items.find((i) => i.id === "insights")?.dismissed).toBe(true); + }); +}); + +describe("shouldShowChecklist and the setup flow", () => { + const doneItems = () => + buildChecklist( + inputs({ + measurementCount: 3, + medicationCount: 1, + dataSourceConnected: true, + notificationsConfigured: true, + insightsConfigured: true, + }), + ); + + it("no longer disappears at five readings for a record that ran the flow", () => { + const items = buildChecklist(inputs({ measurementCount: 10 })); + expect( + shouldShowChecklist({ + onboardingCompletedAt: "2026-01-01T00:00:00Z", + measurementCount: 10, + dismissedAll: false, + items, + onboarding: onboardingState(), + }), + ).toBe(true); + }); + + it("goes when its rows are done, even for a record that ran the flow", () => { + expect( + shouldShowChecklist({ + onboardingCompletedAt: "2026-01-01T00:00:00Z", + measurementCount: 10, + dismissedAll: false, + items: doneItems(), + onboarding: onboardingState(), + }), + ).toBe(false); + }); + + it("goes when the person hides the whole list", () => { + expect( + shouldShowChecklist({ + onboardingCompletedAt: null, + measurementCount: 0, + dismissedAll: true, + items: buildChecklist(inputs()), + onboarding: onboardingState(), + }), + ).toBe(false); + }); + + it("stays while the flow is unfinished, even with every row done", () => { + // The one task the flow offered is not one of the six rows, so "every row + // done" is not the same statement as "the setup finished". + expect( + shouldShowChecklist({ + onboardingCompletedAt: "2026-01-01T00:00:00Z", + measurementCount: 10, + dismissedAll: false, + items: doneItems(), + onboarding: onboardingState({ + firstResult: { + task: "log-reading", + target: null, + completedAt: null, + }, + }), + }), + ).toBe(true); + }); + + it("does not stay pinned by a first-result task the person passed", () => { + const state = onboardingState({ + firstResult: { task: "log-reading", target: null, completedAt: null }, + }); + expect( + shouldShowChecklist({ + onboardingCompletedAt: "2026-01-01T00:00:00Z", + measurementCount: 10, + dismissedAll: false, + items: doneItems(), + onboarding: { + ...state, + steps: state.steps.map((step) => + step.id === "first-result" + ? { ...step, status: "skipped" as const } + : step, + ), + }, + }), + ).toBe(false); + }); + + it("leaves an account that predates the flow on the rule it already had", () => { + // The payload publishes the field for every record, so "nine pending + // steps" must not read as an unfinished setup. + const untouched: OnboardingStateDto = { + steps: defaultOnboardingSteps(), + needs: emptyOnboardingNeeds(), + completedAt: null, + firstResult: null, + }; + expect( + shouldShowChecklist({ + onboardingCompletedAt: "2026-01-01T00:00:00Z", + measurementCount: 10, + dismissedAll: false, + items: buildChecklist(inputs({ measurementCount: 10 })), + onboarding: untouched, + }), + ).toBe(false); + }); +}); diff --git a/src/lib/onboarding/__tests__/needs.test.ts b/src/lib/onboarding/__tests__/needs.test.ts index 7af629791..02ad9b7d1 100644 --- a/src/lib/onboarding/__tests__/needs.test.ts +++ b/src/lib/onboarding/__tests__/needs.test.ts @@ -17,6 +17,7 @@ import { accountHoldsBothUnits, defaultOnboardingSteps, everyOnboardingQuestionSettled, + hasEnteredOnboardingFlow, ONBOARDING_QUESTION_STEP_IDS, emptyOnboardingNeeds, isOnboardingSettled, @@ -224,7 +225,8 @@ describe("applyOnboardingAnswer", () => { describe("isOnboardingSettled", () => { const settled: OnboardingStateDto = { steps: ONBOARDING_STEP_IDS.map((id) => ({ id, status: "done" as const })), - needs: emptyOnboardingNeeds(), + // Q1 answered: the mark of a record that actually entered the flow. + needs: { ...emptyOnboardingNeeds(), recordTarget: "me" }, completedAt: "2026-09-10T08:00:00.000Z", firstResult: { task: "log-reading", @@ -264,6 +266,81 @@ describe("isOnboardingSettled", () => { }), ).toBe(false); }); + + it("settles when the offered task was deliberately passed", () => { + // The task was offered — `firstResult` names it, with no completion — and + // the person passed. Asking only "did it produce anything" answers no + // forever, which pins the checklist open with no way out but dismissing + // rows one by one. + expect( + isOnboardingSettled({ + ...settled, + steps: settled.steps.map((s) => + s.id === "first-result" ? { ...s, status: "skipped" as const } : s, + ), + firstResult: { task: "log-reading", target: null, completedAt: null }, + }), + ).toBe(true); + }); + + it("settles a record that never entered the flow, however it is published", () => { + // The payload carries the field for EVERY record. Without this arm, an + // account that predates the flow reads as permanently unfinished. + expect( + isOnboardingSettled({ + steps: defaultOnboardingSteps(), + needs: emptyOnboardingNeeds(), + completedAt: null, + firstResult: null, + }), + ).toBe(true); + }); +}); + +describe("hasEnteredOnboardingFlow", () => { + const blankState: OnboardingStateDto = { + steps: defaultOnboardingSteps(), + needs: emptyOnboardingNeeds(), + completedAt: null, + firstResult: null, + }; + + it("is false for a record the flow never touched", () => { + expect(hasEnteredOnboardingFlow(null)).toBe(false); + expect(hasEnteredOnboardingFlow(blankState)).toBe(false); + }); + + it("is true once the one required question is answered", () => { + expect( + hasEnteredOnboardingFlow({ + ...blankState, + needs: { ...emptyOnboardingNeeds(), recordTarget: "someone-else" }, + }), + ).toBe(true); + }); + + it("is true for a flow that was completed", () => { + expect( + hasEnteredOnboardingFlow({ + ...blankState, + completedAt: "2026-09-10T08:00:00.000Z", + }), + ).toBe(true); + }); + + it("is not fooled by the units step resolving from the account's columns", () => { + // `resolveOnboardingSteps` marks `units` done for an account that already + // holds both preferences. That record has still never seen a question. + expect( + hasEnteredOnboardingFlow({ + ...blankState, + steps: resolveOnboardingSteps(defaultOnboardingSteps(), { + glucoseUnit: "mg/dL", + unitPreference: "metric", + }), + }), + ).toBe(false); + }); }); describe("the unit preferences the account already holds", () => { diff --git a/src/lib/onboarding/checklist.ts b/src/lib/onboarding/checklist.ts index 9ec9e4496..b67450e7d 100644 --- a/src/lib/onboarding/checklist.ts +++ b/src/lib/onboarding/checklist.ts @@ -7,6 +7,7 @@ */ import { + hasEnteredOnboardingFlow, isOnboardingSettled, type OnboardingNeeds, type OnboardingStateDto, @@ -208,12 +209,18 @@ export function checklistProgress(items: ChecklistItem[]): ChecklistProgress { /** * Should the dashboard render the checklist at all? * - * Visible while the user is still in the setup phase: - * - `onboardingCompletedAt` is null OR - * - they have fewer than 5 measurements + * Never when the whole list is dismissed, and never when there is no + * non-dismissed row left to show. * - * AND the user has not dismissed the entire checklist - * AND there is at least one undone, non-dismissed item. + * For a record that ran the setup flow the design spec retires the old + * five-reading rule: the list goes when its rows are done or the person hides + * it. Five readings were never evidence that anything was set up. A record + * that never entered the flow keeps the pre-v1.39 rule unchanged — first-run + * wizard unfinished, or fewer than five readings. + * + * A list whose rows are all done still stays while the flow itself is + * unfinished, because the one task the flow offered is not one of the six + * rows: "every row done" and "the setup finished" are different statements. */ export function shouldShowChecklist(args: { onboardingCompletedAt: string | null; @@ -221,25 +228,26 @@ export function shouldShowChecklist(args: { dismissedAll: boolean; items: ChecklistItem[]; /** - * v1.39 (C1) — the needs-based setup state. While it is unsettled — a step - * still pending, the flow never confirmed, or a first-result task offered - * and never produced — the checklist stays whatever the reading count says. - * That is the design spec's "it no longer disappears at five measurements": - * five readings are not evidence that the setup finished. - * - * Optional, and absence means nothing is outstanding: a record with no - * onboarding row has no unfinished flow to keep the list open for. + * v1.39 (C1) — the needs-based setup state for this record, or null before + * the account payload resolves. A record that never entered the flow reads + * as empty answers with nine pending steps and is treated as having no + * unfinished setup, which is what keeps every account that predates the flow + * on the rule it already had. */ onboarding?: OnboardingStateDto | null; }): boolean { if (args.dismissedAll) return false; - const stillInSetup = - args.onboardingCompletedAt == null || - args.measurementCount < 5 || - !isOnboardingSettled(args.onboarding); - if (!stillInSetup) return false; + const onboarding = args.onboarding ?? null; const visible = visibleChecklist(args.items); if (visible.length === 0) return false; + + const stillInSetup = + hasEnteredOnboardingFlow(onboarding) || + args.onboardingCompletedAt == null || + args.measurementCount < 5; + if (!stillInSetup) return false; + + if (!isOnboardingSettled(onboarding)) return true; return visible.some((item) => !item.done); } diff --git a/src/lib/onboarding/needs.ts b/src/lib/onboarding/needs.ts index 97a1282ef..c271088eb 100644 --- a/src/lib/onboarding/needs.ts +++ b/src/lib/onboarding/needs.ts @@ -334,6 +334,26 @@ export function everyOnboardingStepSettled( return steps.every((step) => step.status !== "pending"); } +/** + * Did this record ever enter the setup flow? + * + * Q1 is the one question with no skip arm, so an answer to it is the mark of + * having started, and the flow's own completion stamp covers a row whose + * answers a later release stopped recognising. + * + * Deliberately NOT "any step is not pending": `units` resolves to `done` from + * the account's own unit columns, so a record that never saw a question can + * carry a settled step. The published state is additive — every record has an + * `onboarding` field, and a record that predates the flow reads as empty + * answers with nine pending steps — so this is what tells the two apart. + */ +export function hasEnteredOnboardingFlow( + state: OnboardingStateDto | null | undefined, +): boolean { + if (!state) return false; + return state.completedAt !== null || state.needs.recordTarget !== null; +} + /** * Has the person finished setting up? * @@ -343,15 +363,26 @@ export function everyOnboardingStepSettled( * point of the whole flow — it ends on one completed task — so an offer with * no result is not a finished setup. * - * `null` (a record that never entered the flow) settles: there is no - * unfinished flow to keep anything open for. + * Two things settle it outright. `null`, and a record that never entered the + * flow: there is no unfinished setup to keep anything open for, and since the + * payload publishes the field for every record, without that arm every account + * that predates the flow would read as permanently unfinished. + * + * And a first-result step the person deliberately passed. The task is offered + * (`firstResult` records which one, with no completion) and then skipped; the + * offer is still on the row, so asking only "did the offered task produce + * anything" answers "no" forever. A deliberate pass is an answer, not an + * unfinished task. */ export function isOnboardingSettled( state: OnboardingStateDto | null | undefined, ): boolean { if (!state) return true; + if (!hasEnteredOnboardingFlow(state)) return true; if (!everyOnboardingStepSettled(state.steps)) return false; if (state.completedAt === null) return false; + const offered = state.steps.find((step) => step.id === "first-result"); + if (offered?.status === "skipped") return true; return state.firstResult === null || state.firstResult.completedAt !== null; }