Conversation
Signed-off-by: ragnep <ragneinfo@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSeparated mobile and desktop tab scrollers, moved create-curation DOM wrapper, removed per-component class override, relocated countdown timing into WaveLeaderboardTime (feeding TimelineToggleHeader), adjusted countdown/time unit styles, simplified create-action and reworked useMediaQuery subscription logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Wave as WaveLeaderboardTime
participant Header as TimelineToggleHeader
participant Countdown as CompactTimeCountdown
Note left of Wave: Interval (every 1s) recomputes timeLeft
Wave->>Wave: calculateTimeLeft(nextDecisionTime)
Wave-->>Header: props { nextDecisionTime, timeLeft }
Header->>Countdown: render(timeLeft)
Countdown-->>Header: formatted units
Header-->>UI: combined countdown + formatted date shown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
hooks/useMediaQuery.ts (1)
37-42: Consider memoizing the MediaQueryList reference.The
getSnapshotcallback invokesgetMediaQueryList(query)on every call, anduseSyncExternalStoremay call this synchronously multiple times per render. While browsers cacheMediaQueryListobjects for identical queries (so this works correctly), memoizing the reference would make the code more explicit and avoid repeatedmatchMediacalls.♻️ Suggested optimization using useMemo
-import { useCallback, useSyncExternalStore } from "react"; +import { useCallback, useMemo, useSyncExternalStore } from "react"; const getMediaQueryList = (query: string): MediaQueryList | null => { if ( typeof window === "undefined" || typeof window.matchMedia !== "function" ) { return null; } return window.matchMedia(query); }; export function useMediaQuery(query: string): boolean { + const mediaQueryList = useMemo(() => getMediaQueryList(query), [query]); + const subscribe = useCallback( (onStoreChange: () => void) => { - const mediaQueryList = getMediaQueryList(query); if (!mediaQueryList) { return () => {}; } const handler = () => { onStoreChange(); }; if (typeof mediaQueryList.addEventListener === "function") { mediaQueryList.addEventListener("change", handler); return () => mediaQueryList.removeEventListener("change", handler); } mediaQueryList.addListener(handler); return () => mediaQueryList.removeListener(handler); }, - [query] + [mediaQueryList] ); - const getSnapshot = useCallback( - () => getMediaQueryList(query)?.matches ?? false, - [query] - ); + const getSnapshot = useCallback(() => mediaQueryList?.matches ?? false, [mediaQueryList]); return useSyncExternalStore(subscribe, getSnapshot, () => false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useMediaQuery.ts` around lines 37 - 42, The getSnapshot callback currently calls getMediaQueryList(query) on every invocation; memoize the MediaQueryList for the given query (e.g., via useMemo) and have getSnapshot read .matches from that memoized reference instead, and ensure subscribe also uses the same memoized MediaQueryList so match listeners attach/detach against the stable object (references: getSnapshot, getMediaQueryList, subscribe, query, useSyncExternalStore).components/waves/leaderboard/time/TimelineToggleHeader.tsx (1)
65-78: Prefer reusingCompactTimeCountdownto avoid duplicated countdown markup.Line 65 onward duplicates the same “Next winner” unit rendering already present in
components/waves/leaderboard/time/CompactTimeCountdown.tsx. Reusing that component will reduce UI drift risk.♻️ Proposed refactor
-import { TimeUnitDisplay } from "./TimeUnitDisplay"; +import { CompactTimeCountdown } from "./CompactTimeCountdown"; ... - <div className="tw-hidden tw-items-center tw-gap-1.5 tw-whitespace-nowrap tw-text-xs tw-font-medium tw-text-iron-300 md:tw-inline-flex"> - <span className="tw-whitespace-nowrap tw-font-medium tw-text-iron-300"> - Next winner: - </span> - <div className="tw-flex tw-items-center tw-gap-x-1.5"> - {timeLeft.days > 0 && ( - <TimeUnitDisplay value={timeLeft.days} label="days" /> - )} - <TimeUnitDisplay value={timeLeft.hours} label="hrs" /> - <TimeUnitDisplay value={timeLeft.minutes} label="min" /> - <TimeUnitDisplay value={timeLeft.seconds} label="sec" /> - </div> - </div> + <CompactTimeCountdown timeLeft={timeLeft} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/leaderboard/time/TimelineToggleHeader.tsx` around lines 65 - 78, The block in TimelineToggleHeader that manually renders the "Next winner" countdown duplicates CompactTimeCountdown; remove the duplicated markup (the div that contains the span "Next winner:" and the TimeUnitDisplay instances) and instead import and use CompactTimeCountdown, passing the existing timeLeft (or the values days/hours/minutes/seconds) and any label/props needed to render "Next winner" (or wrap CompactTimeCountdown with the span if it needs the label externally). Ensure you remove references to TimeUnitDisplay used only for this block and update imports accordingly so TimelineToggleHeader uses CompactTimeCountdown for the countdown UI.components/brain/my-stream/MyStreamWaveDesktopTabs.tsx (2)
317-325: Deduplicate tab-selection logic to prevent drift.The
onSelectimplementation is duplicated in mobile and desktop toggles. Extract one shared handler (e.g.,handleTabSelect) and reuse it in both places.♻️ Suggested refactor
+ const handleTabSelect = (key: string) => { + if (key.startsWith("curation:")) { + onSelectCuration(key.replace("curation:", "")); + return; + } + + onSelectCuration(null); + setActiveTab(key as MyStreamWaveTab); + }; ... - <TabToggle - options={mobileOptions} - activeKey={activeKey} - onSelect={(key) => { - if (key.startsWith("curation:")) { - onSelectCuration(key.replace("curation:", "")); - return; - } - - onSelectCuration(null); - setActiveTab(key as MyStreamWaveTab); - }} - /> + <TabToggle + options={mobileOptions} + activeKey={activeKey} + onSelect={handleTabSelect} + /> ... - <TabToggle - options={options} - activeKey={activeKey} - onSelect={(key) => { - if (key.startsWith("curation:")) { - onSelectCuration(key.replace("curation:", "")); - return; - } - - onSelectCuration(null); - setActiveTab(key as MyStreamWaveTab); - }} - /> + <TabToggle + options={options} + activeKey={activeKey} + onSelect={handleTabSelect} + />Also applies to: 346-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/my-stream/MyStreamWaveDesktopTabs.tsx` around lines 317 - 325, Extract the duplicated onSelect logic into a shared handler (e.g., handleTabSelect) and use it for both desktop and mobile toggles: move the current inline logic that checks key.startsWith("curation:"), calls onSelectCuration(...) or onSelectCuration(null), and calls setActiveTab(key as MyStreamWaveTab) into a single function named handleTabSelect, then replace the inline onSelect callbacks with handleTabSelect in both places; ensure handleTabSelect accepts the same key parameter and preserves the behavior for curation keys and non-curation keys.
308-356: Mount only one tab tree per breakpoint to avoid doubled UI work.Both mobile and desktop
TabTogglebranches are rendered simultaneously and one is only hidden via CSS. Consider conditionally rendering a single branch (based on breakpoint) to reduce duplicate reconciliation and DOM size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/my-stream/MyStreamWaveDesktopTabs.tsx` around lines 308 - 356, Both mobile and desktop TabToggle trees are mounted together and only hidden via CSS; change to render only one based on breakpoint (e.g., use a media hook like useMediaQuery/useIsMobile or a passed prop) so you don't mount both. Replace the unconditional rendering of the mobile div (ref mobileTabsScrollerRef, mobileOptions, mobileOverflowItems, EllipsisVerticalIcon, CompactMenu, onSelect logic) and the desktop div (ref desktopTabsScrollerRef, options) with a single conditional: if isMobile render the mobile tree and its ref/menu/options, else render the desktop tree and its ref/options; keep the existing onSelect handlers (calling onSelectCuration and setActiveTab) unchanged and ensure refs are only attached to the rendered branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/brain/my-stream/MyStreamWaveDesktopTabs.tsx`:
- Around line 317-325: Extract the duplicated onSelect logic into a shared
handler (e.g., handleTabSelect) and use it for both desktop and mobile toggles:
move the current inline logic that checks key.startsWith("curation:"), calls
onSelectCuration(...) or onSelectCuration(null), and calls setActiveTab(key as
MyStreamWaveTab) into a single function named handleTabSelect, then replace the
inline onSelect callbacks with handleTabSelect in both places; ensure
handleTabSelect accepts the same key parameter and preserves the behavior for
curation keys and non-curation keys.
- Around line 308-356: Both mobile and desktop TabToggle trees are mounted
together and only hidden via CSS; change to render only one based on breakpoint
(e.g., use a media hook like useMediaQuery/useIsMobile or a passed prop) so you
don't mount both. Replace the unconditional rendering of the mobile div (ref
mobileTabsScrollerRef, mobileOptions, mobileOverflowItems, EllipsisVerticalIcon,
CompactMenu, onSelect logic) and the desktop div (ref desktopTabsScrollerRef,
options) with a single conditional: if isMobile render the mobile tree and its
ref/menu/options, else render the desktop tree and its ref/options; keep the
existing onSelect handlers (calling onSelectCuration and setActiveTab) unchanged
and ensure refs are only attached to the rendered branch.
In `@components/waves/leaderboard/time/TimelineToggleHeader.tsx`:
- Around line 65-78: The block in TimelineToggleHeader that manually renders the
"Next winner" countdown duplicates CompactTimeCountdown; remove the duplicated
markup (the div that contains the span "Next winner:" and the TimeUnitDisplay
instances) and instead import and use CompactTimeCountdown, passing the existing
timeLeft (or the values days/hours/minutes/seconds) and any label/props needed
to render "Next winner" (or wrap CompactTimeCountdown with the span if it needs
the label externally). Ensure you remove references to TimeUnitDisplay used only
for this block and update imports accordingly so TimelineToggleHeader uses
CompactTimeCountdown for the countdown UI.
In `@hooks/useMediaQuery.ts`:
- Around line 37-42: The getSnapshot callback currently calls
getMediaQueryList(query) on every invocation; memoize the MediaQueryList for the
given query (e.g., via useMemo) and have getSnapshot read .matches from that
memoized reference instead, and ensure subscribe also uses the same memoized
MediaQueryList so match listeners attach/detach against the stable object
(references: getSnapshot, getMediaQueryList, subscribe, query,
useSyncExternalStore).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b5b6f88-c634-4162-94cf-a73653f345aa
📒 Files selected for processing (8)
components/brain/my-stream/MyStreamWaveDesktopTabs.tsxcomponents/brain/my-stream/tabs/MyStreamWaveCreateCurationAction.tsxcomponents/brain/my-stream/tabs/MyStreamWaveTabsMeme.tsxcomponents/waves/leaderboard/WaveLeaderboardTime.tsxcomponents/waves/leaderboard/time/CompactTimeCountdown.tsxcomponents/waves/leaderboard/time/TimeUnitDisplay.tsxcomponents/waves/leaderboard/time/TimelineToggleHeader.tsxhooks/useMediaQuery.ts
|
simo6529
approved these changes
Apr 11, 2026
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style
Refactor
Bug Fixes / Behavior