Skip to content

Conversation

@ErikBjare
Copy link
Member

@ErikBjare ErikBjare commented May 6, 2025

  • Separated demo and API conversation handling
  • Initialize demo conversations in store on component mount
  • Fixed dependency issues in useEffect hooks
  • Improved error handling for API connection failures
  • Added better logging for conversation initialization

This fixes the issue where demo conversations wouldn't show unless connected to an API server. (which makes chat.gptme.org look pretty bad rn)

This should improve further with #56


Important

Demo conversations now initialize without API connection, with improved error handling and logging in Conversations.tsx.

  • Behavior:
    • Demo conversations are initialized in Conversations.tsx on component mount, independent of API connection.
    • API conversations are fetched and initialized separately, with improved error handling.
  • Functions:
    • Adds initConversation to initialize demo conversations in conversations.ts.
    • Refactors initializeConversations to handle demo and API conversations separately in conversations.ts.
  • Misc:
    • Fixes dependency issues in useEffect hooks in Conversations.tsx.
    • Adds logging for conversation initialization in Conversations.tsx.

This description was created by Ellipsis for 0b087aa. You can customize this summary. It will automatically update as commits are pushed.

- Separated demo and API conversation handling
- Initialize demo conversations in store on component mount
- Fixed dependency issues in useEffect hooks
- Improved error handling for API connection failures
- Added better logging for conversation initialization

This fixes the issue where demo conversations wouldn't show unless
connected to an API server.
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.

Important

Looks good to me! 👍

Reviewed everything up to 7104869 in 2 minutes and 12 seconds. Click for details.
  • Reviewed 170 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 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:55
  • Draft comment:
    Good job adding navigate and route to the dependency array. Ensure that these values are stable or memoized to avoid unnecessary re-renders.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. src/components/Conversations.tsx:107
  • Draft comment:
    Consider adding error handling or awaiting the promise from initializeConversations, so API initialization failures can be managed or logged.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. src/components/Conversations.tsx:148
  • Draft comment:
    Both useObserveEffect and useEffect update 'conversation$'. Consider consolidating these effects to reduce potential redundant updates.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. src/stores/conversations.ts:91
  • Draft comment:
    Comment change only; ensure documentation stays updated with implementation. No functional change detected.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. src/components/Conversations.tsx:37
  • Draft comment:
    Initializing demo conversations on mount is now explicit. Ensure that duplicating demo initialization here and again in the store’s initializeConversations (which also checks for demo convs) doesn't cause unintended redundancy.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
6. src/components/Conversations.tsx:65
  • Draft comment:
    The API query function no longer checks for isConnected inside the function (it now relies solely on the 'enabled' flag). Confirm that this meets your error‐logging and handling expectations when disconnected.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
7. src/components/Conversations.tsx:117
  • Draft comment:
    Combining demoItems and apiItems via useMemo is a clear separation. Verify that the order (demo items first) is intentional and meets UX expectations.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
8. src/stores/conversations.ts:89
  • Draft comment:
    The comment update (from 'Bulk initialize conversations' to 'Initialize conversations') improves clarity without altering functionality. Ensure that documentation reflects the intended behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to ensure that documentation reflects the intended behavior after a comment update. This falls under the rule of not asking the PR author to ensure behavior is intended or to double-check things. It doesn't provide a specific code suggestion or ask for a specific test to be written.

Workflow ID: wflow_aZH6EMd5bKfihixz

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@ErikBjare ErikBjare requested a review from Miyou May 6, 2025 17:52
Copy link
Collaborator

@Miyou Miyou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, minor comments/questions

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.

Important

Looks good to me! 👍

Reviewed 0b087aa in 1 minute and 48 seconds. Click for details.
  • Reviewed 26 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 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:47
  • Draft comment:
    Removed auto-selection for demo conversation. Confirm if not setting a default is intentional.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment violates the rule about not asking for confirmation of intentions. The code change is clear - it removes auto-selection of demo conversations. The author deliberately removed this code, so asking if it's intentional adds no value. Additionally, if this causes any issues, they would be caught during testing. Maybe removing the default selection could cause UX issues if no conversation is selected. Maybe this is a legitimate concern about regression. Even if there are UX implications, asking "is this intentional?" is not helpful. The author clearly made this change deliberately. If there are specific UX issues, they should be raised directly with concrete examples. Delete this comment as it violates the rule about asking for confirmation of intentions and doesn't provide actionable feedback.
2. src/components/Conversations.tsx:51
  • Draft comment:
    Removed 'navigate' and 'route' dependencies from useEffect. Ensure these values are stable to avoid stale closures.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. src/components/Conversations.tsx:148
  • Draft comment:
    ESLint rule disabled for this effect's dependencies. Consider explicitly listing dependencies if possible for clearer intent.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is about a real issue - the effect depends on both allConversations and selectedConversation$.get() but only lists allConversations. However, there may be a reason for this - adding selectedConversation$ as a dependency could cause unnecessary re-renders or infinite loops since it's being read and potentially affects state that could trigger another render. The eslint-disable comment suggests this was an intentional choice. I could be wrong about the potential issues with adding selectedConversation$ as a dependency. Maybe there's a better way to structure this that would allow listing all dependencies. While there might be better ways to structure this, the current code appears to be intentionally avoiding certain dependencies, likely for performance reasons. The comment doesn't provide concrete guidance on how to improve it. The comment identifies a real issue but doesn't provide actionable guidance, and the current structure may be intentional to avoid performance issues.
4. src/components/Conversations.tsx:48
  • Draft comment:
    Removed fallback demo conversation auto-selection. Confirm that not defaulting to a demo when no conversationParam is provided is intentional.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
5. src/components/Conversations.tsx:149
  • Draft comment:
    ESLint disable for exhaustive deps is added here. Verify that omitting dependencies (e.g. selectedConversation$ reference) won’t cause missed updates.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to verify that omitting dependencies won't cause missed updates. This falls under asking the author to ensure behavior is intended, which is against the rules. The comment does not provide a specific suggestion or ask for a specific test to be written.

Workflow ID: wflow_rtwzgkI31fDx3Rns

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@ErikBjare ErikBjare merged commit 5b86355 into master May 7, 2025
1 check passed
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.

3 participants