Add retry page for locations and location details page - #619
Add retry page for locations and location details page#619shakilhossain1 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
The PR introduces broad error handling, but it appears to break Next.js redirect flow and masks unexpected server failures, which is not safe to approve yet.
Blocking issues
redirectIfNearbyAndIfLatitudeAndLongitudeIsNotSet(...)is now wrapped intry/catchand non-Error404Responseerrors are swallowed in fallback UI paths. In Next.js,redirect()works by throwing; these catches will intercept redirect exceptions and prevent intended redirects. Affected files:src/app/[route]/@mapContainer/page.tsx,src/app/[route]/@sidePanel/page.tsx, and the two[locationSlugOrPersonalCareSubCategory]/page.tsxvariants.- Unexpected non-404 errors are no longer rethrown and now return empty map / side-panel error UI (
200render) instead of surfacing500. This can hide real backend failures and cause silent behavioral regressions (stale/empty content with no error boundary). - Risky control-flow changes (redirect/error semantics) were added without accompanying tests to verify redirect behavior, 404 handling, and non-404 failure paths; this leaves high regression risk unguarded.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 153f0d890d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (e instanceof Error404Response) { | ||
| return notFound(); | ||
| } | ||
|
|
||
| return <LocationsMap locationStubs={[]} />; |
There was a problem hiding this comment.
Re-throw redirect control-flow errors in catch block
redirectIfNearbyAndIfLatitudeAndLongitudeIsNotSet calls Next.js redirect(...), which signals redirects by throwing a control-flow error; this catch now treats any non-Error404Response as a recoverable failure and renders an empty map instead. In the sortBy=nearby + missing lat/long cookie flow, users will no longer be redirected to the non-nearby URL and will land on the error/empty state, which is a regression in navigation behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The PR introduces broad error swallowing in route pages, which can mask real server failures as successful empty states. Given the behavioral change and lack of accompanying tests for these fallback paths, this is not safe to approve conservatively.
Blocking issues
- Unexpected non-404 errors are now converted into successful UI fallbacks (
<LocationsMap locationStubs={[]}>/<SidePanelErrorState />) instead of propagating as 500s in multiple pages. This can hide backend outages and silently change failure semantics for users and monitoring. - Returning an empty map on server/data failures is behaviorally ambiguous and can be interpreted as 'no locations exist' rather than 'load failed', which is a potential regression in core user flow.
- No tests are included for the new error-handling branches (404 vs non-404, detail vs list routes, retry state behavior). For risky control-flow changes in data-loading pages, this missing coverage is blocking.
There was a problem hiding this comment.
The PR introduces broad non-404 error swallowing and fallback rendering paths without test coverage, which is risky and can hide real production failures behind 200 responses.
Blocking issues
src/app/[route]/@mapContainer/[locationSlugOrPersonalCareSubCategory]/page.tsxandsrc/app/[route]/@sidePanel/[locationSlugOrPersonalCareSubCategory]/page.tsx: non-404 exceptions that previously surfaced as 500s are now converted into fallback UI (LocationsMapwith empty stubs orSidePanelErrorState). This masks backend/server faults, changes failure semantics, and can break monitoring/alerting expectations.src/app/[route]/@mapContainer/page.tsxandsrc/app/[route]/@sidePanel/page.tsx: broadcatchblocks return successful fallback UI for any non-404 error, creating potential silent regressions instead of explicit failures.- No tests are included for the new error-handling behavior (non-404 failure paths, retry state behavior, and fallback rendering), leaving risky logic changes unverified.
No description provided.