(SP: 2) [Frontend] Refactor Q&A Tabs: Extract Logic & Align Layout#129
Conversation
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@ViktorSvertoka has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracts Q&A tab/search/pagination logic into a dedicated hook, centralizes Q&A types, replaces the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QaSection
participant useQaTabs
participant Router
participant API
participant AccordionList
User->>QaSection: interact (search / change tab / paginate)
QaSection->>useQaTabs: call handlers (setSearch / changeCategory / changePage)
useQaTabs->>Router: replace URL (category, page, search)
useQaTabs->>API: GET /api/questions/{category}?page=&limit=10&locale=&search=
API-->>useQaTabs: return paginated response
useQaTabs-->>QaSection: provide items, totalPages, isLoading
QaSection->>AccordionList: render items
QaSection->>User: update UI (tabs, results, pagination)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In @frontend/components/q&a/useQaTabs.ts:
- Around line 100-104: The fetch in useQaTabs.ts assigns the response to res and
immediately calls res.json(); add a response status check: after receiving res,
if (!res.ok) await res.text() (or include res.status/res.statusText) and throw
or return a structured error so non-2xx responses aren’t blindly parsed as JSON;
update the caller (the code using currentPage, searchParam and expecting
PaginatedResponse<QuestionApiItem>) to handle the thrown error or error return
path accordingly.
- Line 38: pageFromUrl can be NaN if searchParams.get('page') is non-numeric;
validate and fallback to 1 by parsing and checking isFinite/Number.isNaN (e.g.,
parseInt or Number then test), so replace the direct Number(...) assignment for
pageFromUrl with logic that parses the value, verifies it's a positive integer,
and falls back to 1 before using it to initialize state in useQaTabs
(referencing pageFromUrl and searchParams).
🧹 Nitpick comments (5)
frontend/components/q&a/QaSection.tsx (4)
12-12: Naming inconsistency: file exportsTabsSectionbut file is namedQaSection.tsx.The file is named
QaSection.tsxbut exportsfunction TabsSection(). This creates confusion as the import inpage.tsxusesQaSection(relying on the filename). Consider renaming the function to match the file name for clarity.♻️ Suggested fix
-export default function TabsSection() { +export default function QaSection() {
43-48: Add accessible label to the clear search button.The clear button lacks an
aria-label, which impacts accessibility for screen reader users.♿ Suggested fix
<button onClick={clearSearch} className="absolute right-3 top-1/2 -translate-y-1/2" + aria-label={t('clearSearch')} > <X className="h-5 w-5" /> </button>Note: You'll need to add the
clearSearchtranslation key, or use a hardcoded label likearia-label="Clear search".
70-72: Spinner may be invisible due to missing border color.The spinner only has
border-b-2without specifying a color. Depending on the theme, this could render as transparent or match the background.🎨 Suggested fix
<div className="flex justify-center py-12"> - <div className="animate-spin h-8 w-8 border-b-2" /> + <div className="animate-spin h-8 w-8 border-b-2 border-blue-600 rounded-full" /> </div>
53-53: Consider addressing the root cause of!importantoverrides.Using multiple
!importantmodifiers (!bg-transparent !p-0 !h-auto !w-full) suggests the baseTabsListstyles conflict with this use case. This is a code smell that may cause maintenance issues.Consider either:
- Adjusting the base
TabsListcomponent to accept variant props- Using a more specific selector or custom component
frontend/components/q&a/types.ts (1)
1-1:'use client'directive is unnecessary for a types-only file.This file only contains TypeScript type definitions, interfaces, and constants. The
'use client'directive has no effect here since types are erased at compile time and don't execute in the browser. Removing it improves clarity.♻️ Suggested fix
-'use client'; - import { categoryData } from '@/data/category';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/[locale]/q&a/page.tsxfrontend/components/q&a/AccordionList.tsxfrontend/components/q&a/QaSection.tsxfrontend/components/q&a/TabsSection.tsxfrontend/components/q&a/types.tsfrontend/components/q&a/useQaTabs.ts
💤 Files with no reviewable changes (1)
- frontend/components/q&a/TabsSection.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/components/q&a/QaSection.tsx (5)
frontend/components/q&a/useQaTabs.ts (1)
useQaTabs(30-171)frontend/components/ui/tabs.tsx (4)
Tabs(66-66)TabsList(66-66)TabsTrigger(66-66)TabsContent(66-66)frontend/data/category.ts (1)
categoryData(11-22)frontend/components/q&a/AccordionList.tsx (1)
AccordionList(237-255)frontend/components/q&a/Pagination.tsx (1)
Pagination(11-122)
frontend/components/q&a/types.ts (1)
frontend/data/category.ts (1)
categoryData(11-22)
frontend/components/q&a/useQaTabs.ts (2)
frontend/data/category.ts (1)
categoryData(11-22)frontend/components/q&a/types.ts (6)
Locale(7-7)qaConstants(95-97)CategorySlug(8-8)QuestionEntry(70-75)PaginatedResponse(87-93)QuestionApiItem(77-85)
🔇 Additional comments (8)
frontend/components/q&a/AccordionList.tsx (2)
12-25: LGTM! Clean centralization of type imports.The type imports from the centralized
types.tsmodule are well-organized and comprehensive. This improves maintainability by having a single source of truth for Q&A-related types.
27-34: Type guard implementation is correct.The
isListItemBlockguard properly validates the object structure with appropriate null/type checks before accessing properties.frontend/app/[locale]/q&a/page.tsx (1)
3-3: LGTM! Component swap and layout alignment look correct.The switch from
TabsSectiontoQaSectionand the updated container classes (max-w-7xlwith responsive padding) align with the PR objectives to match the Quizzes layout.Also applies to: 21-23
frontend/components/q&a/useQaTabs.ts (2)
72-89: LGTM! Debounce implementation is well-structured.The debounce logic correctly:
- Skips the initial mount using
mountedRef- Clears previous timeouts before setting new ones
- Properly cleans up on unmount via the effect cleanup function
127-139: LGTM! Handlers are properly memoized.The
handleCategoryChange,handlePageChange, andclearSearchcallbacks are correctly wrapped withuseCallbackand have appropriate dependency arrays.Also applies to: 141-148, 150-155
frontend/components/q&a/types.ts (3)
5-8: LGTM! Locale and CategorySlug types are well-derived.Deriving
LocalefromSUPPORTED_LOCALESandCategorySlugfromcategoryDataensures type safety while maintaining a single source of truth.
24-41: Well-designed recursive type structure for nested lists.The circular type references (
ListEntry→ListItemBlock→ListItemChild→BulletListBlock/NumberedListBlock→ListEntry) correctly model nested list structures. TypeScript handles this pattern properly with type aliases.
70-93: LGTM! API and domain interfaces are clean and well-defined.The separation between
QuestionEntry(UI model) andQuestionApiItem(API response model) follows good practices. The genericPaginatedResponse<T>enables reuse for other paginated endpoints.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @frontend/components/q&a/useQaTabs.ts:
- Around line 103-105: The fetch call in useQaTabs is using the raw locale param
instead of the validated localeKey; change the URL construction so it uses
localeKey (not locale) when calling fetch(`/api/questions/...`) and ensure any
useEffect dependency array that watches locale is updated to watch localeKey
instead (update the dependency array referenced near the fetch logic so effects
run based on the validated localeKey).
🧹 Nitpick comments (3)
frontend/components/q&a/useQaTabs.ts (3)
58-73: Variable shadowing:paramsshadows outer scope.The
paramsvariable on line 60 shadows theparamsfromuseParams()on line 33. Consider renaming tosearchParamsorurlParamsfor clarity.♻️ Suggested fix
const updateUrl = useCallback( (category: CategorySlug, page: number, search: string) => { - const params = new URLSearchParams(); + const urlParams = new URLSearchParams(); - if (category !== DEFAULT_CATEGORY) params.set('category', category); - if (page > 1) params.set('page', String(page)); - if (search) params.set('search', search); + if (category !== DEFAULT_CATEGORY) urlParams.set('category', category); + if (page > 1) urlParams.set('page', String(page)); + if (search) urlParams.set('search', search); - const queryString = params.toString(); + const queryString = urlParams.toString(); router.replace(`/q&a${queryString ? `?${queryString}` : ''}`, { scroll: false, }); }, [router] );
75-92: Debounce effect triggers unnecessarily on category change.When
handleCategoryChangeis called, it directly updates URL and resets search. However, this effect also runs becauseactiveis in the dependency array, scheduling a redundant debounced URL update.Consider excluding
activefrom triggering this debounce logic, or adding a condition to skip whensearchQueryis unchanged.♻️ Suggested approach: track previous search to avoid redundant updates
+ const prevSearchRef = useRef(searchFromUrl); useEffect(() => { if (!mountedRef.current) { mountedRef.current = true; return; } + // Skip if search hasn't actually changed + if (searchQuery === prevSearchRef.current) { + return; + } + prevSearchRef.current = searchQuery; if (debounceRef.current) clearTimeout(debounceRef.current); debounceRef.current = setTimeout(() => { setDebouncedSearch(searchQuery); setCurrentPage(1); updateUrl(active, 1, searchQuery); }, DEBOUNCE_MS); return () => { if (debounceRef.current) clearTimeout(debounceRef.current); }; - }, [searchQuery, active, updateUrl]); + }, [searchQuery, updateUrl]);
94-132: Consider adding request cancellation to prevent race conditions.Rapid category or page changes could result in stale responses overwriting fresh data if an older request completes after a newer one. Adding an
AbortControllerwould prevent this.♻️ Suggested implementation with AbortController
useEffect(() => { + const controller = new AbortController(); + async function load() { setIsLoading(true); try { const searchParam = debouncedSearch ? `&search=${encodeURIComponent(debouncedSearch)}` : ''; const res = await fetch( - `/api/questions/${active}?page=${currentPage}&limit=10&locale=${locale}${searchParam}` + `/api/questions/${active}?page=${currentPage}&limit=10&locale=${localeKey}${searchParam}`, + { signal: controller.signal } ); if (!res.ok) { throw new Error(`Failed to load questions: ${res.status}`); } const data: PaginatedResponse<QuestionApiItem> = await res.json(); setItems( data.items.map(item => ({ id: item.id, question: item.question, category: active, answerBlocks: item.answerBlocks, })) ); setTotalPages(data.totalPages); } catch (error) { + if (error instanceof Error && error.name === 'AbortError') { + return; + } console.error('Failed to load questions:', error); setItems([]); setTotalPages(0); } finally { setIsLoading(false); } } load(); + + return () => controller.abort(); - }, [active, currentPage, debouncedSearch, locale]); + }, [active, currentPage, debouncedSearch, localeKey]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/components/q&a/useQaTabs.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/components/q&a/useQaTabs.ts (2)
frontend/data/category.ts (1)
categoryData(11-22)frontend/components/q&a/types.ts (6)
Locale(7-7)qaConstants(95-97)CategorySlug(8-8)QuestionEntry(70-75)PaginatedResponse(87-93)QuestionApiItem(77-85)
🔇 Additional comments (4)
frontend/components/q&a/useQaTabs.ts (4)
1-18: LGTM!Clean imports and well-defined constants. The fallback for
DEFAULT_CATEGORYis good defensive coding.
20-28: LGTM!Good type guards with sensible fallback behavior for invalid inputs.
38-48: LGTM!Robust URL parameter parsing with proper validation for page numbers and category slugs.
134-178: LGTM!Event handlers are well-structured with proper validation and state management. The exposed API is clean and complete for consumer components.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/components/q&a/useQaTabs.ts:
- Around line 35-36: The code unsafely casts params.locale to string; update the
logic around locale and localeKey so you normalize params.locale when calling
resolveLocale: check if params.locale is undefined, if it's an array use the
first element (or choose an appropriate preference), and if still falsy fall
back to a default locale before passing to resolveLocale; adjust the variables
referenced (params.locale, locale, localeKey, resolveLocale) so resolveLocale
always receives a string.
- Around line 94-132: The effect's async load() can leave multiple fetches in
flight causing stale responses to overwrite state; create an AbortController
inside the useEffect, pass controller.signal to the fetch call invoked in
load(), and in the effect cleanup call controller.abort() so prior requests are
cancelled when dependencies change; update the catch block in load() to ignore
abort errors (check error.name === 'AbortError' or similar) and only
setItems/setTotalPages for non-abort failures to avoid writing stale data.
🧹 Nitpick comments (1)
frontend/components/q&a/useQaTabs.ts (1)
75-92: Potential redundant URL update when category changes.When
handleCategoryChangeis called, it immediately callsupdateUrland also setssearchQueryto''. IfsearchQuerywas already'', React won't re-render, but if it wasn't, this effect will fire after 400ms and callupdateUrlagain with the same values.Consider removing
activefrom the dependency array since the effect's purpose is to debounce search input, not category changes. Category changes are already handled synchronously inhandleCategoryChange.Suggested adjustment
}, [searchQuery, active, updateUrl]); + // Note: Consider if `active` dependency is needed here since category changes + // are handled synchronously in handleCategoryChangeAlternatively, add an early return if
searchQueryhasn't changed from the previous value.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/components/q&a/useQaTabs.ts
🔇 Additional comments (4)
frontend/components/q&a/useQaTabs.ts (4)
1-28: Well-structured utilities and constants.The helper functions
resolveLocaleandisCategorySlugprovide proper type narrowing. The fallback pattern forDEFAULT_CATEGORYhandles edge cases gracefully.
58-73: Clean URL synchronization logic.The conditional parameter inclusion keeps URLs clean by omitting default values. Using
router.replacewithscroll: falseis appropriate for state-driven updates.
134-162: Well-structured handlers with proper memoization.The handlers correctly validate inputs, update state synchronously, and maintain URL consistency. Using
useCallbackwith appropriate dependencies ensures stable references for consumers.
164-177: Clean and comprehensive hook API.The return object provides all necessary state and handlers for the Q&A tabs feature. The API aligns well with the PR objectives of extracting tab logic into a reusable hook.
1 participant
Summary
Extract Q&A tab state/fetch/debounce logic into useQaTabs
Consolidate Q&A data types and reuse in AccordionList
Match Q&A tabs styling to Quizzes and align page container width
Changes
Added
useQaTabs.tsandtypes.tsUpdated
AccordionList.tsxto use shared typesReplaced TabsSection.tsx with QaSection.tsx
Adjusted
page.tsxcontainer tomax-w-7xlwith header paddingClose #128
Summary by CodeRabbit
New Features
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.