From e7469d39ecd275ef6ea82fb34ddd3b2c6f3ff82c Mon Sep 17 00:00:00 2001
From: Tyler <tyler@crispygoat.com>
Date: Sat, 20 Jun 2026 10:52:42 -0600
Subject: [PATCH] fix(frontend): normalize empty ?claim= to null; preserve
 sibling params
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- readClaimId: URLSearchParams.get returns '' for ?claim=, not null.
  Normalize to null so the contract matches the JSDoc and consumers
  can treat empty as absent.

- Tests: regression for ?claim= → null; open/close preserves other
  query params (?foo=bar survives open/close unchanged).

- Tighten pushStateMock/replaceStateMock typing to vi.fn<() => void>()
  per code-review nit (drops the verbose ReturnType<typeof vi.fn>).
---
 src/hooks/useDrawerUrlState.test.ts | 48 +++++++++++++++++++++++++++--
 src/hooks/useDrawerUrlState.ts      |  6 +++-
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/src/hooks/useDrawerUrlState.test.ts b/src/hooks/useDrawerUrlState.test.ts
index 0699e37..9819bb7 100644
--- a/src/hooks/useDrawerUrlState.test.ts
+++ b/src/hooks/useDrawerUrlState.test.ts
@@ -61,8 +61,8 @@ describe("useDrawerUrlState", () => {
   // assign to `() => void`. Constraining the generic to `vi.fn<() => void>()`
   // gives a plain function-typed mock that flows directly into `pushState`
   // / `replaceState` slots.
-  let pushStateMock: ReturnType<typeof vi.fn>;
-  let replaceStateMock: ReturnType<typeof vi.fn>;
+  let pushStateMock: ReturnType<typeof vi.fn<() => void>>;
+  let replaceStateMock: ReturnType<typeof vi.fn<() => void>>;
 
   beforeEach(() => {
     pushStateMock = vi.fn();
@@ -111,6 +111,20 @@ describe("useDrawerUrlState", () => {
     unmount();
   });
 
+  it("returns null claimId when ?claim= is present but empty", () => {
+    // Regression: `URLSearchParams.get` returns `""` for `?claim=`, which
+    // a naive implementation would propagate as `claimId: ""`. We treat
+    // empty as absent so the contract is "present-or-null", not
+    // "present-or-empty-string".
+    setLocation("http://localhost/claims?claim=");
+
+    const { result, unmount } = renderHook(() => useDrawerUrlState());
+
+    expect(result.current?.claimId).toBeNull();
+
+    unmount();
+  });
+
   it("open(id) pushes a new history entry containing ?claim=id", () => {
     setLocation("http://localhost/claims");
 
@@ -148,6 +162,36 @@ describe("useDrawerUrlState", () => {
     unmount();
   });
 
+  it("open() and close() preserve other query params (only ?claim= is touched)", () => {
+    // Regression: `buildUrl` must only add/delete the `claim` key and
+    // leave sibling params like `?foo=bar` intact. A bug here would mean
+    // opening a claim nukes unrelated URL state — easy to miss without
+    // an explicit assertion.
+    setLocation("http://localhost/claims?foo=bar&claim=CLM-1");
+
+    const { result, unmount } = renderHook(() => useDrawerUrlState());
+
+    // open replaces the claim value but keeps foo.
+    act(() => {
+      result.current?.open("CLM-2");
+    });
+    const openUrl = pushStateMock.mock.calls[0][2] as string;
+    expect(openUrl).toContain("foo=bar");
+    // `claim=` may be preceded by `?` or `&` depending on param order,
+    // so just assert the param value appears.
+    expect(openUrl).toMatch(/[?&]claim=CLM-2/);
+
+    // close strips claim and keeps foo.
+    act(() => {
+      result.current?.close();
+    });
+    const closeUrl = pushStateMock.mock.calls[1][2] as string;
+    expect(closeUrl).toContain("foo=bar");
+    expect(closeUrl).not.toContain("claim=");
+
+    unmount();
+  });
+
   it("close() pushes a new history entry with the ?claim= param stripped", () => {
     setLocation("http://localhost/claims?claim=CLM-1");
 
diff --git a/src/hooks/useDrawerUrlState.ts b/src/hooks/useDrawerUrlState.ts
index ed109f0..8b411e9 100644
--- a/src/hooks/useDrawerUrlState.ts
+++ b/src/hooks/useDrawerUrlState.ts
@@ -11,7 +11,11 @@ import { useCallback, useEffect, useState } from "react";
  */
 function readClaimId(): string | null {
   const params = new URLSearchParams(window.location.search);
-  return params.get("claim");
+  // `URLSearchParams.get` returns `""` for `?claim=` (param present but
+  // empty) — we normalize that to `null` so the "absent" and "empty"
+  // cases are indistinguishable to consumers, matching the JSDoc.
+  const value = params.get("claim");
+  return value === "" ? null : value;
 }
 
 /**