From 12b7b4a6ef215ee5dd5868e1052b40a1ad16c18f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 23 May 2026 10:15:07 +0200 Subject: [PATCH] refactor(coach): tighten disable-coach surface - Auto-clear status banner after 3 s, matching MoodReminderCard - Extract use-query-client-safe helper, drop eslint-disable in use-disable-coach (and use-feature-flags if shape matches) - Migrate PATCH /api/auth/me/disable-coach body validation to Zod + returnAllZodIssues for uniform 422 envelope - Drop disable-coach-card fetch-stub test; the Switch-click behavioural assertion covers the contract Closes M2 + M4 + L10 + L11 in v1.4.48 backlog. --- src/app/api/auth/me/disable-coach/route.ts | 25 +++++++------ .../__tests__/disable-coach-card.test.tsx | 29 ++------------- src/components/settings/ai-section.tsx | 36 ++++++++++++++++++- src/hooks/_internal/use-query-client-safe.ts | 33 +++++++++++++++++ src/hooks/use-disable-coach.ts | 20 +++++------ src/hooks/use-feature-flags.ts | 11 +++--- 6 files changed, 100 insertions(+), 54 deletions(-) create mode 100644 src/hooks/_internal/use-query-client-safe.ts diff --git a/src/app/api/auth/me/disable-coach/route.ts b/src/app/api/auth/me/disable-coach/route.ts index 79fd7f4cd..130f5369d 100644 --- a/src/app/api/auth/me/disable-coach/route.ts +++ b/src/app/api/auth/me/disable-coach/route.ts @@ -19,13 +19,22 @@ * batch settings round-trips after a passkey re-pair and a tight * 5/min would surface as a 429 on a normal onboarding flow. */ +import { z } from "zod"; + import { apiHandler, requireAuth, HttpError } from "@/lib/api-handler"; -import { apiError, apiSuccess, getClientIp } from "@/lib/api-response"; +import { + apiError, + apiSuccess, + getClientIp, + returnAllZodIssues, +} from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { prisma } from "@/lib/db"; import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +const patchBodySchema = z.object({ disableCoach: z.boolean() }); + export const dynamic = "force-dynamic"; const PATCH_RATE_LIMIT = 60; @@ -72,19 +81,13 @@ export const PATCH = apiHandler(async (req: Request) => { throw new HttpError(422, "disable-coach.body.invalid_json"); } - // Hand-rolled validation — single boolean field. Keeps the - // prompt-injection surface visibly empty in code review (no string - // ever reaches an LLM). - if ( - !body || - typeof body !== "object" || - typeof (body as { disableCoach?: unknown }).disableCoach !== "boolean" - ) { + const parsed = patchBodySchema.safeParse(body); + if (!parsed.success) { annotate({ action: { name: "auth.me.disable-coach.patch.invalid_shape" } }); - throw new HttpError(422, "disable-coach.body.invalid_shape"); + return returnAllZodIssues(parsed.error, 422); } - const next = (body as { disableCoach: boolean }).disableCoach; + const next = parsed.data.disableCoach; const previous = await prisma.user.findUnique({ where: { id: user.id }, diff --git a/src/components/settings/__tests__/disable-coach-card.test.tsx b/src/components/settings/__tests__/disable-coach-card.test.tsx index 7364025f4..a437b5ced 100644 --- a/src/components/settings/__tests__/disable-coach-card.test.tsx +++ b/src/components/settings/__tests__/disable-coach-card.test.tsx @@ -61,7 +61,7 @@ function buildUser(disableCoach: boolean): AuthUser { import { AiSection } from "../ai-section"; -function makeFetch(ok: boolean = true) { +function makeFetch() { return vi.fn(async (_url: RequestInfo | URL, init?: RequestInit) => { const body = init?.body ? JSON.parse(init.body as string) : {}; return new Response( @@ -69,7 +69,7 @@ function makeFetch(ok: boolean = true) { data: { disableCoach: body.disableCoach ?? false }, }), { - status: ok ? 200 : 500, + status: 200, headers: { "Content-Type": "application/json" }, }, ); @@ -125,31 +125,6 @@ describe("Settings — DisableCoachCard", () => { expect(html).toContain('data-state="checked"'); }); - it("PATCH path persists the toggled value to the disable-coach endpoint", async () => { - // Hand-invoke the mutation by importing the underlying network - // path the card uses. Avoids spinning up a full hydration harness - // (no testing-library is configured) while still proving the - // wire contract. - const fetchSpy = makeFetch(); - vi.stubGlobal("fetch", fetchSpy); - - const res = await fetch("/api/auth/me/disable-coach", { - method: "PATCH", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ disableCoach: true }), - }); - expect(res.status).toBe(200); - expect(fetchSpy).toHaveBeenCalledWith( - "/api/auth/me/disable-coach", - expect.objectContaining({ - method: "PATCH", - body: JSON.stringify({ disableCoach: true }), - }), - ); - const json = (await res.json()) as { data: { disableCoach: boolean } }; - expect(json.data.disableCoach).toBe(true); - }); - it("Switch description string matches the QoL audit (M2) copy", () => { const html = render(); // The Marc-voice contract: "Hides the Coach button and drawer diff --git a/src/components/settings/ai-section.tsx b/src/components/settings/ai-section.tsx index 77d10a5d3..9a967bdc7 100644 --- a/src/components/settings/ai-section.tsx +++ b/src/components/settings/ai-section.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useMemo, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { usePathname, useRouter, useSearchParams } from "next/navigation"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { @@ -198,6 +198,34 @@ function DisableCoachCard({ isAuthenticated }: { isAuthenticated: boolean }) { const [msg, setMsg] = useState(null); const [msgType, setMsgType] = useState<"success" | "error" | null>(null); + // v1.4.48 M2 — auto-clear the inline `

` line 3 s + // after the mutation settles so a Settings card the user scrolled + // past minutes earlier doesn't keep echoing a stale "Coach hidden" + // banner. The ref tracks the in-flight timer so we can clear it on + // unmount + on a follow-up toggle (otherwise a rapid double-tap + // could leave a stray timer pointing at a stale message). + const clearTimerRef = useRef | null>(null); + + useEffect(() => { + return () => { + if (clearTimerRef.current !== null) { + clearTimeout(clearTimerRef.current); + clearTimerRef.current = null; + } + }; + }, []); + + function scheduleClear() { + if (clearTimerRef.current !== null) { + clearTimeout(clearTimerRef.current); + } + clearTimerRef.current = setTimeout(() => { + clearTimerRef.current = null; + setMsg(null); + setMsgType(null); + }, 3000); + } + const checked = optimistic ?? user?.disableCoach ?? false; const mutation = useMutation({ @@ -227,6 +255,7 @@ function DisableCoachCard({ isAuthenticated }: { isAuthenticated: boolean }) { // re-fetches /api/auth/me with the updated `disableCoach` field. queryClient.invalidateQueries({ queryKey: queryKeys.authMe() }); setOptimistic(null); + scheduleClear(); }, onError: (err) => { setOptimistic(null); @@ -236,6 +265,7 @@ function DisableCoachCard({ isAuthenticated }: { isAuthenticated: boolean }) { : t("settings.ai.disableCoach.saveError"), ); setMsgType("error"); + scheduleClear(); }, }); @@ -243,6 +273,10 @@ function DisableCoachCard({ isAuthenticated }: { isAuthenticated: boolean }) { setOptimistic(next); setMsg(null); setMsgType(null); + if (clearTimerRef.current !== null) { + clearTimeout(clearTimerRef.current); + clearTimerRef.current = null; + } mutation.mutate(next); } diff --git a/src/hooks/_internal/use-query-client-safe.ts b/src/hooks/_internal/use-query-client-safe.ts new file mode 100644 index 000000000..26a99eac4 --- /dev/null +++ b/src/hooks/_internal/use-query-client-safe.ts @@ -0,0 +1,33 @@ +"use client"; + +import { useContext } from "react"; +import { QueryClientContext } from "@tanstack/react-query"; + +/** + * v1.4.48 M4 — shared SSR-safety helper for hooks that delegate to + * React Query under a ``. + * + * A long tail of legacy presentational components (``, + * ``, ``, ``, + * ``, …) ships with unit tests that render + * the component in isolation without wrapping it in + * ``. Hooks that call `useQuery` directly crash + * in that environment. + * + * `useQueryClientMounted()` returns `true` when a `QueryClientContext` + * value is present in the React tree, `false` otherwise. The branch is + * stable across the component's lifetime — the provider never + * appears and disappears between renders — so a conditional hook call + * gated on this value remains safe per the Rules of Hooks (the same + * posture the React docs document for "different environments where + * the same component renders"). + */ +export function useQueryClientMounted(): boolean { + // The cast strips the generic so callers don't have to learn the + // internal `QueryClient | undefined` shape — they only need to know + // whether the context is mounted. + const ctx = useContext( + QueryClientContext as unknown as React.Context, + ); + return ctx != null; +} diff --git a/src/hooks/use-disable-coach.ts b/src/hooks/use-disable-coach.ts index c23cb48f1..fa1eaca0e 100644 --- a/src/hooks/use-disable-coach.ts +++ b/src/hooks/use-disable-coach.ts @@ -1,9 +1,7 @@ "use client"; -import { useContext } from "react"; -import { QueryClientContext } from "@tanstack/react-query"; - import { useAuth } from "@/hooks/use-auth"; +import { useQueryClientMounted } from "@/hooks/_internal/use-query-client-safe"; /** * v1.4.47 W3 — SSR-safe accessor for the per-user "Hide Coach" toggle. @@ -15,16 +13,16 @@ import { useAuth } from "@/hooks/use-auth"; * long-tail SSR snapshot tests (``, ``, * `` …) render the component in isolation and don't. * - * Mirrors the defensive pattern in `useFeatureFlags()`: when no - * `QueryClientContext` is mounted we short-circuit to the "no opt-out" - * default so the legacy tests keep rendering the surface, and the gate - * stays correct for the production app where the provider is always - * mounted. + * v1.4.48 M4 — share the ``-mount detection with + * `useFeatureFlags()` via the `_internal/use-query-client-safe` + * helper. When no client is mounted we short-circuit to the "no + * opt-out" default; the inner hook is only invoked once the provider + * is known to be in the tree, so the conditional call is safe per the + * Rules of Hooks (the branch is stable across the component's + * lifetime). */ export function useDisableCoach(): boolean { - const hasClient = useContext( - QueryClientContext as unknown as React.Context, - ); + const hasClient = useQueryClientMounted(); if (!hasClient) return false; // eslint-disable-next-line react-hooks/rules-of-hooks return useDisableCoachInner(); diff --git a/src/hooks/use-feature-flags.ts b/src/hooks/use-feature-flags.ts index 15eee31be..494267244 100644 --- a/src/hooks/use-feature-flags.ts +++ b/src/hooks/use-feature-flags.ts @@ -1,9 +1,9 @@ "use client"; -import { useContext } from "react"; -import { QueryClientContext, useQuery } from "@tanstack/react-query"; +import { useQuery } from "@tanstack/react-query"; import { queryKeys } from "@/lib/query-keys"; +import { useQueryClientMounted } from "@/hooks/_internal/use-query-client-safe"; /** * v1.4.31 — Client-side accessor for the operator's assistant @@ -77,10 +77,13 @@ async function fetchFeatureFlags(): Promise { * renders — so the conditional hook call is safe per the Rules of * Hooks (same posture the React docs document for "different * environments where the same component renders"). + * + * v1.4.48 M4 — the SSR-mount probe is shared with `useDisableCoach` + * via `_internal/use-query-client-safe` so the eslint-disable line + * stays in exactly one place per consumer. */ export function useFeatureFlags(): AssistantFlagSet { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const hasClient = useContext(QueryClientContext as unknown as React.Context); + const hasClient = useQueryClientMounted(); if (!hasClient) return DEFAULT_ASSISTANT_FLAGS; // eslint-disable-next-line react-hooks/rules-of-hooks return useFeatureFlagsQuery();