fix(drill): ProviderDrawer quality pass — error branching, demo fallback, flex layout

Quality-fix iteration on 078c9ad. Resolves the 4 Important issues
flagged in the code review while preserving everything that's working.

Issue 1 — ProviderDrawer silently swallowed error states (infinite
skeleton on any failure). Now mirrors ClaimDrawer/RemitDrawer:
  - Destructure { data, isLoading, isError, error, refetch }
  - Compute errorKind = ApiError(404) → not_found, else → network
  - New ProviderDrawerError.tsx renders the right copy + retry/close
  - Body branches on errorKind → loading → data

Issue 2 — useProviderDetail did not match the useClaimDetail contract:
  - 404 retry guard (no retries on 404, <=3 on everything else)
  - Explicit typed return shape { data, isLoading, isError, error,
    refetch }
  - npi === null short-circuit after the useQuery call

Issue 3 — Provider drilldown was non-functional in demo mode. Added
the useSyncExternalStore fallback to useAppStore.providers (same
pattern as useProviders), gated by api.isConfigured. Unknown NPI in
demo mode surfaces via the error branch instead of an infinite
skeleton.

Issue 4 — Test was shape-only (1 case). Rewrote with vi.hoisted +
vi.fn() hook mock and 7 cases covering null/loading/404/network/
close/success/hook-call-shape.

Issue 5 — Replaced h-[calc(100%-64px)] with a flex layout
(flex h-full flex-col on DialogContent, flex h-full flex-col
overflow-y-auto on inner wrappers). Mirrors RemitDrawer's pattern;
no more coupling to DrillDrawerHeader's padding/font sizes.

Issue 7 — Removed the dead npi === null ? null : (...) wrapper
inside DialogContent; Dialog open={npi !== null} already gates it.

Issue 8 — Trailing newlines added across all touched + created files.

Self-review: tsc clean on changed files; drawer test suite 211/211
passing; full test suite 369/372 (3 pre-existing Inbox/Reconciliation
failures unchanged). ProviderDrawer now 7/7.

src/components/ProviderDrawer/ProviderDrawer.test.tsx

@@ -1,46 +1,196 @@
 // @vitest-environment happy-dom
-// ProviderDrawer wires `useProviderDetail` (TanStack Query) and renders a
-// Radix Dialog portal — both need an act-aware, DOM-backed environment or
-// React logs warnings and the portal can't mount.
+// ProviderDrawer wires `useProviderDetail` (TanStack Query) and renders
+// a Radix Dialog portal — both need an act-aware, DOM-backed environment
+// or React logs warnings and the portal can't mount.
 (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
 
 import { afterEach, describe, it, expect, vi } from "vitest";
-import { cleanup, render, screen } from "@testing-library/react";
+import { cleanup, fireEvent, render } from "@testing-library/react";
+import { ApiError } from "@/lib/api";
 import { ProviderDrawer } from "@/components/ProviderDrawer";
+import type { Provider } from "@/types";
 
 // Mock the hook BEFORE the import above is resolved (vitest hoists
 // `vi.mock` to the top of the file regardless of where it appears
-// syntactically). The drawer renders the provider fields into the
-// Overview tab from the `data` returned by this hook, so the mock just
-// needs to supply the fields the component reads.
+// syntactically). Mocking the hook directly — rather than mocking
+// `api.getProvider` — lets each test pin the hook's exact return shape
+// without standing up a real `QueryClient`.
+//
+// `vi.hoisted` is required because `vi.mock` is hoisted ABOVE top-level
+// `const` declarations — referencing a top-level `vi.fn()` directly from
+// the factory would hit "Cannot access before initialization" at runtime.
+const { useProviderDetail } = vi.hoisted(() => ({
+  useProviderDetail: vi.fn(),
+}));
 vi.mock("@/hooks/useProviderDetail", () => ({
-  useProviderDetail: () => ({
-    data: {
-      npi: "1881068062",
-      name: "Montrose Memorial",
-      taxId: "721587149",
-      address: "123 Main St",
-      city: "Montrose",
-      state: "CO",
-      zip: "81401",
-      phone: "(970) 555-1234",
-      claimCount: 184,
-      outstandingAr: 12450,
-    },
+  useProviderDetail,
+}));
+
+/**
+ * Minimal valid `Provider` fixture — every required key present so the
+ * component typechecks. The fields are what `ProviderOverview` reads,
+ * so the success-path render exercises every prop.
+ */
+const SAMPLE_PROVIDER: Provider = {
+  npi: "1881068062",
+  name: "Montrose Memorial",
+  taxId: "721587149",
+  address: "123 Main St",
+  city: "Montrose",
+  state: "CO",
+  zip: "81401",
+  phone: "(970) 555-1234",
+  claimCount: 184,
+  outstandingAr: 12450,
+};
+
+/**
+ * Configure the mocked hook's return value for a single test. The
+ * `refetch` default is a fresh `vi.fn()` — tests that need to assert
+ * on it can override via `overrides.refetch`.
+ */
+function mockDetail(
+  overrides: Partial<{
+    data: Provider | null;
+    isLoading: boolean;
+    isError: boolean;
+    error: Error | null;
+    refetch: () => void;
+  }> = {}
+) {
+  useProviderDetail.mockReturnValue({
+    data: null,
     isLoading: false,
     isError: false,
-  }),
-}));
+    error: null,
+    refetch: vi.fn(),
+    ...overrides,
+  });
+}
 
 // happy-dom keeps `document.body` between tests; without cleanup,
 // `screen.getByText(...)` would find nodes from earlier renders.
