Add counter-clockwise spinning plant icon during token generation #357
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughImplements streaming state management via React Context API. Adds Changes
Sequence DiagramsequenceDiagram
participant App as App Root
participant SP as StreamingProvider
participant Ctx as Context
participant Header as Header Component
participant Message as Message Component
App->>SP: Render with children
SP->>Ctx: Initialize isStreaming = false
SP->>Header: Provide context
SP->>Message: Provide context
Message->>Ctx: useStreaming()
Ctx-->>Message: { isStreaming, setIsStreaming }
Message->>Message: useEffect: pending changes
Message->>Ctx: setIsStreaming(true)
Ctx->>Header: Context updated
Header->>Ctx: useStreaming()
Ctx-->>Header: { isStreaming: true }
Header->>Header: Apply animate-spin-ccw
Note over Message,Header: During streaming lifecycle
Message->>Ctx: setIsStreaming(false)
Header->>Header: Remove animation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)app/layout.tsx (2)
🔇 Additional comments (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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨No code suggestions found for the PR. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
HeadercallsuseStreaming()butStreamingProvideronly wrapsChat, likely causing a runtime crash whenHeaderrenders outside the provider.- Global streaming state is toggled directly by each
BotMessage, which can be incorrect with concurrent streams or on unmount; a ref-counted context is safer. - Minor performance/jank risk: the logo’s
transition-transformcan conflict with the keyframe animation. - The Tailwind
spin-ccwkeyframe/animation looks correct and integrates cleanly with the header logo class toggle.
Additional notes (1)
- Performance |
components/header.tsx:38-41
Thetransition-transform duration-1000on the logo can conflict with and/or add jank when toggling theanimate-spin-ccwanimation. Since the rotation is handled by keyframes, the transition isn’t needed.
Summary of changes
- Introduced a new client
StreamingProvider/useStreamingcontext to track token generation state. - Wrapped the
Chatcomponent’s mobile and desktop layouts withStreamingProvider. - Updated
BotMessageto set the streaming state viauseEffectbased onpendingfromuseStreamableValue. - Modified
Headerto consumeisStreamingand apply aanimate-spin-ccwTailwind animation to the logo while streaming. - Extended Tailwind config with a custom
spin-ccwkeyframe andanimateentry for a 2s linear infinite counter-clockwise rotation.
|
|
||
| export const Header = () => { | ||
| const { toggleCalendar } = useCalendarToggle() | ||
| const { isStreaming } = useStreaming() |
There was a problem hiding this comment.
useStreaming() throws if used outside of StreamingProvider, but StreamingProvider is only mounted under Chat. If Header is rendered at a higher level (e.g., in app/layout.tsx), this will crash at runtime. The provider must wrap Header (or the whole app tree), not just the Chat subtree.
Suggestion
Move StreamingProvider to a top-level provider that wraps Header and page content (e.g., via an app/providers.tsx client component used in app/layout.tsx), and remove the per-Chat wrappers to avoid duplication. For example:
- app/providers.tsx (client):
'use client'
import { StreamingProvider } from '@/components/streaming-context'
export function Providers({ children }: { children: React.ReactNode }) {
return <StreamingProvider>{children}</StreamingProvider>
}- app/layout.tsx (server):
import { Providers } from './providers'
import { Header } from '@/components/header'
export default function RootLayout({ children }: { children: React.ReactNode }) {
return (
<html lang="en">
<body>
<Providers>
<Header />
{children}
</Providers>
</body>
</html>
)
}Then delete the StreamingProvider wrappers added in components/chat.tsx.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
| const { setIsStreaming } = useStreaming() | ||
|
|
||
| useEffect(() => { | ||
| setIsStreaming(pending) | ||
| }, [pending, setIsStreaming]) |
There was a problem hiding this comment.
Using a single boolean setIsStreaming(pending) in each BotMessage can oscillate the global state incorrectly if multiple messages stream concurrently or if a component unmounts mid-stream. This can leave the header stuck spinning or stop it too early.
Suggestion
Refactor the streaming context to track a reference count and expose start()/stop() functions, then call those from BotMessage on pending transitions with an unmount cleanup.
- streaming-context.tsx:
export interface StreamingContextType {
isStreaming: boolean
start: () => void
stop: () => void
}
export const StreamingProvider = ({ children }: { children: React.ReactNode }) => {
const [count, setCount] = useState(0)
const start = () => setCount(c => c + 1)
const stop = () => setCount(c => Math.max(0, c - 1))
return (
<StreamingContext.Provider value={{ isStreaming: count > 0, start, stop }}>
{children}
</StreamingContext.Provider>
)
}- components/message.tsx:
const { start, stop } = useStreaming()
useEffect(() => {
if (pending) start()
else stop()
return () => { if (pending) stop() }
}, [pending, start, stop])Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this safer ref-count approach.
components/chat.tsx
Outdated
| <StreamingProvider> | ||
| <MapDataProvider> {/* Add Provider */} | ||
| <HeaderSearchButton /> |
There was a problem hiding this comment.
Placing StreamingProvider inside Chat means Header (which uses useStreaming) is very likely outside the provider, leading to runtime errors and/or state desynchronization. After lifting the provider to app/layout.tsx (or another common root), these nested providers should be removed to prevent multiple isolated contexts that won’t share state.
Suggestion
Remove the StreamingProvider wrappers from both the mobile and desktop branches of Chat once the provider is placed at the app root. For example:
// Mobile layout
return (
<MapDataProvider>
<HeaderSearchButton />
{/* ...rest unchanged... */}
</MapDataProvider>
)
// Desktop layout
return (
<MapDataProvider>
<HeaderSearchButton />
{/* ...rest unchanged... */}
</MapDataProvider>
)Reply with "@CharlieHelps yes please" if you'd like me to add a commit that removes these wrappers.
| interface StreamingContextType { | ||
| isStreaming: boolean | ||
| setIsStreaming: (streaming: boolean) => void | ||
| } | ||
|
|
||
| const StreamingContext = createContext<StreamingContextType | undefined>(undefined) | ||
|
|
||
| export const useStreaming = () => { | ||
| const context = useContext(StreamingContext) | ||
| if (!context) { | ||
| throw new Error('useStreaming must be used within a StreamingProvider') | ||
| } | ||
| return context | ||
| } | ||
|
|
||
| export const StreamingProvider = ({ children }: { children: ReactNode }) => { | ||
| const [isStreaming, setIsStreaming] = useState(false) | ||
|
|
||
| return ( | ||
| <StreamingContext.Provider value={{ isStreaming, setIsStreaming }}> | ||
| {children} | ||
| </StreamingContext.Provider> | ||
| ) |
There was a problem hiding this comment.
Evolve the streaming context to be ref-count based so it’s resilient to concurrent streams and component unmounts. Right now it only stores a boolean with setIsStreaming, which is not robust across multiple producers. Introducing activeCount with beginStream/endStream avoids flicker and stuck states.
Suggestion
Replace the boolean-only API with a counter-based context. Example:
'use client'
import { createContext, useContext, useState, ReactNode, useMemo, useCallback } from 'react'
interface StreamingContextType {
isStreaming: boolean
beginStream: () => void
endStream: () => void
}
const StreamingContext = createContext<StreamingContextType | undefined>(undefined)
export const useStreaming = () => {
const ctx = useContext(StreamingContext)
if (!ctx) throw new Error('useStreaming must be used within a StreamingProvider')
return ctx
}
export const StreamingProvider = ({ children }: { children: ReactNode }) => {
const [activeCount, setActiveCount] = useState(0)
const beginStream = useCallback(() => setActiveCount(c => c + 1), [])
const endStream = useCallback(() => setActiveCount(c => Math.max(0, c - 1)), [])
const value = useMemo(
() => ({ isStreaming: activeCount > 0, beginStream, endStream }),
[activeCount, beginStream, endStream]
)
return <StreamingContext.Provider value={value}>{children}</StreamingContext.Provider>
}Pair this with the BotMessage hook change to call beginStream/endStream based on pending transitions. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with these changes.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/chat.tsx (1)
88-116: Remove the innerMapDataProviderfrom chat.tsx—it creates a state disconnect between Chat's monitoring logic and Mapbox updates.Chat calls
useMapData()at line 78 to consume from the outer provider (injected by page.tsx). Its useEffect (lines 80–85) monitorsmapData.drawnFeaturesfrom that outer context. However, the code then renders an innerMapDataProviderat lines 87 and 100, wrapping Mapbox inside it. When Mapbox callsuseMapData(), it retrieves the inner provider's isolated state instead of the outer one. This means Mapbox'ssetMapData()calls update only the inner provider, leaving Chat's outer-context useEffect unaware of map interactions.Fix: Delete the
<MapDataProvider>wrapper at lines 87 and 100 (and corresponding closing tags). Mapbox will then use the outer provider from page.tsx, allowing Chat's useEffect to properly observe drawing changes.
♻️ Duplicate comments (1)
components/chat.tsx (1)
122-154: Same nested MapDataProvider issue in desktop layout.This has the identical state disconnect problem as the mobile layout (lines 88-116). The inner
MapDataProvider(line 123) isolates state from the outer provider that Chat reads from.Consider removing the inner
MapDataProviderfrom both layouts and relying on the outer provider from page.tsx, or consolidating to a single provider location.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/chat.tsx(4 hunks)components/header.tsx(3 hunks)components/message.tsx(1 hunks)components/streaming-context.tsx(1 hunks)tailwind.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
components/chat.tsx (4)
components/streaming-context.tsx (1)
StreamingProvider(20-28)components/map/map-data-context.tsx (1)
MapDataProvider(26-34)components/header-search-button.tsx (1)
HeaderSearchButton(19-108)app/page.tsx (1)
Page(9-18)
tailwind.config.ts (1)
components/ui/spinner.tsx (1)
svg(6-19)
components/message.tsx (1)
components/streaming-context.tsx (1)
useStreaming(12-18)
components/header.tsx (2)
components/streaming-context.tsx (1)
useStreaming(12-18)lib/utils/index.ts (1)
cn(11-13)
🔇 Additional comments (7)
tailwind.config.ts (2)
71-74: LGTM: Counter-clockwise keyframes are correct.The keyframes correctly define a counter-clockwise rotation using
-360deg.
79-79: LGTM: Animation configuration is appropriate.The 2-second linear infinite animation provides a smooth streaming indicator.
components/streaming-context.tsx (2)
12-18: LGTM: Standard context hook pattern.The error boundary correctly prevents usage outside the provider.
20-28: LGTM: Clean provider implementation.The provider correctly initializes streaming state and exposes it to children.
components/header.tsx (2)
5-5: LGTM: Correct streaming context integration.The header appropriately reads streaming state without modifying it.
Also applies to: 22-22
33-42: LGTM: Smooth animation implementation.The
transition-transform duration-1000provides a smooth deceleration when streaming stops, which is a nice UX touch.components/chat.tsx (1)
19-19: LGTM: Correct import for streaming provider.
| useEffect(() => { | ||
| setIsStreaming(pending) | ||
| }, [pending, setIsStreaming]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Approve with suggestion: Add explanatory comment.
The implementation correctly tracks streaming state by relying on stable setIsStreaming identity. Only the active streaming message's pending changes will trigger updates.
Consider adding a comment explaining the behavior:
+ // Update global streaming state when this message's streaming status changes.
+ // Only the newest message will have pending=true, older messages remain pending=false.
useEffect(() => {
setIsStreaming(pending)
}, [pending, setIsStreaming])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| setIsStreaming(pending) | |
| }, [pending, setIsStreaming]) | |
| // Update global streaming state when this message's streaming status changes. | |
| // Only the newest message will have pending=true, older messages remain pending=false. | |
| useEffect(() => { | |
| setIsStreaming(pending) | |
| }, [pending, setIsStreaming]) |
🤖 Prompt for AI Agents
In components/message.tsx around lines 17 to 19, add a brief explanatory comment
above the useEffect explaining that the effect intentionally depends on the
stable setIsStreaming setter and that only changes to the active streaming
message's pending prop should update the isStreaming state (e.g., "Depend on
stable setIsStreaming; only the active message's pending toggles will trigger
updates"). Keep the comment concise and clarify intent so future readers know
this dependency is deliberate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/streaming-context.tsx (2)
5-8: Concurrent streams will cause animation flicker and incorrect state.The boolean-based
isStreamingstate cannot track multiple simultaneous streams. If twoBotMessagecomponents stream tokens concurrently and one finishes first, callingsetIsStreaming(false)will stop the logo animation even though the second stream is still active. The previous review correctly identified this critical flaw and proposed a ref-count solution (activeCountwithbeginStream/endStream).Apply the ref-counting approach suggested in the previous review to handle concurrent streams correctly.
12-19: Inconsistent error handling: should throw when used outside provider.The codebase pattern for context hooks is to throw an error when used outside a provider (see
useMapLoading,useMapToggle, anduseProfileTogglein the relevant snippets). Returning silent defaults can hide bugs where a component is incorrectly placed outside theStreamingProvider. The SSR comment is also misleading since the'use client'directive ensures this only runs client-side.Apply this diff to align with the established pattern:
export const useStreaming = () => { const context = useContext(StreamingContext) - if (!context) { - // Return default values if used outside provider (e.g., during SSR) - return { isStreaming: false, setIsStreaming: () => {} } - } + if (!context) { + throw new Error('useStreaming must be used within a StreamingProvider') + } return context }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/streaming-context.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/streaming-context.tsx (4)
components/map-loading-context.tsx (2)
children(11-18)context(20-26)components/map-toggle-context.tsx (1)
context(36-42)components/map/map-context.tsx (1)
children(14-22)components/profile-toggle-context.tsx (2)
ProfileToggleProviderProps(20-22)context(38-44)
| export const StreamingProvider = ({ children }: { children: ReactNode }) => { | ||
| const [isStreaming, setIsStreaming] = useState(false) | ||
|
|
||
| return ( | ||
| <StreamingContext.Provider value={{ isStreaming, setIsStreaming }}> | ||
| {children} | ||
| </StreamingContext.Provider> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Memoize the context value to prevent unnecessary re-renders.
The context value object is recreated on every render, causing all consumers (Header, BotMessage) to re-render even when isStreaming hasn't changed. While setIsStreaming from useState is stable, the wrapper object is not.
Apply this diff to memoize the value:
-import { createContext, useContext, useState, ReactNode } from 'react'
+import { createContext, useContext, useState, ReactNode, useMemo } from 'react'
export const StreamingProvider = ({ children }: { children: ReactNode }) => {
const [isStreaming, setIsStreaming] = useState(false)
+ const value = useMemo(
+ () => ({ isStreaming, setIsStreaming }),
+ [isStreaming]
+ )
+
return (
- <StreamingContext.Provider value={{ isStreaming, setIsStreaming }}>
+ <StreamingContext.Provider value={value}>
{children}
</StreamingContext.Provider>
)
}🤖 Prompt for AI Agents
In components/streaming-context.tsx around lines 21 to 29, the context value
object is recreated every render causing unnecessary consumer re-renders; fix by
memoizing the provider value: import and use React.useMemo to create the value
object ({ isStreaming, setIsStreaming }) and only recompute when isStreaming or
setIsStreaming change (depend on isStreaming is sufficient since setIsStreaming
is stable), then pass that memoized value to StreamingContext.Provider.
|
its not spinning |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 808 🏁 Script executed: Length of output: 2576 🏁 Script executed: Length of output: 1704 🏁 Script executed: Length of output: 5136 🏁 Script executed: Length of output: 2543 🏁 Script executed: Length of output: 14 🏁 Script executed: Length of output: 2562 🏁 Script executed: Length of output: 16 @ngoiyaeric I found the issue! The problem is that the Header component cannot access the streaming state because it's rendered outside the The Problem
The SolutionMove the Update import type { Metadata, Viewport } from 'next'
import { Inter as FontSans } from 'next/font/google'
import './globals.css'
import 'katex/dist/katex.min.css';
import { cn } from '@/lib/utils'
import { ThemeProvider } from '@/components/theme-provider'
import Header from '@/components/header'
import Footer from '@/components/footer'
import { Sidebar } from '@/components/sidebar'
import { Analytics } from "@vercel/analytics/next"
import { SpeedInsights } from "@vercel/speed-insights/next"
import { Toaster } from '@/components/ui/sonner'
import { MapToggleProvider } from '@/components/map-toggle-context'
import { ProfileToggleProvider } from '@/components/profile-toggle-context'
import { CalendarToggleProvider } from '@/components/calendar-toggle-context'
import { MapLoadingProvider } from '@/components/map-loading-context';
import ConditionalLottie from '@/components/conditional-lottie';
import { MapProvider } from '@/components/map/map-context'
import { StreamingProvider } from '@/components/streaming-context' // Add this import
const fontSans = FontSans({
subsets: ['latin'],
variable: '--font-sans'
})
const title = 'Beta'
const description =
'language to Maps'
export const metadata: Metadata = {
metadataBase: new URL('https://www.qcx.world'),
title,
description,
openGraph: {
title,
description
},
twitter: {
title,
description,
card: 'summary_large_image',
creator: '@queueLab'
}
}
export const viewport: Viewport = {
width: 'device-width',
initialScale: 1,
minimumScale: 1,
maximumScale: 1
}
export default function RootLayout({
children
}: Readonly<{
children: React.ReactNode
}>) {
return (
<html lang="en" suppressHydrationWarning>
<body className={cn('font-sans antialiased', fontSans.variable)}>
<StreamingProvider> {/* Add StreamingProvider here */}
<CalendarToggleProvider>
<MapToggleProvider>
<ProfileToggleProvider>
<ThemeProvider
attribute="class"
defaultTheme="earth"
enableSystem
disableTransitionOnChange
themes={['light', 'dark', 'earth']}
>
<MapProvider>
<MapLoadingProvider>
<Header />
<ConditionalLottie />
{children}
<Sidebar />
<Footer />
<Toaster />
</MapLoadingProvider>
</MapProvider>
</ThemeProvider>
</ProfileToggleProvider>
</MapToggleProvider>
</CalendarToggleProvider>
</StreamingProvider> {/* Close StreamingProvider here */}
<Analytics />
<SpeedInsights />
</body>
</html>
)
}Then remove the StreamingProvider from // In chat.tsx - Remove StreamingProvider wrapper from both mobile and desktop layouts
// Remove this import:
// import { StreamingProvider } from './streaming-context'
// Mobile layout - remove StreamingProvider wrapper:
if (isMobile) {
return (
<MapDataProvider>
<HeaderSearchButton />
{/* rest of mobile layout */}
</MapDataProvider>
);
}
// Desktop layout - remove StreamingProvider wrapper:
return (
<MapDataProvider>
<HeaderSearchButton />
{/* rest of desktop layout */}
</MapDataProvider>
);After these changes, the Header component will be able to access the streaming state, and the logo should spin counter-clockwise while tokens are being generated! 🎉 |
User description
This PR implements a feature where the plant icon (logo.svg) in the header spins counter-clockwise whenever tokens are being generated in the system.
Changes Made:
StreamingContextto track token generation state across the applicationBotMessagecomponent to set streaming state based on thependingflag fromuseStreamableValueHeadercomponent to apply a counter-clockwise spinning animation to the logo when streaming is activeanimate-spin-ccwthat rotates the icon -360 degrees over 2 secondsChatcomponent withStreamingProviderto provide streaming contextTechnical Details:
PR Type
Enhancement
Description
Add counter-clockwise spinning animation to plant icon during token generation
Create
StreamingContextto track token generation state globallyUpdate
BotMessagecomponent to sync streaming state with contextApply
animate-spin-ccwanimation to logo in header when streamingDefine custom Tailwind animation rotating -360 degrees over 2 seconds
Diagram Walkthrough
File Walkthrough
streaming-context.tsx
Create streaming context for token generation trackingcomponents/streaming-context.tsx
useStreaminghook for accessing streaming stateStreamingProvidercomponent to wrap applicationisStreamingboolean andsetIsStreamingsetter functionmessage.tsx
Sync BotMessage pending state to streaming contextcomponents/message.tsx
useStreaminghook anduseEffectfrom Reactpendingstate fromuseStreamableValuehookpendingstate to global streaming context viasetIsStreamingpendingchangesheader.tsx
Apply spinning animation to logo when streamingcomponents/header.tsx
useStreaminghook to access streaming stateisStreamingvalue from streaming contextanimate-spin-ccwclass to logo image conditionallychat.tsx
Wrap Chat component with StreamingProvidercomponents/chat.tsx
StreamingProvidercomponentStreamingProviderStreamingProvideras outermost wrapper aroundMapDataProvidertailwind.config.ts
Define counter-clockwise spin animation in Tailwindtailwind.config.ts
spin-ccwkeyframe animation rotating from 0deg to -360degspin-ccwanimation class with 2s duration and linear timingSummary by CodeRabbit
Release Notes