(SP 1) [Shop] Canonicalize legacy filters + fix cart CTA/badge UX#274
(SP 1) [Shop] Canonicalize legacy filters + fix cart CTA/badge UX#274ViktorSvertoka merged 21 commits intodevelopfrom
Conversation
…=new and using sort=newest
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughStabilize checkout button layout with a spinner and SR text, add stock-aware quantity limits and UI states, normalize legacy Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/[locale]/shop/products/page.tsx (1)
40-61:⚠️ Potential issue | 🟠 MajorAvoid stripping pagination from canonical redirects.
needsCanonicaltriggers on anypage, and the redirect removespagefrom the query, so/shop/products?page=2will always canonicalize to page 1. This breaks pagination deep links and conflicts withcatalogQuerySchemaparsing ofpage.🔧 Proposed fix (canonicalize only legacy filter and preserve page)
- const needsCanonical = - !!resolvedSearchParams.page || !!resolvedSearchParams.filter; + const hasLegacyFilter = resolvedSearchParams.filter === 'new'; + const needsCanonical = hasLegacyFilter; if (needsCanonical) { const qsParams = new URLSearchParams(); for (const [k, v] of Object.entries(resolvedSearchParams)) { if (!v) continue; - if (k === 'page') continue; if (k === 'filter') continue; qsParams.set(k, v); } // support legacy ?filter=new => sort=newest - if (resolvedSearchParams.filter === 'new' && !resolvedSearchParams.sort) { + if (hasLegacyFilter && !resolvedSearchParams.sort) { qsParams.set('sort', 'newest'); }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/[locale]/shop/products/page.tsx (1)
56-59:⚠️ Potential issue | 🔴 CriticalImport and use the i18n-aware
redirectfunction from@/i18n/routing.The redirect uses the standard Next.js
redirectfromnext/navigation, but should use the custom i18n-awareredirectfrom@/i18n/routing. The app's routing config haslocalePrefix: 'always', meaning locale must always be in the URL. Thelocaleis already available fromparamsand should be passed to the redirect function.Compare with
dashboard/page.tsx, which correctly imports and uses:import { redirect } from '@/i18n/routing'; redirect({ href: '/login', locale });Proposed fix
-import { redirect } from 'next/navigation'; +import { redirect } from '@/i18n/routing';Then update the redirect call:
- redirect(qs ? `${basePath}?${qs}` : basePath); + redirect({ href: qs ? `${basePath}?${qs}` : basePath, locale });
2 participants
Description
This PR fixes two Shop UX issues:
Related Issue
Issue: #<issue_number>
Changes
Database Changes (if applicable)
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Bug Fixes
Improvements