feat: update page model and integration to support redirects#777
feat: update page model and integration to support redirects#777
Conversation
WalkthroughImplements page-level redirect support across the monorepo by adding an optional Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Coverage Report for packages/configs/vitest-config
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/frontend/src/app/[locale]/[[...slug]]/page.tsx (2)
90-101: Note:redirect()is called inside a try-catch block.Next.js's
redirect()works by throwing a specialNEXT_REDIRECTerror. While the current catch block correctly re-throws unhandled errors (allowing the redirect to work), this pattern is fragile. If future error handling logic is added that doesn't account for the redirect error, it could break silently.The redirect works correctly now because the error doesn't match the 404/401 checks and gets re-thrown at line 184. However, for clarity and maintainability, consider moving the redirect check before the try block or explicitly handling the redirect error type.
♻️ Alternative: Move data fetch and redirect outside try-catch
One option is to restructure so redirect happens outside the try-catch, though this would require more significant refactoring of the error handling flow.
Alternatively, you could explicitly check for the redirect error in the catch block:
} catch (error) { // Allow Next.js redirects to propagate if (error && typeof error === 'object' && 'digest' in error && typeof (error as any).digest === 'string' && (error as any).digest.startsWith('NEXT_REDIRECT')) { throw error; } // ... rest of error handling }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/`[locale]/[[...slug]]/page.tsx around lines 90 - 101, Move the redirect handling out of the try-catch or explicitly allow Next.js redirect errors to propagate: either perform the meta.redirect check immediately after calling sdk.modules.getPage and before entering the error-handling catch, or inside the catch in page.tsx explicitly detect the NEXT_REDIRECT sentinel (e.g., inspect error.digest starting with 'NEXT_REDIRECT') and re-throw it so redirect() can work; update the logic around sdk.modules.getPage / meta.redirect / redirect() and the surrounding catch block accordingly to ensure redirect errors are not swallowed.
99-101: Consider validating the redirect path format.The redirect URL is constructed by concatenating locale and
meta.redirect. While the/${locale}prefix constrains redirects to same-origin paths (good for security), the code assumesmeta.redirectstarts with/. If the CMS provides a value without a leading slash (e.g.,personalinstead of/personal), the resulting URL would be malformed (e.g.,/enpersonal).Consider adding validation or normalization:
♻️ Proposed validation
if (meta.redirect) { - redirect(`/${locale}${meta.redirect}`); + const targetPath = meta.redirect.startsWith('/') ? meta.redirect : `/${meta.redirect}`; + redirect(`/${locale}${targetPath}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/`[locale]/[[...slug]]/page.tsx around lines 99 - 101, Normalize and validate meta.redirect before calling redirect: ensure it is a safe same-origin path by rejecting values that start with '//' or include a scheme ('http:', 'https:'), then normalize to a single leading slash (e.g., use '/' + meta.redirect.replace(/^\/+/, '') if not empty) and finally call redirect(`/${locale}${normalized}`) from the page rendering code (the code that reads meta.redirect and calls redirect(...)); this prevents malformed concatenations like `/enpersonal` and blocks protocol-relative or external URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/frontend/src/app/`[locale]/[[...slug]]/page.tsx:
- Around line 90-101: Move the redirect handling out of the try-catch or
explicitly allow Next.js redirect errors to propagate: either perform the
meta.redirect check immediately after calling sdk.modules.getPage and before
entering the error-handling catch, or inside the catch in page.tsx explicitly
detect the NEXT_REDIRECT sentinel (e.g., inspect error.digest starting with
'NEXT_REDIRECT') and re-throw it so redirect() can work; update the logic around
sdk.modules.getPage / meta.redirect / redirect() and the surrounding catch block
accordingly to ensure redirect errors are not swallowed.
- Around line 99-101: Normalize and validate meta.redirect before calling
redirect: ensure it is a safe same-origin path by rejecting values that start
with '//' or include a scheme ('http:', 'https:'), then normalize to a single
leading slash (e.g., use '/' + meta.redirect.replace(/^\/+/, '') if not empty)
and finally call redirect(`/${locale}${normalized}`) from the page rendering
code (the code that reads meta.redirect and calls redirect(...)); this prevents
malformed concatenations like `/enpersonal` and blocks protocol-relative or
external URLs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 098b8c48-f2d5-4461-9df0-5c3c7b9c4848
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.changeset/all-islands-search.md.changeset/odd-stars-heal.mdapps/api-harmonization/src/modules/page/page.mapper.tsapps/api-harmonization/src/modules/page/page.model.tsapps/frontend/src/app/[locale]/[[...slug]]/page.tsxpackages/framework/src/modules/cms/models/page.model.tspackages/integrations/mocked-dxp/src/modules/cms/mappers/mocks/pages/home.page.ts
2 participants
What does this PR do?
Related Ticket(s)
Key Changes
redirectfield to the CMS Page model in@o2s/framework(
packages/framework/src/modules/cms/models/page.model.ts)redirectto API harmonizationMetadatamodel and page mapper to pass it through from CMS to frontendpage.tsx— ifmeta.redirectis present, performs a Next.js redirect to/${locale}${meta.redirect}/) to redirect to/personal(EN),/personlich(DE),/indywidualny(PL) — matchingdxp-starter-kit behavior
How to test
configs/integrationsto use@o2s/integrations.mocked-dxpforcms,articles, andsearchmodulesnpm installandnpm run devlocalhost:3000/en— should redirect tolocalhost:3000/en/personallocalhost:3000/pl— should redirect tolocalhost:3000/pl/indywidualnylocalhost:3000/de— should redirect tolocalhost:3000/de/personlich/en/personal/accounts) still work normally without redirectMedia (Loom or gif)
Summary by CodeRabbit
Release Notes