Conversation
Signed-off-by: ragnep <ragneinfo@gmail.com>
WalkthroughSwitched active-wave resolution from URL search params to the MyStream in-app context and a local activeWaveId manager; threaded an onClick handler through left‑sidebar wave components and hooks; reordered providers; removed explicit unstable_noStore/noStore usage from the waves route; updated tests and navigation helpers for history-based routing. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(243,244,246,0.9)
participant User
participant WaveUI as Wave Item
participant NavHook as useWaveNavigation
participant MyStream as MyStreamContext / ActiveWaveManager
participant Router as Next.js Router/Link
end
User->>WaveUI: hover
WaveUI->>NavHook: onMouseEnter()
NavHook->>WaveUI: prefetch (if not active)
User->>WaveUI: click
WaveUI->>NavHook: onClick(event)
Note right of NavHook: preventDefault, ensure prefetch
NavHook->>MyStream: setActiveWave(waveId, options)
alt same route type (waves/messages)
NavHook->>MyStream: history.pushState/replaceState (update URL)
else different route type
NavHook->>Router: router.push/replace (navigate)
end
Router-->>User: navigation completes / view updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (1)
78-84: Consider reusing the useWaveNavigation hook pattern.This component duplicates navigation logic that's already encapsulated in the
useWaveNavigationhook (used byWebBrainLeftSidebarWave). Consider refactoring to use a shared navigation hook or utility to maintain consistency and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/waves/page.tsx(0 hunks)components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx(3 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts(4 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx(3 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/CollapsedWave.tsx(3 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/ExpandedWave.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- app/waves/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx (1)
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts (1)
useWaveNavigation(20-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx (1)
36-42: LGTM! Clean onClick propagation pattern.The onClick handler from
useWaveNavigationis properly destructured and passed to bothCollapsedWaveandExpandedWavecomponents, ensuring consistent navigation behavior across collapsed and expanded states.Also applies to: 74-74, 93-93
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/ExpandedWave.tsx (2)
19-19: LGTM! Correctly typed onClick prop.The onClick prop is properly typed to accept the anchor click event, matching the handler signature from
useWaveNavigation.
63-72: LGTM! onClick handler correctly attached.The onClick handler is properly forwarded to the Link element alongside onMouseEnter, enabling client-side navigation with prefetch on hover.
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/CollapsedWave.tsx (1)
13-13: LGTM! Consistent onClick implementation.The onClick prop and its forwarding to the Link element mirrors the implementation in
ExpandedWave.tsx, ensuring consistent navigation behavior across both presentation states.Also applies to: 27-27, 43-43
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (1)
82-82: Verify that router.replace is intentional.Using
router.replaceinstead ofrouter.pushmeans wave navigation won't create browser history entries, preventing users from using the back button to return to previously viewed waves. Please confirm this is the intended UX behavior.components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts (3)
2-2: LGTM! Router properly integrated.The Next.js router is correctly imported and initialized for client-side navigation.
Also applies to: 27-27
51-56: LGTM! Well-structured onClick handler with correct dependencies.The onClick handler properly:
- Prevents default anchor navigation
- Uses
router.replacefor shallow, non-scrolling navigation- Triggers hover effects and prefetch via
onMouseEnter()- Includes all necessary dependencies in the useCallback array
The dependency array correctly includes
href,router, andonMouseEnter, all of which are stable or properly memoized.
54-54: Verify that router.replace is intentional.Using
router.replacemeans wave selections won't create browser history entries. Users won't be able to use the back button to navigate between previously viewed waves. Please confirm this shallow navigation behavior aligns with the intended UX.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (1)
57-58: Consider simplifying activeWaveId extraction.The
?? undefinedconversion is redundant since bothnullandundefinedare handled identically throughout the code (they're both falsy and fail the===checks).Apply this diff to simplify:
- const activeWaveId = activeWave.id ?? undefined; + const activeWaveId = activeWave.id;contexts/wave/hooks/useActiveWaveManager.ts (2)
3-3: Remove unuseduseMemoimport.
useMemoisn’t used in this hook; dropping it will satisfy static analysis and avoid a pointless import.-import { useCallback, useEffect, useMemo, useState } from "react"; +import { useCallback, useEffect, useState } from "react";
10-15: ConsiderglobalThishelpers for environment‑safe window access.Sonar’s
globalThiswarnings are about defensive environment handling. If you want to clear them and make the hook more robust in non‑browser test environments, consider centralizing window access, e.g.:-const getWaveFromUrl = (): string | null => { - if (typeof window === "undefined") return null; - const url = new URL(window.location.href); +const getWaveFromUrl = (): string | null => { + const win = (globalThis as any).window as Window | undefined; + if (!win) return null; + const url = new URL(win.location.href); const wave = url.searchParams.get("wave"); return typeof wave === "string" ? wave : null; };and similarly using
wininstead ofwindowin the effect andsetActiveWave. Not mandatory, but it will align with the static‑analysis guidance.Also applies to: 28-34, 41-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx(5 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts(3 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx(4 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/CollapsedWave.tsx(3 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/ExpandedWave.tsx(3 hunks)components/header/AppHeader.tsx(3 hunks)components/waves/WavesView.tsx(1 hunks)contexts/TitleContext.tsx(3 hunks)contexts/wave/hooks/useActiveWaveManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/ExpandedWave.tsx
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/CollapsedWave.tsx
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
components/header/AppHeader.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
contexts/TitleContext.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
components/waves/WavesView.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (2)
contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)helpers/navigation.helpers.ts (2)
getWaveHomeRoute(71-81)getWaveRoute(21-69)
contexts/wave/hooks/useActiveWaveManager.ts (2)
hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)helpers/navigation.helpers.ts (2)
getWaveRoute(21-69)getWaveHomeRoute(71-81)
🪛 GitHub Check: SonarCloud Code Analysis
contexts/wave/hooks/useActiveWaveManager.ts
[warning] 3-3: Remove this unused import of 'useMemo'.
[warning] 12-12: Prefer globalThis over window.
[warning] 50-50: Prefer globalThis over window.
[warning] 11-11: Prefer globalThis.window over window.
[warning] 41-41: Prefer globalThis.window over window.
[warning] 33-33: Prefer globalThis over window.
[warning] 51-51: Prefer globalThis over window.
[warning] 32-32: Prefer globalThis over window.
[warning] 50-50: Prefer globalThis over window.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (3)
3-3: LGTM!The new imports are correctly added to support the context-based navigation refactor.
Also applies to: 13-13
59-64: LGTM!The href computation correctly implements toggle behavior: clicking an active wave navigates to the home route (deselects it), while clicking an inactive wave navigates to it. Dependencies are complete.
68-73: LGTM!The callback implementation is well-structured:
onWaveHovercorrectly avoids redundant prefetching when the wave is already activehandleWaveClickproperly handles modifier keys and middle-click to preserve browser default behavior (open in new tab/window)- Toggle behavior (clicking active wave sets
activeWavetonull) provides intuitive deselection UX- All dependencies in both
useCallbackhooks are complete and correctThe past review concern about missing dependencies has been fully addressed.
Also applies to: 77-90, 104-107
contexts/TitleContext.tsx (1)
5-5: Active wave now correctly prioritizes stream context over URL.Using
useMyStreamOptionalandmyStream?.activeWave.id ?? searchParams?.get("wave") ?? nullis a clean way to make the in‑app stream context the primary source of truth while preserving URL fallback for initial load. The dependency update forwaveParamin the effect is also consistent.Also applies to: 74-89
components/waves/WavesView.tsx (1)
11-20: Wave selection via MyStream context looks good.Deriving
serialisedWaveIdfrommyStream?.activeWave.idaligns this view with the shared active‑wave state and keeps the rest of the logic unchanged, which is what we want for consistent client‑side navigation.components/header/AppHeader.tsx (1)
8-8: Header now correctly tracks active wave from context.Switching
waveIdtomyStream?.activeWave.idkeeps the header title and back‑button logic in sync with the new MyStream‑driven active‑wave state without changing the existinguseWaveByIdusage.Also applies to: 18-19, 26-27, 50-51
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (1)
77-90: Click handler behavior is robust;event.buttoncheck is likely redundantThe click handler correctly:
- Bails out if the event was already prevented.
- Lets modifier/middle‑clicks fall through to the browser/Next.js for new‑tab/window behavior.
- Prevents default only for a plain primary click, then calls
onWaveHover()and togglesactiveWaveviaactiveWave.set(nextWaveId).- Has a complete dependency list (
activeWave.set,activeWaveId,onWaveHover,wave.id).Minor nit: for a React
onClickon an anchor,event.buttonis typically always0(primary button); middle‑clicks fireonAuxClickinstead. Theevent.button === 1branch is therefore probably never hit and can be dropped to simplify the condition without changing behavior.If you’d like to simplify:
- if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey || event.button === 1) { + if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) { // let browser handle new tab/window return; }contexts/wave/hooks/useActiveWaveManager.ts (1)
40-63:setActiveWavedrops other query params and has an unusedeventoptionTwo points worth tightening up here:
- Loss of unrelated query params
targetis built using onlywaveId,isDirectMessage, andisApp:const target = waveId ? getWaveRoute({ waveId, isDirectMessage, isApp }) : getWaveHomeRoute({ isDirectMessage, isApp });This means any existing search params under
/wavesor/messages(e.g. filters, sort order,serialNo, etc.) are discarded whensetActiveWaveis called. If other features depend on those params, this will be a regression.Consider deriving
extraParamsfrom the current URL (excludingwave) when callinggetWaveRoute, or otherwise preserving the non‑wave search parameters you care about.
- Unused and narrowly‑typed
eventoption
options.event?: MouseEventis currently unused. If call sites pass a ReactMouseEvent, this will be a type mismatch, and right now the event doesn’t affect behavior at all.If the intent is to eventually hijack link clicks to avoid full navigations while still respecting modifier keys, you might:
- Either remove
eventuntil you actually need it, or- Handle it and broaden the type to accept React events, something like:
-import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; +import type React from "react"; - options?: { isDirectMessage?: boolean; replace?: boolean; event?: MouseEvent } + options?: { + isDirectMessage?: boolean; + replace?: boolean; + event?: MouseEvent | React.MouseEvent<HTMLElement>; + } ) => { const currentWindow = globalThis.window; if (!currentWindow) return; + const mouseEvent = options?.event; + if (mouseEvent) { + // Respect modified / non-left clicks and let the browser handle them + if ( + mouseEvent.defaultPrevented || + mouseEvent.button !== 0 || + mouseEvent.metaKey || + mouseEvent.ctrlKey || + mouseEvent.shiftKey || + mouseEvent.altKey + ) { + return; + } + mouseEvent.preventDefault(); + }That keeps default browser behaviors (new tab, etc.) intact while still enabling your custom navigation when appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx(5 hunks)contexts/wave/hooks/useActiveWaveManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (2)
contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)helpers/navigation.helpers.ts (2)
getWaveHomeRoute(71-81)getWaveRoute(21-69)
contexts/wave/hooks/useActiveWaveManager.ts (2)
hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)helpers/navigation.helpers.ts (2)
getWaveRoute(21-69)getWaveHomeRoute(71-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (3)
32-65: Context-drivenactiveWaveIdandhreflogic looks good; please confirm URL semantics are intentionalUsing
useMyStream’sactiveWave.idas the single source of truth and derivingisActive/hreffrom it is clean, and theuseMemodependencies ([activeWaveId, isApp, isDirectMessage, wave.id]) are complete.One behavioral change is that a plain click now only updates
activeWavein context; it no longer updates the URL query string (the URL only matters for modifier‑key/new‑tab navigation viahref). That means the address bar may not reflect the currently active wave anymore, which can affect deep-linking and “copy URL to share” flows if other parts of the app still read?wave=on initial load.If the intention is for MyStream’s
activeWaveto be fully canonical and for the URL to be best‑effort / new‑tab only, this is fine; otherwise, you may want to also sync the URL (e.g., via a router replace) whenactiveWavechanges.
68-74: Hover/prefetch callback is tight and dependencies are correct
onWaveHover’s guard against the currentactiveWaveIdavoids redundant work, and the dependency array covers everything it closes over (activeWaveId,onHover,prefetchWaveData,wave.id). No issues here.
104-107: Link wiring aligns well with new client-side navigation modelPassing the memoized
href, usingonMouseEnter={onWaveHover}for prefetch, andonClick={handleWaveClick}for in‑place activeWave toggling is consistent and keeps standard link behavior for modifier/new‑tab clicks. Looks solid.contexts/wave/hooks/useActiveWaveManager.ts (2)
3-16: Helper encapsulation and SSR guard look solid
getWaveFromUrl’s use ofglobalThis.windowwith a falsy check avoids crashes during SSR/tests while still usingURL/searchParamson the client, and centralizing this logic makes future changes easier. No changes needed here.
24-38: Potential desync if URL changes without going through this manager
activeWaveIdis initialized fromgetWaveFromUrl()and then only updated via:
- explicit calls to
setActiveWave, and- the
popstatelistener (back/forward navigation).Browser
history.pushState/replaceStatecalls (including those triggered by the Next.js router or other custom code) do not emitpopstate, so any navigation that changes thewavequery param without callingsetActiveWavewill leaveactiveWaveIdstale.Please double‑check that all waves navigation which changes the
waveparam now goes through this hook, or consider an additional mechanism to observe URL changes (e.g., wiring into whatever central navigation you still rely on) to keepactiveWaveIdin sync.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contexts/wave/hooks/useActiveWaveManager.ts (1)
24-38: Consider lazy initialization to avoid the extra render cycle.The
activeWaveIdstate starts asnulland is immediately updated in theuseEffect, causing an extra render. SincegetWaveFromUrlis safe to call during initialization (checks forwindow), you could use lazy initialization:- const [activeWaveId, setActiveWaveId] = useState<string | null>(null); + const [activeWaveId, setActiveWaveId] = useState<string | null>(() => getWaveFromUrl());This would eliminate the initial
null→ actual value transition and the corresponding re-render on mount.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/providers/Providers.tsx(1 hunks)contexts/TitleContext.tsx(5 hunks)contexts/wave/hooks/useActiveWaveManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
contexts/TitleContext.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
contexts/wave/hooks/useActiveWaveManager.ts (2)
hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)helpers/navigation.helpers.ts (2)
getWaveRoute(21-69)getWaveHomeRoute(71-81)
components/providers/Providers.tsx (6)
components/brain/my-stream/layout/LayoutContext.tsx (1)
LayoutProvider(166-508)contexts/wave/MyStreamContext.tsx (1)
MyStreamProvider(92-240)contexts/TitleContext.tsx (1)
TitleProvider(71-199)contexts/HeaderContext.tsx (1)
HeaderProvider(22-53)contexts/ScrollPositionContext.tsx (1)
ScrollPositionProvider(14-34)contexts/NavigationHistoryContext.tsx (1)
NavigationHistoryProvider(40-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
components/providers/Providers.tsx (1)
48-62: Provider ordering is correct for the new context dependencies.The reordering correctly places
MyStreamProvideraboveTitleProvider, which is required sinceTitleProvidernow callsuseMyStreamOptional()to accessmyStream?.activeWave.id. TheViewProvider→NavigationHistoryProviderdependency is also preserved.contexts/TitleContext.tsx (2)
87-92: Fallback chain for wave param looks correct.The prioritization of
myStream?.activeWave.idover URL search params aligns with the client-side navigation approach. TheisWaveRoutedetection appropriately covers/waves,/messages, and home page withview=waves.
137-160: Title computation logic is well-structured.The IIFE pattern for
waveTitlekeeps the logic contained. The conditionnotificationCount === 0correctly prevents showing both wave-specific message counts and global notification counts simultaneously.contexts/wave/hooks/useActiveWaveManager.ts (2)
10-16: Helper function is SSR-safe and correct.The
globalThis.windowcheck handles server-side rendering correctly. Minor note: thetypeof wave === "string"check on line 15 is technically redundant sinceURLSearchParams.get()already returnsstring | null, but it doesn't cause any issues.
40-64: Wave ID source inconsistency: Many components still read from searchParams instead of MyStreamContext.Verification shows 21 files reading
searchParams.get('wave')directly, including critical components likeAppLayout.tsx,NavItem.tsx,ViewContext.tsx,BackButton.tsx,MessagesView.tsx, and multipleBrainContent*components. These won't re-render when the active wave changes viasetActiveWave()withpushState.While
TitleContext.tsx:88uses a context-first fallback pattern, most other components bypass context entirely. This suggests either an incomplete migration or a need to clarify the intended architecture:
- If components should be context-aware: Refactor to use
useMyStream()as primary source, with URL params as fallback only for initial load- If URL params are intentional: Document this pattern and accept that in-app navigation won't update components reading searchParams
Affected navigation/layout components that likely need context migration:
AppLayout.tsx,NavItem.tsx,ViewContext.tsx,BackButton.tsx,MessagesView.tsx,HeaderActionButtons.tsx, and brain sidebar/content components.
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
components/home/HomeFeed.tsx (1)
23-39: Consider optimizing the direct message lookup for better performance.The current implementation uses
.some()to check if a wave ID exists indirectMessages.liston every drop click, which is O(n) complexity. For better performance, consider creating a Set of direct message IDs usinguseMemo:Apply this diff to optimize the lookup:
const { directMessages } = useMyStream(); + + const directMessageIds = useMemo( + () => new Set(directMessages.list.map((dm) => dm.id)), + [directMessages.list] + ); const onDropContentClick = useCallback( (drop: ExtendedDrop) => { - const isDirectMessage = directMessages.list.some( - (dm) => dm.id === drop.wave.id - ); + const isDirectMessage = directMessageIds.has(drop.wave.id); const url = getWaveRoute({ waveId: drop.wave.id, serialNo: drop.serial_no, isDirectMessage, isApp, }); router.push(url, { scroll: false }); }, - [directMessages.list, isApp, router] + [directMessageIds, isApp, router] );This change provides O(1) lookup time and ensures the
useCallbackremains effective even ifdirectMessages.listis frequently updated.Don't forget to add
useMemoto the imports:-import React, { useState, useEffect, useCallback } from "react"; +import React, { useState, useEffect, useCallback, useMemo } from "react";__tests__/components/header/AppHeader.test.tsx (1)
69-73: Incomplete mock for MyStreamContext.The mock returns only
{ activeWave: { id: ... } }, but based on the context definition incontexts/wave/MyStreamContext.tsx(line 47),activeWaveshould also include asetfunction. While the current tests may not exercise thesetfunction, providing a complete mock improves test reliability and prevents potential errors if future tests need to verifysetbehavior.Apply this diff to add the missing
setfunction to the mock:(useMyStreamOptional as jest.Mock).mockReturnValue( opts.wave - ? { activeWave: { id: opts.wave.id } } - : { activeWave: { id: null } } + ? { activeWave: { id: opts.wave.id, set: jest.fn() } } + : { activeWave: { id: null, set: jest.fn() } } );contexts/wave/hooks/useActiveWaveManager.ts (2)
11-27: Consider using globalThis.window for consistency.The helper functions use
windowdirectly in multiple places. While this works, usingglobalThis.windowis the modern standard and provides better consistency across environments.Apply this diff to use
globalThis.window:const getWaveFromWindow = (): string | null => { - if (typeof window === "undefined") return null; - const url = new URL(window.location.href); + if (typeof globalThis.window === "undefined") return null; + const url = new URL(globalThis.window.location.href); const wave = url.searchParams.get("wave"); return typeof wave === "string" ? wave : null; }; const getRouteContext = (): { isOnWaves: boolean; isOnMessages: boolean } => { - if (typeof window === "undefined") return { isOnWaves: false, isOnMessages: false }; + if (typeof globalThis.window === "undefined") return { isOnWaves: false, isOnMessages: false }; - const pathname = window.location.pathname; + const pathname = globalThis.window.location.pathname; return { isOnWaves: pathname === "/waves" || pathname.startsWith("/waves/"), isOnMessages: pathname === "/messages" || pathname.startsWith("/messages/"), }; };Based on learnings: SonarCloud static analysis suggests this pattern for better cross-environment compatibility.
84-96: Consider simplifying the conditional structure.The nested if-else at lines 84-96 can be simplified by collapsing the else block into an else-if chain for better readability.
Apply this diff:
if (canUsePushState) { // Same route - fast pushState const method = options?.replace ? "replaceState" : "pushState"; - window.history[method](null, "", target); + globalThis.window.history[method](null, "", target); setActiveWaveId(waveId); - } else { - // Cross-route - proper navigation - if (options?.replace) { - router.replace(target); - } else { - router.push(target); - } + } else if (options?.replace) { + // Cross-route - proper navigation with replace + router.replace(target); + } else { + // Cross-route - proper navigation + router.push(target); }Based on learnings: SonarCloud flags the current structure as having an if statement as the only statement in an else block.
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (1)
105-106: Address or track the TODO comment.The TODO mentions a two-click issue on mobile related to
onMouseEnter. Since this affects user experience on mobile devices, it should either be investigated and resolved, or tracked in a GitHub issue for future work.Would you like me to:
- Open a GitHub issue to track this mobile interaction bug?
- Generate a shell script to investigate where similar patterns exist in the codebase that might have the same issue?
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx (2)
6-27: Context and device-info mocks are correct; consider stronger typing for mocksThe new
useMyStreamanduseDeviceInfomocks look consistent with the updated context‑driven navigation and non‑app environment assumptions. To keep TypeScript types intact instead of casting withas jest.Mock, consider usingjest.mockedorjest.MockedFunctionforusePrefetchWaveData/useMyStream, e.g.:const mockedUseMyStream = jest.mocked(useMyStream);This avoids losing the original hook’s type information while still letting you override implementations.
32-52: Reduce shared mutable state inuseMyStreammock and update click test nameThe pattern with module‑level
activeWaveIdplus multiplemockedUseMyStream.mockImplementationcalls works, but it makes tests a bit more stateful than necessary and slightly harder to scan. You could simplify by stubbing the hook per test with an explicit value instead of shared mutable state, e.g.:mockedUseMyStream.mockReturnValue({ activeWave: { id: '1', set: setActiveWave }, });or via a small helper like
mockActiveWave(id: string | null)called in each Arrange block.Also,
it('pushes shallow route on click', ...)now only asserts thatsetActiveWaveis called, not that a route is pushed. Renaming this to something likesets active wave on clickwould better describe the expected outcome and keep tests self‑documenting, in line with the test‑naming guidance. Based on learnings, descriptive names and localised Arrange steps improve readability.Also applies to: 63-66, 76-79, 84-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx(5 hunks)__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsx(1 hunks)__tests__/components/header/AppHeader.test.tsx(3 hunks)__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx(2 hunks)app/access/page.client.tsx(1 hunks)components/brain/BrainMobile.tsx(3 hunks)components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx(5 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts(3 hunks)components/home/HomeFeed.tsx(2 hunks)contexts/wave/MyStreamContext.tsx(1 hunks)contexts/wave/hooks/useActiveWaveManager.ts(1 hunks)helpers/navigation.helpers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Do not include any comments in the code; it should be self-explanatory
Write correct, up-to-date, bug-free, fully componentized, secure, and efficient code
Include all required imports and ensure proper naming of key components
Use NextJS features that match the current version
**/*.{ts,tsx,js,jsx}: Enforce ≥ 80% line coverage for files changed sincemainvianpm run test
All code must pass ESLint (Next's Core Web Vitals + React Hooks rules) vianpm run lint
Use<Link>from Next.js for internal navigation instead of<a>tags or HTML anchors
Use<Image>fromnext/imageinstead of HTML<img>elements
Files:
__tests__/components/header/AppHeader.test.tsxapp/access/page.client.tsxcontexts/wave/MyStreamContext.tsx__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsxcomponents/brain/BrainMobile.tsxcomponents/home/HomeFeed.tsx__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsxhelpers/navigation.helpers.tscontexts/wave/hooks/useActiveWaveManager.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{tsx,jsx}: Use FontAwesome for icons in React components
Use TailwindCSS for styling in React components
Use react-query for data fetching
Always addreadonlybefore props in React components
Files:
__tests__/components/header/AppHeader.test.tsxapp/access/page.client.tsxcontexts/wave/MyStreamContext.tsx__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsxcomponents/brain/BrainMobile.tsxcomponents/home/HomeFeed.tsx__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: All code must pass TypeScript type checking vianpm run type-check(tsc --noEmit)
Use'use cache'directive at the top of Server Components or functions to explicitly opt-in to caching in Next.js 16
Files:
__tests__/components/header/AppHeader.test.tsxapp/access/page.client.tsxcontexts/wave/MyStreamContext.tsx__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsxcomponents/brain/BrainMobile.tsxcomponents/home/HomeFeed.tsx__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsxhelpers/navigation.helpers.tscontexts/wave/hooks/useActiveWaveManager.ts
**/__tests__/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests should live in
__tests__/directory or be namedComponentName.test.tsx
Files:
__tests__/components/header/AppHeader.test.tsx__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsx__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
🧠 Learnings (13)
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/*.{ts,tsx,js,jsx} : Use the Arrange – Act – Assert pattern for test structure, keeping assertions focused on behaviour, not implementation details
Applied to files:
__tests__/components/header/AppHeader.test.tsx__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsx__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Mock external dependencies and APIs in tests
Applied to files:
__tests__/components/header/AppHeader.test.tsx
📚 Learning: 2025-11-25T08:37:44.679Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: app/api/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:44.679Z
Learning: Applies to app/api/**/*.{ts,tsx,js,jsx} : When needing custom headers or timeouts for external requests, pass them via `@/lib/security/urlGuard` helper options rather than re-implementing your own wrapper.
Applied to files:
app/access/page.client.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Use testing-library/react and testing-library/user-event for React component tests
Applied to files:
__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Use TypeScript and React functional components with hooks
Applied to files:
__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsxcomponents/home/HomeFeed.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
📚 Learning: 2025-11-25T08:35:58.721Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T08:35:58.721Z
Learning: Applies to **/*.{tsx,jsx} : Use react-query for data fetching
Applied to files:
__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsxcomponents/home/HomeFeed.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `<Link>` from Next.js for internal navigation instead of `<a>` tags or HTML anchors
Applied to files:
__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Remove unnecessary Effects; compute state during render or use `useMemo`. If listening to external events, use `useEffectEvent` to access latest props/state without re-running the Effect
Applied to files:
components/home/HomeFeed.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/*.{ts,tsx,js,jsx} : Give each test a clear, descriptive name that conveys the scenario and expected outcome
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/*.{ts,tsx,js,jsx} : Organize test files in `__tests__` directory mirroring source folder structure (components, contexts, hooks, utils) to keep structure familiar
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Applies to **/__tests__/**/*.{ts,tsx,js,jsx} : Tests should live in `__tests__/` directory or be named `ComponentName.test.tsx`
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/app/api/**/*.{ts,tsx,js,jsx} : Place integration tests for API routes under `app/api` in the test structure
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Fix issues with modernization aligned to React 19.2, React Compiler, and Next.js 16 conventions; do not add `// eslint-disable` comments unless explicitly instructed
Applied to files:
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
🧬 Code graph analysis (7)
__tests__/components/header/AppHeader.test.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
app/access/page.client.tsx (1)
constants.ts (1)
API_AUTH_COOKIE(38-38)
__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx (1)
contexts/wave/hooks/useActiveWaveManager.ts (1)
useActiveWaveManager(29-105)
components/brain/BrainMobile.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
components/home/HomeFeed.tsx (4)
types/dropInteractionTypes.ts (1)
ActiveDropState(8-12)contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)helpers/waves/drop.helpers.ts (1)
ExtendedDrop(16-20)helpers/navigation.helpers.ts (1)
getWaveRoute(19-68)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (3)
contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)helpers/navigation.helpers.ts (2)
getWaveHomeRoute(70-80)getWaveRoute(19-68)__mocks__/react-use.js (1)
React(2-2)
contexts/wave/hooks/useActiveWaveManager.ts (1)
hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)
🪛 GitHub Check: SonarCloud Code Analysis
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
[warning] 105-105: Complete the task associated to this "TODO" comment.
contexts/wave/hooks/useActiveWaveManager.ts
[warning] 65-65: Prefer globalThis.window over window.
[warning] 51-51: Prefer globalThis.window over window.
[warning] 73-73: Prefer globalThis over window.
[warning] 22-22: Prefer globalThis over window.
[warning] 87-87: Prefer globalThis over window.
[warning] 73-73: Prefer globalThis over window.
[warning] 13-13: Prefer globalThis over window.
[warning] 12-12: Prefer globalThis.window over window.
[warning] 91-91: 'If' statement should not be the only statement in 'else' block
[warning] 20-20: Prefer globalThis.window over window.
[warning] 56-56: Prefer globalThis over window.
[warning] 57-57: Prefer globalThis over window.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
components/home/HomeFeed.tsx (3)
3-3: LGTM!The addition of
useCallbackto the imports is appropriate for memoizing theonDropContentClickhandler.
14-14: LGTM!The import of
useMyStreamaligns with the PR's migration to context-based wave resolution.
21-21: LGTM!The
useMyStreamhook usage correctly accesses thedirectMessagesfor the new context-based navigation logic.app/access/page.client.tsx (1)
46-51: Protocol-basedsecureflag looks correct; please confirm this matches your deployment expectationsDeriving
isSecurefromglobalThis.location?.protocol === "https:"is technically sound and keeps the cookiesecureon HTTPS while allowing it to work over HTTP (e.g., local dev). It does, however, mean that in any non-HTTPS deployment the auth cookie will be transmitted without theSecureattribute. Please double‑check that you never serve this page over plain HTTP in real environments (staging/prod) and that this behavior is intentional; if you want to lock it down regardless of protocol in those environments, you may want to key this off an env/config flag instead oflocation.protocol.__tests__/components/brain/my-stream/layout/MyStreamNoItems.test.tsx (1)
28-39: LGTM - test correctly validates simplified routing.The test now expects "/waves" for both app and non-app modes, which aligns with the simplified routing logic in
helpers/navigation.helpers.tswheregetWavesBaseRoutenow returns "/waves" unconditionally.__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx (1)
21-53: LGTM - test properly validates history-based navigation.The test correctly migrates from router-based to history-based navigation, using spies on
history.pushStateandhistory.replaceStateto verify the new behavior. The use ofwaitForproperly handles async state updates, andbeforeEachensures test isolation.contexts/wave/MyStreamContext.tsx (1)
45-48: LGTM - well-designed API extension.The
setsignature extension to accept optionalisDirectMessageandreplaceflags maintains backward compatibility while enabling more sophisticated navigation control. This aligns well with the enhancedsetActiveWaveimplementation incontexts/wave/hooks/useActiveWaveManager.ts.components/brain/BrainMobile.tsx (1)
75-76: LGTM - excellent migration strategy with fallback.Deriving
waveIdfrommyStream?.activeWave.idwith a fallback tosearchParams?.get('wave')provides a smooth transition path. This approach supports client-side navigation via pushState (when context is available) while maintaining backward compatibility with URL-based navigation.helpers/navigation.helpers.ts (1)
13-15: LGTM - routing simplification removes unnecessary branching.Removing app-specific routing logic by having
getWavesBaseRouteandgetMessagesBaseRoutealways return "/waves" and "/messages" respectively simplifies the navigation helpers. The_isAppnaming convention clearly indicates the parameter is intentionally unused, maintaining API compatibility with existing callers.components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (1)
57-89: LGTM - context-based navigation correctly implemented.The refactoring successfully migrates from router-based to context-based navigation:
hrefcomputation (lines 59-64) correctly usesactiveWaveIdfrom contextonWaveHovercallback (lines 68-73) properly includes all dependencieshandleWaveClick(lines 77-89) includes complete dependencies:activeWave.set, activeWaveId, isDirectMessage, onWaveHover, wave.idThis addresses the past review comment about incomplete dependencies.
contexts/wave/hooks/useActiveWaveManager.ts (1)
29-104: Based on my verification, I've found important context about the synchronization mechanism. Let me now provide the rewritten review comment:
Verify if external
router.pushcalls with wave parameters properly synchronize with hook state.The hook manages state synchronization via
useEffecton searchParams changes (lines 43-47). While this catches most cases, I found multiple components that bypass the hook'ssetActiveWavemethod and callrouter.pushdirectly with wave parameters:
components/waves/leaderboard/content/WaveLeaderboardDropContent.tsx:22-router.push(/waves?wave=${drop.wave.id}&serialNo=...)components/waves/create-wave/CreateWave.tsx:100-router.push(/waves?wave=${response.id})Synchronization mechanism: After external
router.pushcalls, Next.js updatessearchParams, which triggers the hook'suseEffectto syncactiveWaveId. However, there's a timing window before the effect runs where dependent features (useEnhancedWavesList, useWaveRealtimeUpdater) may read stale state.Recommendation: Consider centralizing wave navigation through
setActiveWaveor adding a callback mechanism to ensureactiveWaveIdupdates atomically with URL changes, eliminating the synchronization lag for downstream consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contexts/wave/hooks/useActiveWaveManager.ts (1)
46-50: Potential infinite loop risk in useEffect.The
activeWaveIddependency at Line 47 creates a risk: ifwaveFromSearchParamschanges to match the currentactiveWaveId, the effect won't run. But if they differ,setActiveWaveIdupdatesactiveWaveId, which is in the dependency array. While this should stabilize after one update, it's fragile.Consider removing
activeWaveIdfrom the dependencies since you're explicitly checking for inequality:useEffect(() => { if (waveFromSearchParams !== activeWaveId) { setActiveWaveId(waveFromSearchParams); } - }, [waveFromSearchParams, activeWaveId]); + }, [waveFromSearchParams]);This prevents the effect from re-running after it updates
activeWaveId.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/brain/direct-messages/DirectMessagesList.tsx(1 hunks)components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx(5 hunks)components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx(4 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts(2 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx(4 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/CollapsedWave.tsx(3 hunks)components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/ExpandedWave.tsx(3 hunks)contexts/wave/hooks/useActiveWaveManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/CollapsedWave.tsx
- components/brain/left-sidebar/web/WebBrainLeftSidebarWave/subcomponents/ExpandedWave.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Do not include any comments in the code; it should be self-explanatory
Write correct, up-to-date, bug-free, fully componentized, secure, and efficient code
Include all required imports and ensure proper naming of key components
Use NextJS features that match the current version
**/*.{ts,tsx,js,jsx}: Enforce ≥ 80% line coverage for files changed sincemainvianpm run test
All code must pass ESLint (Next's Core Web Vitals + React Hooks rules) vianpm run lint
Use<Link>from Next.js for internal navigation instead of<a>tags or HTML anchors
Use<Image>fromnext/imageinstead of HTML<img>elements
Files:
components/brain/direct-messages/DirectMessagesList.tsxcomponents/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsxcontexts/wave/hooks/useActiveWaveManager.tscomponents/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.tscomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsxcomponents/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{tsx,jsx}: Use FontAwesome for icons in React components
Use TailwindCSS for styling in React components
Use react-query for data fetching
Always addreadonlybefore props in React components
Files:
components/brain/direct-messages/DirectMessagesList.tsxcomponents/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsxcomponents/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: All code must pass TypeScript type checking vianpm run type-check(tsc --noEmit)
Use'use cache'directive at the top of Server Components or functions to explicitly opt-in to caching in Next.js 16
Files:
components/brain/direct-messages/DirectMessagesList.tsxcomponents/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsxcontexts/wave/hooks/useActiveWaveManager.tscomponents/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.tscomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsxcomponents/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Use TypeScript and React functional components with hooks
Applied to files:
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsxcomponents/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.tscomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `<Link>` from Next.js for internal navigation instead of `<a>` tags or HTML anchors
Applied to files:
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.tscomponents/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Fix issues with modernization aligned to React 19.2, React Compiler, and Next.js 16 conventions; do not add `// eslint-disable` comments unless explicitly instructed
Applied to files:
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
📚 Learning: 2025-11-25T08:35:58.721Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T08:35:58.721Z
Learning: Applies to **/*.{tsx,jsx} : Use react-query for data fetching
Applied to files:
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx
🧬 Code graph analysis (4)
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx (3)
contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)hooks/usePrefetchWaveData.ts (1)
usePrefetchWaveData(5-21)hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)
contexts/wave/hooks/useActiveWaveManager.ts (1)
hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)
components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts (1)
__mocks__/react-use.js (1)
React(2-2)
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (4)
contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)hooks/usePrefetchWaveData.ts (1)
usePrefetchWaveData(5-21)hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)helpers/navigation.helpers.ts (2)
getWaveHomeRoute(70-80)getWaveRoute(19-68)
🪛 GitHub Check: SonarCloud Code Analysis
contexts/wave/hooks/useActiveWaveManager.ts
[warning] 21-21: Compare with undefined directly instead of using typeof.
[warning] 54-54: Compare with undefined directly instead of using typeof.
[warning] 71-71: Compare with undefined directly instead of using typeof.
[warning] 12-12: Compare with undefined directly instead of using typeof.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
components/brain/left-sidebar/waves/UnifiedWavesListWaves.tsx (1)
83-84: LGTM! Clean prop threading.The
isDirectMessageprop is properly added with appropriate typing, default value, and forwarded to child components. This enables direct-message-specific navigation behavior downstream.Also applies to: 103-103, 167-167, 225-225
components/brain/direct-messages/DirectMessagesList.tsx (1)
140-140: LGTM! Proper integration.The
isDirectMessageprop is correctly passed to enable direct-message-specific routing behavior.contexts/wave/hooks/useActiveWaveManager.ts (2)
11-17: Server-side safety checks are correct; ignore SonarCloud warnings.The
typeof globalThis.window === "undefined"checks at Lines 12, 21, 54, and 71 are appropriate for Next.js SSR/SSG contexts. While SonarCloud suggests comparing withundefineddirectly, thetypeofguard is the standard pattern to prevent ReferenceErrors in environments wherewindowis not defined.Based on learnings: Next.js requires these guards for client-side-only code accessing browser APIs.
Also applies to: 20-30, 53-64, 71-74
87-102: History API usage is safe and properly synchronized—the review concerns don't apply to Next.js App Router.The codebase uses Next.js App Router (via
useRouterfromnext/navigation), which does not exposerouter.events. The review comment assumes Pages Router behavior, which is not applicable here.Verified implementation details:
- History API (
pushState/replaceState) is correctly limited to same-route navigation only (lines 92–96)- Cross-route navigation properly uses
router.push/router.replace(lines 98–102)- State synchronization is handled via:
- popstate listener (lines 53–64): Syncs state on back/forward button clicks
- searchParams effect (lines 46–50): Keeps
activeWaveIdin sync after any router navigation- This pattern is consistently used elsewhere in the codebase (
NextGenCollection.tsx,useHomeTabs.ts) without issues- Route detection via
getRouteContext()correctly distinguishes between/wavesand/messagesroutesNo middleware or route guards would be affected since the App Router doesn't use event listeners for navigation. The implementation is sound.
components/brain/left-sidebar/waves/BrainLeftSidebarWave.tsx (2)
77-89: Excellent click handler implementation.The
handleWaveClickproperly:
- Checks for already-prevented events (Line 79)
- Preserves new-tab/window behavior with modifier keys (Lines 80-82)
- Prevents default navigation (Line 83)
- Triggers hover/prefetch before navigation (Line 84)
- Toggles active state correctly (Line 85)
- Uses context-driven navigation (Line 86)
- Has complete dependencies (Line 88)
105-106: Good touch screen optimization.Conditionally disabling
onMouseEnterfor touch devices (Line 105) and always providingonClick(Line 106) ensures proper behavior across device types while avoiding unnecessary hover effects on touch screens.components/brain/left-sidebar/web/WebBrainLeftSidebarWave/hooks/useWaveNavigation.ts (2)
59-78: Well-implemented onClick handler.The handler correctly:
- Respects already-prevented events (Line 61)
- Preserves browser new-tab/window behavior for modifier keys and middle/right mouse buttons (Lines 62-70)
- Delegates to
onMouseEnterfor prefetch/hover logic (Line 73)- Toggles active wave state (Line 74)
- Passes
isDirectMessagecontext (Line 75)- Has complete dependencies (Line 77)
83-84: Good API design for device-specific behavior.Making
onMouseEnterconditional on!hasTouchScreen(Line 83) while always exposingonClick(Line 84) provides optimal UX across devices.components/brain/left-sidebar/web/WebBrainLeftSidebarWave/index.tsx (2)
33-33: Clean context integration.The component properly integrates with
useMyStreamto accessactiveWaveand passes the necessary context values (activeWave.id,activeWave.set,hasTouchScreen) touseWaveNavigation. The returnedonClickis correctly forwarded to child components.Also applies to: 38-47
79-79: Proper onClick forwarding.The
onClickhandler is correctly passed to bothCollapsedWave(Line 79) andExpandedWave(Line 98), ensuring consistent navigation behavior across display modes.Also applies to: 98-98
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contexts/wave/hooks/useActiveWaveManager.ts (1)
50-62: Redundant SSR guard.The check at line 52 already guards against undefined
globalThis.window. The destructuring and subsequent null check at lines 54-55 is redundant sinceglobalThis.windowis guaranteed to be defined after line 52.useEffect(() => { if (globalThis.window === undefined) return; - const { window: browserWindow } = globalThis; - if (!browserWindow) return; - const onPopState = () => { setActiveWaveId(getWaveFromWindow()); }; - browserWindow.addEventListener("popstate", onPopState); - return () => browserWindow.removeEventListener("popstate", onPopState); + window.addEventListener("popstate", onPopState); + return () => window.removeEventListener("popstate", onPopState); }, []);__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx (2)
63-66: Reduce duplication in per-test MyStream mockingIn both “does not prefetch when hovering active wave” and “computes href based on current wave”, you reassign
activeWaveIdand then re‑callmockedUseMyStream.mockImplementation(...), even though the implementation already closes overactiveWaveIdfrombeforeEach. You can simplify by only updatingactiveWaveIdin each test and relying on the existing mock:it('does not prefetch when hovering active wave', async () => { - activeWaveId = '1'; - mockedUseMyStream.mockImplementation(() => ({ - activeWave: { id: activeWaveId, set: setActiveWave }, - })); + activeWaveId = '1'; @@ it('computes href based on current wave', () => { @@ - activeWaveId = '1'; - mockedUseMyStream.mockImplementation(() => ({ - activeWave: { id: activeWaveId, set: setActiveWave }, - })); + activeWaveId = '1';This keeps the intent the same while avoiding repeated setup logic.
Also applies to: 76-79
84-89: Rename click test to describe the new context-based behaviorThe test name “pushes shallow route on click” no longer matches the assertion, which now verifies
setActiveWaveis called. Renaming the test will better document intent and avoid confusion:- it('pushes shallow route on click', async () => { + it('sets the active wave in MyStream on click', async () => { @@ - expect(setActiveWave).toHaveBeenCalledWith('1', { isDirectMessage: false }); + expect(setActiveWave).toHaveBeenCalledWith('1', { isDirectMessage: false });This aligns with the guideline to give each test a clear, descriptive name that reflects the expected outcome. Based on learnings, this improves test readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx(5 hunks)__tests__/contexts/wave/hooks/useActiveWaveManager.test.tsx(2 hunks)contexts/wave/hooks/useActiveWaveManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/contexts/wave/hooks/useActiveWaveManager.test.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Do not include any comments in the code; it should be self-explanatory
Write correct, up-to-date, bug-free, fully componentized, secure, and efficient code
Include all required imports and ensure proper naming of key components
Use NextJS features that match the current version
**/*.{ts,tsx,js,jsx}: Enforce ≥ 80% line coverage for files changed sincemainvianpm run test
All code must pass ESLint (Next's Core Web Vitals + React Hooks rules) vianpm run lint
Use<Link>from Next.js for internal navigation instead of<a>tags or HTML anchors
Use<Image>fromnext/imageinstead of HTML<img>elements
Files:
contexts/wave/hooks/useActiveWaveManager.ts__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: All code must pass TypeScript type checking vianpm run type-check(tsc --noEmit)
Use'use cache'directive at the top of Server Components or functions to explicitly opt-in to caching in Next.js 16
Files:
contexts/wave/hooks/useActiveWaveManager.ts__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{tsx,jsx}: Use FontAwesome for icons in React components
Use TailwindCSS for styling in React components
Use react-query for data fetching
Always addreadonlybefore props in React components
Files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
**/__tests__/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests should live in
__tests__/directory or be namedComponentName.test.tsx
Files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/*.{ts,tsx,js,jsx} : Use the Arrange – Act – Assert pattern for test structure, keeping assertions focused on behaviour, not implementation details
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/*.{ts,tsx,js,jsx} : Give each test a clear, descriptive name that conveys the scenario and expected outcome
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/*.{ts,tsx,js,jsx} : Organize test files in `__tests__` directory mirroring source folder structure (components, contexts, hooks, utils) to keep structure familiar
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Applies to **/__tests__/**/*.{ts,tsx,js,jsx} : Tests should live in `__tests__/` directory or be named `ComponentName.test.tsx`
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Applies to __tests__/**/__tests__/**/app/api/**/*.{ts,tsx,js,jsx} : Place integration tests for API routes under `app/api` in the test structure
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:14.939Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:14.939Z
Learning: Fix issues with modernization aligned to React 19.2, React Compiler, and Next.js 16 conventions; do not add `// eslint-disable` comments unless explicitly instructed
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
📚 Learning: 2025-11-25T08:37:36.715Z
Learnt from: CR
Repo: 6529-Collections/6529seize-frontend PR: 0
File: __tests__/AGENTS.md:0-0
Timestamp: 2025-11-25T08:37:36.715Z
Learning: Use testing-library/react and testing-library/user-event for React component tests
Applied to files:
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx
🧬 Code graph analysis (2)
contexts/wave/hooks/useActiveWaveManager.ts (2)
hooks/useDeviceInfo.ts (1)
useDeviceInfo(23-93)helpers/navigation.helpers.ts (2)
getWaveRoute(19-68)getWaveHomeRoute(70-80)
__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx (2)
hooks/usePrefetchWaveData.ts (1)
usePrefetchWaveData(5-21)contexts/wave/MyStreamContext.tsx (1)
useMyStream(243-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
contexts/wave/hooks/useActiveWaveManager.ts (4)
11-17: LGTM!The SSR-safe window access pattern using
globalThis.windowis correct, and the URL parsing logic properly extracts the wave parameter.
20-30: LGTM!The route context detection correctly handles both exact matches and nested routes for
/wavesand/messagespaths.
37-48: LGTM!The state initialization and synchronization with
searchParamsis correctly implemented. The effect properly handles updates from router-based navigation.
64-103: LGTM on the navigation strategy.The conditional use of
pushStatefor same-route navigation (waves→waves or messages→messages) while falling back torouter.push/router.replacefor cross-route navigation is a good performance optimization. The early return when already on the target URL prevents unnecessary state updates.__tests__/components/brain/left-sidebar/waves/BrainLeftSidebarWave.test.tsx (1)
6-6: Mocks for MyStream and device info cleanly align tests with the new navigation modelThe way you mock
useMyStream(withactiveWave+setwired tosetActiveWave) anduseDeviceInfo(forcingisApp: false) keeps these tests focused on the left-sidebar wave behavior rather than router or environment details. The sharedbeforeEachthat resetsactiveWaveIdand the mock implementation provides a clear default state for all scenarios. No changes needed here.Also applies to: 14-20, 27-27, 32-33, 48-51
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/navigation/BackButton.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Do not include any comments in the code; it should be self-explanatory
Write correct, up-to-date, bug-free, fully componentized, secure, and efficient code
Include all required imports and ensure proper naming of key components
Use NextJS features that match the current version
**/*.{ts,tsx,js,jsx}: Enforce ≥ 80% line coverage for files changed sincemainvianpm run test
All code must pass ESLint (Next's Core Web Vitals + React Hooks rules) vianpm run lint
Use<Link>from Next.js for internal navigation instead of<a>tags or HTML anchors
Use<Image>fromnext/imageinstead of HTML<img>elements
Files:
components/navigation/BackButton.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{tsx,jsx}: Use FontAwesome for icons in React components
Use TailwindCSS for styling in React components
Use react-query for data fetching
Always addreadonlybefore props in React components
Files:
components/navigation/BackButton.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: All code must pass TypeScript type checking vianpm run type-check(tsc --noEmit)
Use'use cache'directive at the top of Server Components or functions to explicitly opt-in to caching in Next.js 16
Files:
components/navigation/BackButton.tsx
🧬 Code graph analysis (1)
components/navigation/BackButton.tsx (1)
contexts/wave/MyStreamContext.tsx (1)
useMyStreamOptional(251-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
|
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.