Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErikBjare
Copy link
Member

@ErikBjare ErikBjare commented May 6, 2025

TODO

  • Re-use ChatInput component for input
  • Better examples (don't we already have some in ChatInput?)
  • Better styling
  • Make it actually possible to create a new conversation (broken)

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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 Ellipsis 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, {
Copy link
Contributor

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.

Copy link
Member Author

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()]);
Copy link
Contributor

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.

Suggested change
}, [selectedConversation$.get()]);
}, [selectedConversation$]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant