fix(frontend): normalize empty ?claim= to null; preserve sibling params

- 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>).
This commit is contained in:
Tyler
2026-06-20 10:52:42 -06:00
parent 234cd8b28b
commit e7469d39ec
2 changed files with 51 additions and 3 deletions
+46 -2
View File
@@ -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");
+5 -1
View File
@@ -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;
}
/**