feat: integrate Zendesk articles and categories #623
Conversation
WalkthroughAdds a full Zendesk Articles integration (service, mapper, module, exports), updates mocked CMS with Zendesk categories/pages, parallelizes OAS/typegen scripts for multiple APIs, hardens category-fetch error handling, updates UI RichText props, registers articles integration in Config, adds comprehensive Zendesk docs, and applies minor markdown/formatting fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as ArticleController
participant Service as ZendeskArticleService
participant ZendeskAPI as Zendesk API
participant Mapper as ArticleMapper
Client->>Controller: GET /articles/:id?locale
Controller->>Service: getArticle({ id, locale })
Service->>ZendeskAPI: resolve slug → articleId
ZendeskAPI-->>Service: articleId
par Parallel enrichment
Service->>ZendeskAPI: fetch article data
ZendeskAPI-->>Service: article payload
and
Service->>ZendeskAPI: fetch category/section
ZendeskAPI-->>Service: category/section payload
and
Service->>ZendeskAPI: fetch attachments & users
ZendeskAPI-->>Service: attachments/users payload
end
Service->>Mapper: mapArticle(article, locale, category?, author?, attachments)
Mapper-->>Service: Articles.Model.Article
Service-->>Controller: Observable<Article>
Controller-->>Client: Article response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/integrations/mocked/src/modules/cms/mappers/cms.page.mapper.ts`:
- Around line 331-333: getAlternativePages is missing the new Zendesk page IDs
so locale switching doesn't resolve alternatives; update the getAlternativePages
implementation to include all PAGE_ZENDESK_* constants (e.g.,
PAGE_ZENDESK_WARRANTY_AND_REPAIR_PL, PAGE_ZENDESK_MAINTENANCE_PL and any other
Zendesk PAGE_ZENDESK_* entries added in mapPage/getAllPages) in the appropriate
arrays or switch branches where other page IDs are listed (the same locations
referenced around lines shown) so the Zendesk pages return their locale
alternates just like the other pages.
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts`:
- Around line 94-133: The Zendesk HTML body is stored and later rendered
unsanitized (see parseBodyIntoSections and ArticleSectionText.content), allowing
scripts to execute; fix by sanitizing the raw HTML before assigning it to
section content and before passing into RichText: integrate a trusted sanitizer
(e.g., DOMPurify) and run DOMPurify.sanitize(body) (or equivalent) inside
parseBodyIntoSections and anywhere ArticleSectionText.content is set, ensuring
extractLeadFromBody still operates on either the sanitized HTML or the original
text-stripped output; update imports and add tests verifying script tags and
event handlers are removed.
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.service.ts`:
- Around line 31-48: The mapLocaleToZendesk function currently omits 'pl' and
doesn't normalize hyphenated or mixed-case locales; update the localeMap inside
mapLocaleToZendesk to include 'pl': 'pl-pl', normalize the input locale to
lower-case at the start of mapLocaleToZendesk, check for '-' on the normalized
value (so hyphenated values like 'en-US' become 'en-us'), and return either the
mapped value or the lower-cased original to ensure the Zendesk format is always
lower-case and includes pl mapping.
- Around line 321-339: fetchArticle currently replaces the original Axios/HTTP
error with a new Error, losing the original status used by upstream handlers
(e.g., getArticle). Instead of wrapping into a plain new Error, propagate the
original error (or create an Error that preserves the original error.status and
message) in the catchError so callers can still read error.status; update
fetchArticle (and apply the same pattern to other fetch helpers) to rethrow the
original error object or copy its status property onto the thrown Error while
preserving the original message and stack.
🧹 Nitpick comments (6)
packages/framework/src/modules/articles/articles.controller.ts (2)
18-21: Consider validating or providing a default forlocaleparameter.The
localequery parameter is implicitly optional. If a client omits it,undefinedwill be passed to the service, which may cause unexpected behavior if the service expects a valid locale string.♻️ Potential approaches
Option 1: Make locale required via validation pipe in the DTO or use
@Query('locale', new ParseStringPipe()).Option 2: Provide a sensible default:
`@Get`('/categories/:id') -getCategory(`@Param`('id') id: string, `@Query`('locale') locale: string) { +getCategory(`@Param`('id') id: string, `@Query`('locale') locale: string = 'en') { return this.articleService.getCategory({ id, locale }); }
28-31: Naming inconsistency: path paramidis passed asslugto service.The route parameter is named
idbut it's passed to the service asslug. This semantic mismatch could confuse API consumers expecting an ID and maintainers reading the code.♻️ Suggested alignment options
Option 1: Rename the route parameter to match the semantic meaning:
-@Get(':id') -getArticle(`@Param`('id') id: string, `@Query`('locale') locale: string) { - return this.articleService.getArticle({ slug: id, locale }); +@Get(':slug') +getArticle(`@Param`('slug') slug: string, `@Query`('locale') locale: string) { + return this.articleService.getArticle({ slug, locale }); }Option 2: If the service actually expects an ID, update the service call:
- return this.articleService.getArticle({ slug: id, locale }); + return this.articleService.getArticle({ id, locale });packages/integrations/zendesk/scripts/oas/fetch-zendesk-oas.ts (1)
13-18: Inconsistent path resolution with sibling.mtsfile.This file uses
__dirname(CommonJS), whilegenerate-zendesk-types.mtsin the same directory usesdirname(fileURLToPath(import.meta.url))(ESM). For consistency and future-proofing (especially if migrating to ESM), consider aligning the approach.♻️ Suggested refactor for ESM-compatible path resolution
import fs from 'node:fs'; import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); interface OASConfig {Note: This would also require renaming the file to
.mtsor ensuring your tsconfig/package.json supports ESM for.tsfiles.packages/blocks/article-list/src/api-harmonization/article-list.service.ts (1)
3-35: Avoid swallowing non‑404 category lookup failures.
Right now any error becomesundefined, which can hide outages/timeouts. Consider only swallowing “not found” and rethrowing everything else (adjust predicate to your HTTP client’s error shape).🔧 Proposed adjustment (narrow the catch)
-import { Observable, catchError, concatMap, forkJoin, map, of } from 'rxjs'; +import { Observable, catchError, concatMap, forkJoin, map, of, throwError } from 'rxjs'; ... - const category = cms.categoryId - ? this.articlesService.getCategory({ id: cms.categoryId, locale: headers['x-locale'] }).pipe( - catchError(() => of(undefined as Articles.Model.Category | undefined)), // If category not found, continue without it - ) + const category = cms.categoryId + ? this.articlesService.getCategory({ id: cms.categoryId, locale: headers['x-locale'] }).pipe( + catchError((err) => + err?.status === 404 + ? of(undefined as Articles.Model.Category | undefined) + : throwError(() => err), + ), + ) : of(undefined);packages/blocks/category-list/src/api-harmonization/category-list.service.ts (1)
3-41: Don’t mask unexpected category errors.
Mapping every failure tonullcan hide upstream outages. Consider rethrowing non‑404 errors (adjust predicate to your client’s error shape).🔧 Proposed adjustment (narrow the catch)
-import { Observable, catchError, concatMap, forkJoin, map, of } from 'rxjs'; +import { Observable, catchError, concatMap, forkJoin, map, of, throwError } from 'rxjs'; ... cms.categoryIds.map((categoryId: string) => this.articlesService.getCategory({ id: categoryId, locale: headers['x-locale'] }).pipe( - catchError(() => of(null)), // Return null if category not found + catchError((err) => + err?.status === 404 ? of(null) : throwError(() => err), + ), ), ),packages/integrations/mocked/src/modules/cms/mappers/cms.category.mapper.ts (1)
423-527: Populate Zendesk mock blocks with visible titles/descriptions.Empty strings will render blank headings in mocked category pages; consider using localized labels (can mirror page SEO titles) to keep mocks realistic.
♻️ Suggested update (apply similarly for DE/PL)
const MOCK_ZENDESK_WARRANTY_BLOCK_EN: CMS.Model.CategoryBlock.CategoryBlock = { @@ - title: '', - description: '', + title: 'Warranty & Repair', + description: 'Warranty & Repair', @@ const MOCK_ZENDESK_MAINTENANCE_BLOCK_EN: CMS.Model.CategoryBlock.CategoryBlock = { @@ - title: '', - description: '', + title: 'Maintenance', + description: 'Maintenance',
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts`:
- Around line 171-177: Guard against undefined article.id before calling
parseBodyIntoSections: compute a safe id (e.g., const articleId = article.id ??
/* fallback or throw/log */) and use that instead of the non-null assertion
(replace article.id! with articleId). Update the mapping logic in the block that
builds sections so you either provide a deterministic fallback ID or
early-return/log an error when article.id is missing, then call
parseBodyIntoSections(article.body, articleId, article.created_at || '',
article.updated_at || '', attachments).
🧹 Nitpick comments (4)
packages/integrations/zendesk/src/modules/articles/zendesk-article.mapper.ts (4)
23-27: Consider externalizing locale path configuration.The hardcoded path mapping works for the current 3 locales but will require code changes when adding new languages. Consider moving this to a configuration file or environment-based config for easier maintenance.
59-90: Consider extracting common slug extraction logic.Both
extractSlugFromUrlandextractCategorySlugFromUrlshare identical patterns differing only in the path segment (articlesvscategories). A generic helper would reduce duplication.♻️ Proposed refactor
+function extractSlugFromUrlByType(htmlUrl: string | undefined, id: number | undefined, type: 'articles' | 'categories'): string { + if (!htmlUrl && id) { + return id.toString(); + } + if (!htmlUrl) { + return ''; + } + const regex = new RegExp(`${type}\\/(\\d+)(?:-([^/]+))?`); + const match = htmlUrl.match(regex); + if (match) { + return match[2] ? `${match[1]}-${match[2]}` : match[1] || ''; + } + return id?.toString() || ''; +} + function extractSlugFromUrl(htmlUrl?: string, id?: number): string { - if (!htmlUrl && id) { - return id.toString(); - } - if (!htmlUrl) { - return ''; - } - const match = htmlUrl.match(/articles\/(\d+)(?:-([^/]+))?/); - if (match) { - return match[2] ? `${match[1]}-${match[2]}` : match[1] || ''; - } - return id?.toString() || ''; + return extractSlugFromUrlByType(htmlUrl, id, 'articles'); } function extractCategorySlugFromUrl(htmlUrl?: string, id?: number): string { - if (!htmlUrl && id) { - return id.toString(); - } - if (!htmlUrl) { - return ''; - } - const match = htmlUrl.match(/categories\/(\d+)(?:-([^/]+))?/); - if (match) { - return match[2] ? `${match[1]}-${match[2]}` : match[1] || ''; - } - return id?.toString() || ''; + return extractSlugFromUrlByType(htmlUrl, id, 'categories'); }
235-243: Simplify avatar assignment by removing IIFE.The IIFE pattern adds unnecessary complexity. A simple ternary or variable assignment is clearer.
♻️ Proposed refactor
author: author ? { name: author.name || '', email: author.email, position: author.role, - avatar: (() => { - const photoUrl = getAvatarUrl(author); - return photoUrl - ? { - url: photoUrl, - alt: author.name || '', - } - : undefined; - })(), + avatar: getAvatarUrl(author) + ? { + url: getAvatarUrl(author)!, + alt: author.name || '', + } + : undefined, } : undefined,Or extract to a helper:
function mapAuthorAvatar(author: ZendeskUser): { url: string; alt: string } | undefined { const photoUrl = getAvatarUrl(author); return photoUrl ? { url: photoUrl, alt: author.name || '' } : undefined; }
269-300: Extract shared image selection logic to reduce duplication.The thumbnail/image selection logic is duplicated between
mapArticle(lines 181-212) andmapArticles(lines 269-300). Extract to a helper function.♻️ Proposed helper
interface ImageSelection { thumbnail?: { url: string; alt: string }; image?: { url: string; alt: string }; } function selectImagesFromAttachments( attachments: ZendeskAttachment[], fallbackAlt: string, ): ImageSelection { const inlineImages = attachments.filter( (att) => att.inline && att.content_type?.startsWith('image/') && att.content_url, ); const nonInlineImages = attachments.filter( (att) => !att.inline && att.content_type?.startsWith('image/') && att.content_url, ); const thumbnail = inlineImages[0] ? { url: inlineImages[0].content_url!, alt: inlineImages[0].file_name || fallbackAlt } : nonInlineImages[0] ? { url: nonInlineImages[0].content_url!, alt: nonInlineImages[0].file_name || fallbackAlt } : undefined; const image = nonInlineImages[0] ? { url: nonInlineImages[0].content_url!, alt: nonInlineImages[0].file_name || fallbackAlt } : inlineImages[0] ? { url: inlineImages[0].content_url!, alt: inlineImages[0].file_name || fallbackAlt } : undefined; return { thumbnail, image }; }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.service.ts`:
- Around line 246-251: The current catchError block in
zendesk-article.service.ts (the one that checks error instanceof
NotFoundException and then returns a new NotFoundException using options.id)
masks all other errors; change it so it only converts/returns a
NotFoundException when the incoming error is already a NotFoundException or when
the upstream response clearly indicates 404, and for all other errors rethrow
the original error (i.e., return throwError(() => error)) so network/auth/server
errors are preserved; locate the catchError usage around the code that
references options.id and modify the logic to preserve and rethrow non-NotFound
errors instead of replacing them.
- Around line 189-198: The mapper call currently passes articles.length as the
total, breaking offset pagination; modify the code that calls mapArticles so it
reads the total from the Zendesk API response's count field (e.g.,
response.count or data.count) and pass that value instead of articles.length to
mapArticles, and remove or update the incorrect comment; update any variable
names used to access the API payload (the object that contains articles and
count) so mapArticles receives the true total for pagination.
🧹 Nitpick comments (3)
packages/integrations/zendesk/src/modules/articles/zendesk-article.service.ts (3)
132-137: Fragile 404 detection via string matching.Checking
error?.message?.includes('404')(line 133) is unreliable—it could match unrelated messages containing "404". Prefer checkingerror?.status === 404only, or ensure the upstreamfetchArticlepreserves/exposes the HTTP status properly.♻️ Suggested simplification
catchError((error) => { - if (error?.status === 404 || error?.message?.includes('404')) { + if (error?.status === 404) { return of(undefined); } return throwError(() => error); }),Note: This requires
fetchArticleto preserve the original error's status (see related past review comment on lines 322-340).
296-300: Address the TODO: implement article search.The fallback to
getArticleListignores search-specific parameters (query text, filters). This may return unexpected results for users expecting search functionality.Would you like me to open an issue to track implementing Zendesk's search endpoint (
/api/v2/help_center/articles/search)?
388-389: Redundant NaN check.
Number(options.category)returnsNaNfor non-numeric strings, andNaNis falsy. SocategoryId && !isNaN(categoryId)is equivalent to justcategoryId.♻️ Simplified check
- const useCategoryEndpoint = categoryId && !isNaN(categoryId); + const useCategoryEndpoint = !!categoryId;
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/docs/docs/integrations/articles/zendesk/features.md`:
- Around line 149-151: Update the fenced code blocks that are missing language
identifiers by changing their opening fences from ``` to ```text for each
occurrence; specifically update the blocks containing
"/{locale-base}/{category-id}-{category-name}/{article-id}-{article-title}", the
example "/help-and-support/12345-Maintenance/67890-Tool-Care-Guide", and the
"Help Center" tree (and the other occurrences noted at 159-161 and 191-196) so
the opening fence reads ```text to satisfy markdownlint.
In `@apps/docs/docs/integrations/articles/zendesk/how-to-setup.md`:
- Line 7: Replace the unhyphenated phrase "Open Self Service" in the sentence
"Zendesk Help Center integration for articles in your Open Self Service
application." with the hyphenated form "Open Self‑Service" for consistency;
update the exact string in this file (look for the phrase "Open Self Service
application") and run a repo-wide check to replace any other occurrences to
maintain consistent styling across docs.
In `@apps/docs/docs/integrations/articles/zendesk/overview.md`:
- Line 25: Replace the phrase "Open Self Service" in the overview sentence with
the hyphenated form "Open Self‑Service" to match project style; locate the
sentence that currently reads "This integration connects your Open Self Service
application with Zendesk Help Center…" and update it to use the hyphenated
product name (Open Self‑Service).
|
Tip For best results, initiate chat on the files or code changes.
Just let me know what you need! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/docs/docs/integrations/articles/zendesk/how-to-setup.md`:
- Around line 74-76: Update the Zendesk token auth docs to state the Basic Auth
must be Base64 of the literal string "{email_address}/token:{api_token}"
(include the literal "/token" between the email and colon), and fix both the
ZENDESK_API_TOKEN explanation block and the example that currently shows
"{email}:{api_token}" or "token:{api_token}" so they use
"{email_address}/token:{api_token}" (clarify that "/token" is literal, not a
placeholder).
In
`@packages/integrations/zendesk/src/modules/articles/zendesk-article.service.ts`:
- Around line 173-175: When handling an empty page in the article fetch, we
currently return mapArticlesWithCategories(articles, 0, options.locale, ...,
...) which discards the actual response.count and breaks pagination; change the
branch that checks if (articles.length === 0) to pass the original
response.count (or response.total) into mapArticlesWithCategories instead of 0
so the total is preserved for pagination (i.e., call
mapArticlesWithCategories(articles, response.count, options.locale, [], [], [])
using the same response object that provided articles).
In `@packages/ui/src/components/RichText/RichText.tsx`:
- Around line 241-248: The RichText mapping uses a non-existent 'figure' variant
for TypographyComp; update the typographyVariants definition (the export in
typography.tsx) to add a 'figure' variant with the desired styles or change the
RichText mapping to use an existing variant (e.g., 'image' or 'lead') instead;
locate the TypographyComp usage in RichText.tsx and the typographyVariants
object in typography.tsx and either add an entry keyed 'figure' to
typographyVariants or replace 'variant: "figure"' with a valid existing variant
name so the component no longer references a missing variant.
🧹 Nitpick comments (3)
packages/ui/src/components/RichText/RichText.tsx (1)
21-30: Maketagoptional to match the default and avoid a misleading API surface.
tagis defaulted to'p'but still typed as required, which is inconsistent for direct TSX usage ofTypographyComp. Consider making it optional.♻️ Proposed fix
-const TypographyComp: FC<Readonly<TypographyProps & { children: ReactNode; tag: string }>> = ({ +const TypographyComp: FC<Readonly<TypographyProps & { children: ReactNode; tag?: string }>> = ({ children, tag = 'p', variant, ...props }) => {packages/integrations/zendesk/scripts/oas/generate-zendesk-types.mts (1)
47-55: ConsiderPromise.allSettledfor complete error visibility.Using
Promise.allwill fail fast on the first error, potentially hiding failures in other configs. If you want to see all failures for debugging:♻️ Optional refactor using Promise.allSettled
async function generateAllTypes(): Promise<void> { - try { - await Promise.all(typeGenConfigs.map((config) => generateTypes(config))); - console.log('All Zendesk types generated successfully'); - } catch (error) { - console.error('Error generating Zendesk types:', error); + const results = await Promise.allSettled(typeGenConfigs.map((config) => generateTypes(config))); + const failures = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected'); + if (failures.length > 0) { + failures.forEach((f) => console.error('Generation failed:', f.reason)); process.exit(1); } + console.log('All Zendesk types generated successfully'); }This is optional—the current approach is fine if fail-fast behavior is preferred.
packages/integrations/zendesk/scripts/oas/fetch-zendesk-oas.ts (1)
23-37: Consider adding a timeout to the fetch call.The
fetchcall has no timeout, which could cause the script to hang indefinitely if the Zendesk API is unresponsive. For a build/dev script, adding a reasonable timeout improves reliability:♻️ Optional: Add fetch timeout
async function fetchOAS(config: OASConfig): Promise<void> { try { - const response = await fetch(config.url); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 30000); // 30s timeout + const response = await fetch(config.url, { signal: controller.signal }); + clearTimeout(timeout); if (!response.ok) { throw new Error(`Failed to fetch OAS: ${response.statusText}`); }This is optional for a development script but recommended for CI environments.
What does this PR do?
Integrates Zendesk Help Center API to enable article and category management in O2S
articles, categories, sections, and attachments.
Key Changes
Zendesk Help Center API Integration:
Summary by CodeRabbit