-
Notifications
You must be signed in to change notification settings - Fork 2.2k
enhancement: show assistant info on message #5064
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
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.
LGTM
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.
Important
Looks good to me! 👍
Reviewed everything up to 7c6bda1 in 1 minute and 37 seconds. Click for details.
- Reviewed
196
lines of code in6
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. web-app/src/containers/StreamingContent.tsx:24
- Draft comment:
Verify that the 'showAssistant' logic (using the last message's role) handles consecutive assistant messages as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify the behavior of theshowAssistant
logic, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
2. web-app/src/containers/ThreadContent.tsx:235
- Draft comment:
Hardcoded defaults for assistant avatar ('👋') and name ('Jan') are used; consider making these configurable. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. web-app/src/hooks/useAppState.ts:35
- Draft comment:
Directly accessing assistant state via useAssistant.getState() may lead to stale values; consider subscribing to changes for dynamic updates. - 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% This is a legitimate concern in React/Zustand applications. When using getState() directly, you bypass the reactive subscription system. However, in this specific case, the assistant value is only captured once when updating the streaming content - it's not being used in a render context where staleness would be an issue. The suggested alternative of subscribing would actually be more complex and potentially unnecessary here. I might be underestimating the impact of stale assistant state. There could be race conditions or edge cases where the assistant changes right as content is being updated. While race conditions are theoretically possible, the practical impact is minimal since this is a one-time capture of the assistant state when creating/updating a message. If the assistant changes after, it's reasonable for the message to retain the original assistant context. The comment raises a theoretically valid concern but the current implementation is actually appropriate for this use case. The comment should be removed as it would lead to unnecessary complexity without clear benefit.
4. web-app/src/hooks/useMessages.ts:35
- Draft comment:
Ensure that the assistant metadata added in new messages via useAssistant.getState() remains in sync if the assistant state updates. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the author to ensure that a specific behavior is maintained, which violates the rule against asking the author to ensure behavior. It does not provide a specific suggestion or ask for a test to be written.
5. web-app/src/routes/threads/$threadId.tsx:199
- Draft comment:
Confirm that the 'showAssistant' prop logic (checking previous message role) is consistent across components and covers all edge cases. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. web-app/src/utils/formatDate.ts:1
- Draft comment:
Consider parameterizing the locale in formatDate to support internationalization if needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_KpwnaKLalRNFvDvA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
This pull request introduces enhancements to the
StreamingContent
andThreadContent
components to improve the display of assistant-related metadata and formatting, along with updates to state management hooks to include assistant information. Additionally, a utility function for date formatting has been added. Below are the most important changes grouped by theme:Enhancements to
StreamingContent
andThreadContent
Components:StreamingContent
to useuseMessages
for fetching messages and added logic to conditionally display assistant metadata in theThreadContent
component. (web-app/src/containers/StreamingContent.tsx
, [1] [2]ThreadContent
to accept a newshowAssistant
prop and render assistant details (avatar, name, and formatted creation date) when applicable. (web-app/src/containers/ThreadContent.tsx
, [1] [2] [3]State Management Updates:
useAppState
to include assistant metadata when updatingstreamingContent
. (web-app/src/hooks/useAppState.ts
, [1] [2]useMessages
to include assistant metadata in newly created messages. (web-app/src/hooks/useMessages.ts
, [1] [2]Utility Function Addition:
formatDate
utility function to format dates in a user-friendly manner, used in the assistant metadata display. (web-app/src/utils/formatDate.ts
, web-app/src/utils/formatDate.tsR1-R10)Miscellaneous:
ThreadDetail
to pass theshowAssistant
prop toThreadContent
based on message roles and sequence. (web-app/src/routes/threads/$threadId.tsx
, web-app/src/routes/threads/$threadId.tsxR199-R203)Fixes Issues
Self Checklist
Important
Enhances message components to display assistant metadata, updates state management, and adds a date formatting utility.
StreamingContent
: UsesuseMessages
to fetch messages and conditionally displays assistant metadata inThreadContent
.ThreadContent
: AcceptsshowAssistant
prop to render assistant details (avatar, name, formatted date).useAppState
: Includes assistant metadata inupdateStreamingContent()
.useMessages
: Adds assistant metadata to new messages inaddMessage()
.formatDate
function for user-friendly date formatting.ThreadDetail
: PassesshowAssistant
prop toThreadContent
based on message roles and sequence.This description was created by
for 7c6bda1. You can customize this summary. It will automatically update as commits are pushed.