-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: create social network navigation alike on browse threads #168
feat: create social network navigation alike on browse threads #168
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes weave a tapestry of improved navigation, enhanced accordion functionality, and refined URL handling for a more seamless user experience and optimized system performance. Key updates include asynchronous sign-out capability, refined accordion behavior with URL synchronization, and enhanced slug generation for cleaner and more readable URLs. Changes
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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: 5
apps/masterbots.ai/components/shared/thread-double-accordion.tsx
Outdated
Show resolved
Hide resolved
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.
@TopETH merge the latest changes, notice that we now lib/url
and lib/username
for slug and url encoding
…ke-on-browse-threads
Done. |
apps/masterbots.ai/components/shared/thread-double-accordion.tsx
Outdated
Show resolved
Hide resolved
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: 3
import { useEffect, useRef } from 'react' | ||
import { useThread } from '@/hooks/use-thread' | ||
import { ThreadHeading } from './thread-heading' | ||
import { ThreadAccordion } from './thread-accordion' | ||
import { toSlug } from '@/lib/url' | ||
let initialUrl = null |
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.
Ensure that the newly added imports (useEffect
, useRef
, useThread
) are utilized effectively in the component. The declaration of initialUrl
as null
is a good practice for initialization, but consider encapsulating this within a useRef
for consistency and to avoid potential bugs in future modifications.
- let initialUrl = null
+ const initialUrl = useRef(null)
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.
import { useEffect, useRef } from 'react' | |
import { useThread } from '@/hooks/use-thread' | |
import { ThreadHeading } from './thread-heading' | |
import { ThreadAccordion } from './thread-accordion' | |
import { toSlug } from '@/lib/url' | |
let initialUrl = null | |
import { useEffect, useRef } from 'react' | |
import { useThread } from '@/hooks/use-thread' | |
import { ThreadHeading } from './thread-heading' | |
import { ThreadAccordion } from './thread-accordion' | |
import { toSlug } from '@/lib/url' | |
const initialUrl = useRef(null) |
const { activeThread, setActiveThread } = useThread() | ||
const [state, setState] = useSetState({ | ||
isOpen: false, | ||
firstQuestion: | ||
thread.messages.find(m => m.role === 'user').content || 'not found', | ||
firstResponse: | ||
thread.messages.find(m => m.role === 'assistant').content || 'not found' | ||
thread.messages.find(m => m.role === 'assistant')?.content || 'not found', | ||
value: [] | ||
}) | ||
|
||
useEffect(() => { | ||
if (initialUrl) return | ||
initialUrl = location.href |
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.
The state initialization correctly captures the initial state of the accordion based on the thread data. However, the first useEffect
block that sets initialUrl
could be optimized by checking for initialUrl.current
instead of initialUrl
if changed to use useRef
.
- if (initialUrl) return
- initialUrl = location.href
+ if (initialUrl.current) return
+ initialUrl.current = location.href
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.
const { activeThread, setActiveThread } = useThread() | |
const [state, setState] = useSetState({ | |
isOpen: false, | |
firstQuestion: | |
thread.messages.find(m => m.role === 'user').content || 'not found', | |
firstResponse: | |
thread.messages.find(m => m.role === 'assistant').content || 'not found' | |
thread.messages.find(m => m.role === 'assistant')?.content || 'not found', | |
value: [] | |
}) | |
useEffect(() => { | |
if (initialUrl) return | |
initialUrl = location.href | |
const { activeThread, setActiveThread } = useThread() | |
const [state, setState] = useSetState({ | |
isOpen: false, | |
firstQuestion: | |
thread.messages.find(m => m.role === 'user').content || 'not found', | |
firstResponse: | |
thread.messages.find(m => m.role === 'assistant')?.content || 'not found', | |
value: [] | |
}) | |
useEffect(() => { | |
if (initialUrl.current) return | |
initialUrl.current = location.href |
useEffect(() => { | ||
if (activeThread !== thread || state.isOpen) return | ||
setState({ isOpen: true, value: [`pair-${thread.threadId}`] }) | ||
}, []) |
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.
The third useEffect
hook is intended to open the accordion when the active thread matches the current thread and it is not already open. However, the dependency array is empty, which means this effect will only run once at component mount. Consider adding activeThread
and state.isOpen
to the dependencies to ensure it reacts to changes.
- }, [])
+ }, [activeThread, state.isOpen])
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.
useEffect(() => { | |
if (activeThread !== thread || state.isOpen) return | |
setState({ isOpen: true, value: [`pair-${thread.threadId}`] }) | |
}, []) | |
useEffect(() => { | |
if (activeThread !== thread || state.isOpen) return | |
setState({ isOpen: true, value: [`pair-${thread.threadId}`] }) | |
}, [activeThread, state.isOpen]) |
ThreadAccordion is used in many different places, make sure it works correctly on all of the , updating the url needs to happen both we you open ThreadDialog on chat and when you expand DoubleThreadDialog in browse, you also need to make the chat landing and browse landing pages work. that 4 different places |
Summary by CodeRabbit
New Features
Bug Fixes