refactor(articles): update article slugs to be relative and adjust mapping logic#704
Merged
refactor(articles): update article slugs to be relative and adjust mapping logic#704
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
WalkthroughNormalized article/category slugs (removed leading slashes), changed slug matching to suffix/endsWith across integrations, made mocked service emissions lazy, added Zendesk article-link → slug replacement in the Zendesk mapper, adjusted RichText image rendering, and pinned markdown-to-jsx to 9.7.0. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/integrations/mocked/src/modules/articles/mocks/article11.content.mocks.ts (1)
5-26:⚠️ Potential issue | 🟡 MinorPre-existing
category.idinconsistency forart-011across locales.The EN version of
art-011(Line 24) usescategory.id: 'warranty-and-repair'(hyphens), while the DE (Line 189) and PL (Line 256) counterparts usecategory.id: 'warranty_repair'(underscore). Since this PR touches this file, it's a good opportunity to align all three locales. If the system performs any category-based grouping or filtering oncategory.id, this mismatch would causeart-011to appear under a different category in EN than in DE/PL.🛠️ Suggested alignment
// EN art-011 (line 24) category: { - id: 'warranty-and-repair', + id: 'warranty_repair', title: 'Warranty & Repair', },Also applies to: 169-232, 235-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/articles/mocks/article11.content.mocks.ts` around lines 5 - 26, The category.id for article 'art-011' is inconsistent across locales (EN uses 'warranty-and-repair' while DE/PL use 'warranty_repair'); pick one canonical id and make them consistent by updating the category.id values for article id 'art-011' in the EN, DE, and PL mock entries (update the 'category' object inside the article entries), ensuring all three use the same string (either 'warranty-and-repair' or 'warranty_repair') so category-based grouping/filtering behaves consistently.
🧹 Nitpick comments (3)
packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts (1)
54-56: JSDoc example implies lowercasing that the function doesn't performThe comment states
Returns: "12345-article-title"(all lowercase), butextractSlugFromUrlreturns the slug in whatever case the original URL uses (e.g.12345-Article-Title). If Zendesk always uses lowercase slugs this is harmless in practice, but the comment is misleading.📝 Proposed doc fix
- * Returns: "12345-article-title" or just the ID as fallback + * Returns: "12345-Article-Title" (preserves original URL casing) or just the ID as fallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts` around lines 54 - 56, The JSDoc for extractSlugFromUrl incorrectly claims the slug is lowercased; update the comment to reflect actual behavior by changing the example/return line to indicate the function preserves original case (e.g., Returns: "12345-Article-Title" or just the ID as fallback) or explicitly state "preserves original case" so the docs match the extractSlugFromUrl implementation.packages/ui/src/components/RichText/RichText.tsx (1)
51-60:alt={props.alt}is redundant —...propsalready includes it.
altis included in the...propsspread, so the trailingalt={props.alt}simply overrides with the same value. It's harmless but adds noise.♻️ Proposed cleanup
const ImgComp: FC<Readonly<React.ImgHTMLAttributes<HTMLImageElement> & { children?: ReactNode }>> = ({ children: _children, ...props }) => { return ( <Typography variant="image" asChild> - <img {...props} alt={props.alt} /> + <img {...props} /> </Typography> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/RichText/RichText.tsx` around lines 51 - 60, The ImgComp component spreads image attributes via ...props and then redundantly sets alt={props.alt}; remove the redundant alt={props.alt} override in the ImgComp return so the <img> relies solely on the spread props (locate the ImgComp functional component to update the JSX).packages/integrations/mocked/src/modules/articles/articles.service.ts (1)
13-13:deferwrapping is correct forgetCategory— consider applying it to other sync-mapper methods too.Without
defer, aNotFoundExceptionthrown bymapCategorywould propagate synchronously before the Observable is created, potentially bypassing NestJS exception filters that handle errors from the Observable stream. Wrapping indeferensures the error is emitted as an Observable error on subscription, which is the correct RxJS pattern.
getCategoryListandgetArticlecurrently don't throw, but wrapping their sync mapper calls indeferas well would make the pattern uniform and safe against future changes that add throwing behavior.♻️ Proposed optional consistency fix
getCategory(options: Articles.Request.GetCategoryParams): Observable<Articles.Model.Category> { return defer(() => of(mapCategory(options.locale, options.id))); } getCategoryList(options: Articles.Request.GetCategoryListQuery): Observable<Articles.Model.Categories> { - return of(mapCategories(options.locale, options)); + return defer(() => of(mapCategories(options.locale, options))); } getArticle(params: Articles.Request.GetArticleParams): Observable<Articles.Model.Article | undefined> { - return of(mapArticle(params.locale, params.slug)); + return defer(() => of(mapArticle(params.locale, params.slug))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/mocked/src/modules/articles/articles.service.ts` at line 13, The current getCategory uses defer(() => of(mapCategory(...))) to ensure synchronous exceptions become Observable errors; apply the same pattern to the other sync-mapper methods so thrown errors don't escape before subscription: wrap mapArticleList call in getCategoryList and mapArticle call in getArticle with defer(() => of(...)) (referencing getCategoryList, getArticle, mapArticleList, mapArticle) so both methods emit errors through the Observable stream consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/integrations/mocked/src/modules/articles/articles.mapper.ts`:
- Line 54: The current article lookup uses articles.find((article) =>
slug.endsWith(article.slug)) which can produce false positives across
path-segment boundaries; change the predicate in that articles.find call to
require either an exact match (slug === article.slug) or a path-segment anchored
match (slug.endsWith('/' + article.slug)) so only whole-segment matches (not
bare suffixes) are returned.
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts`:
- Around line 95-98: The slug extraction currently allows fragments and query
strings because extractSlugFromUrl uses a too-broad character class (e.g.,
[^/]+) so strings like "Title#anchor" or "Title?query" are captured; update
extractSlugFromUrl to tighten the article-slug capture to exclude "?" and "#"
(use a character class like [^/?#]+) so it stops at query or fragment
separators, then keep replaceZendeskArticleLinks as-is to pass the cleaned slug
through; ensure you update any related regex groups in extractSlugFromUrl to
reflect the new class so match[2] contains only the valid slug.
In `@packages/ui/package.json`:
- Line 69: The package.json change pins "markdown-to-jsx" to 9.7.0 which removes
important bug fixes; revert the dependency in packages/ui's package.json to a
caret range that includes 9.7.4 (e.g., "^9.7.4") or the previous version
specifier, or alternatively add an explicit resolutions/overrides entry that
forces 9.7.4+ while documenting any known incompatibility; if you must pin to
9.7.0, add a clear comment and a linked issue/repro steps explaining the blocker
so reviewers can evaluate the regression.
---
Outside diff comments:
In
`@packages/integrations/mocked/src/modules/articles/mocks/article11.content.mocks.ts`:
- Around line 5-26: The category.id for article 'art-011' is inconsistent across
locales (EN uses 'warranty-and-repair' while DE/PL use 'warranty_repair'); pick
one canonical id and make them consistent by updating the category.id values for
article id 'art-011' in the EN, DE, and PL mock entries (update the 'category'
object inside the article entries), ensuring all three use the same string
(either 'warranty-and-repair' or 'warranty_repair') so category-based
grouping/filtering behaves consistently.
---
Duplicate comments:
In `@apps/frontend/package.json`:
- Line 63: The package.json change pins "markdown-to-jsx" to 9.7.0 which
duplicates the problematic downgrade in packages/ui; restore a semver range that
includes the fixed release (e.g., change the dependency for "markdown-to-jsx"
back to "^9.7.4" or match whatever range is used in packages/ui) so the fix for
verbatim rendering of <pre>/<script>/<style>/<textarea> is included; ensure the
version string is consistent with packages/ui to avoid conflicting dependency
resolutions.
---
Nitpick comments:
In `@packages/integrations/mocked/src/modules/articles/articles.service.ts`:
- Line 13: The current getCategory uses defer(() => of(mapCategory(...))) to
ensure synchronous exceptions become Observable errors; apply the same pattern
to the other sync-mapper methods so thrown errors don't escape before
subscription: wrap mapArticleList call in getCategoryList and mapArticle call in
getArticle with defer(() => of(...)) (referencing getCategoryList, getArticle,
mapArticleList, mapArticle) so both methods emit errors through the Observable
stream consistently.
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts`:
- Around line 54-56: The JSDoc for extractSlugFromUrl incorrectly claims the
slug is lowercased; update the comment to reflect actual behavior by changing
the example/return line to indicate the function preserves original case (e.g.,
Returns: "12345-Article-Title" or just the ID as fallback) or explicitly state
"preserves original case" so the docs match the extractSlugFromUrl
implementation.
In `@packages/ui/src/components/RichText/RichText.tsx`:
- Around line 51-60: The ImgComp component spreads image attributes via ...props
and then redundantly sets alt={props.alt}; remove the redundant alt={props.alt}
override in the ImgComp return so the <img> relies solely on the spread props
(locate the ImgComp functional component to update the JSX).
packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts
Show resolved
Hide resolved
Contributor
Coverage Report for packages/configs/vitest-config
File CoverageNo changed files found. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/poor-badgers-grow.md:
- Around line 2-5: Update the changeset to use patch bumps for the bugfix:
replace 'minor' with 'patch' for the packages '@o2s/integrations.zendesk',
'@o2s/integrations.mocked', '@o2s/frontend', and '@o2s/ui' in the changeset
entry so the versioning reflects a bugfix rather than a minor feature addition;
if the change truly introduces a new public behavior, instead leave as minor and
add an explicit note explaining the breaking/behavioral change for consumers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/integrations/strapi-cms/src/modules/articles/articles.service.ts (2)
63-78:⚠️ Potential issue | 🟠 Major
endsWithmatching is not path-segment-safe and the fallback silently serves the wrong articleTwo distinct bugs in the resolution logic at line 73:
Bug 1 — Path-segment-unsafe
endsWith:
String.prototype.endsWithoperates at character level, not path-segment level. Suppose the DB contains two articles with slugs"my-article"and"sub/my-article". A request for"sub/my-article"runs:"sub/my-article".endsWith("my-article") → true ← wrong match if first in array "sub/my-article".endsWith("sub/my-article") → true ← correct match
Array.findreturns the first truthy match. Because the GraphQLendsWith: $slugfilter sends onlyslugSuffix(the last segment), both articles can appear in the result, and whichever is first determines the winner. A request for/support/my-articlecould silently return the root-levelmy-articlearticle instead.The path-boundary–safe predicate is:
slug === a.slug || slug.endsWith('/' + a.slug)To prefer the most specific match when several candidates are valid, sort by slug length descending before calling
find.Bug 2 — Silent wrong-article fallback:
|| pages.data.articles[0]!returns the first article when no slug match is found, silently serving wrong content. Should throwNotFoundException.🐛 Proposed fix
- const article = pages.data.articles.find((a) => slug.endsWith(a.slug)) || pages.data.articles[0]!; + const article = pages.data.articles + .filter((a) => slug === a.slug || slug.endsWith('/' + a.slug)) + .sort((a, b) => b.slug.length - a.slug.length)[0]; + + if (!article) { + throw new NotFoundException(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/strapi-cms/src/modules/articles/articles.service.ts` around lines 63 - 78, The getArticle method currently uses slug.endsWith(a.slug) and falls back to pages.data.articles[0], which can return the wrong article; update the resolution logic so you first sort pages.data.articles by slug length descending, then find an article using a path-segment-safe predicate (slug === a.slug || slug.endsWith('/' + a.slug)), and if no match is found throw NotFoundException instead of returning pages.data.articles[0]; keep the rest of the flow (mapArticle, baseUrl, graphqlService.getArticle, forkJoin) unchanged.
63-78:⚠️ Potential issue | 🟠 Major
endsWithmatching is not path-segment-safe, and the fallback silently serves the wrong articleTwo distinct bugs on line 73:
Bug 1 — Array ordering determines correctness.
slugSuffix(the last path segment) is sent to GraphQL'sendsWithfilter, which returns every DB article whose slug ends with that segment — including slugs like"my-article"and"sub/my-article". The TypeScriptfindthen checksslug.endsWith(a.slug):slug = "sub/my-article" "sub/my-article".endsWith("my-article") → true ← wrong match if first in the array "sub/my-article".endsWith("sub/my-article") → true ← correct, but only checked if "my-article" didn't arrive firstBecause
Array.findreturns the first truthy result, whichever candidate the DB returns first wins, making routing non-deterministic when multiple articles share a slug suffix.Bug 2 — Silent wrong-article fallback.
|| pages.data.articles[0]!silently serves unrelated content when no article matches. Should throwNotFoundException.The path-segment-safe fix is to anchor the match to a
/boundary and sort candidates by specificity so the most specific slug always wins:🐛 Proposed fix
- const article = pages.data.articles.find((a) => slug.endsWith(a.slug)) || pages.data.articles[0]!; + const article = pages.data.articles + .filter((a) => slug === a.slug || slug.endsWith('/' + a.slug)) + .sort((a, b) => b.slug.length - a.slug.length)[0]; + + if (!article) { + throw new NotFoundException(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/strapi-cms/src/modules/articles/articles.service.ts` around lines 63 - 78, The current getArticle implementation uses endsWith unsafely and falls back to pages.data.articles[0], causing wrong/undeterministic matches; update getArticle so after receiving pages.data.articles you (1) perform a path-segment-safe match: consider an article a a candidate if either a.slug === slugSuffix or slug === a.slug or slug.endsWith('/' + a.slug) (this anchors the suffix to a segment boundary), (2) sort candidates by specificity (e.g., descending a.slug.length) and pick the first result so the most specific slug wins, and (3) if no candidate remains, throw NotFoundException instead of returning pages.data.articles[0]; adjust references inside getArticle (slug, slugSuffix, pages.data.articles, mapArticle) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/next.config.ts`:
- Around line 41-44: The images config entry using protocol: 'https' and
hostname: '*.zendesk.com' is too broad and missing a pathname restriction;
either replace '*.zendesk.com' with the exact Zendesk subdomain(s) your app uses
(pin the known hostnames) or, if you truly cannot enumerate them, add a
restrictive pathname field to this same config object to limit allowed URL paths
and avoid implicit '**' wildcard behavior; also verify whether you need a
single-level '*' versus multi-level '**' for the hostname and adjust accordingly
to match actual Zendesk image hostnames.
In `@packages/integrations/strapi-cms/src/modules/articles/articles.service.ts`:
- Around line 64-65: The slug handling allows empty or "/" to produce slugSuffix
= "" which matches every article; add an early guard before computing
slug/slugSuffix: check params.slug (trimmed) and if it's empty or exactly "/"
then return a safe missing result (e.g., null or a 404) instead of querying;
keep references to params.slug, slug, and slugSuffix in your change and ensure
downstream GraphQL filter logic never receives an empty string.
- Around line 64-65: The code computes slugSuffix from params.slug but doesn't
guard against root/empty slugs so slugSuffix can be "" which matches everything;
update the logic in the articles service (where params.slug, slug, slugSuffix
and the subsequent endsWith checks are used) to first normalize params.slug
(remove leading slash) and then explicitly handle an empty string by returning
null or throwing a not-found error (or skipping the Strapi query) instead of
passing "" into the query; ensure any use of slugSuffix in the Strapi query or
TypeScript endsWith() check is skipped or short-circuited when slug === "" to
prevent matching all articles.
In
`@packages/integrations/strapi-cms/src/modules/articles/graphql/queries/getArticle.graphql`:
- Around line 1-5: The GraphQL query getArticle currently uses the endsWith
filter on articles (filters: { slug: { endsWith: $slug } }) which causes suffix
matches; change the filter to use eq with the full normalized slug (filters: {
slug: { eq: $slug } }) and update the caller in the service that invokes
getArticle to pass the complete normalized slug (not just the slugSuffix).
Ensure the service replaces any existing logic that built or passed slugSuffix
and stops relying on the multi-candidate fallback (and remove/adjust the
client-side fallback comparison like slug.endsWith(a.slug)) so the query returns
a single exact match predictably.
- Around line 1-5: The GraphQL query getArticle currently filters articles with
filters: { slug: { endsWith: $slug } } which causes broad matches; change the
filter to use exact equality (slug: { eq: $slug }) and update the service call
that supplies the variable (stop passing only the last segment, e.g., slugSuffix
or similar) to pass the full normalized slug path instead (the variable name
should be slug) so the query returns a single unambiguous article without
post-filtering via .find().
In `@packages/ui/src/components/RichText/RichText.tsx`:
- Around line 53-70: The ImgComp component currently uses Next.js Image with the
fill prop and forceful CSS overrides which break the fill contract and also
renders when src is empty; change ImgComp so it returns null when src is falsy,
remove the fill prop and the forced/important positioning classes (e.g., remove
className="relative! h-auto!" on the Image and stop coercing src with (src as
string) || ''), and instead render Image with explicit sizing (width/height or
rely on sizes with a natural layout) and keep the wrapper as the layout
container; update references in ImgComp to use the actual src/alt only when
present and ensure object-contain is applied to an absolutely positioned image
only if using fill, otherwise use sizing to preserve aspect ratio.
---
Outside diff comments:
In `@packages/integrations/strapi-cms/src/modules/articles/articles.service.ts`:
- Around line 63-78: The getArticle method currently uses slug.endsWith(a.slug)
and falls back to pages.data.articles[0], which can return the wrong article;
update the resolution logic so you first sort pages.data.articles by slug length
descending, then find an article using a path-segment-safe predicate (slug ===
a.slug || slug.endsWith('/' + a.slug)), and if no match is found throw
NotFoundException instead of returning pages.data.articles[0]; keep the rest of
the flow (mapArticle, baseUrl, graphqlService.getArticle, forkJoin) unchanged.
- Around line 63-78: The current getArticle implementation uses endsWith
unsafely and falls back to pages.data.articles[0], causing wrong/undeterministic
matches; update getArticle so after receiving pages.data.articles you (1)
perform a path-segment-safe match: consider an article a a candidate if either
a.slug === slugSuffix or slug === a.slug or slug.endsWith('/' + a.slug) (this
anchors the suffix to a segment boundary), (2) sort candidates by specificity
(e.g., descending a.slug.length) and pick the first result so the most specific
slug wins, and (3) if no candidate remains, throw NotFoundException instead of
returning pages.data.articles[0]; adjust references inside getArticle (slug,
slugSuffix, pages.data.articles, mapArticle) accordingly.
packages/integrations/strapi-cms/src/modules/articles/articles.service.ts
Show resolved
Hide resolved
packages/integrations/strapi-cms/src/modules/articles/graphql/queries/getArticle.graphql
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.changeset/poor-badgers-grow.md:
- Around line 2-5: The changeset currently marks '@o2s/integrations.zendesk' and
'@o2s/ui' as minor while the reviewer notes those changes are consumer-visible;
decide whether those changes are truly internal: if the zendesk change to
"replacing article body links with relative slugs" and the ui changes (new
ImgComp, Image rendering behavior, pinning markdown-to-jsx to 9.7.0) are
intended to be public API changes keep them as minor, otherwise change their
entries in the .changeset (update the lines for '@o2s/integrations.zendesk' and
'@o2s/ui') from minor to patch to reflect purely internal/bugfix updates so the
release type matches the actual consumer-visible impact.
marcinkrasowski
approved these changes
Feb 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Related Ticket(s)
Key Changes
How to test
Media (Loom or gif)
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style