From baec7101381bf80ddb92c2715d83bbd3b04041ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 12 Jun 2026 19:07:53 +0200 Subject: [PATCH] fix(coach): the launcher stops blinking out on chart hovers and sits evenly in the corner The chart-tooltip auto-hide made the button vanish on every hover tooltip anywhere on the page, which read as a glitch; a tooltip under the cursor never reaches the lower-right corner anyway. The observer chain behind it goes with it. The right inset now mirrors the desktop bottom inset. --- .../__tests__/chart-tooltip-observer.test.ts | 70 ------- .../__tests__/layout-coach-fab.test.tsx | 5 +- .../insights/chart-tooltip-observer.ts | 181 ------------------ src/components/insights/layout-coach-fab.tsx | 31 +-- .../insights/use-chart-tooltip-active.ts | 27 --- 5 files changed, 11 insertions(+), 303 deletions(-) delete mode 100644 src/components/insights/__tests__/chart-tooltip-observer.test.ts delete mode 100644 src/components/insights/chart-tooltip-observer.ts delete mode 100644 src/components/insights/use-chart-tooltip-active.ts diff --git a/src/components/insights/__tests__/chart-tooltip-observer.test.ts b/src/components/insights/__tests__/chart-tooltip-observer.test.ts deleted file mode 100644 index 4a1bd3793..000000000 --- a/src/components/insights/__tests__/chart-tooltip-observer.test.ts +++ /dev/null @@ -1,70 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it } from "vitest"; - -import { - __resetChartTooltipObserverForTests, - getChartTooltipActive, - getChartTooltipActiveServer, - subscribeChartTooltipActive, -} from "../chart-tooltip-observer"; - -/** - * v1.4.33 (F15) — chart-tooltip observer contract tests. - * - * The full DOM-side behaviour (MutationObserver watching `[class*= - * "recharts-tooltip-wrapper"]` visibility flips) lives behind a guard - * that no-ops when `document` is undefined — which is the case in the - * default vitest node environment. These tests pin the SSR-safe path - * and the subscribe/snapshot contract that `useSyncExternalStore` - * leans on. - * - * The integration with a real Recharts tooltip is exercised via the - * Playwright suite (mobile insight chart hover → FAB fades). - */ - -describe("chart-tooltip-observer", () => { - beforeEach(() => { - __resetChartTooltipObserverForTests(); - }); - - afterEach(() => { - __resetChartTooltipObserverForTests(); - }); - - it("getChartTooltipActiveServer always returns false", () => { - // The SSR snapshot must never claim a tooltip is active on the - // server — there's no DOM to inspect, and a stale `true` would - // render the FAB invisible on the initial pass. - expect(getChartTooltipActiveServer()).toBe(false); - }); - - it("getChartTooltipActive returns false when no wrappers exist", () => { - expect(getChartTooltipActive()).toBe(false); - }); - - it("subscribeChartTooltipActive returns an unsubscribe function", () => { - const unsubscribe = subscribeChartTooltipActive(() => undefined); - expect(typeof unsubscribe).toBe("function"); - unsubscribe(); - }); - - it("no-ops cleanly on a server-side render (no document available)", () => { - // Document is undefined in the vitest node environment — the - // module's `start()` early-returns. Subscribing should still - // succeed; the listener simply never fires. - let calls = 0; - const unsubscribe = subscribeChartTooltipActive(() => { - calls += 1; - }); - expect(getChartTooltipActive()).toBe(false); - expect(calls).toBe(0); - unsubscribe(); - }); - - it("supports multiple subscribers without throwing", () => { - const a = subscribeChartTooltipActive(() => undefined); - const b = subscribeChartTooltipActive(() => undefined); - expect(getChartTooltipActive()).toBe(false); - a(); - b(); - }); -}); diff --git a/src/components/insights/__tests__/layout-coach-fab.test.tsx b/src/components/insights/__tests__/layout-coach-fab.test.tsx index 445b7c909..f2f545df5 100644 --- a/src/components/insights/__tests__/layout-coach-fab.test.tsx +++ b/src/components/insights/__tests__/layout-coach-fab.test.tsx @@ -58,8 +58,9 @@ describe("", () => { // Fixed bottom-right, above the mobile bottom-nav band. The plain // desktop offset kicks in at `md:` — the same breakpoint where the // bottom-nav hides (`md:hidden`) — so the FAB never floats mid-air - // in the 768-1023px band. - expect(html).toContain("fixed right-4"); + // in the 768-1023px band. The right inset mirrors the desktop + // bottom inset so the corner gap is even. + expect(html).toContain("fixed right-6"); expect(html).toContain( "bottom-[calc(env(safe-area-inset-bottom,0px)+5rem)]", ); diff --git a/src/components/insights/chart-tooltip-observer.ts b/src/components/insights/chart-tooltip-observer.ts deleted file mode 100644 index 18f3edc89..000000000 --- a/src/components/insights/chart-tooltip-observer.ts +++ /dev/null @@ -1,181 +0,0 @@ -/** - * v1.4.33 (F15) — chart-tooltip active observer. - * - * The mobile Coach FAB at `bottom-right` of every routed `/insights` - * sub-page used to overlay the Recharts tooltip bubble when a user - * tapped a data point near the bottom-right of the chart. We can't - * route a `tooltipActive` boolean directly from the chart to the FAB - * because the chart sits inside the routed page subtree while the FAB - * is mounted at the layout level — and the chart component itself is - * out of this work-item's touch surface. - * - * Instead, listen at the DOM level. Recharts paints every active - * tooltip into a div whose class always carries `recharts-tooltip- - * wrapper`. The `` upstream component flips the - * inline `visibility` from `hidden` (idle) to `visible` (cursor over - * a point or long-press on touch). A single shared `MutationObserver` - * watches every such wrapper for `style`-attribute changes and feeds - * a tiny external store; consumers subscribe via `useSyncExternalStore` - * to avoid prop-drilling or react-context coupling between unrelated - * subtrees. - * - * The store is module-scoped so the listener stays a singleton — the - * observer connects on the first subscriber and disconnects on the - * last unmount. SSR-safe: every browser API touch is gated on - * `typeof document`. - */ - -let activeCount = 0; -const listeners = new Set<() => void>(); - -let observer: MutationObserver | null = null; -let listening = false; - -// Track wrappers we've already seen so we don't double-count their -// visibility transitions. A `WeakSet` lets the GC reclaim removed -// wrapper elements naturally. -const knownWrappers = new WeakSet(); - -function isVisible(wrapper: HTMLElement): boolean { - // Recharts surfaces `visibility: visible` via inline style when the - // tooltip should paint, and clears it otherwise. We mirror that exact - // contract — `getComputedStyle()` is not required because the inline - // style is always set. - return wrapper.style.visibility === "visible"; -} - -function emit() { - for (const fn of listeners) fn(); -} - -function syncWrapperState(wrapper: HTMLElement, nextVisible: boolean) { - const flagged = wrapper.dataset.coachTooltipActive === "1"; - if (nextVisible && !flagged) { - wrapper.dataset.coachTooltipActive = "1"; - activeCount += 1; - emit(); - } else if (!nextVisible && flagged) { - delete wrapper.dataset.coachTooltipActive; - activeCount = Math.max(0, activeCount - 1); - emit(); - } -} - -function adoptWrapper(wrapper: HTMLElement) { - if (knownWrappers.has(wrapper)) return; - knownWrappers.add(wrapper); - // Seed the active-count with whatever state the wrapper carried on - // first sight so a tooltip already painted before our observer - // attached still counts. - syncWrapperState(wrapper, isVisible(wrapper)); -} - -function scanForWrappers(root: ParentNode) { - // `querySelectorAll` returns every existing wrapper inside the - // mutated subtree on every mutation — cheap because the DOM - // typically holds 0–1 wrappers at a time. - const wrappers = root.querySelectorAll( - '[class*="recharts-tooltip-wrapper"]', - ); - for (const wrapper of wrappers) adoptWrapper(wrapper); -} - -function handleMutations(mutations: MutationRecord[]) { - for (const mutation of mutations) { - if (mutation.type === "childList") { - for (const node of mutation.addedNodes) { - if (!(node instanceof HTMLElement)) continue; - if (node.matches?.('[class*="recharts-tooltip-wrapper"]')) { - adoptWrapper(node); - } else { - scanForWrappers(node); - } - } - for (const node of mutation.removedNodes) { - if (!(node instanceof HTMLElement)) continue; - // Removed wrappers cannot contribute to active count anymore; - // drop the flag so a re-added wrapper re-counts cleanly. - if (node.dataset?.coachTooltipActive === "1") { - delete node.dataset.coachTooltipActive; - activeCount = Math.max(0, activeCount - 1); - emit(); - } - } - } else if (mutation.type === "attributes" && mutation.target instanceof HTMLElement) { - const target = mutation.target; - if (target.matches('[class*="recharts-tooltip-wrapper"]')) { - syncWrapperState(target, isVisible(target)); - } - } - } -} - -function start() { - if (listening) return; - if (typeof document === "undefined") return; - listening = true; - observer = new MutationObserver(handleMutations); - observer.observe(document.body, { - subtree: true, - childList: true, - attributes: true, - attributeFilter: ["style"], - }); - // Seed with wrappers that already exist in the DOM at subscribe-time. - scanForWrappers(document.body); -} - -function stop() { - if (!listening) return; - listening = false; - observer?.disconnect(); - observer = null; - // Reset the counter — re-seeding on the next subscriber re-scans - // the DOM anyway. - activeCount = 0; -} - -/** - * Subscribe-API used by `useSyncExternalStore`. Connects the singleton - * observer on the first subscriber and disconnects on the last. - */ -export function subscribeChartTooltipActive(listener: () => void): () => void { - listeners.add(listener); - if (listeners.size === 1) start(); - return () => { - listeners.delete(listener); - if (listeners.size === 0) stop(); - }; -} - -/** - * Snapshot getter for `useSyncExternalStore`. Returns `true` while at - * least one Recharts tooltip wrapper is currently visible. - */ -export function getChartTooltipActive(): boolean { - return activeCount > 0; -} - -/** - * SSR snapshot — no chart tooltip can be active on the server, so we - * always return `false`. Keeps the hook safe on the initial render - * before hydration. - */ -export function getChartTooltipActiveServer(): boolean { - return false; -} - -/** - * Test-only reset hook. Vitest tears the DOM down between tests but - * the module-scoped counter survives the teardown; exposing a reset - * keeps cross-test state from bleeding. - */ -export function __resetChartTooltipObserverForTests(): void { - activeCount = 0; - listeners.clear(); - if (observer) { - observer.disconnect(); - observer = null; - } - listening = false; -} diff --git a/src/components/insights/layout-coach-fab.tsx b/src/components/insights/layout-coach-fab.tsx index 0ce143008..bfade4a0d 100644 --- a/src/components/insights/layout-coach-fab.tsx +++ b/src/components/insights/layout-coach-fab.tsx @@ -13,7 +13,6 @@ import { useFeatureFlags } from "@/hooks/use-feature-flags"; import { useDisableCoach } from "@/hooks/use-disable-coach"; import { queryKeys } from "@/lib/query-keys"; -import { useChartTooltipActive } from "./use-chart-tooltip-active"; import { apiGet } from "@/lib/api/api-fetch"; /** @@ -30,10 +29,11 @@ import { apiGet } from "@/lib/api/api-fetch"; * The FAB hides inside the Coach view itself (`/insights/coach`) — a * launcher pointing at the page the user is already on is noise. * - * v1.4.33 (F15) — keeps the chart-tooltip auto-hide so it never - * overlays a Recharts tooltip on the lower right; it also yields while - * a data-list selection bar is mounted (CSS `:has()` gate) so it never - * covers the bar's delete action. + * It yields while a data-list selection bar is mounted (CSS `:has()` + * gate) so it never covers the bar's delete action. The v1.4.33 + * chart-tooltip auto-hide is gone: blinking out on every chart hover + * read as a glitch, and a hover tooltip under the cursor never reaches + * the lower-right corner anyway. */ const NUDGE_SEEN_STORAGE_KEY = "healthlog-coach-nudge-seen"; @@ -63,7 +63,6 @@ export function LayoutCoachFab() { const launch = useCoachLaunch(); const flags = useFeatureFlags(); const disableCoach = useDisableCoach(); - const tooltipActive = useChartTooltipActive(); // Local "seen on this device" stamp — the nudge timestamp the user // last dismissed by opening the chat. Lazy initialiser per the @@ -109,16 +108,6 @@ export function LayoutCoachFab() { prevUnreadRef.current = unread; }, [unread, t]); - // While a chart tooltip hides the FAB it is `aria-hidden` + - // `tabIndex={-1}` — but an element that ALREADY holds focus keeps it. - // Drop the focus so a hidden control is never the active element. - const buttonRef = useRef(null); - useEffect(() => { - if (tooltipActive && document.activeElement === buttonRef.current) { - buttonRef.current?.blur(); - } - }, [tooltipActive]); - // Visiting the Coach page itself counts as reading the nudge on this // device — persist the stamp so the dot stays gone after leaving. useEffect(() => { @@ -154,23 +143,21 @@ export function LayoutCoachFab() { return ( <>