-
Notifications
You must be signed in to change notification settings - Fork 339
feat: pin llamaAI chats #2426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: pin llamaAI chats #2426
Conversation
📝 WalkthroughWalkthroughAdds localStorage-backed pinned sessions with an event-driven API; session items gain a pin toggle and subscribe to pinned state, and the chat history sidebar separates pinned sessions (sorted by lastActivity) from unpinned sessions (grouped by date), updating reactively on pin changes. Changes
Sequence DiagramsequenceDiagram
participant User
participant SessionItem
participant PinnedSvc as pinnedSessions
participant Storage as BrowserStorage
participant Sidebar as ChatHistorySidebar
User->>SessionItem: Click pin icon
SessionItem->>PinnedSvc: togglePinSession(sessionId)
PinnedSvc->>Storage: read current pinned list
PinnedSvc->>Storage: write updated pinned list
PinnedSvc->>PinnedSvc: dispatch "pinnedSessionsChange"
Sidebar->>PinnedSvc: notified via subscription
Sidebar->>PinnedSvc: read current pinned list
Sidebar->>Sidebar: recompute groups & ordering
Sidebar->>User: render updated sidebar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/containers/LlamaAI/components/SessionItem.tsx (3)
🔇 Additional comments (4)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/containers/LlamaAI/utils/pinnedSessions.ts:
- Around line 34-38: subscribeToPinnedSessions currently accesses window
directly which breaks during SSR; guard the call by checking typeof window !==
'undefined' (or globalThis) and if absent return a no-op subscribe/unsubscribe
pair. Specifically, in subscribeToPinnedSessions add an early branch that
returns a function that does nothing when window is undefined so
useSyncExternalStore can safely call it during server rendering, otherwise
attach window.addEventListener('pinnedSessionsChange', callback) and return the
real remover window.removeEventListener('pinnedSessionsChange', callback).
- Around line 20-32: togglePinSession directly uses window.localStorage and
window.dispatchEvent which will crash in SSR; wrap the side-effect portion in a
guard like typeof window !== 'undefined' (or return early when window is
undefined) so that reading via getStoredPinnedSessions still works server-side
but storage/dispatch only run in the browser; update togglePinSession to compute
nextPinned as before but only call
window.localStorage.setItem(PINNED_SESSIONS_KEY, ...) and
window.dispatchEvent(...) inside that guard and keep the same boolean return
behavior.
🧹 Nitpick comments (4)
src/containers/LlamaAI/components/ChatHistorySidebar.tsx (2)
1-2: Consolidate React imports.
useSyncExternalStorecan be imported alongside the other React hooks on line 1 instead of a separate import.Proposed fix
-import { useEffect, useMemo, useRef } from 'react' -import { useSyncExternalStore } from 'react' +import { useEffect, useMemo, useRef, useSyncExternalStore } from 'react'
48-53: Use the exported constant instead of hardcoded string.The localStorage key
'llamaai-pinned-sessions'is hardcoded here, butPINNED_SESSIONS_KEYis already exported from the utils module. Using the constant ensures consistency if the key ever changes.Proposed fix
import { isSessionPinned, subscribeToPinnedSessions } from '../utils/pinnedSessions' +import { PINNED_SESSIONS_KEY } from '../utils/pinnedSessions'Then update the getSnapshot:
const _pinnedSessions = useSyncExternalStore( subscribeToPinnedSessions, - () => localStorage.getItem('llamaai-pinned-sessions') ?? '[]', + () => localStorage.getItem(PINNED_SESSIONS_KEY) ?? '[]', () => '[]' )src/containers/LlamaAI/components/SessionItem.tsx (2)
1-5: Consolidate React imports.Same as in ChatHistorySidebar -
useSyncExternalStorecan be combined with the other React imports on line 1.Proposed fix
-import React, { useEffect, useRef, useState } from 'react' +import React, { useEffect, useRef, useState, useSyncExternalStore } from 'react' import { useRouter } from 'next/router' import * as Ariakit from '@ariakit/react' import { useMutation, useQueryClient } from '@tanstack/react-query' -import { useSyncExternalStore } from 'react'
28-33: Use the exported constant for the localStorage key.The hardcoded key
'llamaai-pinned-sessions'should be replaced withPINNED_SESSIONS_KEYfor consistency with the utils module.Proposed fix
-import { isSessionPinned, togglePinSession, subscribeToPinnedSessions } from '../utils/pinnedSessions' +import { isSessionPinned, togglePinSession, subscribeToPinnedSessions, PINNED_SESSIONS_KEY } from '../utils/pinnedSessions'Then update the getSnapshot:
useSyncExternalStore( subscribeToPinnedSessions, - () => localStorage.getItem('llamaai-pinned-sessions') ?? '[]', + () => localStorage.getItem(PINNED_SESSIONS_KEY) ?? '[]', () => '[]' )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/containers/LlamaAI/components/ChatHistorySidebar.tsxsrc/containers/LlamaAI/components/SessionItem.tsxsrc/containers/LlamaAI/utils/pinnedSessions.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/containers/LlamaAI/components/SessionItem.tsx (3)
src/containers/LlamaAI/utils/pinnedSessions.ts (3)
subscribeToPinnedSessions(34-39)isSessionPinned(15-18)togglePinSession(20-32)src/components/Tooltip.tsx (1)
Tooltip(30-64)src/components/Icon.tsx (1)
Icon(108-114)
src/containers/LlamaAI/components/ChatHistorySidebar.tsx (1)
src/containers/LlamaAI/utils/pinnedSessions.ts (2)
subscribeToPinnedSessions(34-39)isSessionPinned(15-18)
🔇 Additional comments (5)
src/containers/LlamaAI/utils/pinnedSessions.ts (1)
3-13: Well-designed localStorage accessor with proper error handling.Good defensive coding with SSR check, try/catch for JSON parse errors, and array validation.
src/containers/LlamaAI/components/ChatHistorySidebar.tsx (1)
55-92: Grouping logic is well-structured.The separation of pinned/unpinned sessions with proper sorting and the reactive dependency on
_pinnedSessionsis correctly implemented.src/containers/LlamaAI/components/SessionItem.tsx (3)
37-40: Pin toggle handler is correctly implemented.Properly stops event propagation to prevent triggering the parent session click, and delegates to the utility function.
173-191: Pin button UI implementation looks good.The tooltip appropriately changes based on pin state, disabled states cover all pending operations, and the visual feedback (rotation + fill) provides clear indication of the pinned state.
184-190: The--icon-fillCSS variable is properly supported and working correctly.The pin icon in the SVG sprite at
public/icons/v34.svgexplicitly references this variable withfill="var(--icon-fill)". The Icon component passes all style props directly to the SVG element, which cascades through the<use>element to the symbol. The code in SessionItem.tsx will work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/containers/LlamaAI/components/SessionItem.tsx:
- Around line 27-32: Replace the hardcoded localStorage key in the
useSyncExternalStore selector with the exported constant PINNED_SESSIONS_KEY
from pinnedSessions.ts: import PINNED_SESSIONS_KEY at the top of
SessionItem.tsx, then change the selector function passed to
useSyncExternalStore (the one calling
localStorage.getItem('llamaai-pinned-sessions') ?? '[]') to use
localStorage.getItem(PINNED_SESSIONS_KEY) ?? '[]' so the component reads the key
from the single source of truth.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/containers/LlamaAI/components/ChatHistorySidebar.tsxsrc/containers/LlamaAI/components/SessionItem.tsxsrc/containers/LlamaAI/utils/pinnedSessions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/containers/LlamaAI/components/ChatHistorySidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/containers/LlamaAI/components/SessionItem.tsx (3)
src/containers/LlamaAI/utils/pinnedSessions.ts (3)
subscribeToPinnedSessions(34-43)isSessionPinned(15-18)togglePinSession(20-32)src/components/Tooltip.tsx (1)
Tooltip(30-64)src/components/Icon.tsx (1)
Icon(108-114)
🔇 Additional comments (10)
src/containers/LlamaAI/utils/pinnedSessions.ts (5)
1-1: LGTM!Clear and descriptive constant name for the localStorage key.
3-13: LGTM!The
getStoredPinnedSessionshelper is well-implemented with proper SSR safety (typeof window === 'undefined'check), defensive JSON parsing wrapped in try-catch, and array type validation to handle corrupted localStorage data.
15-18: LGTM!Simple and correct implementation. The
includes()check is appropriate for a reasonably-sized list of pinned sessions.
20-32: LGTM!The toggle logic correctly handles add/remove, persists to localStorage, dispatches a custom event for subscribers, and returns the new pinned state. SSR safety is properly handled.
34-43: LGTM!Clean subscription pattern that returns an unsubscribe function. The SSR safety check correctly returns a no-op cleanup function when
windowis undefined.src/containers/LlamaAI/components/SessionItem.tsx (5)
1-1: LGTM!Appropriate import of
useSyncExternalStorefor subscribing to external state.
12-12: LGTM!Correct imports from the new pinned sessions utility module.
34-39: LGTM!The
isPinnedderivation andhandleTogglePinhandler are correctly implemented. Usinge.stopPropagation()prevents the click from bubbling to the parent session click handler.
168-168: LGTM!The
pr-20padding on hover accommodates the new pin button alongside existing action buttons.
172-187: LGTM!Well-implemented pin button UI:
- Consistent styling with existing action buttons (edit, menu)
- Proper disabled states during other operations
- Good accessibility with tooltip indicating action
- Visual feedback via rotation and fill for pinned state
What
Why
Screenshots
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.