Conversation
📝 WalkthroughWalkthroughThis PR refactors the MyStream wave UI by splitting MyStreamWave into a Suspense-wrapped wrapper and extracted MyStreamWaveContent component, updates MyStreamContext to add wave registration on active wave selection and browser online/offline sync, adds comprehensive test coverage across components and hooks, modifies React Query cache updates to dynamically scan cache entries, configures periodic refetch intervals for sidebar waves overview, and introduces refactored helper hooks in usePinnedWavesServer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 (11)
hooks/usePinnedWavesServer.ts (2)
211-220: Nit: redundant type predicates onArray.prototype.find.
data/pageare already typed asApiWave[], so(wave): wave is ApiWave => ...adds no narrowing value. A plain predicate keeps the intent clearer.♻️ Optional simplification
- if (Array.isArray(data)) { - return data.find((wave): wave is ApiWave => wave.id === waveId); - } - - for (const page of data.pages) { - const wave = page.find((item): item is ApiWave => item.id === waveId); + if (Array.isArray(data)) { + return data.find((wave) => wave.id === waveId); + } + + for (const page of data.pages) { + const wave = page.find((item) => item.id === waveId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/usePinnedWavesServer.ts` around lines 211 - 220, The find predicates use redundant type-guard annotations `(wave): wave is ApiWave => ...` even though `data` and `page` are already `ApiWave[]`; replace those with simple boolean predicates like `wave => wave.id === waveId` in both the Array.isArray(data) branch and the `for (const page of data.pages)` loop (referencing variables `data`, `page`, `wave`, and `waveId` in this file) to simplify the code and keep intent clearer.
176-184: Use explicit comparison for pinned filter check.
Boolean(queryParams.pinned)treats any truthy value as "not a main waves query". Currently, onlyApiWavesPinFilter.Pinnedis used in WAVES_OVERVIEW queries, so no invalidation gap exists in practice. However, for defensive programming and clarity, match explicitly on the Pinned variant:Suggested refinement
- if (Boolean(queryParams.pinned)) { - return false; - } + if (queryParams.pinned === ApiWavesPinFilter.Pinned) { + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/usePinnedWavesServer.ts` around lines 176 - 184, The pinned filter check currently uses Boolean(queryParams.pinned) which treats any truthy value as a match; update the check to explicitly compare against the Pinned enum value (e.g., queryParams.pinned === ApiWavesPinFilter.Pinned) inside the same function (usePinnedWavesServer) so only the intended ApiWavesPinFilter.Pinned variant causes the early return; ensure the ApiWavesPinFilter symbol is available/imported in the module and keep the rest of the viewer_identity comparison logic unchanged.app/waves/waves-page.shared.tsx (1)
138-140: Suspense boundary wrapping is fine; confirm intent offallback={null}.With
fallback={null}, any future suspending work insideWavesPageClient(e.g.,use()of a promise, lazy loading) will show a blank screen rather than a skeleton. That’s acceptable if the client tree is never expected to suspend post-hydration, but worth keeping in mind when extending the subtree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/waves/waves-page.shared.tsx` around lines 138 - 140, The Suspense wrapper around WavesPageClient uses fallback={null}, which will render a blank screen if the client tree suspends; confirm whether that was intentional or replace it with a visible fallback (e.g., a small LoadingSkeleton or Loader component) so users see a placeholder during suspense. Locate the Suspense component that wraps WavesPageClient and either document the intent to keep fallback={null} or swap fallback={null} for a lightweight presentational component (e.g., LoadingSkeleton, WavesPlaceholder) that matches the page’s UX while async work resolves.app/messages/page.tsx (1)
15-22: Consider dropping the unusedsearchParamsprop.Since the component is now synchronous and
_searchParamsis unused, you could remove the prop entirely to simplify the signature. Next.js 15 still supports page components without props. Keeping it only matters if you plan to consume search params soon; otherwise it’s dead surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/messages/page.tsx` around lines 15 - 22, Remove the unused searchParams prop from the page component signature: update the export default function MessagesPage declaration to take no parameters (remove the searchParams/_searchParams argument and its Promise type) and delete the associated type annotation block; ensure there are no remaining references to _searchParams inside MessagesPage and adjust any tests or imports that might have expected the old signature.__tests__/hooks/useWavesOverview.test.tsx (1)
50-75: LGTM – forwards polling options touseInfiniteQuery.Good coverage for the new pass-through. One minor nit: you could also add a negative test asserting the defaults (e.g.,
refetchIntervalInBackground: falsewhen not provided) to lock in the default contract defined in the hook signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useWavesOverview.test.tsx` around lines 50 - 75, Add a complementary negative test to ensure useWavesOverview uses the hook defaults when polling options are omitted: in __tests__/hooks/useWavesOverview.test.tsx add a test that calls renderHook(() => useWavesOverview({ type: ApiWavesOverviewType.RecentlyDroppedTo })) (no refetchInterval/refetchIntervalInBackground) and assert useInfiniteQueryMock was called with expect.objectContaining({ refetchIntervalInBackground: false }) (and optionally default refetchInterval value), so the default contract in useWavesOverview is locked in.__tests__/contexts/wave/MyStreamContext.resume.test.tsx (1)
94-99: Restoredocument.visibilityStateafter the suite to avoid test pollution.
setDocumentVisibilityStatemutates the globaldocumentviadefinePropertyand is never reset. Because JSDOM is typically shared across test files in the same worker, leaving it as"hidden"after this suite can silently affect later tests that readdocument.visibilityState. Consider capturing the original descriptor and restoring it inafterAll, or at least resetting to"visible"inafterEach.♻️ Proposed fix
describe("MyStreamProvider resume sync", () => { const mainRefetch = jest.fn(); const dmRefetch = jest.fn(); + const originalVisibility = Object.getOwnPropertyDescriptor( + Document.prototype, + "visibilityState" + ); beforeEach(() => { jest.clearAllMocks(); setDocumentVisibilityState("visible"); useWavesListMock.mockReturnValue(createListData(mainRefetch)); useDmWavesListMock.mockReturnValue(createListData(dmRefetch)); }); + + afterAll(() => { + if (originalVisibility) { + Object.defineProperty(document, "visibilityState", originalVisibility); + } + });Also applies to: 122-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/contexts/wave/MyStreamContext.resume.test.tsx` around lines 94 - 99, The helper setDocumentVisibilityState mutates the global document.visibilityState without restoring it, risking test pollution; capture the original descriptor (e.g., via Object.getOwnPropertyDescriptor(document, "visibilityState")) before redefining it in setDocumentVisibilityState and restore that descriptor in an afterAll (or reset to "visible" in afterEach) so the global JSDOM state is returned to its prior value; update the test file to save the original descriptor and call Object.defineProperty(document, "visibilityState", originalDescriptor) during teardown.hooks/useWavesOverview.ts (1)
107-125: Minor: the deferred retry can still fire after unmount.The
setTimeoutbranches invokequery.fetchNextPage()/query.refetch()30s later with no cleanup, so a stale deferred retry can run after the consumer unmounts or afterlastErrorTimestampresets. TanStack will typically no-op on an unmounted observer, so this is mostly benign; if you want to tighten it, capture the timer id and clear it in a cleanup effect. Not a new issue introduced by this PR, just flagging adjacent to thevoidadditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useWavesOverview.ts` around lines 107 - 125, The deferred retry setTimeouts in the useCallback handlers fetchNextPage and refetch can fire after the component unmounts; capture the timer IDs when scheduling a retry and store them in a ref (e.g., retryTimerRef), then clear those timers in a useEffect cleanup (clearTimeout) to prevent stale callbacks from running; update both fetchNextPage and refetch to save the timer id returned by setTimeout into the ref and ensure the cleanup effect clears any existing timer when the component unmounts or when lastErrorTimestamp/query changes.components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsx (1)
11-29: Minor: include prioryour_latest_drop_timestampin theMath.max.
latestDropTimestampis computed fromtimestamp,wave.last_drop_time, andwave.metrics.latest_drop_timestamp, but it is then assigned toyour_latest_drop_timestampwithout consulting its previous value. If server data or clock skew ever leavesyour_latest_drop_timestampahead oflatest_drop_timestamp, this overwrite can move it backwards. Cheap to harden:🛡️ Proposed fix
const latestDropTimestamp = Math.max( timestamp, wave.last_drop_time, - wave.metrics.latest_drop_timestamp + wave.metrics.latest_drop_timestamp, + wave.metrics.your_latest_drop_timestamp ?? 0 );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsx` around lines 11 - 29, The updateWaveDropMetrics function currently computes latestDropTimestamp using timestamp, wave.last_drop_time and wave.metrics.latest_drop_timestamp but then unconditionally sets your_latest_drop_timestamp to that value; include the previous your_latest_drop_timestamp in the Math.max so we never move the user's latest drop backwards (i.e., compute latestDropTimestamp = Math.max(timestamp, wave.last_drop_time, wave.metrics.latest_drop_timestamp, wave.metrics.your_latest_drop_timestamp) while handling a possible undefined your_latest_drop_timestamp with a safe default).__tests__/hooks/useDmWavesList.test.tsx (1)
18-23: Optional: preferjest.mocked(...)overrequire(...) as jest.Mock.
jest.mocked(useAuth)(with normal ES imports) gives proper typing and avoids mixing CJSrequirewith ESM imports in a TS test file.♻️ Proposed refactor
-const useAuthMock = require("@/components/auth/Auth").useAuth as jest.Mock; -const useSeizeConnectContextMock = - require("@/components/auth/SeizeConnectContext") - .useSeizeConnectContext as jest.Mock; -const useWavesOverviewMock = require("@/hooks/useWavesOverview") - .useWavesOverview as jest.Mock; +import { useAuth } from "@/components/auth/Auth"; +import { useSeizeConnectContext } from "@/components/auth/SeizeConnectContext"; +import { useWavesOverview } from "@/hooks/useWavesOverview"; + +const useAuthMock = jest.mocked(useAuth); +const useSeizeConnectContextMock = jest.mocked(useSeizeConnectContext); +const useWavesOverviewMock = jest.mocked(useWavesOverview);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useDmWavesList.test.tsx` around lines 18 - 23, Replace the CommonJS require-as-mock pattern with typed Jest helpers: import the hooks (useAuth, useSeizeConnectContext, useWavesOverview) via normal ES imports and then wrap them with jest.mocked(...) to get proper typings instead of using useAuthMock/useSeizeConnectContextMock/useWavesOverviewMock created via require(...) as jest.Mock; update any test usages to reference the jest.mocked(...) instances and ensure the corresponding modules are jest.mock(...)'d at top of the test file.components/brain/my-stream/MyStreamWave.tsx (1)
8-14: LGTM — Suspense wrapper is clean.One optional consideration:
fallback={null}means users see an empty panel briefly whenuseWaveDatasuspends on initial load. If that’s visible on slower connections, a lightweight skeleton here (or at the parent boundary) would be a nicer UX. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/my-stream/MyStreamWave.tsx` around lines 8 - 14, The Suspense boundary in the MyStreamWave component currently uses fallback={null}, which can show an empty panel while MyStreamWaveContent (and its useWaveData hook) suspends; replace the null fallback with a lightweight skeleton or spinner component (or accept a fallback prop from the parent) so MyStreamWave renders a brief placeholder during initial load — locate MyStreamWave and swap fallback={null} for a small Skeleton/Loader component (or thread a fallback prop through MyStreamWave to keep the boundary flexible).components/brain/my-stream/MyStreamWaveContent.tsx (1)
116-138: Consider memoizingonDropClick/onSelectCurationwithuseCallback.These handlers are recreated on every render and are passed down to multiple child components (
MyStreamWaveChat,MyStreamWaveLeaderboard,MyStreamWaveSubmissions,WaveWinners,MyStreamWaveMyVotes,MyStreamWaveCurationContent, andMyStreamWaveTabs). If any of these memoize by prop identity, they’ll re-render unnecessarily. Since the component re-renders wheneverwaves.list/directMessages.listchanges (which is frequent), wrapping inuseCallbackis worthwhile.♻️ Proposed change
- const onDropClick = (drop: ExtendedDrop) => { + const onDropClick = useCallback((drop: ExtendedDrop) => { queryClient.setQueryData<ApiDrop>( [QueryKey.DROP, { drop_id: drop.id }], drop as ApiDrop ); const params = new URLSearchParams(searchParams.toString() || ""); params.set("drop", drop.id); router.push(`${pathname}?${params.toString()}`, { scroll: false }); - }; + }, [queryClient, searchParams, pathname, router]); - const onSelectCuration = (curationId: string | null) => { + const onSelectCuration = useCallback((curationId: string | null) => { const params = new URLSearchParams(searchParams.toString() || ""); if (curationId) { params.set("curation", curationId); } else { params.delete("curation"); } const nextQuery = params.toString(); const nextUrl = nextQuery ? `${pathname}?${nextQuery}` : pathname; router.replace(nextUrl, { scroll: false }); - }; + }, [searchParams, pathname, router]);Requires adding
useCallbackto the React import on line 3.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/my-stream/MyStreamWaveContent.tsx` around lines 116 - 138, Wrap the onDropClick and onSelectCuration handlers in useCallback and add useCallback to the React import so their identities are stable across renders; specifically, convert the functions named onDropClick and onSelectCuration into callbacks with appropriate dependency arrays (include queryClient, searchParams, pathname, router for onDropClick and include searchParams, pathname, router for onSelectCuration) so child components that rely on prop identity won't re-render unnecessarily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/contexts/wave/MyStreamContext.resume.test.tsx`:
- Around line 94-99: The helper setDocumentVisibilityState mutates the global
document.visibilityState without restoring it, risking test pollution; capture
the original descriptor (e.g., via Object.getOwnPropertyDescriptor(document,
"visibilityState")) before redefining it in setDocumentVisibilityState and
restore that descriptor in an afterAll (or reset to "visible" in afterEach) so
the global JSDOM state is returned to its prior value; update the test file to
save the original descriptor and call Object.defineProperty(document,
"visibilityState", originalDescriptor) during teardown.
In `@__tests__/hooks/useDmWavesList.test.tsx`:
- Around line 18-23: Replace the CommonJS require-as-mock pattern with typed
Jest helpers: import the hooks (useAuth, useSeizeConnectContext,
useWavesOverview) via normal ES imports and then wrap them with jest.mocked(...)
to get proper typings instead of using
useAuthMock/useSeizeConnectContextMock/useWavesOverviewMock created via
require(...) as jest.Mock; update any test usages to reference the
jest.mocked(...) instances and ensure the corresponding modules are
jest.mock(...)'d at top of the test file.
In `@__tests__/hooks/useWavesOverview.test.tsx`:
- Around line 50-75: Add a complementary negative test to ensure
useWavesOverview uses the hook defaults when polling options are omitted: in
__tests__/hooks/useWavesOverview.test.tsx add a test that calls renderHook(() =>
useWavesOverview({ type: ApiWavesOverviewType.RecentlyDroppedTo })) (no
refetchInterval/refetchIntervalInBackground) and assert useInfiniteQueryMock was
called with expect.objectContaining({ refetchIntervalInBackground: false }) (and
optionally default refetchInterval value), so the default contract in
useWavesOverview is locked in.
In `@app/messages/page.tsx`:
- Around line 15-22: Remove the unused searchParams prop from the page component
signature: update the export default function MessagesPage declaration to take
no parameters (remove the searchParams/_searchParams argument and its Promise
type) and delete the associated type annotation block; ensure there are no
remaining references to _searchParams inside MessagesPage and adjust any tests
or imports that might have expected the old signature.
In `@app/waves/waves-page.shared.tsx`:
- Around line 138-140: The Suspense wrapper around WavesPageClient uses
fallback={null}, which will render a blank screen if the client tree suspends;
confirm whether that was intentional or replace it with a visible fallback
(e.g., a small LoadingSkeleton or Loader component) so users see a placeholder
during suspense. Locate the Suspense component that wraps WavesPageClient and
either document the intent to keep fallback={null} or swap fallback={null} for a
lightweight presentational component (e.g., LoadingSkeleton, WavesPlaceholder)
that matches the page’s UX while async work resolves.
In `@components/brain/my-stream/MyStreamWave.tsx`:
- Around line 8-14: The Suspense boundary in the MyStreamWave component
currently uses fallback={null}, which can show an empty panel while
MyStreamWaveContent (and its useWaveData hook) suspends; replace the null
fallback with a lightweight skeleton or spinner component (or accept a fallback
prop from the parent) so MyStreamWave renders a brief placeholder during initial
load — locate MyStreamWave and swap fallback={null} for a small Skeleton/Loader
component (or thread a fallback prop through MyStreamWave to keep the boundary
flexible).
In `@components/brain/my-stream/MyStreamWaveContent.tsx`:
- Around line 116-138: Wrap the onDropClick and onSelectCuration handlers in
useCallback and add useCallback to the React import so their identities are
stable across renders; specifically, convert the functions named onDropClick and
onSelectCuration into callbacks with appropriate dependency arrays (include
queryClient, searchParams, pathname, router for onDropClick and include
searchParams, pathname, router for onSelectCuration) so child components that
rely on prop identity won't re-render unnecessarily.
In `@components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsx`:
- Around line 11-29: The updateWaveDropMetrics function currently computes
latestDropTimestamp using timestamp, wave.last_drop_time and
wave.metrics.latest_drop_timestamp but then unconditionally sets
your_latest_drop_timestamp to that value; include the previous
your_latest_drop_timestamp in the Math.max so we never move the user's latest
drop backwards (i.e., compute latestDropTimestamp = Math.max(timestamp,
wave.last_drop_time, wave.metrics.latest_drop_timestamp,
wave.metrics.your_latest_drop_timestamp) while handling a possible undefined
your_latest_drop_timestamp with a safe default).
In `@hooks/usePinnedWavesServer.ts`:
- Around line 211-220: The find predicates use redundant type-guard annotations
`(wave): wave is ApiWave => ...` even though `data` and `page` are already
`ApiWave[]`; replace those with simple boolean predicates like `wave => wave.id
=== waveId` in both the Array.isArray(data) branch and the `for (const page of
data.pages)` loop (referencing variables `data`, `page`, `wave`, and `waveId` in
this file) to simplify the code and keep intent clearer.
- Around line 176-184: The pinned filter check currently uses
Boolean(queryParams.pinned) which treats any truthy value as a match; update the
check to explicitly compare against the Pinned enum value (e.g.,
queryParams.pinned === ApiWavesPinFilter.Pinned) inside the same function
(usePinnedWavesServer) so only the intended ApiWavesPinFilter.Pinned variant
causes the early return; ensure the ApiWavesPinFilter symbol is
available/imported in the module and keep the rest of the viewer_identity
comparison logic unchanged.
In `@hooks/useWavesOverview.ts`:
- Around line 107-125: The deferred retry setTimeouts in the useCallback
handlers fetchNextPage and refetch can fire after the component unmounts;
capture the timer IDs when scheduling a retry and store them in a ref (e.g.,
retryTimerRef), then clear those timers in a useEffect cleanup (clearTimeout) to
prevent stale callbacks from running; update both fetchNextPage and refetch to
save the timer id returned by setTimeout into the ref and ensure the cleanup
effect clears any existing timer when the component unmounts or when
lastErrorTimestamp/query changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4888dac3-57bf-4ab7-b3a1-528cd8704945
📒 Files selected for processing (20)
__tests__/components/brain/my-stream/MyStreamWave.register.test.tsx__tests__/components/react-query-wrapper/utils/increaseWavesOverviewDropsCount.test.ts__tests__/contexts/wave/MyStreamContext.resume.test.tsx__tests__/contexts/wave/hooks/useEnhancedWavesListCore.test.tsx__tests__/hooks/useDmWavesList.test.tsx__tests__/hooks/usePinnedWavesServer.test.tsx__tests__/hooks/useWavesList.test.tsx__tests__/hooks/useWavesOverview.test.tsxapp/messages/page.tsxapp/waves/waves-page.shared.tsxcomponents/brain/my-stream/MyStreamWave.tsxcomponents/brain/my-stream/MyStreamWaveContent.tsxcomponents/react-query-wrapper/utils/increaseWavesOverviewDropsCount.tsxcomponents/react-query-wrapper/utils/query-utils.tscontexts/wave/MyStreamContext.tsxcontexts/wave/hooks/useEnhancedWavesListCore.tshooks/useDmWavesList.tshooks/usePinnedWavesServer.tshooks/useWavesList.tshooks/useWavesOverview.ts
ragnep
approved these changes
Apr 17, 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
Release Notes
New Features
Bug Fixes
Performance