Skip to content

feat: useRouter().pathname → usePathname() (#928)#938

Merged
NoopDog merged 4 commits into
mainfrom
fran/928-useRouter-pathname-to-usePathname
May 26, 2026
Merged

feat: useRouter().pathname → usePathname() (#928)#938
NoopDog merged 4 commits into
mainfrom
fran/928-useRouter-pathname-to-usePathname

Conversation

@frano-m

@frano-m frano-m commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Part of #884 (Next.js 16 upgrade plan).

Migrates one safe call site from useRouter().pathname (next/router) to usePathname() (next/navigation):

  • src/views/ExportMethodView/exportMethodView.tsxpathname is compared against static exportMethods[].route strings. These are non-dynamic routes, so resolved URL == route pattern; the migration is behaviour-preserving. Removed the now-unused useRouter import.

Uses usePathname() ?? "" to handle the string | null return type, mirroring the existing pattern in Header.tsx.

Deferrals

Two sites are left on useRouter().pathname for the same underlying reason — pathname needs to be in the route-pattern form (with bracketed dynamic segments) because the surrounding code interacts with other Next.js APIs that also use the route-pattern form. Migrating only pathname would mix forms and break behaviour.

Tests

New unit tests in tests/stateSyncManager_utils.test.ts cover all four exports of UseStateSync/utils.ts (wasPop, hasParams, isSynced, stringifyQuery) — 20 cases, including ones that pin the wasPop contract: both sides must be in the same form (route pattern or resolved URL).

Test plan

  • npm run lint — clean
  • npm run check-format — clean
  • npm run test-compile — clean
  • npm test — 447/447 pass (new file: 20 tests)
  • Manually verified against data-biosphere (anvil-cmg dev) and brc-analytics on fran/x-nextjs-test — filter pages and dynamic detail pages behave the same as before.

Closes #928

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR advances the Next.js 16 upgrade plan by replacing useRouter().pathname usages (from next/router) with usePathname() (from next/navigation) at two call sites, improving pathname matching behavior on dynamic routes while keeping one known-sensitive pathname usage intentionally deferred.

Changes:

  • Updated ExportMethodView to derive the current path via usePathname() and removed the useRouter dependency.
  • Updated useStateSync to use usePathname() so popstate/back-forward detection can compare against the resolved URL (fixing dynamic-route mismatch).
  • Added an inline deferral note in useUpdateURLCatalogParams explaining why useRouter().pathname remains for now (to preserve dynamic route interpolation behavior), tracked for follow-up work.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/views/ExportMethodView/exportMethodView.tsx Migrates pathname read to usePathname() and removes useRouter import.
src/hooks/useUpdateURLCatalogParam.ts Adds rationale/comment documenting intentional deferral of pathname migration in this hook.
src/hooks/stateSyncManager/hooks/UseStateSync/hook.ts Migrates pathname read to usePathname() to correctly match resolved history URLs in popstate handling.

@frano-m frano-m marked this pull request as ready for review May 26, 2026 05:34
@NoopDog NoopDog requested a review from Copilot May 26, 2026 05:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread src/hooks/stateSyncManager/hooks/UseStateSync/hook.ts Outdated
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/hooks/stateSyncManager/hooks/UseStateSync/hook.ts:37

  • useStateSync now uses usePathname() (resolved URL) but wasPop() compares against NextHistoryState.url and prepends basePath. In Next.js history state, as is the resolved URL (see src/hooks/stateSyncManager/hooks/UseBeforePopState/hook.ts where state.as is treated as the navigated URL). This mismatch can cause wasPop() to return false on back/forward navigations (especially on dynamic routes), leading to incorrect state/URL sync behavior. Consider updating wasPop() to compare against nextHistoryState.as (stripping query) and revisiting basePath handling (avoid double-prepending if usePathname() already includes basePath).
  const { basePath, isReady, query } = useRouter();
  const pathname = usePathname() ?? "";
  const { onClearPopRef, popRef } = useWasPop();

  // Extract the query from the state.
  const stateQuery = state.query as NextRouter["query"];

  // Extract the param keys from the state.
  const paramKeys = state.paramKeys;

  // Dispatch action to sync state <-> URL.
  useEffect(() => {
    // Do nothing if the router is not ready.
    if (!isReady) return;

    // Do nothing; URL and state are in sync.
    if (isSynced(stateQuery, query)) {
      onClearPopRef();
      return;
    }

    // Dispatch action sync URL >> state.
    if (wasPop(basePath, pathname, popRef.current)) {
      // When the user navigates with the back/forward buttons.

Comment thread tests/stateSyncManager_utils.test.ts
Comment thread tests/stateSyncManager_utils.test.ts Outdated
frano-m and others added 2 commits May 26, 2026 15:54
…#928)

NextHistoryState.url is the href stored by Next.js — the route pattern on
dynamic routes — not the resolved URL, so the previous code was correctly
using useRouter().pathname (also a route pattern). Migrating that site to
usePathname() (resolved URL) would have broken wasPop() on dynamic routes.

Reverts the useStateSync portion of the migration. Updates the unit tests
to reflect the actual NextHistoryState contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 26, 2026 05:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@NoopDog NoopDog merged commit 4a24996 into main May 26, 2026
3 checks passed
@frano-m frano-m deleted the fran/928-useRouter-pathname-to-usePathname branch May 26, 2026 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P1.1] Next.js 16 prep: useRouter().pathname → usePathname()

3 participants