Add Trending Topics feature with Focus Mode#251
Conversation
Implement a new Trending Topics card that displays popular topics based on snippet labels. Features include: - Discovery Mode: Shows top 10 trending topics with sparkline visualizations - Focus Mode: Click a topic to see detailed analytics with interactive chart - Responsive detail chart with hover tooltips and smooth animations - Prefetching on hover for faster Focus Mode transitions - Skeleton loaders for perceived performance - Support for timespan filters (24h, 7d, 30d, all) - Batch filter updates to prevent race conditions - Full i18n support with Spanish translations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a trending-topics feature: RPC-backed API functions and types, React Query hooks, new chart/sparkline UI components and TrendingCard, translations, filter/time-span support, and integration into the search interface’s empty-search layout. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchInterface
participant TrendingCard
participant QueryCache as QueryClient
participant API
User->>SearchInterface: empty search renders
SearchInterface->>TrendingCard: mount (Discovery)
TrendingCard->>QueryCache: query trendingKeys.topics(timespan, filters, lang)
QueryCache->>API: fetchTrendingTopics(...)
API-->>QueryCache: TrendingTopicsResponse
QueryCache-->>TrendingCard: cached response
TrendingCard-->>User: render topics + sparklines
User->>TrendingCard: hover topic
TrendingCard->>QueryCache: prefetch trendingKeys.topicDetails(topicId,...)
QueryCache->>API: fetchTopicDetails(...)
API-->>QueryCache: TopicDetailsResponse (cached)
User->>TrendingCard: click topic
TrendingCard->>TrendingCard: switch to Focus mode
TrendingCard->>QueryCache: get topicDetails
QueryCache-->>TrendingCard: TopicDetailsResponse
TrendingCard->>DetailChart: render detailed chart
DetailChart-->>User: interactive chart + tooltip
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
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 |
Summary of ChangesHello @lsukaristone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature that allows users to discover and analyze trending topics within the application. By providing both a high-level overview of popular subjects and a detailed drill-down capability, it enhances user engagement and provides valuable insights into content trends. The feature is designed for responsiveness, performance, and international accessibility, making it a robust addition to the user interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-implemented Trending Topics feature, complete with Discovery and Focus modes, interactive charts, and responsive design. The use of React Query for data fetching, skeleton loaders for improved perceived performance, and i18n support are all excellent choices. My review focuses on enhancing code quality, maintainability, and performance. I've identified opportunities to reduce code duplication by extracting shared logic into helper functions, particularly in the API and chart components. Additionally, I've suggested improvements for the new chart components to make them more efficient and maintainable, such as optimizing data point lookups and removing magic numbers. A few minor inconsistencies in the UI logic have also been noted. Overall, this is a strong contribution, and addressing these suggestions will further improve the robustness and long-term health of the codebase.
| const path = useMemo(() => { | ||
| if (!data || data.length === 0) return '' | ||
|
|
||
| const max = Math.max(...data, 1) // Ensure at least 1 to avoid division by zero | ||
| const min = Math.min(...data, 0) | ||
| const range = max - min || 1 | ||
|
|
||
| const padding = 2 | ||
| const chartWidth = width - padding * 2 | ||
| const chartHeight = height - padding * 2 | ||
|
|
||
| const points = data.map((value, index) => { | ||
| const x = padding + (index / (data.length - 1 || 1)) * chartWidth | ||
| const y = padding + chartHeight - ((value - min) / range) * chartHeight | ||
| return { x, y } | ||
| }) | ||
|
|
||
| // Create the line path | ||
| const linePath = points.map((point, index) => (index === 0 ? `M ${point.x} ${point.y}` : `L ${point.x} ${point.y}`)).join(' ') | ||
|
|
||
| return linePath | ||
| }, [data, width, height]) | ||
|
|
||
| const areaPath = useMemo(() => { | ||
| if (!fillColor || !data || data.length === 0) return '' | ||
|
|
||
| const max = Math.max(...data, 1) | ||
| const min = Math.min(...data, 0) | ||
| const range = max - min || 1 | ||
|
|
||
| const padding = 2 | ||
| const chartWidth = width - padding * 2 | ||
| const chartHeight = height - padding * 2 | ||
|
|
||
| const points = data.map((value, index) => { | ||
| const x = padding + (index / (data.length - 1 || 1)) * chartWidth | ||
| const y = padding + chartHeight - ((value - min) / range) * chartHeight | ||
| return { x, y } | ||
| }) | ||
|
|
||
| const linePath = points.map((point, index) => (index === 0 ? `M ${point.x} ${point.y}` : `L ${point.x} ${point.y}`)).join(' ') | ||
|
|
||
| // Close the path for fill | ||
| const lastPoint = points[points.length - 1] | ||
| const firstPoint = points[0] | ||
| const closePath = `L ${lastPoint.x} ${height - padding} L ${firstPoint.x} ${height - padding} Z` | ||
|
|
||
| return linePath + closePath | ||
| }, [data, width, height, fillColor]) |
There was a problem hiding this comment.
There is significant code duplication between the useMemo hooks for path and areaPath. Both hooks repeat the logic for calculating max, min, range, padding, chartWidth, chartHeight, and points. This is inefficient and makes the code harder to maintain.
To fix this, you should calculate these shared metrics in a single useMemo hook. Then, path and areaPath can be calculated in their own useMemo hooks that depend on the shared metrics, avoiding redundant calculations.
| const filterObj: Record<string, unknown> = {} | ||
|
|
||
| if (filters.languages && filters.languages.length > 0) { | ||
| filterObj.languages = filters.languages | ||
| } | ||
| if (filters.states && filters.states.length > 0) { | ||
| filterObj.states = filters.states | ||
| } | ||
| if (filters.sources && filters.sources.length > 0) { | ||
| filterObj.sources = filters.sources | ||
| } | ||
| if (filters.politicalSpectrum) { | ||
| filterObj.politicalSpectrum = filters.politicalSpectrum | ||
| } |
There was a problem hiding this comment.
The logic for building filterObj is duplicated in fetchTopicDetails (here) and fetchTrendingTopics (lines 58-71). To improve maintainability and reduce code duplication, consider extracting this logic into a shared helper function. For example:
const buildFilterObject = (filters: Partial<SnippetFilters>): Record<string, unknown> => {
const filterObj: Record<string, unknown> = {};
// ... filter building logic ...
return filterObj;
}You can then call const filterObj = buildFilterObject(filters); in both API functions.
| // Create smooth curve using cardinal spline | ||
| const linePath = points.map((point, index) => { |
There was a problem hiding this comment.
This comment mentions "cardinal spline", but the implementation below (and the comment on line 78) uses a quadratic bezier curve. To avoid confusion, please update this comment to be consistent with the code.
| // Create smooth curve using cardinal spline | |
| const linePath = points.map((point, index) => { | |
| // Create smooth curve using quadratic bezier |
| // Find the closest point | ||
| let closestIndex = 0 | ||
| let closestDist = Infinity | ||
|
|
||
| chartData.points.forEach((point, index) => { | ||
| const dist = Math.abs(point.x - x) | ||
| if (dist < closestDist) { | ||
| closestDist = dist | ||
| closestIndex = index | ||
| } | ||
| }) |
There was a problem hiding this comment.
The current implementation to find the closest data point iterates through all points on every mouse move, which has a time complexity of O(n). For a large number of data points, this could become a performance bottleneck. Since the points array is sorted by the x-coordinate, you can optimize this by using a binary search algorithm (O(log n)) to find the closest index more efficiently.
| <div | ||
| className='absolute pointer-events-none bg-orange-900 dark:bg-orange-100 text-white dark:text-orange-900 text-xs px-2 py-1 rounded shadow-lg whitespace-nowrap z-10' | ||
| style={{ | ||
| left: Math.min(Math.max(points[hoveredIndex].x, 40), width - 60), |
There was a problem hiding this comment.
The tooltip positioning logic uses magic numbers 40 and 60. This makes the code harder to understand and maintain. Consider defining these as named constants (e.g., TOOLTIP_X_OFFSET, TOOLTIP_CONTAINER_MARGIN) or deriving them from other chart properties like padding to make their purpose clearer.
src/components/ui/trending-card.tsx
Outdated
| @@ -0,0 +1,387 @@ | |||
| import { useState, useEffect, useRef, useCallback } from 'react' | |||
| import { TrendingUp, Loader, Filter, ArrowLeft, TrendingDown, Minus, X } from 'lucide-react' | |||
There was a problem hiding this comment.
The Loader component is imported but never used in this file. Please remove the unused import to keep the code clean.
| import { TrendingUp, Loader, Filter, ArrowLeft, TrendingDown, Minus, X } from 'lucide-react' | |
| import { TrendingUp, Filter, ArrowLeft, TrendingDown, Minus, X } from 'lucide-react' |
| const timer = setTimeout(() => setIsFading(false), 300) | ||
| return () => clearTimeout(timer) | ||
| } | ||
| prevDataRef.current = currentDataKey as string |
There was a problem hiding this comment.
The type assertion currentDataKey as string is unsafe. currentDataKey can be undefined if topicData?.topic?.id is undefined (e.g., during loading or if the topic is not found). This can lead to runtime errors or unexpected behavior. You should handle the undefined case gracefully.
| prevDataRef.current = currentDataKey as string | |
| prevDataRef.current = currentDataKey ?? null |
| </CardHeader> | ||
| <CardContent className='pb-4'> | ||
| {isLoading ? ( | ||
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> |
There was a problem hiding this comment.
The TopicSkeleton's count prop is always 5, but when the card is expanded, you fetch 10 topics (as defined on line 102). This creates a visual inconsistency during the loading state. The skeleton count should match the number of items being fetched.
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> | |
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 10 : 5} /> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 484ca0f in 2 minutes and 36 seconds. Click for details.
- Reviewed
1122lines of code in10files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/apis/trending.ts:31
- Draft comment:
Consider adding more contextual info to the error logs (e.g. include parameters) for easier debugging when supabase.rpc fails. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. src/components/SearchInterface.tsx:170
- Draft comment:
The TrendingCard is integrated in a conditional grid layout; ensure these responsive classes meet all design scenarios. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. src/components/ui/detail-chart.tsx:41
- Draft comment:
Using ResizeObserver is effective; verify browser compatibility or add a polyfill for older browsers. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. src/components/ui/detail-chart.tsx:75
- Draft comment:
The quadratic bezier approach for smoothing (lines 75-81) works, but consider advanced spline techniques (e.g. using d3-shape) if smoother curves are desired. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
5. src/components/ui/detail-chart.tsx:259
- Draft comment:
Tooltip position is computed using clamping; double-check on very small container widths to ensure proper display. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. src/components/ui/sparkline.tsx:22
- Draft comment:
Good use of useMemo to compute the SVG path; if performance issues occur with many data points, consider optimizing the mapping logic. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
7. src/components/ui/trending-card.tsx:145
- Draft comment:
The fade animation timeout of 300ms is hardcoded; consider defining this duration as a constant for easier adjustment and consistency. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
8. src/constants/translations.ts:78
- Draft comment:
Translations for trending topics and Focus Mode are well integrated. Ensure that key naming (e.g. 'center-left' vs 'center_left') remains consistent across the codebase. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
9. src/hooks/useSnippetFilters.tsx:40
- Draft comment:
Ensure consistency in politicalSpectrum values ('center_left' in code vs 'center-left' in translations) to avoid mismatches. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
10. src/hooks/useTrendingTopics.tsx:6
- Draft comment:
The query keys include the entire 'filters' object. Ensure that 'filters' is stable or memoized to prevent unnecessary refetches. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
Workflow ID: wflow_019UqoDGBfMfwp0X
You can customize 
| setTimespan(newTimespan) | ||
| } | ||
|
|
||
| const handleTopicClick = (labelId: string) => { |
There was a problem hiding this comment.
In handleTopicClick, the parameter labelId represents a topic ID. Rename it to topicId for clarity.
| </CardHeader> | ||
| <CardContent className='pb-4'> | ||
| {isLoading ? ( | ||
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> |
There was a problem hiding this comment.
TopicSkeleton is rendered with count set as expanded ? 5 : 5; this makes both cases identical. Verify if different counts are intended for expanded vs collapsed modes.
![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)
src/components/SearchInterface.tsx (1)
40-40: Inverted logic confirmed:dismiss_welcome_cardshould hide the card, not show it.The variable
showWelcomeCardis assigneduser?.user_metadata?.dismiss_welcome_cardwithout negation, but the semantic intent is that whendismiss_welcome_cardistrue, the card should be hidden. This contradicts the pattern used inHeaderBar.tsx(line 29), which correctly uses negation:const showInfoIcon = !user?.user_metadata?.dismiss_welcome_card.Apply this fix:
- const showWelcomeCard = user?.user_metadata?.dismiss_welcome_card + const showWelcomeCard = !user?.user_metadata?.dismiss_welcome_card
🧹 Nitpick comments (14)
src/components/ui/detail-chart.tsx (2)
14-22: Avoid array literal as default prop to prevent potential re-renders.Using
labels = []as a default prop creates a new array reference on each render, which can cause unnecessary re-renders in child components or effects that depend onlabels.Apply this diff:
+const EMPTY_LABELS: string[] = [] + export default function DetailChart({ data, - labels = [], + labels = EMPTY_LABELS, width: propWidth, height = 100,
241-252: Use explicit boolean conversion for safer conditional rendering.While
hoveredIndex !== nullis safe, combining it withpoints[hoveredIndex]could theoretically leak a falsy value if points contained unexpected data. Using explicit boolean conversion is more defensive.- {hoveredIndex !== null && points[hoveredIndex] && ( + {hoveredIndex !== null && points[hoveredIndex] != null && (src/types/trending.ts (1)
24-27: Consider makingtopicoptional or nullable for error handling.If
get_topic_detailsRPC returns no matching topic, the response might havetopic: null. The current type requirestopicto always be defined, which could cause runtime type mismatches.export interface TopicDetailsResponse { timespan: string - topic: TopicDetails + topic: TopicDetails | null }Alternatively, handle this at the API layer by throwing an error when no topic is found.
Verify the RPC behavior: does
get_topic_detailsreturnnullfor the topic field when no matching topic exists?src/apis/trending.ts (2)
2-3: Useimport typefor type-only imports.These imports are only used as types, so they should use
import typefor better tree-shaking and to clearly indicate intent.-import { TrendingTopicsResponse, TopicDetailsResponse } from '@/types/trending' -import { SnippetFilters } from '@/hooks/useSnippetFilters' +import type { TrendingTopicsResponse, TopicDetailsResponse } from '@/types/trending' +import type { SnippetFilters } from '@/hooks/useSnippetFilters'
16-29: Extract duplicate filter-building logic into a helper function.The filter object construction is duplicated between
fetchTopicDetailsandfetchTrendingTopics. Extract this into a reusable helper.+const buildFilterObject = (filters: Partial<SnippetFilters>): Record<string, unknown> | null => { + const filterObj: Record<string, unknown> = {} + + if (filters.languages?.length) filterObj.languages = filters.languages + if (filters.states?.length) filterObj.states = filters.states + if (filters.sources?.length) filterObj.sources = filters.sources + if (filters.politicalSpectrum) filterObj.politicalSpectrum = filters.politicalSpectrum + + return Object.keys(filterObj).length > 0 ? filterObj : null +} export const fetchTopicDetails = async ({ // ... }): Promise<TopicDetailsResponse> => { - const filterObj: Record<string, unknown> = {} - - if (filters.languages && filters.languages.length > 0) { - filterObj.languages = filters.languages - } - // ... rest of filter building - const { data, error } = await supabase.rpc('get_topic_details', { p_topic_id: topicId, p_timespan: timespan, - p_filter: Object.keys(filterObj).length > 0 ? filterObj : null, + p_filter: buildFilterObject(filters), p_language: language })Also applies to: 58-71
src/components/ui/sparkline.tsx (1)
22-70: Reduce code duplication betweenpathandareaPathcomputations.Both
useMemohooks duplicate the same point calculation logic. Extract the shared computation to avoid redundancy and potential inconsistencies.-const path = useMemo(() => { - if (!data || data.length === 0) return '' - const max = Math.max(...data, 1) - const min = Math.min(...data, 0) - const range = max - min || 1 - const padding = 2 - const chartWidth = width - padding * 2 - const chartHeight = height - padding * 2 - const points = data.map((value, index) => { - const x = padding + (index / (data.length - 1 || 1)) * chartWidth - const y = padding + chartHeight - ((value - min) / range) * chartHeight - return { x, y } - }) - const linePath = points.map((point, index) => (index === 0 ? `M ${point.x} ${point.y}` : `L ${point.x} ${point.y}`)).join(' ') - return linePath -}, [data, width, height]) - -const areaPath = useMemo(() => { - if (!fillColor || !data || data.length === 0) return '' - // ... duplicated point calculation -}, [data, width, height, fillColor]) +const { linePath, areaPath } = useMemo(() => { + if (!data || data.length === 0) return { linePath: '', areaPath: '' } + + const max = Math.max(...data, 1) + const min = Math.min(...data, 0) + const range = max - min || 1 + const padding = 2 + const chartWidth = width - padding * 2 + const chartHeight = height - padding * 2 + + const points = data.map((value, index) => { + const x = padding + (index / (data.length - 1 || 1)) * chartWidth + const y = padding + chartHeight - ((value - min) / range) * chartHeight + return { x, y } + }) + + const linePath = points.map((point, index) => + index === 0 ? `M ${point.x} ${point.y}` : `L ${point.x} ${point.y}` + ).join(' ') + + if (!fillColor) return { linePath, areaPath: '' } + + const lastPoint = points[points.length - 1] + const firstPoint = points[0] + const closePath = `L ${lastPoint.x} ${height - padding} L ${firstPoint.x} ${height - padding} Z` + + return { linePath, areaPath: linePath + closePath } +}, [data, width, height, fillColor])src/hooks/useSnippetFilters.tsx (1)
119-125: Based on my verification of the codebase, I've found that:
Actual usage patterns: All calls to
setFiltersandsetFilterare single, atomic updates within event handlers. There are no rapid successive calls tosetFiltersin the codebase.Found usages:
trending-card.tsx: 2 single calls (lines 129, 135)SearchInterface.tsx: UsessetFilterwith debounce (line 122)Sidebar.tsx: Multiple singlesetFiltercalls in various event handlersDesign purpose: The comment "Set multiple filters at once to avoid race conditions" refers to merging multiple filter fields into a single update—not handling multiple sequential calls.
Closure behavior: While the closure does capture
filtersfrom the render it was created in, this is not problematic because the actual usage pattern is single atomic updates per event. Even ifsetSearchParamsupdates asynchronously, it would only affect behavior with rapid successive calls, which don't occur.Rewritten Review Comment:
setFiltersclosure pattern is acceptable given current usage, but document the constraint.The closure over
filtersis safe for the current single-call-per-event usage patterns observed in the codebase. However, if future use cases require rapid successive updates within the same event handler, this pattern could lose intermediate state.Consider adding a code comment clarifying that
setFiltersis designed for atomic multi-field updates within a single event, not sequential updates:// Set multiple filters at once to avoid race conditions. // Note: Intended for atomic updates within a single event handler. // For rapid successive updates, refactor to use functional updates.This prevents accidental misuse if the hook evolves.
src/components/ui/trending-card.tsx (5)
2-2: Remove unused importLoader.The
Loadericon is imported but never used in the component.-import { TrendingUp, Loader, Filter, ArrowLeft, TrendingDown, Minus, X } from 'lucide-react' +import { TrendingUp, Filter, ArrowLeft, TrendingDown, Minus, X } from 'lucide-react'
166-170: Use object destructuring forchangePercent.Minor style improvement per ESLint's
prefer-destructuringrule.const { topic } = topicData - const changePercent = topic.changePercent + const { changePercent } = topic const isPositive = changePercent > 0 const isNegative = changePercent < 0
260-284: Add explicittype="button"to prevent unintended form submission.Native
<button>elements default totype="submit", which can cause issues if this component is ever placed inside a form.<button key={topic.id} + type='button' onClick={() => handleTopicClick(topic.id)} onMouseEnter={() => prefetchTopicDetails(topic.id)}
303-311: Add explicittype="button"to the back button.Same as the topic buttons—add
type="button"to prevent default submit behavior.<button + type='button' onClick={handleExitFocusMode} className='flex items-center gap-1 text-orange-600 hover:text-orange-800 dark:text-orange-400 dark:hover:text-orange-200' >
369-384: Consider extracting the render logic to reduce nested ternaries.The nested ternaries here are hard to follow. Consider extracting to a helper or using early returns.
// Alternative approach with a helper function const renderContent = () => { if (isLoading) { return focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 10 : 5} /> } if (error) { return ( <p className='text-sm text-orange-700 dark:text-orange-300'> {focusedTopicId ? (t.errorLoadingTopic || 'Unable to load topic details') : (t.errorLoadingTrending || 'Unable to load trending topics')} </p> ) } return focusedTopicId ? renderFocusMode() : renderDiscoveryMode() } // Then in CardContent: <CardContent className='pb-4'> {renderContent()} </CardContent>src/hooks/useTrendingTopics.tsx (2)
3-4: Useimport typefor type-only imports.These imports are only used as type annotations, so they should use
import typefor proper tree-shaking and to clarify intent.-import { TrendingTopicsResponse, TopicDetailsResponse } from '@/types/trending' -import { SnippetFilters } from '@/hooks/useSnippetFilters' +import type { TrendingTopicsResponse, TopicDetailsResponse } from '@/types/trending' +import type { SnippetFilters } from '@/hooks/useSnippetFilters'
45-50: Non-null assertion is runtime-safe but consider a type guard.The
topicId!assertion on line 47 is safe at runtime becauseenabled: enabled && !!topicIdprevents thequeryFnfrom executing whentopicIdis undefined. However, to satisfy TypeScript more explicitly and avoid the lint warning, you could add a runtime guard:return useQuery<TopicDetailsResponse, Error>({ queryKey: trendingKeys.topicDetails(topicId || '', timespan, filters, language), - queryFn: () => fetchTopicDetails({ topicId: topicId!, timespan, filters, language }), + queryFn: () => { + if (!topicId) throw new Error('topicId is required') + return fetchTopicDetails({ topicId, timespan, filters, language }) + }, staleTime: 1000 * 60 * 5, // Cache for 5 minutes enabled: enabled && !!topicId })Alternatively, keep the assertion if the team accepts this common React Query pattern.
📜 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 (10)
src/apis/trending.ts(1 hunks)src/components/SearchInterface.tsx(3 hunks)src/components/ui/detail-chart.tsx(1 hunks)src/components/ui/sparkline.tsx(1 hunks)src/components/ui/trending-card.tsx(1 hunks)src/components/ui/welcome-card.tsx(1 hunks)src/constants/translations.ts(2 hunks)src/hooks/useSnippetFilters.tsx(7 hunks)src/hooks/useTrendingTopics.tsx(1 hunks)src/types/trending.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/components/SearchInterface.tsx (3)
src/lib/utils.ts (1)
cn(4-6)src/components/ui/welcome-card.tsx (1)
WelcomeCard(9-64)src/components/ui/trending-card.tsx (1)
TrendingCard(80-387)
src/components/ui/trending-card.tsx (7)
src/hooks/useSnippetFilters.tsx (1)
Timespan(4-4)src/providers/language.tsx (1)
useLanguage(16-22)src/providers/sidebar.tsx (1)
useSidebar(36-42)src/constants/translations.ts (1)
translations(1-178)src/hooks/useTrendingTopics.tsx (3)
useTrendingTopics(14-30)useTopicDetails(32-51)trendingKeys(6-12)src/apis/trending.ts (1)
fetchTopicDetails(5-44)src/lib/utils.ts (1)
cn(4-6)
src/hooks/useTrendingTopics.tsx (3)
src/hooks/useSnippetFilters.tsx (1)
SnippetFilters(6-19)src/types/trending.ts (2)
TrendingTopicsResponse(8-11)TopicDetailsResponse(24-27)src/apis/trending.ts (2)
fetchTrendingTopics(46-86)fetchTopicDetails(5-44)
src/components/ui/detail-chart.tsx (1)
src/lib/utils.ts (1)
cn(4-6)
src/apis/trending.ts (2)
src/hooks/useSnippetFilters.tsx (1)
SnippetFilters(6-19)src/types/trending.ts (2)
TopicDetailsResponse(24-27)TrendingTopicsResponse(8-11)
src/components/ui/welcome-card.tsx (1)
src/components/ui/card.tsx (1)
Card(41-41)
🪛 ESLint
src/components/SearchInterface.tsx
[error] 34-34: Missing file extension for "@/lib/utils"
(import/extensions)
[error] 175-175: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 178-178: Do not nest ternary expressions.
(no-nested-ternary)
src/components/ui/sparkline.tsx
[error] 82-82: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
src/components/ui/trending-card.tsx
[error] 2-2: 'Loader' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 3-3: Missing file extension for "@/components/ui/card"
(import/extensions)
[error] 4-4: Missing file extension for "@/components/ui/button"
(import/extensions)
[error] 5-5: Missing file extension for "@/components/ui/sparkline"
(import/extensions)
[error] 6-6: Missing file extension for "@/components/ui/detail-chart"
(import/extensions)
[error] 7-7: Missing file extension for "@/hooks/useTrendingTopics"
(import/extensions)
[error] 8-8: Missing file extension for "@/apis/trending"
(import/extensions)
[error] 9-9: Import "Timespan" is only used as types.
(@typescript-eslint/consistent-type-imports)
[error] 9-9: Missing file extension for "@/hooks/useSnippetFilters"
(import/extensions)
[error] 10-10: Missing file extension for "@/providers/language"
(import/extensions)
[error] 11-11: Missing file extension for "@/providers/sidebar"
(import/extensions)
[error] 12-12: Missing file extension for "@/constants/translations"
(import/extensions)
[error] 13-13: Missing file extension for "@/lib/utils"
(import/extensions)
[error] 33-33: Prop name expanded doesn’t match rule ^(is|has)[A-Z]([A-Za-z0-9]?)+
(react/boolean-prop-naming)
[error] 49-49: Do not use Array index in keys
(react/no-array-index-key)
[error] 144-144: Expected to return a value at the end of arrow function.
(consistent-return)
[error] 147-147: 't' is already declared in the upper scope on line 84 column 9.
(@typescript-eslint/no-shadow)
[error] 167-167: Use object destructuring.
(prefer-destructuring)
[error] 213-213: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 214-214: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 226-233: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 260-284: Missing an explicit type attribute for button
(react/button-has-type)
[error] 303-311: Missing an explicit type attribute for button
(react/button-has-type)
[error] 324-324: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 356-365: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 370-383: Do not nest ternary expressions.
(no-nested-ternary)
[error] 372-383: Do not nest ternary expressions.
(no-nested-ternary)
src/hooks/useTrendingTopics.tsx
[error] 2-2: Missing file extension for "@/apis/trending"
(import/extensions)
[error] 3-3: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 3-3: Missing file extension for "@/types/trending"
(import/extensions)
[error] 4-4: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 4-4: Missing file extension for "@/hooks/useSnippetFilters"
(import/extensions)
[error] 10-10: Method 'topicDetails' has too many parameters (4). Maximum allowed is 3.
(@typescript-eslint/max-params)
[error] 24-30: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
[error] 44-51: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
[error] 47-47: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
src/components/ui/detail-chart.tsx
[error] 2-2: Missing file extension for "@/lib/utils"
(import/extensions)
[error] 16-16: labels has a/an array literal as default prop. This could lead to potential infinite render loop in React. Use a variable reference instead of array literal.
(react/no-object-type-as-default-prop)
[error] 48-48: Arrow function expected no return value.
(consistent-return)
[error] 87-89: Unexpected string concatenation.
(prefer-template)
[error] 94-94: Unary operator '++' used.
(no-plusplus)
[error] 174-174: Do not use Array index in keys
(react/no-array-index-key)
[error] 204-204: Do not nest ternary expressions.
(no-nested-ternary)
[error] 229-229: Do not use Array index in keys
(react/no-array-index-key)
[error] 241-252: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 256-270: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 266-268: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
src/apis/trending.ts
[error] 1-1: Missing file extension for "@/lib/supabase"
(import/extensions)
[error] 2-2: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 2-2: Missing file extension for "@/types/trending"
(import/extensions)
[error] 3-3: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 3-3: Missing file extension for "@/hooks/useSnippetFilters"
(import/extensions)
src/hooks/useSnippetFilters.tsx
[error] 4-4: Union type Timespan constituents must be sorted.
(@typescript-eslint/sort-type-constituents)
🔇 Additional comments (8)
src/components/ui/welcome-card.tsx (1)
21-21: LGTM!Removing the bottom margin from
Cardis appropriate since spacing is now managed by the parent grid'sgap-4inSearchInterface.tsx.src/components/SearchInterface.tsx (1)
170-181: Grid layout implementation looks good.The responsive grid adjusts correctly based on
showSidebarandshowWelcomeCardstates. TheTrendingCardexpands to full width when the welcome card is dismissed.src/constants/translations.ts (1)
77-88: LGTM!The new translation keys for the Trending Topics feature are complete and consistent between English and Spanish. All Focus Mode and Discovery Mode strings are properly localized.
Also applies to: 165-176
src/hooks/useSnippetFilters.tsx (1)
44-45: LGTM!The default values for
timespan('7d') and optionalfocusedTopicare handled correctly. The URL stays clean by omitting the default timespan value.src/components/ui/sparkline.tsx (1)
72-85: LGTM!The empty state handling with a baseline and the conditional area fill rendering are implemented correctly. The SVG structure is clean and the rendering logic handles edge cases well.
src/components/ui/trending-card.tsx (1)
114-121: Nice prefetch pattern for improved UX.The
prefetchTopicDetailson hover is a good pattern for reducing perceived latency when entering Focus Mode.src/hooks/useTrendingTopics.tsx (2)
6-12: Query key factory is well-structured.Good use of the query key factory pattern for consistent cache management. The 4-parameter
topicDetailshelper exceeds the ESLintmax-paramsrule, but this is acceptable for query key builders where each parameter directly maps to cache dimensions.
25-29: LGTM - proper React Query configuration.Good stale time configuration for trending data caching. The 5-minute cache is reasonable for this type of content.
| <defs> | ||
| <linearGradient id='areaGradient' x1='0' y1='0' x2='0' y2='1'> | ||
| <stop offset='0%' stopColor={fillColor} stopOpacity='0.3' /> | ||
| <stop offset='100%' stopColor={fillColor} stopOpacity='0.05' /> | ||
| </linearGradient> | ||
| </defs> |
There was a problem hiding this comment.
Potential gradient ID collision with multiple chart instances.
The hardcoded id='areaGradient' will cause visual issues if multiple DetailChart components render on the same page, as all will reference the same gradient definition.
Consider using a unique ID per instance:
+import { useId } from 'react'
// ... inside component:
+const gradientId = useId()
// ... in SVG:
-<linearGradient id='areaGradient' x1='0' y1='0' x2='0' y2='1'>
+<linearGradient id={gradientId} x1='0' y1='0' x2='0' y2='1'>
// ... for the area path:
-<path d={areaPath} fill='url(#areaGradient)' />
+<path d={areaPath} fill={`url(#${gradientId})`} />Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/detail-chart.tsx around lines 165 to 170, the gradient uses
a hardcoded id='areaGradient' which will collide when multiple DetailChart
instances render; change to generate and use a unique gradient id per component
instance (for example derive one from React's useId() or create a stable id with
useRef and a global counter or UUID prop), set that id on the <linearGradient>
and update any references (e.g., fill="url(#...)" or xlink:href) to use the new
unique id so each chart references its own gradient; ensure the chosen method is
SSR-safe (useId or a stable useRef) so id remains consistent between server and
client.
| </CardHeader> | ||
| <CardContent className='pb-4'> | ||
| {isLoading ? ( | ||
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> |
There was a problem hiding this comment.
Redundant ternary: expanded ? 5 : 5 always evaluates to 5.
This appears to be a copy-paste oversight. Did you intend to show a different skeleton count when not expanded (e.g., 3)?
- focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} />
+ focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 10 : 5} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> | |
| focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 10 : 5} /> |
🤖 Prompt for AI Agents
In src/components/ui/trending-card.tsx around line 371, the TopicSkeleton call
uses a redundant ternary expanded ? 5 : 5 that always yields 5; replace the
ternary with the intended counts (e.g., expanded ? 5 : 3) or a single literal if
the count should always be 5—update the JSX to TopicSkeleton count={expanded ? 5
: 3} (or count={5} if no variance) to remove the pointless ternary.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const searchTerm = searchParams.get('searchTerm') as SnippetFilters['searchTerm'] | ||
| const timespan = (searchParams.get('timespan') as SnippetFilters['timespan']) || '7d' | ||
| const focusedTopic = searchParams.get('focusedTopic') as SnippetFilters['focusedTopic'] |
There was a problem hiding this comment.
The new timespan field is defaulted to '7d' in useSnippetFilters and included in the returned filters object. Because SearchInterface passes filters straight into useSnippets/fetchSnippets, every snippets RPC call now carries p_filter containing timespan: '7d' even when the user has not chosen a timespan (and the feature summary claims the default should be “all”). If the backend honors this key, the search page will silently drop any snippets older than seven days for all users. Consider leaving timespan undefined unless explicitly selected so existing snippet queries remain unbounded.
Useful? React with 👍 / 👎.
- Add 90d timespan filter option - Change default timespan from 'all' to '30d' for better initial load - Auto-exit Focus Mode when user adds a second label filter - Use keepPreviousData to show previous content while fetching new data - Prevents blank screen during mode transitions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9a9eb9e in 2 minutes and 24 seconds. Click for details.
- Reviewed
77lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/components/ui/trending-card.tsx:41
- Draft comment:
Ensure '90d' timespan is supported by the backend and documentation is updated. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that a specific timespan is supported by the backend and that the documentation is updated. This falls under the category of asking the author to double-check things and ensure behavior, which is against the rules.
2. src/components/ui/trending-card.tsx:89
- Draft comment:
Default timespan changed to '30d'; confirm this is intentional (default in URL is '7d'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is asking the PR author to "confirm this is intentional", which directly violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended". The comment also mentions a "default in URL is 7d" but I don't see any URL-related code in this file or the diff - this seems to be referencing cross-file context which I should ignore per the rules. The comment is not providing actionable feedback; it's just asking for confirmation of an intentional change. Perhaps the comment is highlighting a potential inconsistency between different parts of the codebase (URL default vs component default), which could be a legitimate issue worth addressing. The author might not be aware of this inconsistency. Even if there is an inconsistency, the comment is phrased as "confirm this is intentional" which is explicitly against the rules. Additionally, the rules state to "Ignore cross-file issues. Only think about the file you are reviewing." The URL default mentioned is not visible in this file or diff, so this appears to be a cross-file concern. This comment should be deleted because it asks the PR author to confirm their intention, which violates the review rules. It also appears to reference cross-file context (URL defaults) which should be ignored.
3. src/components/ui/trending-card.tsx:98
- Draft comment:
Memoize filters.labels to avoid unnecessary effect executions due to array reference changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a speculative performance optimization comment. The reviewer is suggesting thatfilters.labelsmight cause unnecessary effect executions, but there's no evidence in the diff or visible code that this is actually happening. Thefiltersobject comes fromuseSnippetFilters()hook, and without seeing that implementation, I can't verify if the array reference is stable or not. Many well-designed hooks already memoize their return values. This comment falls into the category of "speculative" - it's saying "this might be a problem" rather than "this is definitely a problem." According to the rules, I should not keep speculative comments. Additionally, this is a code quality/performance suggestion that lacks clear evidence of an actual issue. The comment could be valid iffilters.labelsis indeed being recreated on every render, which would cause the effect to run unnecessarily. However, many modern React hooks properly memoize their return values, so this might not be an issue at all. Without profiling data or seeing the hook implementation, this is purely speculative. While the critique is fair, the rules explicitly state to avoid speculative comments and to only keep comments with strong evidence. Since I cannot see theuseSnippetFiltersimplementation and there's no evidence of actual performance issues, this comment should be deleted. If this were a real problem, it would need to be identified through profiling or by examining the hook implementation. This comment is speculative and lacks strong evidence of an actual problem. Without seeing theuseSnippetFiltershook implementation or profiling data showing unnecessary re-renders, I cannot confirm this is a real issue. The comment should be deleted per the rule against speculative comments.
4. src/components/ui/trending-card.tsx:380
- Draft comment:
Ternary for TopicSkeleton count is redundant (expanded ? 5 : 5); simplify to a single value. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. src/hooks/useSnippetFilters.tsx:4
- Draft comment:
New '90d' in Timespan type; ensure backend/API handles '90d' correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the backend/API handles the new '90d' timespan correctly. This is a request for confirmation or verification, which violates the rule against asking the author to ensure behavior is intended or tested.
6. src/hooks/useTrendingTopics.tsx:29
- Draft comment:
Using 'placeholderData: keepPreviousData' seems off. Consider using 'keepPreviousData: true' instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% In TanStack Query v5, the API changed. The old v4 syntax waskeepPreviousData: true, but in v5 this was replaced withplaceholderData: keepPreviousDatawherekeepPreviousDatais a function imported from the library. The code is importingkeepPreviousDatafrom '@tanstack/react-query' and using it as a value forplaceholderData, which is the correct v5 syntax. The comment appears to be suggesting the old v4 API, which would be incorrect if this codebase is using v5. Since the code successfully importskeepPreviousDataas a named export, this strongly suggests they're using v5 where this is the correct syntax. I might be wrong about the version being used. Perhaps this codebase is using v4 and the import is incorrect, or perhaps there's a compatibility layer. Without seeing the package.json or knowing the exact version, I can't be 100% certain which API is correct. However, the fact thatkeepPreviousDatais being imported as a named export from '@tanstack/react-query' is strong evidence that this is v5, because in v4 this export didn't exist. If this were v4, the import would fail at build time, which violates the rule that we shouldn't comment on things that would be caught by the build. The syntax used in the PR is correct for v5. The comment appears to be incorrect. The syntaxplaceholderData: keepPreviousDatais the correct modern TanStack Query v5 API, whilekeepPreviousData: truewas the old v4 API. Since the import works, this is likely v5, making the comment wrong. I should delete this comment.
7. src/hooks/useTrendingTopics.tsx:50
- Draft comment:
Same issue in topic details: 'placeholderData: keepPreviousData' may be misused; prefer 'keepPreviousData: true'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests changingplaceholderData: keepPreviousDatatokeepPreviousData: true. However, based on my knowledge of TanStack Query, in v5 the API changed from usingkeepPreviousData: trueas an option to usingplaceholderData: keepPreviousDatawherekeepPreviousDatais an imported function. The code in the diff showskeepPreviousDatabeing imported from '@tanstack/react-query' and used as a value forplaceholderData, which is the correct v5 syntax. The comment appears to be incorrect and suggesting the old v4 API pattern. I might be wrong about the TanStack Query version being used in this project. Perhaps this is using v4 wherekeepPreviousData: trueis indeed the correct syntax, and the import ofkeepPreviousDatais incorrect. Without seeing the package.json or knowing the exact version, I cannot be 100% certain. While I can't be absolutely certain of the version, the fact that the code successfully importskeepPreviousDatafrom '@tanstack/react-query' strongly suggests this is v5, where this usage is correct. If it were v4, the import would fail at build time, which violates the rule about not commenting on things caught by the build. The comment is likely incorrect. The comment appears to be incorrect about the TanStack Query API usage. The code uses the v5 syntax correctly withplaceholderData: keepPreviousData. If the import were wrong, it would be caught at build time. This comment should be deleted.
8. src/hooks/useTrendingTopics.tsx:8
- Draft comment:
Query keys include the 'filters' object; ensure its reference is stable to avoid unnecessary refetches. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_TPLrNVA4g71nRojU
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/components/ui/trending-card.tsx (4)
1-2: Remove unusedLoaderimport
Loaderfromlucide-react(Line 2) is never used in this component; it should be dropped to keep the module clean and satisfy the linter.
132-140: RenamelabelIdtotopicIdfor clarityIn
handleTopicClick(Lines 136–140), the argument is a topic ID, not a label ID. Renaming the parameter totopicIdimproves readability and matches the rest of the naming in this file.- const handleTopicClick = (labelId: string) => { - // Enter Focus Mode: set focusedTopic and replace labels with just this one - setFilters({ focusedTopic: labelId, labels: [labelId] }) + const handleTopicClick = (topicId: string) => { + // Enter Focus Mode: set focusedTopic and replace labels with just this one + setFilters({ focusedTopic: topicId, labels: [topicId] }) }
147-151: FixuseEffectfade hook: avoid shadowingtand unsafeas string, and satisfyconsistent-returnIn the fade effect (Lines 152–163):
- The
topics.map(t => t.id)callback parametertshadows thettranslations object from Line 85.currentDataKeycan beundefined, yet is cast tostringwithas string, which is both unsafe and unnecessary.- The effect only returns a cleanup function on one branch, which trips
consistent-returnrules.You can address all three at once:
- useEffect(() => { - const currentDataKey = focusedTopicId - ? topicData?.topic?.id - : JSON.stringify(topics.map(t => t.id)) - if (prevDataRef.current !== null && prevDataRef.current !== currentDataKey) { - setIsFading(true) - const timer = setTimeout(() => setIsFading(false), 300) - return () => clearTimeout(timer) - } - prevDataRef.current = currentDataKey as string - }, [topics, topicData, focusedTopicId]) + useEffect(() => { + const currentDataKey = focusedTopicId + ? topicData?.topic?.id ?? null + : JSON.stringify(topics.map(topic => topic.id)) + + if (prevDataRef.current !== null && prevDataRef.current !== currentDataKey) { + setIsFading(true) + const timer = setTimeout(() => setIsFading(false), 300) + prevDataRef.current = currentDataKey + return () => clearTimeout(timer) + } + + prevDataRef.current = currentDataKey + return undefined + }, [topics, topicData, focusedTopicId])Also applies to: 152-163
379-392: Align skeleton count with fetched topic limit and simplify nested ternaryIn the loading branch (Lines 379–392):
TopicSkeletonis rendered withcount={expanded ? 5 : 5}even though the query useslimit: expanded ? 10 : 5(Line 111). This creates a mismatch between expected and skeleton rows.- The nested ternary mixing loading/error/main states is small but slightly harder to scan.
I’d at least fix the count to match the query limit; flattening the ternary into early returns or small helper functions is optional.
- {isLoading ? ( - focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> - ) : error ? ( + {isLoading ? ( + focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 10 : 5} /> + ) : error ? (src/hooks/useSnippetFilters.tsx (1)
4-5: Defaultingtimespanto'7d'still forces a filter even when user hasn’t chosen oneLine 44 initializes
timespanto'7d'when the URL param is absent, and Lines 17–19 expose it viafilters.timespan. That means any consumer that passesfiltersthrough to backend calls will always send a non‑undefined timespan, effectively enforcing a 7‑day window by default.This seems to reintroduce the concern from the earlier review: if the intended default for snippet queries is “all time”,
timespanshould likely remainundefineduntil explicitly selected, or default to'all'instead of'7d'. At minimum, please double‑check howSnippetFilters.timespanis consumed downstream (e.g., RPCp_filterhandling) so you don’t silently drop older snippets.If you want, I can help draft a small refactor to:
- Make
timespandefault toundefined/'all'at the hook level, and- Ensure trending‑specific timespan behavior stays local to the Trending UI.
You can also run a quick search to see which APIs actually usetimespan:#!/bin/bash # Find all usages of SnippetFilters.timespan and where it’s passed to RPCs rg -n "timespan" --type ts --type tsx -C3Also applies to: 17-19, 44-45
🧹 Nitpick comments (5)
src/hooks/useSnippetFilters.tsx (1)
4-5: Typing and newsetFiltershelper look good; minor lint nit onTimespanorderingThe new
Timespantype, addedtimespan/focusedTopicfields, and thesetFiltershelper around Line 119 form a coherent API and should help avoid multi‑update race conditions.setFilters({ ...filters, ...updates })is correct given the hook’s dependency array and URL rewrite semantics.Only nit: ESLint wants the
Timespanunion on Line 4 sorted for consistency ('24h' | '30d' | '7d' | '90d' | 'all', or whatever your rule prefers).Also applies to: 17-19, 71-77, 94-97, 119-125, 141-142
src/components/ui/trending-card.tsx (3)
45-60: Array index as key inTopicSkeletonis acceptable but could be improvedUsing the array index as the
key(Line 50) is fine here because the skeleton rows are purely visual placeholders and won’t be re‑ordered, but if you want to silencereact/no-array-index-key, you could key by a stable value such as a prefix plusi.
247-297: Discovery Mode layout and interactions are solid; consider addingtype="button"The Discovery Mode list (Lines 247–297) wires up click and hover prefetch nicely and uses responsive grid behavior.
One minor HTML/React nit: the
<button>at Line 269 lacks an explicittype. In a form context, it would default totype="submit", which can cause surprises. It’s safer to mark these UI buttons as plain buttons.- {topics.map((topic, index) => ( - <button + {topics.map((topic, index) => ( + <button + type='button' key={topic.id}
300-376: Header and timespan selector look good; addtype="button"on header buttonsThe header (Lines 300–376) cleanly separates Discovery vs Focus Mode and wires up the timespan selector.
For robustness in mixed form contexts, both header buttons should declare
type="button":
- Back button (Lines 312–320).
- Close button (Lines 365–374).
- <button + <button + type='button' onClick={handleExitFocusMode} @@ - <Button + <Button + type='button' variant='ghost'src/hooks/useTrendingTopics.tsx (1)
1-4: Consider switching type‑only imports toimport typeto satisfy lintAll of
TrendingTopicsResponse,TopicDetailsResponse, andSnippetFilters(Lines 3–4) appear to be used purely as types. If your ESLint config enforcesconsistent-type-imports, you can silence it by refactoring:-import { TrendingTopicsResponse, TopicDetailsResponse } from '@/types/trending' -import { SnippetFilters } from '@/hooks/useSnippetFilters' +import type { TrendingTopicsResponse, TopicDetailsResponse } from '@/types/trending' +import type { SnippetFilters } from '@/hooks/useSnippetFilters'Please confirm this matches how your TS config treats type imports.
📜 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 (3)
src/components/ui/trending-card.tsx(1 hunks)src/hooks/useSnippetFilters.tsx(7 hunks)src/hooks/useTrendingTopics.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks/useTrendingTopics.tsx (3)
src/hooks/useSnippetFilters.tsx (1)
SnippetFilters(6-19)src/types/trending.ts (2)
TrendingTopicsResponse(8-11)TopicDetailsResponse(24-27)src/apis/trending.ts (2)
fetchTrendingTopics(46-86)fetchTopicDetails(5-44)
🪛 ESLint
src/components/ui/trending-card.tsx
[error] 2-2: 'Loader' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 3-3: Missing file extension for "@/components/ui/card"
(import/extensions)
[error] 4-4: Missing file extension for "@/components/ui/button"
(import/extensions)
[error] 5-5: Missing file extension for "@/components/ui/sparkline"
(import/extensions)
[error] 6-6: Missing file extension for "@/components/ui/detail-chart"
(import/extensions)
[error] 7-7: Missing file extension for "@/hooks/useTrendingTopics"
(import/extensions)
[error] 8-8: Missing file extension for "@/apis/trending"
(import/extensions)
[error] 9-9: Import "Timespan" is only used as types.
(@typescript-eslint/consistent-type-imports)
[error] 9-9: Missing file extension for "@/hooks/useSnippetFilters"
(import/extensions)
[error] 10-10: Missing file extension for "@/providers/language"
(import/extensions)
[error] 11-11: Missing file extension for "@/providers/sidebar"
(import/extensions)
[error] 12-12: Missing file extension for "@/constants/translations"
(import/extensions)
[error] 13-13: Missing file extension for "@/lib/utils"
(import/extensions)
[error] 33-33: Prop name expanded doesn’t match rule ^(is|has)[A-Z]([A-Za-z0-9]?)+
(react/boolean-prop-naming)
[error] 50-50: Do not use Array index in keys
(react/no-array-index-key)
[error] 153-153: Expected to return a value at the end of arrow function.
(consistent-return)
[error] 156-156: 't' is already declared in the upper scope on line 85 column 9.
(@typescript-eslint/no-shadow)
[error] 176-176: Use object destructuring.
(prefer-destructuring)
[error] 222-222: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 223-223: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 235-242: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 269-293: Missing an explicit type attribute for button
(react/button-has-type)
[error] 312-320: Missing an explicit type attribute for button
(react/button-has-type)
[error] 333-333: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 365-374: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 379-392: Do not nest ternary expressions.
(no-nested-ternary)
[error] 381-392: Do not nest ternary expressions.
(no-nested-ternary)
src/hooks/useTrendingTopics.tsx
[error] 2-2: Missing file extension for "@/apis/trending"
(import/extensions)
[error] 3-3: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 3-3: Missing file extension for "@/types/trending"
(import/extensions)
[error] 4-4: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 4-4: Missing file extension for "@/hooks/useSnippetFilters"
(import/extensions)
[error] 10-10: Method 'topicDetails' has too many parameters (4). Maximum allowed is 3.
(@typescript-eslint/max-params)
[error] 24-31: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
[error] 45-53: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
[error] 48-48: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
src/hooks/useSnippetFilters.tsx
[error] 4-4: Union type Timespan constituents must be sorted.
(@typescript-eslint/sort-type-constituents)
🔇 Additional comments (6)
src/components/ui/trending-card.tsx (5)
37-43: Timespan options and local state are coherentThe
TIMESPAN_OPTIONSand localtimespanstate (Lines 37–43 and 88–90) are internally consistent, and scoping timespan to this component (no URL coupling) is a good choice for the Trending card.Also applies to: 88-90
96-105: Focus Mode entry/exit logic is clear and consistent with filtersLines 96–105 and 142–145 correctly manage
focusedTopicandlabelsin and out of Focus Mode viasetFilters, and the guard to exit Focus Mode when multiple labels are selected is a nice touch.Also applies to: 142-145
175-231: Focus Mode stats rendering looks correctThe Focus Mode block (Lines 175–231) cleanly renders the topic stats and change indicator, including the positive/negative/flat cases and the
timespan !== 'all'guard. The use of icons and localized text is clear.
235-242: Filter indicator and messaging align with “global filters only” intentLines 235–242 and 331–343 correctly show filter indicators only when non‑label filters are active, which matches the
hasActiveFiltersbehavior and avoids confusing Focus Mode’s label selection with global filters.Also applies to: 331-343
106-122: Verification confirms query key consistency and cache hit behaviorThe trending queries implementation is correct. I've verified that
trendingKeys.topicDetailsis constructed with the same argument order across all usages:
- Definition (useTrendingTopics.tsx:10):
trendingKeys.topicDetails(topicId: string, timespan: string, filters: Partial<SnippetFilters>, language: string)- useTopicDetails hook call (useTrendingTopics.tsx:47):
trendingKeys.topicDetails(topicId || '', timespan, filters, language)✓- prefetchTopicDetails call (trending-card.tsx:126):
trendingKeys.topicDetails(topicId, timespan, filters, language)✓All three locations use the same argument order (topicId, timespan, filters, language), ensuring cache hits work as intended.
src/hooks/useTrendingTopics.tsx (1)
6-12:useTrendingTopicshook and key structure look correctThe
trendingKeyshelpers anduseTrendingTopicshook (Lines 6–12 and 14–31) provide a clear, reusable query key and correctly wire throughtimespan,filters,language, andlimit, with a 5‑minute stale time andkeepPreviousDatafor smooth transitions.Also applies to: 14-31
| export const useTopicDetails = ({ | ||
| topicId, | ||
| timespan, | ||
| filters, | ||
| language, | ||
| enabled = true | ||
| }: { | ||
| topicId: string | undefined | ||
| timespan: string | ||
| filters: Partial<SnippetFilters> | ||
| language: string | ||
| enabled?: boolean | ||
| }) => { | ||
| return useQuery<TopicDetailsResponse, Error>({ | ||
| queryKey: trendingKeys.topicDetails(topicId || '', timespan, filters, language), | ||
| queryFn: () => fetchTopicDetails({ topicId: topicId!, timespan, filters, language }), | ||
| staleTime: 1000 * 60 * 5, // Cache for 5 minutes | ||
| placeholderData: keepPreviousData, // Keep showing previous data while fetching | ||
| enabled: enabled && !!topicId | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# First, let's find and examine the useTrendingTopics.tsx file
fd -n "useTrendingTopics.tsx" --type fRepository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 2
stderr:
error: Found argument '-n' which wasn't expected, or isn't valid in this context
If you tried to supply '-n' as a value rather than a flag, use '-- -n'
Usage: fd [OPTIONS] [pattern] [path]...
For more information try '--help'
</shell_result>
Script executed:
#!/bin/bash
# Find and read the useTrendingTopics.tsx file
fd "useTrendingTopics.tsx" --type fRepository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 0
stdout:
src/hooks/useTrendingTopics.tsx
</shell_result>
Script executed:
#!/bin/bash
# Read the file with line numbers, focusing on lines 33-53
cat -n src/hooks/useTrendingTopics.tsx | sed -n '25,60p'Repository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 0
stdout:
25 return useQuery<TrendingTopicsResponse, Error>({
26 queryKey: trendingKeys.topics(timespan, filters, language),
27 queryFn: () => fetchTrendingTopics({ timespan, filters, language, limit }),
28 staleTime: 1000 * 60 * 5, // Cache for 5 minutes
29 placeholderData: keepPreviousData // Keep showing previous data while fetching
30 })
31 }
32
33 export const useTopicDetails = ({
34 topicId,
35 timespan,
36 filters,
37 language,
38 enabled = true
39 }: {
40 topicId: string | undefined
41 timespan: string
42 filters: Partial<SnippetFilters>
43 language: string
44 enabled?: boolean
45 }) => {
46 return useQuery<TopicDetailsResponse, Error>({
47 queryKey: trendingKeys.topicDetails(topicId || '', timespan, filters, language),
48 queryFn: () => fetchTopicDetails({ topicId: topicId!, timespan, filters, language }),
49 staleTime: 1000 * 60 * 5, // Cache for 5 minutes
50 placeholderData: keepPreviousData, // Keep showing previous data while fetching
51 enabled: enabled && !!topicId
52 })
53 }
</shell_result>
Script executed:
#!/bin/bash
# Check for ESLint configuration files
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) 2>/dev/nullRepository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 0
stdout:
./.eslintrc.json
./cypress/.eslintrc.cjs
</shell_result>
Script executed:
#!/bin/bash
# Read the main ESLint config
cat .eslintrc.jsonRepository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 0
stdout:
{
"root": true,
"env": {
"browser": true,
"node": true,
"es2021": true
},
"parser": "@typescript-eslint/parser",
"parserOptions": {
"project": ["./tsconfig.json"]
},
"plugins": ["react-prefer-function-component"],
"extends": [
"eslint:all",
"plugin:@typescript-eslint/all",
"plugin:import/recommended",
"plugin:import/typescript",
"plugin:react/all",
"airbnb",
"airbnb-typescript",
"airbnb/hooks",
"plugin:react/jsx-runtime",
"plugin:react-prefer-function-component/recommended",
"prettier"
],
"rules": {
"no-dupe-else-if": "error",
"no-promise-executor-return": "error",
"no-unreachable-loop": "error",
"no-useless-backreference": "error",
"require-atomic-updates": "error",
"default-case-last": "error",
"grouped-accessor-pairs": "error",
"no-constructor-return": "error",
"no-implicit-coercion": "error",
"prefer-regex-literals": "error",
"capitalized-comments": "error",
"react/require-default-props": "off",
"no-restricted-syntax": [
"error",
{
"selector": "ForInStatement",
"message": "for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array."
},
{
"selector": "LabeledStatement",
"message": "Labels are a form of GOTO; using them makes code confusing and hard to maintain and understand."
},
{
"selector": "WithStatement",
"message": "`with` is disallowed in strict mode because it makes code impossible to predict and optimize."
}
],
"no-void": "off",
"jsx-a11y/click-events-have-key-events": "off",
"jsx-a11y/no-static-element-interactions": "off",
"@typescript-eslint/padding-line-between-statements": "off",
"@typescript-eslint/prefer-enum-initializers": "off",
"@typescript-eslint/prefer-readonly-parameter-types": "off",
"@typescript-eslint/prefer-regexp-exec": "off",
"@typescript-eslint/explicit-function-return-type": "off",
"@typescript-eslint/no-magic-numbers": "off",
"@typescript-eslint/explicit-module-boundary-types": "off",
"@typescript-eslint/init-declarations": "off",
"@typescript-eslint/no-confusing-void-expression": [
"error",
{
"ignoreArrowShorthand": true
}
],
"@typescript-eslint/non-nullable-type-assertion-style": "off",
"@typescript-eslint/strict-boolean-expressions": "off",
"@typescript-eslint/no-implicit-any-catch": "off",
"@typescript-eslint/member-ordering": "off",
"@typescript-eslint/prefer-includes": "off",
"@typescript-eslint/no-restricted-imports": "off",
"@typescript-eslint/no-namespace": "off",
"import/no-deprecated": "error",
"import/order": "off",
"import/no-extraneous-dependencies": [
"error",
{
"devDependencies": [
"cypress.config.ts",
"vite.config.ts",
"src/setupTests.ts",
"src/testUtils.tsx",
"src/mocks/**",
"**/__tests__/*.{ts,tsx}"
]
}
],
"react/no-did-update-set-state": "off",
"react/no-find-dom-node": "off",
"react/no-is-mounted": "off",
"react/no-redundant-should-component-update": "off",
"react/no-render-return-value": "off",
"react/no-string-refs": "off",
"react/no-this-in-sfc": "off",
"react/no-will-update-set-state": "off",
"react/prefer-es6-class": "off",
"react/no-unused-state": "off",
"react/prefer-stateless-function": "off",
"react/require-render-return": "off",
"react/sort-comp": "off",
"react/state-in-constructor": "off",
"react/static-property-placement": "off",
"react/jsx-props-no-spreading": "off",
"react/boolean-prop-naming": [
"error",
{
"validateNested": true
}
],
"react/function-component-definition": "off",
"react-hooks/exhaustive-deps": "off",
"react/no-unstable-nested-components": "error",
"react/jsx-handler-names": [
"error",
{
"eventHandlerPrefix": "on",
"eventHandlerPropPrefix": "on",
"checkInlineFunction": true
}
],
"react/jsx-key": "error",
"react/jsx-no-bind": [
"error",
{
"ignoreRefs": false,
"allowArrowFunctions": true,
"allowFunctions": true,
"allowBind": false,
"ignoreDOMComponents": false
}
],
"react/jsx-no-constructed-context-values": "error",
"react/jsx-no-script-url": "error",
"react/jsx-no-useless-fragment": "error",
"unicorn/prevent-abbreviations": "off"
},
"settings": {
"react": {
"version": "detect"
}
},
"overrides": [
{
"files": ["src/**/*.ts?(x)"],
"parserOptions": {
"project": ["./tsconfig.json"]
}
},
{
"files": ["vite.config.ts", "cypress.config.ts"],
"parserOptions": {
"project": ["./tsconfig.node.json"]
}
},
{
"files": ["**/__tests__/**/*.ts?(x)"],
"extends": ["plugin:testing-library/react"],
"rules": {
"@typescript-eslint/no-magic-numbers": ["off"],
"testing-library/no-await-sync-events": [
"error",
{
"eventModules": ["fire-event"]
}
],
"testing-library/no-manual-cleanup": "error",
"testing-library/prefer-explicit-assert": "error",
"testing-library/prefer-user-event": "error"
}
}
]
}
</shell_result>
Script executed:
#!/bin/bash
# Search for usages of useTopicDetails
rg -n "useTopicDetails" --type tsx --type tsRepository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 2
stderr:
unrecognized file type: tsx
</shell_result>
Script executed:
#!/bin/bash
# Search for usages of useTopicDetails
rg -n "useTopicDetails" -t tsRepository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 0
stdout:
src/hooks/useTrendingTopics.tsx:33:export const useTopicDetails = ({
src/components/ui/trending-card.tsx:7:import { useTrendingTopics, useTopicDetails, trendingKeys } from '@/hooks/useTrendingTopics'
src/components/ui/trending-card.tsx:115: const { data: topicData, isLoading: topicLoading, error: topicError } = useTopicDetails({
</shell_result>
Script executed:
#!/bin/bash
# Read the trending-card.tsx file around line 115 to see how useTopicDetails is called
cat -n src/components/ui/trending-card.tsx | sed -n '105,130p'Repository: PublicDataWorks/verdad-frontend
<shell_result>
Repository: PublicDataWorks/verdad-frontend
Exit code: 0
stdout:
105
106 // Fetch trending topics (Discovery Mode)
107 const { data: trendingData, isLoading: trendingLoading, error: trendingError } = useTrendingTopics({
108 timespan,
109 filters,
110 language,
111 limit: expanded ? 10 : 5
112 })
113
114 // Fetch topic details (Focus Mode)
115 const { data: topicData, isLoading: topicLoading, error: topicError } = useTopicDetails({
116 topicId: focusedTopicId,
117 timespan,
118 filters,
119 language,
120 enabled: !!focusedTopicId
121 })
122
123 // Prefetch topic details on hover for faster transitions
124 const prefetchTopicDetails = useCallback((topicId: string) => {
125 queryClient.prefetchQuery({
126 queryKey: trendingKeys.topicDetails(topicId, timespan, filters, language),
127 queryFn: () => fetchTopicDetails({ topicId, timespan, filters, language }),
128 staleTime: 1000 * 60 * 5
129 })
130 }, [queryClient, timespan, filters, language])
</shell_result>
Based on my verification:
The review comment is accurate and the suggested fix is appropriate.
Verification Results:
- ✅ The
topicId!non-null assertion exists at line 48 inuseTopicDetails - ✅ The
no-non-null-assertionrule is active (inherited from@typescript-eslint/allin ESLint config) - ✅ The early return fix is compatible with existing callers:
trending-card.tsx(line 115) callsuseTopicDetailswithenabled: !!focusedTopicId- The caller already understands the pattern of using the
enabledflag
- ✅ The fix maintains identical runtime behavior while satisfying the lint rule
- ✅ The current code has an inconsistency:
queryKeyusestopicId || ''butqueryFnusestopicId!— the fix resolves this
Avoid topicId! by narrowing before calling useQuery
In useTopicDetails (Lines 33–53), the topicId! non‑null assertion in the queryFn (Line 48) breaks no-non-null-assertion even though you correctly gate the query with enabled: enabled && !!topicId.
You can keep the same behavior and satisfy the lint rule by returning early when topicId is undefined:
export const useTopicDetails = ({
topicId,
timespan,
filters,
language,
enabled = true
}: {
topicId: string | undefined
@@
-}) => {
- return useQuery<TopicDetailsResponse, Error>({
- queryKey: trendingKeys.topicDetails(topicId || '', timespan, filters, language),
- queryFn: () => fetchTopicDetails({ topicId: topicId!, timespan, filters, language }),
- staleTime: 1000 * 60 * 5, // Cache for 5 minutes
- placeholderData: keepPreviousData, // Keep showing previous data while fetching
- enabled: enabled && !!topicId
- })
-}
+}) => {
+ if (!topicId || !enabled) {
+ return useQuery<TopicDetailsResponse, Error>({
+ queryKey: trendingKeys.topicDetails('', timespan, filters, language),
+ queryFn: () => fetchTopicDetails({ topicId: '', timespan, filters, language }),
+ enabled: false,
+ staleTime: 1000 * 60 * 5,
+ placeholderData: keepPreviousData
+ })
+ }
+
+ return useQuery<TopicDetailsResponse, Error>({
+ queryKey: trendingKeys.topicDetails(topicId, timespan, filters, language),
+ queryFn: () => fetchTopicDetails({ topicId, timespan, filters, language }),
+ staleTime: 1000 * 60 * 5,
+ placeholderData: keepPreviousData,
+ enabled: true
+ })
+}🧰 Tools
🪛 ESLint
[error] 45-53: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.
(arrow-body-style)
[error] 48-48: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
🤖 Prompt for AI Agents
In src/hooks/useTrendingTopics.tsx around lines 33–53, remove the non‑null
assertion topicId! by narrowing topicId first: add an early return for the case
when topicId is falsy (use the same queryKey with '' or an appropriate
placeholder) and return a disabled/no‑op useQuery so the main useQuery call that
follows can safely reference topicId without using !; update queryFn to use the
narrowed topicId and keep enabled behavior unchanged.
- Show compact filter summary pills (languages, states, sources, political spectrum) instead of generic "Based on your filters" text - Auto-enter Focus Mode when exactly one label filter is selected - Hide Trending Card when 2+ label filters are manually added (not via Focus Mode) - Add icons for each filter type: Globe (languages), MapPin (states), Radio (sources), Gauge (political spectrum) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ca9571b in 1 minute and 1 seconds. Click for details.
- Reviewed
160lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/components/SearchInterface.tsx:170
- Draft comment:
The trending card now renders only when searchTerm is empty and (no labels OR a focused topic is set. Please add a comment explaining this condition. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. src/components/ui/trending-card.tsx:110
- Draft comment:
TIMESPAN_OPTIONS includes a '90d' option, even though the PR description lists only 24h, 7d, 30d, and all. Please confirm if the extra '90d' entry is intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/components/ui/trending-card.tsx:169
- Draft comment:
The focusedTopicId is set as filters.focusedTopic || (filters.labels?.length === 1 ? filters.labels[0] : undefined). Please add a clarifying comment to document this logic and ensure it’s the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. src/components/ui/trending-card.tsx:440
- Draft comment:
TopicSkeleton is called with count={expanded ? 5 : 5}. Likely this is a mistake; consider using a different count for expanded vs. non-expanded view. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_QORlmCBbZxeGxP2U
You can customize
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/components/ui/trending-card.tsx (3)
201-205: Rename parameter for clarity.The parameter
labelIdrepresents a topic ID. Rename it totopicIdfor clarity.Apply this diff:
- const handleTopicClick = (labelId: string) => { + const handleTopicClick = (topicId: string) => { // Enter Focus Mode: set focusedTopic and replace labels with just this one - setFilters({ focusedTopic: labelId, labels: [labelId] }) + setFilters({ focusedTopic: topicId, labels: [topicId] }) setShowSidebar(true) }
218-228: Fix variable shadowing, unsafe type assertion, and inconsistent return.The
useEffecthas three issues already identified in previous reviews:
- Variable
tin the map callback (line 221) shadows the translations objecttdeclared on line 158- Type assertion
as string(line 227) is unsafe sincecurrentDataKeycan be undefined- Inconsistent return pattern - should always return cleanup function or undefined
Apply this diff:
useEffect(() => { const currentDataKey = focusedTopicId ? topicData?.topic?.id - : JSON.stringify(topics.map(t => t.id)) + : JSON.stringify(topics.map(topic => topic.id)) if (prevDataRef.current !== null && prevDataRef.current !== currentDataKey) { setIsFading(true) const timer = setTimeout(() => setIsFading(false), 300) return () => clearTimeout(timer) } - prevDataRef.current = currentDataKey as string + prevDataRef.current = currentDataKey ?? null + return undefined }, [topics, topicData, focusedTopicId])
440-440: Fix skeleton count mismatch.The skeleton count is
expanded ? 5 : 5(always 5), but when expanded, you fetch 10 topics (line 176). This creates a visual inconsistency during loading.Apply this diff:
- focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 5 : 5} /> + focusedTopicId ? <FocusModeSkeleton /> : <TopicSkeleton count={expanded ? 10 : 5} />
📜 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 (2)
src/components/SearchInterface.tsx(3 hunks)src/components/ui/trending-card.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/SearchInterface.tsx (2)
src/lib/utils.ts (1)
cn(4-6)src/components/ui/trending-card.tsx (1)
TrendingCard(154-456)
🪛 ESLint
src/components/ui/trending-card.tsx
[error] 2-2: 'Filter' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 3-3: Missing file extension for "@/components/ui/card"
(import/extensions)
[error] 4-4: Missing file extension for "@/components/ui/button"
(import/extensions)
[error] 5-5: Missing file extension for "@/components/ui/sparkline"
(import/extensions)
[error] 6-6: Missing file extension for "@/components/ui/detail-chart"
(import/extensions)
[error] 7-7: Missing file extension for "@/hooks/useTrendingTopics"
(import/extensions)
[error] 8-8: Missing file extension for "@/apis/trending"
(import/extensions)
[error] 9-9: Import "Timespan" is only used as types.
(@typescript-eslint/consistent-type-imports)
[error] 9-9: Missing file extension for "@/hooks/useSnippetFilters"
(import/extensions)
[error] 10-10: Missing file extension for "@/providers/language"
(import/extensions)
[error] 11-11: Missing file extension for "@/providers/sidebar"
(import/extensions)
[error] 12-12: Missing file extension for "@/constants/translations"
(import/extensions)
[error] 13-13: Missing file extension for "@/lib/utils"
(import/extensions)
[error] 89-91: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 94-94: Do not use Array index in keys
(react/no-array-index-key)
[error] 106-106: Prop name expanded doesn’t match rule ^(is|has)[A-Z]([A-Za-z0-9]?)+
(react/boolean-prop-naming)
[error] 123-123: Do not use Array index in keys
(react/no-array-index-key)
[error] 218-218: Expected to return a value at the end of arrow function.
(consistent-return)
[error] 221-221: 't' is already declared in the upper scope on line 158 column 9.
(@typescript-eslint/no-shadow)
[error] 241-241: Use object destructuring.
(prefer-destructuring)
[error] 287-287: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 288-288: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 300-304: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 331-355: Missing an explicit type attribute for button
(react/button-has-type)
[error] 374-382: Missing an explicit type attribute for button
(react/button-has-type)
[error] 425-434: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 439-452: Do not nest ternary expressions.
(no-nested-ternary)
[error] 441-452: Do not nest ternary expressions.
(no-nested-ternary)
src/components/SearchInterface.tsx
[error] 34-34: Missing file extension for "@/lib/utils"
(import/extensions)
[error] 170-181: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 175-175: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 178-178: Do not nest ternary expressions.
(no-nested-ternary)
| {isEmpty(searchTerm) && (filters.labels.length === 0 || filters.focusedTopic) && ( | ||
| <div className={cn( | ||
| 'mb-6 grid gap-4', | ||
| showSidebar ? 'xl:grid-cols-2' : 'md:grid-cols-2' | ||
| )}> | ||
| {showWelcomeCard && <WelcomeCard />} | ||
| <TrendingCard | ||
| expanded={!showWelcomeCard} | ||
| className={showWelcomeCard ? '' : (showSidebar ? 'xl:col-span-2' : 'md:col-span-2')} | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Critical: Potential string leak in render condition.
The condition (filters.labels.length === 0 || filters.focusedTopic) will evaluate to the focusedTopic string value when it's truthy, causing React to attempt rendering that string before the JSX block. For example, if focusedTopic is "immigration", React would try to render "immigration" && <div>...</div>, which displays the string "immigration".
Apply this diff to coerce the condition to a boolean:
- {isEmpty(searchTerm) && (filters.labels.length === 0 || filters.focusedTopic) && (
+ {isEmpty(searchTerm) && (filters.labels.length === 0 || !!filters.focusedTopic) && (
<div className={cn(🧰 Tools
🪛 ESLint
[error] 170-181: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 175-175: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
[error] 178-178: Do not nest ternary expressions.
(no-nested-ternary)
🤖 Prompt for AI Agents
In src/components/SearchInterface.tsx around lines 170 to 181, the render
condition uses (filters.labels.length === 0 || filters.focusedTopic) which can
return a string when focusedTopic is truthy and leak into the output; change the
expression to coerce focusedTopic to a boolean (e.g., use !!filters.focusedTopic
or Boolean(filters.focusedTopic)) so the whole OR expression is a boolean,
keeping the surrounding JSX conditional intact.
| @@ -0,0 +1,456 @@ | |||
| import { useState, useEffect, useRef, useCallback } from 'react' | |||
| import { TrendingUp, Filter, ArrowLeft, TrendingDown, Minus, X, Globe, MapPin, Radio, Gauge } from 'lucide-react' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
The Filter icon is imported but never used in this file.
Apply this diff:
-import { TrendingUp, Filter, ArrowLeft, TrendingDown, Minus, X, Globe, MapPin, Radio, Gauge } from 'lucide-react'
+import { TrendingUp, ArrowLeft, TrendingDown, Minus, X, Globe, MapPin, Radio, Gauge } from 'lucide-react'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { TrendingUp, Filter, ArrowLeft, TrendingDown, Minus, X, Globe, MapPin, Radio, Gauge } from 'lucide-react' | |
| import { TrendingUp, ArrowLeft, TrendingDown, Minus, X, Globe, MapPin, Radio, Gauge } from 'lucide-react' |
🧰 Tools
🪛 ESLint
[error] 2-2: 'Filter' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In src/components/ui/trending-card.tsx around line 2 the import list includes
Filter which is not used; remove Filter from the named imports from
'lucide-react' so the import only includes the actually used icons (TrendingUp,
ArrowLeft, TrendingDown, Minus, X, Globe, MapPin, Radio, Gauge) to eliminate the
unused-import warning and keep imports minimal.
| <button | ||
| key={topic.id} | ||
| onClick={() => handleTopicClick(topic.id)} | ||
| onMouseEnter={() => prefetchTopicDetails(topic.id)} | ||
| className={cn( | ||
| 'flex items-center justify-between gap-2 rounded-md px-2 py-1.5 text-left transition-colors hover:bg-orange-200/50 dark:hover:bg-orange-800/50', | ||
| expanded && index >= 5 && 'hidden lg:flex' | ||
| )}> | ||
| <span className='flex-1 truncate text-sm font-medium text-orange-900 dark:text-orange-100'> | ||
| {topic.text} | ||
| </span> | ||
| <div className='flex items-center gap-2'> | ||
| <Sparkline | ||
| data={topic.sparkline} | ||
| width={60} | ||
| height={20} | ||
| strokeColor='#ea580c' | ||
| fillColor='#ea580c' | ||
| strokeWidth={1.5} | ||
| /> | ||
| <span className='min-w-[2.5rem] text-right text-xs font-semibold text-orange-600 dark:text-orange-400'> | ||
| {topic.count.toLocaleString()} | ||
| </span> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit button type.
Buttons without an explicit type attribute default to "submit", which can cause unintended form submissions. Add type="button" to make the intent clear.
Apply this diff:
<button
key={topic.id}
+ type="button"
onClick={() => handleTopicClick(topic.id)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| key={topic.id} | |
| onClick={() => handleTopicClick(topic.id)} | |
| onMouseEnter={() => prefetchTopicDetails(topic.id)} | |
| className={cn( | |
| 'flex items-center justify-between gap-2 rounded-md px-2 py-1.5 text-left transition-colors hover:bg-orange-200/50 dark:hover:bg-orange-800/50', | |
| expanded && index >= 5 && 'hidden lg:flex' | |
| )}> | |
| <span className='flex-1 truncate text-sm font-medium text-orange-900 dark:text-orange-100'> | |
| {topic.text} | |
| </span> | |
| <div className='flex items-center gap-2'> | |
| <Sparkline | |
| data={topic.sparkline} | |
| width={60} | |
| height={20} | |
| strokeColor='#ea580c' | |
| fillColor='#ea580c' | |
| strokeWidth={1.5} | |
| /> | |
| <span className='min-w-[2.5rem] text-right text-xs font-semibold text-orange-600 dark:text-orange-400'> | |
| {topic.count.toLocaleString()} | |
| </span> | |
| </div> | |
| </button> | |
| <button | |
| key={topic.id} | |
| type="button" | |
| onClick={() => handleTopicClick(topic.id)} | |
| onMouseEnter={() => prefetchTopicDetails(topic.id)} | |
| className={cn( | |
| 'flex items-center justify-between gap-2 rounded-md px-2 py-1.5 text-left transition-colors hover:bg-orange-200/50 dark:hover:bg-orange-800/50', | |
| expanded && index >= 5 && 'hidden lg:flex' | |
| )}> | |
| <span className='flex-1 truncate text-sm font-medium text-orange-900 dark:text-orange-100'> | |
| {topic.text} | |
| </span> | |
| <div className='flex items-center gap-2'> | |
| <Sparkline | |
| data={topic.sparkline} | |
| width={60} | |
| height={20} | |
| strokeColor='#ea580c' | |
| fillColor='#ea580c' | |
| strokeWidth={1.5} | |
| /> | |
| <span className='min-w-[2.5rem] text-right text-xs font-semibold text-orange-600 dark:text-orange-400'> | |
| {topic.count.toLocaleString()} | |
| </span> | |
| </div> | |
| </button> |
🧰 Tools
🪛 ESLint
[error] 331-355: Missing an explicit type attribute for button
(react/button-has-type)
🤖 Prompt for AI Agents
In src/components/ui/trending-card.tsx around lines 331 to 355, the button
element is missing an explicit type attribute and will default to "submit" in
forms; update the JSX for this button to include type="button" (i.e., add a type
prop to the button element) so it does not trigger unintended form submissions
while preserving existing props and event handlers.
| <button | ||
| onClick={handleExitFocusMode} | ||
| className='flex items-center gap-1 text-orange-600 hover:text-orange-800 dark:text-orange-400 dark:hover:text-orange-200' | ||
| > | ||
| <ArrowLeft className='h-5 w-5' /> | ||
| <span className='text-sm font-medium'> | ||
| {t.backToTrending || 'Back to trending'} | ||
| </span> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit button type.
Same as the button in Discovery Mode - add type="button" to prevent unintended form submissions.
Apply this diff:
<button
+ type="button"
onClick={handleExitFocusMode}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| onClick={handleExitFocusMode} | |
| className='flex items-center gap-1 text-orange-600 hover:text-orange-800 dark:text-orange-400 dark:hover:text-orange-200' | |
| > | |
| <ArrowLeft className='h-5 w-5' /> | |
| <span className='text-sm font-medium'> | |
| {t.backToTrending || 'Back to trending'} | |
| </span> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={handleExitFocusMode} | |
| className='flex items-center gap-1 text-orange-600 hover:text-orange-800 dark:text-orange-400 dark:hover:text-orange-200' | |
| > | |
| <ArrowLeft className='h-5 w-5' /> | |
| <span className='text-sm font-medium'> | |
| {t.backToTrending || 'Back to trending'} | |
| </span> | |
| </button> |
🧰 Tools
🪛 ESLint
[error] 374-382: Missing an explicit type attribute for button
(react/button-has-type)
🤖 Prompt for AI Agents
In src/components/ui/trending-card.tsx around lines 374 to 382, the button lacks
an explicit type which can cause it to act as a submit button inside a form; add
type="button" to the button element so it won’t trigger form submission, keeping
the existing onClick, classes, and children unchanged.
| {focusedTopicId && ( | ||
| <Button | ||
| variant='ghost' | ||
| size='sm' | ||
| onClick={handleExitFocusMode} | ||
| className='h-7 w-7 p-0 text-orange-600 hover:bg-orange-200/50 hover:text-orange-800 dark:text-orange-400' | ||
| > | ||
| <X className='h-4 w-4' /> | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Potential string leak in conditional render.
When focusedTopicId is a non-empty string, the condition focusedTopicId && <Button> will first render the string value before the Button component. Coerce to boolean with !!.
Apply this diff:
{/* Close button in Focus Mode */}
- {focusedTopicId && (
+ {!!focusedTopicId && (
<Button📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {focusedTopicId && ( | |
| <Button | |
| variant='ghost' | |
| size='sm' | |
| onClick={handleExitFocusMode} | |
| className='h-7 w-7 p-0 text-orange-600 hover:bg-orange-200/50 hover:text-orange-800 dark:text-orange-400' | |
| > | |
| <X className='h-4 w-4' /> | |
| </Button> | |
| )} | |
| {!!focusedTopicId && ( | |
| <Button | |
| variant='ghost' | |
| size='sm' | |
| onClick={handleExitFocusMode} | |
| className='h-7 w-7 p-0 text-orange-600 hover:bg-orange-200/50 hover:text-orange-800 dark:text-orange-400' | |
| > | |
| <X className='h-4 w-4' /> | |
| </Button> | |
| )} |
🧰 Tools
🪛 ESLint
[error] 425-434: Potential leaked value that might cause unintentionally rendered values or rendering crashes
(react/jsx-no-leaked-render)
🤖 Prompt for AI Agents
In src/components/ui/trending-card.tsx around lines 425 to 434, the conditional
render uses "focusedTopicId && <Button...>" which can cause the string value to
be rendered before the Button; coerce the condition to a boolean (e.g., use
"!!focusedTopicId" or "Boolean(focusedTopicId)") so only the Button is rendered
when focus exists, and replace the existing condition accordingly.
3 participants
Summary
Trending Topics Feature Overview
Overview
This feature adds a Trending Topics card to the search interface that shows the most mentioned labels over time. It has two modes:
Key Files
src/components/ui/trending-card.tsxsrc/hooks/useTrendingTopics.tsxsrc/apis/trending.tssrc/types/trending.tssrc/components/ui/sparkline.tsxsrc/components/ui/detail-chart.tsxsrc/components/SearchInterface.tsxBackend Requirements
Two Supabase RPC functions are needed:
1.
get_trending_topics2.
get_topic_detailsHow the UI Works
Discovery Mode:
Focus Mode:
Visibility Logic (
SearchInterface.tsx:170):Filter Summary Pills
When sidebar filters (languages, states, sources, political spectrum) are active, compact pills appear showing what's filtered:
Timespan Options
Local to the trending card only (doesn't affect main snippet filters):
Default: 30d
Key UX Decisions
keepPreviousData- Shows old data while fetching new, prevents blank screensTesting Notes
🤖 Generated with Claude Code