feat: support page revamp #77
Conversation
📝 WalkthroughWalkthroughReworks support article listing to be tag-aware and searchable, extends Strapi types and cached params, updates fetch hook to accept tag/search filters, converts article detail page to an async server component with a small client companion, centralizes Markdown rendering, and exposes a per-request nonce via meta tag. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant L as SupportArticleList (client)
participant T as SupportTags
participant S as SearchBar
participant H as useFetchSupportArticles
participant API as Strapi API
U->>L: Load page / interact
L->>T: render tags (active or grouped)
L->>S: render search input
T-->>L: onClick(tag|null)
S-->>L: onChange(searchQuery)
L->>H: fetch({page, pageSize, tag?, search?})
H->>API: GET /articles?filters[tag|search]...
API-->>H: Articles + meta
H-->>L: Data
alt active tag present
L-->>U: Paginated grid of filtered articles
else no active tag
L-->>U: Grouped-by-tag sections + per-tag "View all"
end
sequenceDiagram
autonumber
participant Browser as U
participant NextServer as S
participant Page as support/[slug]/page.tsx
participant API as Strapi
participant ClientComp as SupportArticleClient
U->>S: Request /support/[slug]
S->>Page: invoke async server component
Page->>API: GET /articles?filters[slug]=...&populate=...
alt article found
API-->>Page: article data
Page-->>S: render HTML (SEO Script, Banner, SupportArticleContent, <ClientComp/>)
S-->>U: HTML
U->>ClientComp: hydrate client component
ClientComp-->>U: back button + optional tag display
else missing/error
Page-->>S: notFound()
S-->>U: 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
9-28: Update JSDoc to document new parameters.The JSDoc comment should be updated to include the newly added
tagandsearchparameters.Apply this diff:
* @param {string} options.slug - Optional slug to filter by specific article + * @param {string} options.tag - Optional tag to filter articles by + * @param {string} options.search - Optional search query to filter by title or summary * @param {boolean} options.populateContent - Whether to include full content (default: false)
🧹 Nitpick comments (12)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
85-94: Consider extracting URL construction to a helper function.The fetch URL construction on line 88 is extremely long (400+ characters) and hard to read/maintain. Consider extracting this to a dedicated helper function that builds the URL from parameters.
Example refactor:
function buildSupportArticlesUrl({ sort, page, pageSize, populateContent, slug, encodedTag, encodedSearch }: { sort: string page: number pageSize: number populateContent: boolean slug?: string encodedTag?: string encodedSearch?: string }): string { const baseUrl = `${process.env.NEXT_PUBLIC_STRAPI_URL}/api/support-articles` const params = new URLSearchParams({ 'populate[0]': 'featuredImg', 'fields[0]': 'slug', 'fields[1]': 'summary', 'fields[2]': 'title', 'fields[3]': 'publishedAt', 'fields[4]': 'tags', 'sort[0]': `publishedAt:${sort}`, 'pagination[page]': page.toString(), 'pagination[pageSize]': pageSize.toString(), 'pagination[withCount]': 'true' }) if (populateContent) params.append('fields[5]', 'content') if (slug) params.append('filters[slug][$eq]', slug) if (encodedTag) params.append('filters[tags][$contains]', encodedTag) if (encodedSearch) { params.append('filters[$or][0][title][$containsi]', encodedSearch) params.append('filters[$or][1][summary][$containsi]', encodedSearch) } return `${baseUrl}?${params.toString()}` }Then use:
const res = await fetch(buildSupportArticlesUrl({...}), {...})app/[lang]/(resources)/support/_components/SupportTags.tsx (2)
20-27: Simplify the active state check.The condition
active === null || active === undefinedis redundant. Sinceactiveis typed asstring | null | undefined, you can simplify this check.Apply this diff:
- className={`rounded-full border px-4 py-2 text-sm ${ - active === null || active === undefined ? 'bg-white/10' : 'bg-transparent' - }`}> + className={`rounded-full border px-4 py-2 text-sm ${ + !active ? 'bg-white/10' : 'bg-transparent' + }`}>
17-39: Consider accessibility enhancements for the tag filter.While the component is functional, consider these accessibility improvements:
- Add
aria-labelto the "All" button (e.g., "Show all articles")- Add
role="group"andaria-label="Filter by tag"to the container- Add
aria-pressedto buttons to indicate toggle stateExample enhancement:
- <div className={`${className ?? ''} mb-10`}> + <div className={`${className ?? ''} mb-10`} role="group" aria-label="Filter by tag"> <div className={'flex flex-wrap gap-3 justify-center'}> <button type={'button'} onClick={() => onClick(null)} + aria-label="Show all articles" + aria-pressed={!active} className={`rounded-full border px-4 py-2 text-sm ${ active === null || active === undefined ? 'bg-white/10' : 'bg-transparent' }`}> {'All'} </button> {tags.map(t => ( <button type={'button'} key={t} onClick={() => onClick(t)} + aria-pressed={active === t} className={`rounded-full border px-4 py-2 text-sm ${active === t ? 'bg-white/10' : 'bg-transparent'}`}> {t} </button> ))} </div> </div>app/[lang]/(resources)/_components/SupportArticleList.tsx (7)
58-70: Optimization: fetchPageSize is always equal to pageSize.Lines 58-59 assign
fetchPageSize = pageSizeunconditionally. The variablefetchPageSizeadds no value and can be replaced withpageSizedirectly.Apply this diff to simplify:
- // server-side filtering: always use the configured pageSize; grouped view will show previews and - // provide "View all" links to the paginated tag view instead of fetching the entire dataset. - const fetchPage = activeTag ? page : 1 - const fetchPageSize = pageSize - const {articles, pagination, isLoading} = useFetchSupportArticles({ - page: fetchPage, - pageSize: fetchPageSize, + page: activeTag ? page : 1, + pageSize, sort, populateContent, cacheArticles, tag: activeTag, search: searchQuery, skip: false })
76-76: Remove unused variable.
filteredArticlesis assigned the value ofarticlesbut doesn't perform any filtering. Since the server already returns filtered results, directly usearticlesthroughout the component.Apply this diff to remove the unused variable:
- // Server returns results already filtered by tag and search; render `articles` directly - const filteredArticles = articles - // When in the grouped view (no active tag), prepare tags and grouped lists const groupedTags = useMemo(() => { - if (!filteredArticles) { + if (!articles) { return [] } - return Array.from(new Set(filteredArticles.flatMap(a => a.tags ?? []))).sort() - }, [filteredArticles]) + return Array.from(new Set(articles.flatMap(a => a.tags ?? []))).sort() + }, [articles]) const groupedArticles = useMemo(() => { const map: Record<string, TSupportArticle[]> = {} for (const t of groupedTags) { - map[t] = (filteredArticles || []).filter(a => (a.tags ?? []).includes(t)) + map[t] = (articles || []).filter(a => (a.tags ?? []).includes(t)) } return map - }, [filteredArticles, groupedTags]) + }, [articles, groupedTags])And update references in the render section:
- {!filteredArticles || filteredArticles.length === 0 ? ( + {!articles || articles.length === 0 ? ( <p className={'my-20 text-center text-2xl text-gray-400'} role={'status'}> @@ -172,7 +172,7 @@ </div> <div className={cl('grid gap-6 md:grid-cols-2 lg:grid-cols-3', gridClassName)}> - {filteredArticles.map((article: TSupportArticle) => ( + {articles.map((article: TSupportArticle) => ( <ArticleCard
79-84: Handle undefined articles gracefully.The null-check on Line 80 only guards against
undefined/nullbut doesn't account for an empty array. While not a bug (empty array returns empty groupedTags), the logic would be clearer by checking array length.Apply this diff to make the intent clearer:
const groupedTags = useMemo(() => { - if (!filteredArticles) { + if (!articles || articles.length === 0) { return [] } - return Array.from(new Set(filteredArticles.flatMap(a => a.tags ?? []))).sort() - }, [filteredArticles]) + return Array.from(new Set(articles.flatMap(a => a.tags ?? []))).sort() + }, [articles])
130-157: Review the responsive width constraint.Lines 131-156 wrap SupportTags and SearchBar in a
w-1/2container. On smaller screens this may be too narrow. Consider using responsive width classes likew-full lg:w-1/2for better mobile UX.Apply this diff to improve mobile responsiveness:
<div className={'flex w-full justify-center'}> - <div className={'w-1/2'}> + <div className={'w-full px-4 lg:w-1/2 lg:px-0'}> <SupportTags
138-148: Simplify tag state derivation.Line 138 uses
urlTag ?? tag ?? null, butactiveTag(Line 54) already computesurlTag ?? tag. ReuseactiveTagfor consistency.Apply this diff:
<SupportTags tags={ activeTag ? Array.from(new Set((articles || []).flatMap(a => a.tags ?? []))) : groupedTags } - active={urlTag ?? tag ?? null} + active={activeTag ?? null} onClick={(t: string | null) => {
160-165: Enhance empty state accessibility.The empty state paragraph uses
role={'status'}which is appropriate for dynamic content updates. However, consider addingaria-live="polite"to ensure screen readers announce changes when search/filter results change.Apply this diff:
<p - className={'my-20 text-center text-2xl text-gray-400'} - role={'status'}> + className={'my-20 text-center text-2xl text-gray-400'} + role={'status'} + aria-live={'polite'}> {emptyMessage} </p>
46-48: Language parameter may be undefined for root routesThere are
app/layout.tsxandapp/page.tsxat the root (outside[lang]), solangcan legitimately be undefined and defaulting to''is expected. If you intend to requirelangacross all routes, consider moving these files underapp/[lang]or adding a redirect from/to a default language.app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
22-22: Enhance sourceTag display with semantic HTML.The source tag paragraph could benefit from a more descriptive label or semantic tag rendering for clarity.
Consider applying this diff for better UX:
- {sourceTag && <p className={'mb-2 text-sm text-gray-400'}>{sourceTag}</p>} + {sourceTag && ( + <p className={'mb-2 text-sm text-gray-400'}> + <span className={'font-semibold'}>{'Category: '}</span> + {sourceTag} + </p> + )}app/[lang]/(resources)/support/[slug]/page.tsx (1)
47-48: Consider making baseUrl configurable with fallback.The
baseUrldefaults to'https://shapeshift.com'ifNEXT_PUBLIC_SITE_URLis missing. This may cause incorrect URLs in non-production environments.Apply this diff to make the fallback more explicit:
// Generate structured data for the support article - const baseUrl = process.env.NEXT_PUBLIC_SITE_URL || 'https://shapeshift.com' + const baseUrl = process.env.NEXT_PUBLIC_SITE_URL ?? (process.env.NODE_ENV === 'production' ? 'https://shapeshift.com' : 'http://localhost:3000') const articleSchema = generateSupportArticleSchema(article, baseUrl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
app/[lang]/(resources)/_components/SupportArticleList.tsx(4 hunks)app/[lang]/(resources)/support/[slug]/page.tsx(2 hunks)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx(1 hunks)app/[lang]/(resources)/support/_components/SupportTags.tsx(1 hunks)app/[lang]/_components/SearchBar.tsx(2 hunks)app/[lang]/_components/strapi/types.ts(1 hunks)app/[lang]/_contexts/CachedArticlesContext.tsx(5 hunks)app/[lang]/_hooks/useFetchSupportArticles.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (2)
app/[lang]/_components/strapi/types.ts (3)
TSupportArticle(303-317)TPagination(125-130)TArticleListResponse(319-324)app/[lang]/_contexts/CachedArticlesContext.tsx (1)
useCachedArticles(114-121)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
useFetchSupportArticles(29-144)app/[lang]/_components/strapi/types.ts (1)
TSupportArticle(303-317)app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
SupportTags(3-40)app/[lang]/_components/SearchBar.tsx (1)
SearchBar(9-23)
app/[lang]/_contexts/CachedArticlesContext.tsx (1)
app/[lang]/_components/strapi/types.ts (1)
TArticleListResponse(319-324)
app/[lang]/(resources)/support/[slug]/page.tsx (2)
app/[lang]/_utils/schema.ts (1)
generateSupportArticleSchema(112-138)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
SupportArticleClient(9-25)
🔇 Additional comments (15)
app/[lang]/_components/strapi/types.ts (1)
303-317: LGTM! Well-documented optional fields for tag filtering.The addition of optional
tagsandtypefields toTSupportArticlealigns well with the tag-based filtering introduced across the PR. The inline comment provides helpful context about the recent addition.app/[lang]/_components/SearchBar.tsx (2)
3-7: LGTM! Flexible className prop added.The optional
inputClassNameprop enhances reusability without breaking existing usage.
18-18: Potential className conflict with width utilities.When
inputClassNamecontains width classes (e.g.,w-full), both the provided width and the defaultw-1/2will be in the className string, leading to specificity conflicts. Consider either:
- Replacing the entire className when
inputClassNameis provided, or- Documenting that
inputClassNameshould include width if customization is needed.Apply this diff for approach 1 (full replacement):
- className={`${inputClassName ?? 'w-1/2'} rounded-2xl border-none bg-white/5 px-5 py-6 text-white outline-none focus:border-none focus:outline-none focus:ring-1 focus:ring-white/50`} + className={inputClassName ?? 'w-1/2 rounded-2xl border-none bg-white/5 px-5 py-6 text-white outline-none focus:border-none focus:outline-none focus:ring-1 focus:ring-white/50'}Or for approach 2, add a JSDoc comment:
+/** + * @param inputClassName - Custom className for the input (should include width if specified) + */ export function SearchBar({searchQuery, setSearchQueryAction, inputClassName}: TSearchProps): JSX.Element {Likely an incorrect or invalid review comment.
app/[lang]/_hooks/useFetchSupportArticles.ts (2)
67-81: LGTM! Cache invalidation correctly includes tag and search.The cache checking logic properly validates the new
tagandsearchparameters alongside existing filters.
116-141: LGTM! Effect dependencies are comprehensive.All new parameters (
tag,search) and their cached counterparts are correctly included in the dependency array.app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
3-16: LGTM! Clean component signature with appropriate guards.The props are well-typed and the early return for empty tags prevents unnecessary rendering.
app/[lang]/_contexts/CachedArticlesContext.tsx (2)
21-31: LGTM! Well-structured type extension for filtering.The addition of optional
type,tag, andsearchfields toTCachedParamsis clean and maintains backward compatibility. The comment provides helpful context.
55-65: LGTM! Consistent initialization across context and state.The new filter fields are correctly initialized to
undefinedin both the context default and state initialization, ensuring consistency.Also applies to: 85-94
app/[lang]/(resources)/_components/SupportArticleList.tsx (3)
94-113: LGTM: ArticleCard implementation is clean and accessible.The component correctly constructs tag-aware URLs, uses semantic HTML, and provides good visual hierarchy.
184-207: LGTM: Grouped view implementation is clean and user-friendly.The grouped-by-tag sections with "View all" links provide good UX and clear navigation. The
.slice(0, 3)preview is a reasonable default.
211-242: LGTM: Pagination logic correctly conditional on activeTag.Pagination only renders when
activeTagis present and there are multiple pages, which aligns with the hybrid view design.app/[lang]/(resources)/support/[slug]/page.tsx (4)
15-16: LGTM: Server component conversion improves SEO and performance.Converting this page to an async server component with server-side data fetching is an excellent architectural choice for a content-driven page.
30-44: LGTM: Fetch logic with appropriate error handling.The server-side fetch correctly handles missing tokens, response errors, and missing article data with appropriate calls to
notFound().
61-69: LGTM: Suspense integration is clean and provides good loading UX.The Suspense boundary with a loading skeleton for the Back button provides a smooth user experience while SupportArticleClient hydrates.
71-75: Remove unnecessary null-check for article.content The Strapi types declarecontent: string(non-optional), so adding a guard isn’t needed.
thesmithdao
force-pushed
the
support-lp
branch
3 times, most recently
from
9381fb0 to
b22e16b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
16-23: Accessibility improvements successfully implemented.The previous review comments have been addressed—
type="button",aria-label, and responsivehidden lg:flexclasses are all in place.
🧹 Nitpick comments (2)
app/[lang]/_hooks/useFetchSupportArticles.ts (2)
88-88: Consider using field names without array indices for maintainability.The hardcoded field array indices (
fields[0]=slug,fields[1]=summary, etc.) are brittle—if the Strapi schema or field order changes, this breaks. Strapi typically allowsfields=slug,summary,title,publishedAt,tagssyntax without indices.Apply this approach for more maintainable field selection:
-`${process.env.NEXT_PUBLIC_STRAPI_URL}/api/support-articles?populate[0]=featuredImg&fields[0]=slug&fields[1]=summary&fields[2]=title&fields[3]=publishedAt&fields[4]=tags&sort[0]=publishedAt:${sort}&pagination[page]=${page}&pagination[pageSize]=${pageSize}&pagination[withCount]=true${populateContent ? '&fields[5]=content' : ''}${slug ? `&filters[slug][$eq]=${slug}` : ''}${encodedTag ? `&filters[tags][$contains]=${encodedTag}` : ''}${encodedSearch ? `&filters[$or][0][title][$containsi]=${encodedSearch}&filters[$or][1][summary][$containsi]=${encodedSearch}` : ''}` +`${process.env.NEXT_PUBLIC_STRAPI_URL}/api/support-articles?populate=featuredImg&fields=slug,summary,title,publishedAt,tags${populateContent ? ',content' : ''}&sort=publishedAt:${sort}&pagination[page]=${page}&pagination[pageSize]=${pageSize}&pagination[withCount]=true${slug ? `&filters[slug][$eq]=${slug}` : ''}${encodedTag ? `&filters[tags][$contains]=${encodedTag}` : ''}${encodedSearch ? `&filters[$or][0][title][$containsi]=${encodedSearch}&filters[$or][1][summary][$containsi]=${encodedSearch}` : ''}`
126-130: Optional: Simplify pagination dependencies.Lines 127-130 list individual pagination properties, but line 126 already references
cachedResponse.meta.pagination. The granular properties are redundant. Consider removing lines 127-130 to reduce verbosity without changing behavior.cachedResponse.data, cachedResponse.meta.pagination, - cachedResponse.meta.pagination.page, - cachedResponse.meta.pagination.pageCount, - cachedResponse.meta.pagination.pageSize, - cachedResponse.meta.pagination.total, page,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/[lang]/(resources)/_components/SupportArticleList.tsx(4 hunks)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx(2 hunks)app/[lang]/(resources)/support/[slug]/page.tsx(2 hunks)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx(1 hunks)app/[lang]/(resources)/support/_components/SupportTags.tsx(1 hunks)app/[lang]/_components/SearchBar.tsx(2 hunks)app/[lang]/_components/strapi/types.ts(1 hunks)app/[lang]/_contexts/CachedArticlesContext.tsx(5 hunks)app/[lang]/_hooks/useFetchSupportArticles.ts(4 hunks)app/layout.tsx(3 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/[lang]/(resources)/support/_components/SupportTags.tsx
- app/[lang]/_contexts/CachedArticlesContext.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
app/[lang]/(resources)/support/[slug]/page.tsx (3)
app/[lang]/_utils/schema.ts (1)
generateSupportArticleSchema(112-138)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
SupportArticleClient(9-27)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (1)
SupportArticleContent(16-234)
app/layout.tsx (1)
next.config.ts (1)
headers(13-53)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
useFetchSupportArticles(29-144)app/[lang]/_components/strapi/types.ts (1)
TSupportArticle(303-317)app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
SupportTags(3-40)app/[lang]/_components/SearchBar.tsx (1)
SearchBar(9-23)
app/[lang]/_hooks/useFetchSupportArticles.ts (2)
app/[lang]/_components/strapi/types.ts (3)
TSupportArticle(303-317)TPagination(125-130)TArticleListResponse(319-324)app/[lang]/_contexts/CachedArticlesContext.tsx (1)
useCachedArticles(114-121)
🔇 Additional comments (5)
app/[lang]/_components/SearchBar.tsx (1)
6-18: LGTM! Clean implementation of customizable input width.The optional
inputClassNameprop follows good patterns: properly typed, uses nullish coalescing for the default, and integrates cleanly into the existing component.app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
9-27: LGTM! Clean client component for navigation and tag context.The component correctly reads query parameters, provides accessible back navigation, and conditionally displays the source tag. Implementation is straightforward and follows Next.js App Router patterns.
app/[lang]/_hooks/useFetchSupportArticles.ts (3)
85-88: LGTM! Proper URL encoding prevents injection.The use of
encodeURIComponentfor bothtagandsearchcorrectly prevents URL injection attacks. The Strapi filter syntax for tag contains and search OR conditions looks correct.
116-141: Dependency array is complete.All new parameters (
tag,search) are correctly included in both their primitive forms and ascachedParamsproperties. The array correctly tracks all relevant dependencies.
73-75: Resolved:TCachedParamsincludestagandsearch
Confirmedtag?andsearch?are defined onTCachedParamsinCachedArticlesContext.tsx, so the cache equality check is valid.
app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx
Outdated
Show resolved
Hide resolved
thesmithdao
force-pushed
the
support-lp
branch
from
b22e16b to
eb024d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
86-92: Optimize groupedArticles computation.The current implementation filters
filteredArticlesfor each tag (O(tags × articles)). For better performance, consider grouping articles in a single pass:const groupedArticles = useMemo(() => { const map: Record<string, TSupportArticle[]> = {} - for (const t of groupedTags) { - map[t] = (filteredArticles || []).filter(a => (a.tags ?? []).includes(t)) + for (const article of filteredArticles || []) { + for (const t of article.tags ?? []) { + if (!map[t]) map[t] = [] + map[t].push(article) + } } return map -}, [filteredArticles, groupedTags]) +}, [filteredArticles])
94-113: Consider hoisting ArticleCard outside the component.Defining
ArticleCardinside the parent component causes it to be recreated on every render. For a minor performance optimization, consider hoisting it outside as a standalone component or memoizing it withuseCallback. This is optional and unlikely to cause noticeable issues given the current usage.
132-155: Past review comment addressed; consider memoizing tag derivation.The tag pill survival logic (lines 135-140) correctly ensures that the active tag remains in the list even when filtered results are empty, addressing the previous review concern. For a minor performance improvement, consider extracting the IIFE into a
useMemo:+const displayedTags = useMemo(() => { + if (!activeTag) return groupedTags + const derived = Array.from(new Set((articles || []).flatMap(a => a.tags ?? []))) + return derived.length > 0 ? derived : [activeTag] +}, [activeTag, articles, groupedTags]) + <SupportTags - tags={ - activeTag - ? (() => { - const derived = Array.from( - new Set((articles || []).flatMap(a => a.tags ?? [])) - ) - return derived.length > 0 ? derived : [activeTag] - })() - : groupedTags - } + tags={displayedTags} active={activeTag} onClick={(t: string | null) => {
215-247: Consider syncing page state with URL query params.Pagination state is kept in local state (
page) but not reflected in the URL. If a user navigates to page 3 and refreshes, they'll be reset to page 1. For better UX and shareable URLs, consider syncing thepagestate with a?page=query parameter.Example:
// Read page from URL on mount const urlPage = searchParams?.get('page') const [page, setPage] = useState(urlPage ? parseInt(urlPage, 10) : initialPage) // Update URL when page changes function handlePageChange({selected}: {selected: number}) { const newPage = selected + 1 setPage(newPage) const params = new URLSearchParams(Array.from(searchParams || [])) params.set('page', newPage.toString()) router.push(`/${lang}/support?${params.toString()}`) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/[lang]/(resources)/_components/SupportArticleList.tsx(4 hunks)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx(2 hunks)app/[lang]/(resources)/support/[slug]/page.tsx(2 hunks)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx(1 hunks)app/[lang]/(resources)/support/_components/SupportTags.tsx(1 hunks)app/[lang]/_components/SearchBar.tsx(2 hunks)app/[lang]/_components/strapi/types.ts(1 hunks)app/[lang]/_contexts/CachedArticlesContext.tsx(5 hunks)app/[lang]/_hooks/useFetchSupportArticles.ts(4 hunks)app/layout.tsx(3 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/[lang]/_components/SearchBar.tsx
- app/layout.tsx
- app/[lang]/_contexts/CachedArticlesContext.tsx
- app/[lang]/(resources)/support/_components/SupportTags.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/[lang]/(resources)/support/[slug]/page.tsx (3)
app/[lang]/_utils/schema.ts (1)
generateSupportArticleSchema(112-138)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
SupportArticleClient(9-27)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (1)
SupportArticleContent(16-234)
app/[lang]/_hooks/useFetchSupportArticles.ts (2)
app/[lang]/_components/strapi/types.ts (3)
TSupportArticle(303-317)TPagination(125-130)TArticleListResponse(319-324)app/[lang]/_contexts/CachedArticlesContext.tsx (1)
useCachedArticles(114-121)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
useFetchSupportArticles(29-144)app/[lang]/_components/strapi/types.ts (1)
TSupportArticle(303-317)app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
SupportTags(3-40)app/[lang]/_components/SearchBar.tsx (1)
SearchBar(9-23)
🔇 Additional comments (11)
tsconfig.json (1)
36-36: Confirm type-check coverage forscriptsExcluding the
scriptsdirectory means any TypeScript there will no longer be validated by this config. If we still rely on TS scripts (e.g., build/maintenance tooling), we should either keep them included or give them their owntsconfig. Can you confirm we have coverage elsewhere before merging this?app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
9-27: LGTM! Client component properly handles navigation and tag display.The component correctly uses client-side hooks (
useRouter,useSearchParams) to manage back navigation and conditionally display the source tag. The Back button accessibility improvements (type, aria-label, responsive hiding) have been properly addressed per previous review feedback.app/[lang]/_components/strapi/types.ts (1)
314-316: LGTM! Type extensions support tag-based filtering.The addition of optional
tagsandtypefields toTSupportArticlealigns with the broader tag-aware filtering and grouping features introduced in this PR. The optional nature ensures backward compatibility with existing data.app/[lang]/_hooks/useFetchSupportArticles.ts (3)
29-54: LGTM! Hook parameters correctly extended for tag and search filtering.The addition of
tagandsearchparameters properly extends the hook's capabilities to support the tag-aware filtering and client-side search features described in the PR objectives.
85-88: LGTM! URL construction and filtering logic are correct.The Strapi filter syntax is properly formed:
filters[tags][$contains]for tag matchingfilters[$or][0][title][$containsi]andfilters[$or][1][summary][$containsi]for case-insensitive search across title and summaryThe conditional field indexing for
contentwhenpopulateContentis true is also correct.
61-81: LGTM! Cache checks correctly include new filter parameters.The cache equality logic properly accounts for
tagandsearchparameters, ensuring cached results are only used when all filtering conditions match.app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (2)
84-110: LGTM! Image renderer correctly fixed to prevent prop override.The destructuring pattern
{src, alt, ...rest}and spreading onlyrestensures the sanitizedsrcStringandaltTextare preserved and not overwritten by the original props. The fixed-height container and lazy loading also help prevent cumulative layout shift (CLS) as intended.This resolves the critical issue flagged in the previous review.
17-149: LGTM! Centralized component mapping improves maintainability.The refactor to a single
componentsobject passed to ReactMarkdown enhances code organization and makes custom rendering logic easier to maintain. The styling and semantic choices for headers, code blocks, tables, and other elements are appropriate.app/[lang]/(resources)/support/[slug]/page.tsx (2)
15-47: LGTM! Server-side data fetching and validation correctly implemented.The conversion to an async server component with proper slug validation (regex check), environment variable error handling (throws descriptive error), and server-side Strapi fetch is well executed. The conditional Authorization header and appropriate use of
notFound()for missing articles align with Next.js best practices.All previously flagged issues (slug validation, config error handling) have been properly addressed.
62-70: LGTM! Suspense boundary correctly isolates client component.Wrapping
SupportArticleClientin a Suspense boundary with a skeleton fallback is a best practice for server components that include client-side interactivity. The skeleton matches the visual layout of the Back button, providing a smooth loading experience.app/[lang]/(resources)/_components/SupportArticleList.tsx (1)
56-60: Verify grouped view article coverage. When no tag is active, the hook fetches onlypageSize(default 12) items then groups and slices to 3 per tag—if there are more distinct tags or uneven distribution, some tags or preview slots will be omitted. Confirm this matches product requirements or adjust the fetch size or grouping strategy. [app/[lang]/(resources)/_components/SupportArticleList.tsx:56-60]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/[lang]/(resources)/_components/SupportArticleList.tsx (2)
130-162: Consider memoizing the derived tag list.The inline IIFE (lines 135-140) runs on every render. While the past review comment has been addressed functionally (empty results now preserve the active tag), memoizing this computation would improve performance.
+ const displayedTags = useMemo(() => { + if (!activeTag) return groupedTags + const derived = Array.from(new Set((articles || []).flatMap(a => a.tags ?? []))) + return derived.length > 0 ? derived : [activeTag] + }, [activeTag, articles, groupedTags]) + // ... later in JSX ... <SupportTags - tags={ - activeTag - ? (() => { - const derived = Array.from( - new Set((articles || []).flatMap(a => a.tags ?? [])) - ) - return derived.length > 0 ? derived : [activeTag] - })() - : groupedTags - } + tags={displayedTags} active={activeTag}
215-247: Consider adding forcePage prop for controlled pagination.The pagination correctly renders only for tag-filtered views. However, without
forcePage, the component maintains its own internal state for the active page, which could desynchronize from yourpagestate if the user navigates via other means (e.g., browser back button or tag changes).<ReactPaginate + forcePage={page - 1} pageCount={pagination?.pageCount ?? 1} pageRangeDisplayed={5} marginPagesDisplayed={2} onPageChange={({selected}) => setPage(selected + 1)}This ensures the visual active page always matches your component state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/[lang]/(resources)/_components/SupportArticleList.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
useFetchSupportArticles(29-144)app/[lang]/_components/strapi/types.ts (1)
TSupportArticle(303-317)app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
SupportTags(3-40)app/[lang]/_components/SearchBar.tsx (1)
SearchBar(9-23)
🔇 Additional comments (12)
app/[lang]/(resources)/_components/SupportArticleList.tsx (12)
1-20: LGTM! Imports are well-organized.All dependencies are properly imported and organized by source (external, internal, local, types).
22-31: LGTM! Type definition is complete.The props interface correctly includes all configuration options, including the new
tagprop for URL-driven filtering.
33-54: LGTM! State management is well-structured.The component correctly initializes state, derives
activeTagfrom URL parameters (with fallback to prop), and sets up client-side search state.
56-73: LGTM! Data fetching logic is correct.The conditional fetching strategy correctly handles both grouped (no tag) and paginated (with tag) views. The page reset on tag/search change prevents stale pagination state.
75-92: LGTM! Data preparation is efficient.Server-side filtering is correctly leveraged, and the memoized grouping logic efficiently prepares data for the grouped view with appropriate dependencies.
94-113: LGTM! ArticleCard component is well-implemented.The component correctly constructs tag-aware URLs and provides consistent article card rendering throughout the page.
115-118: LGTM! Loading state handled appropriately.The skeleton provides good UX during initial load.
120-128: LGTM! Page header is well-structured.Title and description are properly centered and styled.
164-170: LGTM! Empty state is accessible.Proper use of
role="status"for screen readers.
171-187: LGTM! Paginated tag view correctly implements PR objectives.The active tag is displayed as a heading and passed to
ArticleCard(line 182), ensuring article URLs preserve tag context as required.
188-213: LGTM! Grouped view correctly implements PR objectives.Each tag section shows exactly 3 preview cards with a "View all" link to the filtered paginated view. Tag context is preserved in article URLs.
249-256: LGTM! Component structure is complete.Proper closing with Banner component at the bottom.
There was a problem hiding this comment.
Weird tab bahavior: https://jam.dev/c/676eac47-c855-4d8b-bd81-f35aee5887d2
They should probably behave more like radio buttons, rather than having all unselected options disappear.
Search input needs debouncing or other optimizations: https://jam.dev/c/70c35a1c-ea7d-4368-af07-b152736ba268
Space poorly used on categories with only 1 article:
app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx
Outdated
Show resolved
Hide resolved
|
I'm converting to draft so I can work on those comments =) |
thesmithdao
force-pushed
the
support-lp
branch
from
65f5967 to
c9870c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/[lang]/(resources)/_components/SupportArticleList.tsx(5 hunks)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx(2 hunks)app/[lang]/(resources)/support/[slug]/page.tsx(2 hunks)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx(1 hunks)app/[lang]/(resources)/support/_components/SupportTags.tsx(1 hunks)app/[lang]/_components/SearchBar.tsx(2 hunks)app/[lang]/_components/strapi/types.ts(1 hunks)app/[lang]/_contexts/CachedArticlesContext.tsx(5 hunks)app/[lang]/_hooks/useFetchSupportArticles.ts(4 hunks)app/layout.tsx(3 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/layout.tsx
- app/[lang]/_contexts/CachedArticlesContext.tsx
- tsconfig.json
- app/[lang]/_components/SearchBar.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/[lang]/_hooks/useFetchSupportArticles.ts (2)
app/[lang]/_components/strapi/types.ts (3)
TSupportArticle(303-317)TPagination(125-130)TArticleListResponse(319-324)app/[lang]/_contexts/CachedArticlesContext.tsx (1)
useCachedArticles(114-121)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
useFetchSupportArticles(29-144)app/[lang]/_components/strapi/types.ts (1)
TSupportArticle(303-317)app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
SupportTags(3-43)app/[lang]/_components/SearchBar.tsx (1)
SearchBar(9-23)
app/[lang]/(resources)/support/[slug]/page.tsx (3)
app/[lang]/_utils/schema.ts (1)
generateSupportArticleSchema(112-138)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
SupportArticleClient(9-27)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (1)
SupportArticleContent(15-237)
There was a problem hiding this comment.
I updated the support pages and a few helpers: in SupportArticleList.tsx so server queries use the configured pageSize), added an allTags discovery/merge flow so the tag bar behaves like radio buttons (active tag stays visible); in SupportArticleContent.tsx I tightened types by replacing any usages (cast components to Components and safely read image src without any).
thesmithdao
force-pushed
the
support-lp
branch
from
c9870c4 to
058079e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/[lang]/(resources)/support/_components/SupportTags.tsx (2)
21-28: Consider adding accessibility attributes for active state.The "All" button would benefit from
aria-current="page"oraria-pressedwhen active to better communicate state to assistive technologies.Apply this diff to improve accessibility:
<button type={'button'} onClick={() => onClick(null)} + aria-pressed={active === null || active === undefined} className={`rounded-full border text-sm whitespace-nowrap px-3 py-1.5 sm:px-4 sm:py-2 ${ active === null || active === undefined ? 'bg-blue' : 'bg-transparent' }`}> {'All'} </button>
29-39: Consider adding accessibility attributes for tag buttons.Similar to the "All" button, tag filter buttons would benefit from
aria-pressedto communicate active state to screen readers.Apply this diff:
<button type={'button'} key={t} onClick={() => onClick(t)} + aria-pressed={active === t} className={`rounded-full border text-sm whitespace-nowrap px-3 py-1.5 sm:px-4 sm:py-2 ${ active === t ? 'bg-blue' : 'bg-transparent' }`}> {t} </button>app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (1)
71-95: Consider extracting language detection logic.The code block renderer has inline language detection (
/language-(\w+)/.exec(className || '')) that could be extracted to a helper function for better testability and reusability.Example refactor:
function extractLanguage(className?: string): string | null { if (!className) return null const match = /language-(\w+)/.exec(className) return match ? match[1] : null }Then use in the renderer:
-const match = /language-(\w+)/.exec(className || '') +const language = extractLanguage(className) -return match ? ( +return language ? ( <div className={'relative'}> - <div className={'absolute right-2 top-2 text-xs text-gray-400'}>{match[1]}</div> + <div className={'absolute right-2 top-2 text-xs text-gray-400'}>{language}</div>app/[lang]/(resources)/_components/SupportArticleList.tsx (1)
239-240: Consider documenting the 9-item limit.The
.slice(0, 9)limit for preview articles is hardcoded without explanation. Consider extracting this to a named constant or adding a comment explaining why 9 items (likely to match a 3-column grid).Example:
const PREVIEW_LIMIT = 9 // 3 rows × 3 columns in desktop gridThen use:
- .slice(0, 9) + .slice(0, PREVIEW_LIMIT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/[lang]/(resources)/_components/SupportArticleList.tsx(5 hunks)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx(2 hunks)app/[lang]/(resources)/support/[slug]/page.tsx(2 hunks)app/[lang]/(resources)/support/_components/SupportTags.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/[lang]/(resources)/_components/SupportArticleList.tsx (4)
app/[lang]/_hooks/useFetchSupportArticles.ts (1)
useFetchSupportArticles(29-144)app/[lang]/_components/strapi/types.ts (1)
TSupportArticle(303-317)app/[lang]/(resources)/support/_components/SupportTags.tsx (1)
SupportTags(3-43)app/[lang]/_components/SearchBar.tsx (1)
SearchBar(9-23)
app/[lang]/(resources)/support/[slug]/page.tsx (3)
app/[lang]/_utils/schema.ts (1)
generateSupportArticleSchema(112-138)app/[lang]/(resources)/support/_components/SupportArticleClient.tsx (1)
SupportArticleClient(9-27)app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (1)
SupportArticleContent(30-325)
🔇 Additional comments (9)
app/[lang]/(resources)/support/[slug]/SupportArticleContent.tsx (2)
18-28: LGTM: sanitizeParams implementation addresses past concerns.The helper function correctly filters out react-markdown internal props (
node,inline,index,children) before spreading onto DOM elements, addressing the previous concern about unknown props causing React warnings.
138-171: LGTM: Image renderer correctly handles src destructuring.The fix from the previous review has been properly applied—
srcandaltare extracted from the params object and not included in therestspread, ensuring Next/Image receives only the sanitizedsrcStringandaltTextwithout the original values overwriting them.app/[lang]/(resources)/_components/SupportArticleList.tsx (3)
224-250: LGTM: Active tag rendering correctly handles single-tag view.The conditional branch properly renders a flat grid when
activeTagis active, preventing the duplication issue flagged in previous comments. The fallback tofilteredArticleswhengroupedArticles[activeTag]is undefined ensures edge cases are handled.
303-303: LGTM: Pagination correctly shown only for tag-filtered views.The condition
activeTag && pagination && pagination.pageCount > 1properly limits pagination to the paginated tag view, preventing it from appearing in the grouped preview mode as intended by the PR objectives.
109-142: No sequential fetch: discovery prefetch runs in parallel
The discovery hook’sskip: allTags !== nullcheck only gates subsequent calls—on mount both the filtered and unfiltered fetches fire concurrently whenallTagsis null, so there’s no user-fetch → discovery-fetch waterfall.app/[lang]/(resources)/support/[slug]/page.tsx (4)
21-24: LGTM: Slug validation properly prevents injection.The regex validation (
/^[A-Za-z0-9-]+$/) and earlynotFound()return correctly address the security concern from the previous review, ensuring only valid slugs proceed to the API call.
28-30: LGTM: Configuration error handling improved.Throwing an explicit
Errorinstead of callingnotFound()ensures configuration issues surface during build/deployment rather than appearing as 404s to users, as recommended in the previous review.
35-39: LGTM: Cache control prevents stale content.The
cache: 'no-store'option ensures support articles are always fetched fresh from Strapi, addressing the concern from the previous review about default force-cache behavior in app router server components.
64-72: Verify Suspense boundary placement for optimal loading UX.The Suspense wrapper around
SupportArticleClientprovides a loading skeleton for the back button and tag label, which is good. However, confirm that the main article content (lines 74-77) doesn't cause layout shift while the server component is rendering, since it's outside the Suspense boundary.Based on the server component architecture, the entire page should render server-side before streaming to the client, so the article content should be immediately available. Verify this behavior in your testing.
2 participants
Support Page Revamp
Summary
Adds a grouped-by-tag support landing (preview cards), tag-filtered paginated pages for SEO, server-rendered article pages preserving tag context, client search, and robust markdown rendering.
Key changes
/support: grouped tag overview with preview cards and “View all”./support?tag=<Tag>: tag-filtered, paginated list (SEO-friendly)./support/<slug>?tag=<Tag>: article page that preserves source tag.ype="button"; tag-discovery preserves master list.'no-store'.Quick tests
/support→ grouped sections with preview cards + “View all”./support?tag=<Tag>(pagination if > pageSize)./support/<slug>?tag=<Tag>; Back returns to filtered list.Acceptance
Before merge
After merge