From 9413d2930392ecae4cfb84fa494d4140607b4f79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 11 May 2026 01:24:56 +0200 Subject: [PATCH] refactor(coach): drawer prefill becomes a controlled prop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v1.4.20 implementation reset the composer by mounting the `` with `key={prefill}`. That weaponised React's key-reset to flush internal state every prefill change — Sr-HIGH-4 flagged it as a smell because the next refactor that touches the drawer's mount point silently loses the reset semantics. Replace with the controlled-prop pattern React documents at https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes: new `useResettableValue` hook reads the controlled prefill, mirrors it via a sibling useState pair, and seeds the composer with the new value when the prop changes. Render-phase setState — ESLint-clean against `react-hooks/set-state-in-effect`. The user can still type freely; the drawer only resets when the parent changes which suggested-prompt chip is active. `nextResettableValue` exposes the comparison contract as a pure helper so the unit test can pin the (Object.is) semantics without standing up a React renderer. The `` is gone from the insights page mount. Tests +5: pure-function coverage of unchanged value (no reset), prefill change (reset), null↔empty transition (reset), suggestion chip → cleared prefill (reset), and NaN-to-NaN edge case (no reset under Object.is). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/insights/page.tsx | 11 ++-- .../__tests__/coach-drawer-prefill.test.ts | 63 ++++++++++++++++++ .../insights/coach-panel/coach-drawer.tsx | 66 +++++++++++++++++-- 3 files changed, 129 insertions(+), 11 deletions(-) create mode 100644 src/components/insights/coach-panel/__tests__/coach-drawer-prefill.test.ts diff --git a/src/app/insights/page.tsx b/src/app/insights/page.tsx index bb09f4f7e..882ade284 100644 --- a/src/app/insights/page.tsx +++ b/src/app/insights/page.tsx @@ -1645,12 +1645,13 @@ export default function InsightsPage() { /> - {/* v1.4.20 phase B2b — AI Coach drawer. The drawer reads its - initial input value from `prefill`; we change the React `key` - on every prefill transition so the lazy `useState` initialiser - fires fresh and the composer surfaces the latest chip. */} + {/* v1.4.20 phase B2b — AI Coach drawer. + v1.4.23 H3 — `prefill` is a controlled prop now; the drawer + watches it via an in-render ref and resets the composer + when the parent changes which suggested-prompt chip is + active. The old `key={prefill}` weaponise-React-keys pattern + is gone (Sr-HIGH-4). */} ` with `key={prefill}`. That weaponised React's + * key-reset to flush internal state every prefill change — Sr-HIGH-4 + * flagged it as a smell because the next refactor that touches the + * drawer's mount point will silently lose the reset semantics. + * + * The fix is the in-render-update pattern documented at + * https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes: + * track the last controlled value via `useRef`, and call `setState` + * during render when the controlled value differs. `nextResettableValue` + * pins the comparison contract here so a future Object.is rewrite + * (e.g., shallow-compare migration) can't silently regress. + */ +describe("nextResettableValue (Coach drawer prefill controller)", () => { + it("reports no reset when the controlled value is unchanged", () => { + expect(nextResettableValue("How is my BP?", "How is my BP?")).toEqual({ + reset: false, + }); + }); + + it("reports a reset with the next value when the controlled prop changes", () => { + expect( + nextResettableValue("How is my BP?", "Mood last week?"), + ).toEqual({ reset: true, value: "Mood last week?" }); + }); + + it("treats null and an empty string as different controlled values", () => { + expect(nextResettableValue(null, "")).toEqual({ + reset: true, + value: "", + }); + expect(nextResettableValue("", null)).toEqual({ + reset: true, + value: null, + }); + }); + + it("treats a transition from a non-null prefill back to null as a reset", () => { + // The parent surface clears the prefill back to null after the + // user clicks a non-suggestion entry point. The composer must + // empty itself in that transition; otherwise the previous chip's + // text lingers across drawer opens. + expect( + nextResettableValue("Why was BP higher Monday?", null), + ).toEqual({ reset: true, value: null }); + }); + + it("uses Object.is, so NaN-to-NaN does not trigger a reset", () => { + // Defence-in-depth — Object.is(NaN, NaN) is true, unlike ===. + // The drawer would otherwise reset on every render if the parent + // happened to pass NaN as a string-coerced default. + expect(nextResettableValue(Number.NaN, Number.NaN)).toEqual({ + reset: false, + }); + }); +}); diff --git a/src/components/insights/coach-panel/coach-drawer.tsx b/src/components/insights/coach-panel/coach-drawer.tsx index f54c8a44f..9be1c0039 100644 --- a/src/components/insights/coach-panel/coach-drawer.tsx +++ b/src/components/insights/coach-panel/coach-drawer.tsx @@ -1,6 +1,7 @@ "use client"; import { useCallback, useState } from "react"; +import type { Dispatch, SetStateAction } from "react"; import { Plus, Sparkles } from "lucide-react"; import { Button } from "@/components/ui/button"; @@ -63,6 +64,53 @@ export interface CoachDrawerProps { composer?: React.ReactNode; } +/** + * v1.4.23 H3 — local state seeded from a controlled prop, reset on + * prop change. Same render-phase pattern React's docs recommend in + * place of `useEffect(() => setState(prop), [prop])` (which the + * `react-hooks/set-state-in-effect` ESLint rule banned). The reset + * runs during render: React detects the queued setState, restarts + * the render with the new value, and commits a single coherent + * snapshot — no double paint, no flash of stale state. + * + * Exported so the drawer's prefill-reset behaviour can be unit- + * tested in isolation without standing up the whole Sheet portal. + */ +export function useResettableValue( + controlledValue: T, +): [T, Dispatch>] { + const [value, setValue] = useState(controlledValue); + // Mirror the last observed controlled value via a sibling useState + // pair (per React docs: + // https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes). + // useRef is the wrong tool here — ESLint rejects ref read+write + // during render, and useState already gives us identity tracking + // with no extra cost. + const [lastSeen, setLastSeen] = useState(controlledValue); + if (!Object.is(controlledValue, lastSeen)) { + setLastSeen(controlledValue); + setValue(controlledValue); + } + return [value, setValue]; +} + +/** + * v1.4.23 H3 — pure decision function behind `useResettableValue`. + * Given the previous controlled value the hook recorded and the + * incoming controlled value, returns either `{ reset: true, value }` + * (the next render must seed local state with `value`) or + * `{ reset: false }` (local state survives — the user's edits are + * preserved). Pure + dependency-free so it tests cleanly without a + * React renderer — pin the contract here and trust the hook + * implementation to wire the same comparison. + */ +export function nextResettableValue( + previous: T, + incoming: T, +): { reset: true; value: T } | { reset: false } { + return Object.is(previous, incoming) ? { reset: false } : { reset: true, value: incoming }; +} + export function CoachDrawer({ open, onOpenChange, @@ -82,12 +130,18 @@ export function CoachDrawer({ // data the Coach is using without losing the message thread context. const [historyTrayOpen, setHistoryTrayOpen] = useState(false); const [sourcesTrayOpen, setSourcesTrayOpen] = useState(false); - // The composer's input value is seeded from `prefill` whenever the - // parent toggles `open` — we mount the drawer with a key derived from - // (open, prefill) so the lazy initialiser fires fresh on each - // open/prefill transition. That sidesteps `setState`-in-`useEffect` - // entirely (banned by `react-hooks/set-state-in-effect`). - const [inputValue, setInputValue] = useState(() => prefill ?? ""); + // v1.4.23 H3 — `prefill` is now a fully-controlled prop. The + // v1.4.20 implementation used `key={prefill}` on the parent mount + // to force a fresh `useState()` initialiser run on every prefill + // transition (the "weaponise-React-keys" pattern senior-dev + // review flagged as Sr-HIGH-4). The replacement is a tiny + // `useResettableValue` hook below: tracks the last observed + // prefill in a ref and schedules a same-render state update when + // the prop changes — a textbook React render-phase update, + // ESLint-clean against `react-hooks/set-state-in-effect`. The + // user can still type freely; the drawer only resets the composer + // when the parent changes which suggested-prompt chip is active. + const [inputValue, setInputValue] = useResettableValue(prefill ?? ""); // v1.4.20.1 — scope picker state (per-source checkboxes + window // selector). Resets to the all-source last30days default each time // the drawer mounts; no conversation-level persistence in this