Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
📝 WalkthroughWalkthroughNavigation labels and routes were changed (introducing "Discover", renaming "Stream"→"Home"), home-tab state and storage logic were removed, navigation active-state logic simplified to path-based checks, helpers consolidated to Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/components/navigation/ViewContext.test.tsx (1)
65-81: Test data does not match the test description.This test is labeled "handles route navigation" suggesting it tests generic route behavior, but the
NavItemusesname: "Home"which triggers the specialHomehandling inhandleNavClick. Thehref: "/home"is never used because the Home-specific branch always callsrouter.push(getHomeLatestRoute())which returns"/".Consider either:
- Rename this test to clarify it's testing Home navigation (but then it duplicates the test below), or
- Change the item name to something other than "Home" (e.g., "Network") to test generic route navigation.
Proposed fix to test generic route navigation
it("handles route navigation", () => { render( <ViewProvider> <TestNavComponent item={ { kind: "route", - name: "Home", - href: "/home", - icon: "h", + name: "Network", + href: "/network", + icon: "n", } as NavItem } /> </ViewProvider> ); - expect(push).toHaveBeenCalledWith("/"); + expect(push).toHaveBeenCalledWith("/network"); });
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contexts/TitleContext.tsx (2)
93-111: Route-change reset never runs due to duplicated routeRef update.Because the earlier effect (Line 93-100) sets
routeRef.currentbefore this effect executes, the guard at Line 103 short-circuits and the reset (setWaveData(null)/setStreamHasNewItems(false)) never fires on route changes. This defeats the intended cleanup.Consider consolidating into a single effect so the reset reliably executes on route changes.
🔧 Proposed consolidation
- useEffect(() => { - if (routeRef.current === pathname) { - return; - } - routeRef.current = pathname; - const defaultTitle = getDefaultTitleForRoute(pathname); - setTitle(defaultTitle); - }, [pathname]); - - useEffect(() => { - if (routeRef.current !== pathname) { - routeRef.current = pathname; - queryRef.current = searchParams; - const defaultTitle = getDefaultTitleForRoute(pathname); - setTitle(defaultTitle); - setWaveData(null); - setStreamHasNewItems(false); - } - }, [pathname, searchParams]); + useEffect(() => { + if (routeRef.current === pathname) { + return; + } + routeRef.current = pathname; + queryRef.current = searchParams; + const defaultTitle = getDefaultTitleForRoute(pathname); + setTitle(defaultTitle); + setWaveData(null); + setStreamHasNewItems(false); + }, [pathname, searchParams]);
132-155:isHomeFeedTabis undefined after tab-state removal.This symbol is no longer declared in the file, which will fail type-check/build. Replace it with a route-based flag (e.g., home pathname) or remove the branch if no longer needed.
✅ Suggested fix (route-based home flag)
const isWaveRoute = pathname?.startsWith("/waves") || pathname?.startsWith("/messages") || (pathname === "/" && searchParams?.get("view") === "waves"); + const isHomeRoute = pathname === "/"; const computedTitle = useMemo(() => { const waveTitle = (() => { if (!waveParam || !waveData) return null; let newItemsText = ""; if (waveData.newItemsCount > 0 && notificationCount === 0) { const messageText = waveData.newItemsCount === 1 ? "message" : "messages"; newItemsText = `(${waveData.newItemsCount} new ${messageText}) `; } return `${newItemsText}${waveData.name} | Brain`; })(); - if (isHomeFeedTab) { + if (isHomeRoute) { if (waveTitle) return waveTitle; const prefix = streamHasNewItems && notificationCount === 0 ? "(New messages) My Stream" : "My Stream"; return `${prefix} | Brain`; } @@ }, [ - isHomeFeedTab, + isHomeRoute, isWaveRoute, waveParam, waveData, streamHasNewItems, title, notificationCount, ]);
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contexts/TitleContext.tsx (1)
93-111: Duplicate useEffect causes wave state reset to never execute.The first effect (lines 93-100) updates
routeRef.currentwhen pathname changes. Since effects run in declaration order, by the time the second effect (lines 102-111) executes,routeRef.currentalready equalspathname, making the conditionrouteRef.current !== pathnamealways false on pathname change.Result:
setWaveData(null)andsetStreamHasNewItems(false)on lines 108-109 are dead code—they never run when navigating between routes.🐛 Proposed fix: Consolidate into a single effect
- useEffect(() => { - if (routeRef.current === pathname) { - return; - } - routeRef.current = pathname; - const defaultTitle = getDefaultTitleForRoute(pathname); - setTitle(defaultTitle); - }, [pathname]); - - useEffect(() => { - if (routeRef.current !== pathname) { - routeRef.current = pathname; - queryRef.current = searchParams; - const defaultTitle = getDefaultTitleForRoute(pathname); - setTitle(defaultTitle); - setWaveData(null); - setStreamHasNewItems(false); - } - }, [pathname, searchParams]); + useEffect(() => { + if (routeRef.current === pathname) { + return; + } + routeRef.current = pathname; + queryRef.current = searchParams; + const defaultTitle = getDefaultTitleForRoute(pathname); + setTitle(defaultTitle); + setWaveData(null); + setStreamHasNewItems(false); + }, [pathname, searchParams]);
Signed-off-by: prxt6529 <prxt@6529.io>
|
2 participants
Summary by CodeRabbit
New Features
Updates
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.