-
Notifications
You must be signed in to change notification settings - Fork 12
feat: added WelcomeView for cleaner conversation starts #56
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: master
Are you sure you want to change the base?
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to d4a6413 in 2 minutes and 33 seconds. Click for details.
- Reviewed
245
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/components/Conversations.tsx:143
- Draft comment:
Avoid using a function call (selectedConversation$.get()) inside the dependency array. This may cause the effect to run on every render. Instead, consider storing the value in a variable and using that in the dependency array. - Reason this comment was not posted:
Marked as duplicate.
2. src/components/Conversations.tsx:121
- Draft comment:
There are two separate effects updating conversation$ – one using useObserveEffect on selectedConversation$ and another useEffect on allConversations. Consider consolidating them to avoid potential redundant updates. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/components/Conversations.tsx:121
- Draft comment:
There are two effects updating conversation$ (one via useObserveEffect on selectedConversation$ and one via useEffect on allConversations). Consider consolidating these updates to avoid redundant calls or potential race conditions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/components/LeftSidebar.tsx:42
- Draft comment:
The handleNewConversation function now only clears the conversation parameter and toggles the sidebar. Verify that removing the API call for creating a new conversation is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. src/components/WelcomeView.tsx:28
- Draft comment:
Generating a conversationId using a timestamp string may risk collisions under high-frequency use. Consider using a UUID generator for robust uniqueness. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While UUIDs would provide better theoretical uniqueness guarantees, the current approach using ISO timestamps with millisecond precision is likely sufficient for this use case. The chance of collisions would require two users to create conversations in the exact same millisecond. This seems to be a chat interface where sub-millisecond creation of conversations by a single user is prevented by the UI and network latency anyway. The comment raises a valid theoretical concern about uniqueness. In a high-scale distributed system, timestamp collisions could potentially occur. Given this is a client-side React component for a chat interface, the timestamp approach provides sufficient practical uniqueness. The UI prevents rapid submissions and network latency adds further spacing between requests. The comment should be removed as it suggests a change that adds complexity without providing meaningful benefit for this specific use case.
6. src/components/WelcomeView.tsx:33
- Draft comment:
The subscribeToEvents call registers empty callbacks for several events. If these events are not handled, consider removing the subscription or adding comments to clarify why these callbacks are intentionally left empty. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Empty callbacks are often a valid pattern when you need to provide a complete event handler interface but don't need all events. The code navigates away immediately after subscribing (line 56), suggesting this is just initial setup. The error handler is implemented which is the critical path. The other events being empty doesn't indicate a problem. I could be wrong about the purpose of these events - maybe some of them are critical for the chat functionality to work properly. Without seeing the API implementation, I can't be 100% certain these callbacks can safely be empty. While I can't be completely certain, the code structure suggests this is just initial conversation setup before navigation. If the empty callbacks were problematic, it would likely cause immediate issues that would have been caught during development. The comment should be deleted as empty callbacks are a common and valid pattern when implementing interfaces where you don't need all events. This appears to be working as intended.
Workflow ID: wflow_OInX9MnLAWUBQvHQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
await api.createConversation(conversationId, [{ role: 'user', content: input }]); | ||
|
||
// Subscribe to events for the new conversation | ||
api.subscribeToEvents(conversationId, { |
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.
Consider cleaning up the subscription to events (via api.subscribeToEvents
) when the component unmounts to avoid potential memory leaks.
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.
Yeah we should re-think this. It should be easy to do this from WelcomeView, we need a simpler API we can call and then navigate to the newly created conversation.
if (!selectedConversation$.get()) { | ||
setLeftSidebarOpen(false); | ||
} | ||
}, [selectedConversation$.get()]); |
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.
Avoid using .get()
directly in the dependency array. This may not trigger the effect reliably when the observable changes. Use a subscription hook (e.g. use$
) or include the observable itself in the dependency.
}, [selectedConversation$.get()]); | |
}, [selectedConversation$]); |
TODO