Skip to content

Cache nearby locations for local pagination - #618

Open
doobneek wants to merge 4 commits into
mainfrom
nearby-pagination-cache
Open

Cache nearby locations for local pagination#618
doobneek wants to merge 4 commits into
mainfrom
nearby-pagination-cache

Conversation

@doobneek

@doobneek doobneek commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add a server API route that reuses the existing route/filter/taxonomy logic to fetch chunked full location rows
  • add a client-side nearby locations cache that seeds from SSR data, backfills remaining results in the background, and serves page changes locally
  • switch the locations list pager to local history updates so moving between ?page= values does not rerun the full App Router data fetch flow

Verification

  • npm run check-types
  • npm run lint (existing repo warnings only)
  • npm run build with NEXT_PUBLIC_GO_GETTA_PROD_URL and NEXT_PUBLIC_GOOGLE_MAPS_API_KEY set in the shell

@doobneek
doobneek temporarily deployed to CI_CD_PIPELINE March 9, 2026 22:52 — with GitHub Actions Inactive
@jbeard4
jbeard4 temporarily deployed to yourpeer-nearby-paginat-vxlcxs March 9, 2026 22:53 Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not safe to approve. The PR introduces risky pagination/cache behavior changes with at least one likely functional regression and missing coverage for critical paths.

Blocking issues

  • Likely pagination metadata regression: use-cached-locations-pagination stores cachedDataset.numberOfPages from /api/locations-pagination responses fetched with pageSize=200, then UI pager uses that value as display-page bounds. If backend numberOfPages is computed from requested pageSize, pager bounds will shrink by ~10x and disable navigation early (files: src/components/use-cached-locations-pagination.ts, src/components/locations-container.tsx, src/app/api/locations-pagination/route.ts).
  • Input validation gap in new API route: pageSize accepts 0 (parsePositiveInteger allows >= 0), and Math.min(..., MAX_PAGE_SIZE) does not enforce a lower bound. This can propagate an invalid page size to getFullLocationData, with undefined/broken behavior under malformed requests (src/app/api/locations-pagination/route.ts).
  • Risky logic added without tests: no evidence of tests for cache split/merge correctness, page-boundary behavior, and pager integration while loading. Given the new client-side caching + background fetch + URL mutation flow, missing tests are a blocking regression risk.

@doobneek
doobneek temporarily deployed to CI_CD_PIPELINE March 9, 2026 23:02 — with GitHub Actions Inactive
@doobneek

doobneek commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the blocking feedback in 8dae7d3.

  • Fixed the display-page metadata regression by removing chunk-page numberOfPages from the internal API response and deriving display-page bounds from resultCount in the cache layer.
  • Tightened request parsing so pageSize=0 and other malformed values fall back to the default background page size instead of flowing through as 0.
  • Added focused node tests for cache split/merge behavior, display-page math, loading fallback visibility, and page-size parsing.

Verification rerun:

  • npm test
  • npm run check-types
  • npm run lint (existing repo warnings only)
  • npm run build with NEXT_PUBLIC_GO_GETTA_PROD_URL and NEXT_PUBLIC_GOOGLE_MAPS_API_KEY set in the shell

@jbeard4
jbeard4 temporarily deployed to yourpeer-nearby-paginat-vxlcxs March 9, 2026 23:02 Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I cannot safely approve this PR due to blocking risk in the new pagination path.

Blocking issues

  • src/app/api/locations-pagination/route.ts accepts unbounded pageNumber (parseNonNegativeInteger only checks >=0). This allows arbitrarily large page offsets against upstream location queries, creating a resource-exhaustion/DoS risk. Add an upper bound (or reject out-of-range pages based on known max pages/resultCount).
  • Client pagination now depends on window.history.pushState in src/components/use-cached-locations-pagination.ts and reactive useSearchParams updates, but there are no integration tests covering URL/state sync, back/forward behavior, and pager correctness. This is risky logic with clear regression potential and should be covered before approval.

@doobneek
doobneek temporarily deployed to CI_CD_PIPELINE March 9, 2026 23:12 — with GitHub Actions Inactive
@doobneek

doobneek commented Mar 9, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the latest blocking review in 54fa96c.

  • Added an explicit upper bound for internal background pageNumber requests and return 400 when the request exceeds that supported range.
  • Extracted the URL mutation path into a pure navigation helper and added coverage for URL sync + scroll reset behavior, alongside the existing cache-math and request-parsing tests.

Current verification:

  • npm test
  • npm run check-types
  • npm run lint (existing repo warnings only)
  • npm run build with NEXT_PUBLIC_GO_GETTA_PROD_URL and NEXT_PUBLIC_GOOGLE_MAPS_API_KEY set in the shell

The PR can still show Changes requested until that bot review is dismissed or replaced, because GitHub keeps the prior review state even after follow-up commits land.

@jbeard4
jbeard4 temporarily deployed to yourpeer-nearby-paginat-vxlcxs March 9, 2026 23:12 Inactive

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I found a blocking pagination-cache regression risk that makes this unsafe to approve as-is.

Blocking issues

  • Potential infinite background refetch loop / request storm when result set exceeds API cap. The client prefetch loop in src/components/use-cached-locations-pagination.ts computes missingBackgroundPages from numberOfPages and attempts to fetch all of them, but the API hard-rejects pageNumber > 100 in src/lib/locations-pagination-request.ts and src/app/api/locations-pagination/route.ts. Failed pages are removed from loadingBackgroundPages but never marked terminal, so they remain "missing" and can be retried repeatedly on each effect run. This is a regression/perf risk and should be fixed by capping client prefetch to supported max page number (or treating 400-cap pages as permanently exhausted) and adding tests for this boundary behavior.

@jbeard4

jbeard4 commented Mar 9, 2026

Copy link
Copy Markdown
Member

@doobneek1 Is this PR associated with a particular issue number?

@doobneek

doobneek commented Mar 9, 2026 via email

Copy link
Copy Markdown
Collaborator Author

@jbeard4
jbeard4 temporarily deployed to yourpeer-nearby-paginat-vxlcxs March 10, 2026 01:38 Inactive
@doobneek

Copy link
Copy Markdown
Collaborator Author

Addressed the remaining bot review points in fe270ca.

  • removed the nearby pagination hook's dependency on reactive useSearchParams and now sync the local page state from pushState plus popstate
  • capped background prefetch to the API-supported background-page range and fall back to a full navigation when a requested page sits outside the local cacheable range
  • added regression coverage for cache-range boundaries, URL/page round-tripping, and local-vs-hard navigation boundary behavior

Verification:

  • npm test
  • npm run check-types
  • npm run lint (existing repo warnings only)
  • npm run build with NEXT_PUBLIC_GO_GETTA_PROD_URL and NEXT_PUBLIC_GOOGLE_MAPS_API_KEY set in the shell

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.

2 participants