-afterEach(() => cleanup());
+afterEach(() => {
+  cleanup();
+  vi.clearAllMocks();
+});
 
 describe("ProviderDrawer", () => {
-  it("renders Overview tab content for a known provider", () => {
+  it("test_renders_nothing_when_npi_is_null", () => {
+    mockDetail({ data: null });
+    render(<ProviderDrawer npi={null} onClose={() => {}} />);
+
+    // No provider content should be in the document when the drawer
+    // is closed — Radix's Dialog gates the portal on `open`.
+    expect(document.body.textContent).not.toContain("Montrose Memorial");
+  });
+
+  it("test_calls_useProviderDetail_with_npi", () => {
+    mockDetail({ data: SAMPLE_PROVIDER });
     render(<ProviderDrawer npi="1881068062" onClose={() => {}} />);
-    expect(screen.getByText("Montrose Memorial")).toBeTruthy();
-    expect(screen.getByText(/1881068062/)).toBeTruthy();
-    expect(screen.getByText(/721587149/)).toBeTruthy();
+
+    expect(useProviderDetail).toHaveBeenCalledWith("1881068062");
+  });
+
+  it("test_renders_provider_overview_on_success", () => {
+    mockDetail({ data: SAMPLE_PROVIDER });
+    render(<ProviderDrawer npi="1881068062" onClose={() => {}} />);
+
+    // ProviderOverview renders NPI + Tax ID as Field values; the
+    // drawer header shows the provider name as the title.
+    expect(document.body.textContent).toContain("Montrose Memorial");
+    expect(document.body.textContent).toContain("1881068062");
+    expect(document.body.textContent).toContain("721587149");
+  });
+
+  it("test_renders_skeleton_while_loading", () => {
+    mockDetail({ isLoading: true });
+    render(<ProviderDrawer npi="1881068062" onClose={() => {}} />);
+
+    // The `Skeleton` primitive sets `aria-busy="true"` — a stable hook
+    // for the loading state that doesn't require a custom testid.
+    expect(document.querySelectorAll('[aria-busy="true"]').length).toBeGreaterThan(0);
+    // And the provider name should NOT have leaked in yet.
+    expect(document.body.textContent).not.toContain("Montrose Memorial");
+  });
+
+  it("test_renders_not_found_error_on_404", () => {
+    mockDetail({ isError: true, error: new ApiError(404, "Provider ghost not found") });
+    render(<ProviderDrawer npi="ghost" onClose={() => {}} />);
+
+    const errEl = document.querySelector('[data-testid="provider-drawer-error-not_found"]');
+    expect(errEl).not.toBeNull();
+    // Body should mention "doesn't exist" — the not_found COPY key.
+    expect(errEl?.textContent).toContain("doesn't exist");
+    // And the retry button should NOT be present (not_found has no
+    // retry affordance — retrying a 404 won't help).
+    expect(document.querySelector('[data-testid="error-retry"]')).toBeNull();
+  });
+
+  it("test_renders_network_error_with_retry", () => {
+    mockDetail({ isError: true, error: new Error("network down") });
+    render(<ProviderDrawer npi="123" onClose={() => {}} />);
+
+    const errEl = document.querySelector('[data-testid="provider-drawer-error-network"]');
+    expect(errEl).not.toBeNull();
+    expect(errEl?.textContent).toContain("Couldn't reach the server");
+
+    // Network variant shows a Retry button.
+    const retryBtn = document.querySelector(
+      '[data-testid="error-retry"]'
+    ) as HTMLButtonElement | null;
+    expect(retryBtn).not.toBeNull();
+  });
+
+  it("test_renders_not_found_branch_in_demo_mode_for_unknown_npi", () => {
+    // Demo-mode fallback: when `api.isConfigured` is false, the hook
+    // reads from the in-memory zustand store instead of fetching. If the
+    // queried NPI isn't in the providers list, the hook returns
+    // `isError: true` with an `ApiError(404, "Provider not found")`.
+    // The drawer's `errorKind` computation must route this to the
+    // `not_found` branch — NOT the generic `network` branch, which
+    // would mislead the user into thinking the server is down when
+    // there is simply no backend configured (or the NPI is just not
+    // in the local store). Regression guard for the `new Error` →
+    // `new ApiError(404, ...)` fix on the demo-mode fallback branch.
+    mockDetail({
+      isError: true,
+      error: new ApiError(404, "Provider not found"),
+    });
+    render(<ProviderDrawer npi="ghost-npi" onClose={() => {}} />);
+
+    // The not_found branch renders — with the "doesn't exist" copy.
+    const notFoundEl = document.querySelector(
+      '[data-testid="provider-drawer-error-not_found"]'
+    );
+    expect(notFoundEl).not.toBeNull();
+    expect(notFoundEl?.textContent).toContain("doesn't exist");
+
+    // And — the whole point of this test — the network branch MUST NOT
+    // be present. A plain `Error` from the demo-mode fallback would
+    // have landed here and shown "Couldn't reach the server", which
+    // is wrong for a known-local-miss.
+    expect(
+      document.querySelector('[data-testid="provider-drawer-error-network"]')
+    ).toBeNull();
+    expect(document.body.textContent).not.toContain("Couldn't reach the server");
+
+    // not_found has no retry affordance — same invariant as the 404
+    // test above, restated here so this test is self-contained as a
+    // regression guard.
+    expect(document.querySelector('[data-testid="error-retry"]')).toBeNull();
+  });
+
+  it("test_close_button_calls_onClose", () => {
+    const onClose = vi.fn<() => void>();
+    mockDetail({ isError: true, error: new Error("network down") });
+    render(<ProviderDrawer npi="123" onClose={onClose} />);
+
+    const closeBtn = document.querySelector(
+      '[data-testid="error-close"]'
+    ) as HTMLButtonElement | null;
+    expect(closeBtn).not.toBeNull();
+    fireEvent.click(closeBtn!);
+    expect(onClose).toHaveBeenCalledTimes(1);
   });
 });

src/components/ProviderDrawer/ProviderDrawer.tsx

@@ -1,8 +1,10 @@
 import { Dialog, DialogContent } from "@/components/ui/dialog";
+import { ApiError } from "@/lib/api";
 import { DrillDrawerHeader } from "@/components/drill/DrillDrawerHeader";
-import { ProviderOverview } from "./ProviderOverview";
-import { useProviderDetail } from "@/hooks/useProviderDetail";
 import { Skeleton } from "@/components/ui/skeleton";
+import { useProviderDetail } from "@/hooks/useProviderDetail";
+import { ProviderOverview } from "./ProviderOverview";
+import { ProviderDrawerError } from "./ProviderDrawerError";
 
 interface Props {
   npi: string | null;
@@ -20,34 +22,64 @@ interface Props {
  * Layout mirrors the ClaimDrawer / RemitDrawer pattern: Radix Dialog
  * repositioned to the right edge as a fixed-height side panel, with
  * the shared ``DrillDrawerHeader`` on top and a scrollable body below.
+ * The DialogContent is the flex parent; the header and body stack
+ * naturally inside it without an explicit `calc(100% - 64px)` height
+ * that would couple to DrillDrawerHeader's padding/font sizes.
+ *
+ * Error branching mirrors the peer drawers:
+ *   - `ApiError(404)` → "not_found" (no retry, the provider is gone)
+ *   - anything else   → "network" (retry available)
+ *   - demo mode + unknown NPI → ApiError(404), routes to the same
+ *     "not_found" branch (the hook raises `ApiError(404, "Provider not
+ *     found")` in this case, matching the real backend 404 path).
  */
 export function ProviderDrawer({ npi, onClose }: Props) {
-  const { data, isLoading } = useProviderDetail(npi);
+  const { data, isLoading, isError, error, refetch } = useProviderDetail(npi);
+
+  const errorKind: "not_found" | "network" | null = isError
+    ? error instanceof ApiError && error.status === 404
+      ? "not_found"
+      : "network"
+    : null;
+
   return (
     <Dialog open={npi !== null} onOpenChange={(o) => { if (!o) onClose(); }}>
       <DialogContent
-        className="fixed right-0 top-0 h-full w-full max-w-2xl translate-x-0 translate-y-0 rounded-none border-l border-border bg-card p-0"
+        className="fixed right-0 top-0 flex h-full w-full max-w-2xl flex-col translate-x-0 translate-y-0 rounded-none border-l border-border bg-card p-0"
         aria-describedby={undefined}
       >
-        {npi === null ? null : (
-          <>
+        {errorKind ? (
+          <ProviderDrawerError
+            kind={errorKind}
+            onRetry={() => {
+              void refetch();
+            }}
+            onClose={onClose}
+          />
+        ) : isLoading || !data ? (
+          <div className="flex h-full flex-col overflow-y-auto">
             <DrillDrawerHeader
               eyebrow="Provider"
-              title={data?.name ?? "Loading…"}
+              title="Loading…"
               onClose={onClose}
             />
-            <div className="p-6 overflow-y-auto h-[calc(100%-64px)]">
-              {isLoading || !data ? (
-                <div className="space-y-2">
-                  <Skeleton variant="row" />
-                  <Skeleton variant="row" />
-                  <Skeleton variant="row" />
-                </div>
-              ) : (
-                <ProviderOverview provider={data} />
-              )}
+            <div className="space-y-2 p-6">
+              <Skeleton variant="row" />
+              <Skeleton variant="row" />
+              <Skeleton variant="row" />
             </div>
-          </>
+          </div>
+        ) : (
+          <div className="flex h-full flex-col overflow-y-auto">
+            <DrillDrawerHeader
+              eyebrow="Provider"
+              title={data.name}
+              onClose={onClose}
+            />
+            <div className="p-6">
+              <ProviderOverview provider={data} />
+            </div>
+          </div>
         )}
       </DialogContent>
     </Dialog>

src/components/ProviderDrawer/ProviderDrawerError.tsx

@@ -0,0 +1,70 @@
+import { AlertCircle, WifiOff } from "lucide-react";
+import { Button } from "@/components/ui/button";
+
+type ProviderDrawerErrorProps = {
+  kind: "not_found" | "network";
+  onRetry?: () => void;
+  onClose: () => void;
+};
+
+const COPY = {
+  not_found: {
+    eyebrow: "NOT FOUND",
+    message: "This provider doesn't exist or has been removed.",
+  },
+  network: {
+    eyebrow: "CONNECTION",
+    message: "Couldn't reach the server. Check your connection and try again.",
+  },
+} as const;
+
+/**
+ * Error state for the provider drill-down drawer (SP21 Task 2.2).
+ *
+ * Two shapes — `not_found` (the NPI in the URL doesn't resolve on the
+ * server, e.g. a stale deep link) and `network` (the request failed).
+ * The not_found variant has no retry affordance (retrying won't help),
+ * the network variant does when `onRetry` is supplied.
+ *
+ * Visual twin of `RemitDrawerError` / `ClaimDrawerError` so the drawer
+ * family feels like one component family — same icon size, same eyebrow
+ * color, same button spacing.
+ */
+export function ProviderDrawerError({
+  kind,
+  onRetry,
+  onClose,
+}: ProviderDrawerErrorProps) {
+  const { eyebrow, message } = COPY[kind];
+  const Icon = kind === "network" ? WifiOff : AlertCircle;
+
+  return (
+    <div
+      className="flex flex-col items-start gap-4 p-6 bg-[color:var(--m-surface)] text-[color:var(--m-ink-primary)]"
+      role="alert"
+      data-testid={`provider-drawer-error-${kind}`}
+    >
+      <div className="flex items-center gap-2">
+        <Icon
+          className="h-5 w-5 text-[color:var(--m-error)]"
+          strokeWidth={1.75}
+          aria-hidden
+        />
+        <span className="eyebrow text-[color:var(--m-error)]">
+          {eyebrow}
+        </span>
+      </div>
+      <p className="text-sm text-[color:var(--m-ink-secondary)] max-w-sm">{message}</p>
+      <div className="flex items-center gap-2">
+        {kind === "network" && onRetry ? (
+          <Button variant="outline" size="sm" onClick={onRetry} data-testid="error-retry">
+            Retry
+          </Button>
+        ) : null}
+        <Button variant="ghost" size="sm" onClick={onClose} data-testid="error-close">
+          Close
+        </Button>
+      </div>
+    </div>
+  );
+}

src/hooks/useProviderDetail.ts

@@ -1,26 +1,87 @@
 import { useQuery } from "@tanstack/react-query";
-import { api } from "@/lib/api";
+import { useSyncExternalStore } from "react";
+import { api, ApiError } from "@/lib/api";
+import { useAppStore } from "@/store";
 import type { Provider } from "@/types";
 
 /**
  * Per-provider detail drawer query (SP21 Task 2.2).
  *
- * Drives ``GET /api/config/providers/{npi}`` via ``api.getProvider``.
- * The endpoint (added in Task 1.6) returns the full ``Provider`` shape
- * with ``recent_claims`` and ``recent_activity`` arrays populated when
- * the backend can serve them — Phase 3 will render those; Phase 2 only
- * consumes the base fields.
- *
- * `npi === null` (drawer closed) disables the query so no fetch is
- * issued. ``staleTime`` is 60 s — provider directory rows change
- * infrequently, but we still want the drawer to refresh when a user
- * reopens it across a long session.
+ * Twin of `useClaimDetail` — same return shape, same retry semantics,
+ * same demo-mode fallback story. Returns `{ data, isLoading, isError,
+ * error, refetch }`:
+ *  - `npi === null` (drawer closed): the query is disabled and the
+ *    hook short-circuits to the empty drawer state so a closed drawer
+ *    doesn't burn a network request or a TanStack cache slot.
+ *  - `npi` is set AND a backend is configured: fetches
+ *    `GET /api/config/providers/{npi}` via `api.getProvider`. Cached
+ *    60 s — provider directory rows change infrequently, but we still
+ *    want the drawer to refresh when a user reopens it across a long
+ *    session.
+ *  - On 404: the hook's retry predicate short-circuits (no retries) so
+ *    the drawer's not-found state appears immediately rather than
+ *    being masked by three back-to-back retry attempts.
+ *  - `!api.isConfigured` (demo mode): no fetch is issued. The hook
+ *    reads from the in-memory zustand store (`useAppStore.providers`).
+ *    An unknown NPI surfaces as `isError: true` so the drawer's
+ *    error branch handles it instead of spinning on a missing fetch.
  */
-export function useProviderDetail(npi: string | null) {
-  return useQuery<Provider>({
+export function useProviderDetail(npi: string | null): {
+  data: Provider | null;
+  isLoading: boolean;
+  isError: boolean;
+  error: Error | null;
+  refetch: () => void;
+} {
+  const fallback = useSyncExternalStore(
+    (cb) => useAppStore.subscribe(cb),
+    () => useAppStore.getState().providers,
+    () => useAppStore.getState().providers
+  );
+
+  const q = useQuery<Provider>({
     queryKey: ["provider-detail", npi],
     queryFn: () => api.getProvider(npi as string),
-    enabled: npi !== null,
+    enabled: npi !== null && api.isConfigured,
     staleTime: 60 * 1000,
+    retry: (failureCount, error) => {
+      if (error instanceof ApiError && error.status === 404) return false;
+      return failureCount < 3;
+    },
   });
+
+  if (!api.isConfigured) {
+    const provider =
+      npi === null ? null : fallback.find((p) => p.npi === npi) ?? null;
+    return {
+      data: provider,
+      isLoading: false,
+      isError: provider === null && npi !== null,
+      error:
+        provider === null && npi !== null
+          ? new ApiError(404, "Provider not found")
+          : null,
+      refetch: () => {},
+    };
+  }
+
+  if (npi === null) {
+    return {
+      data: null,
+      isLoading: false,
+      isError: false,
+      error: null,
+      refetch: () => {},
+    };
+  }
+
+  return {
+    data: q.data ?? null,
+    isLoading: q.isLoading,
+    isError: q.isError,
+    error: q.error,
+    refetch: () => {
+      void q.refetch();
+    },
+  };
 }