Thread options#135
Conversation
tropicrainforest
left a comment
There was a problem hiding this comment.
While reviewing this PR, I noticed three related issues. Only the first one is directly related to the current PR; the other two are observations/suggestions for follow-up.
-
When the user is viewing a thread and deletes that same thread, the UI currently stays on a page that no longer exists. Ideally, the user should be redirected to a safe location (for example, the “new chat” page as OpenAI is doing the same) instead of remaining on the deleted thread. The proposed solution and implementation details are explained in the review comments.
-
When opening the History side panel, the main chatbit page content shifts slightly to the left. This appears to be caused by scroll locking on the body. Enabling scrolling on the Offcanvas component seems to prevent this shift:
<Offcanvas show={showThreadHistory} onHide={setShowThreadHistory} scroll>
-
If a thread is deleted or the user manually enters an invalid
thread_idin the URL, the page currently renders empty with no feedback. This can be confusing.A better UX would be either:
- Redirecting the user to the new chat page along with a red toast, or
- Showing a clear warning that the thread does not exist (or both).
For example, a simple inline warning could be rendered like this:
const rearrangedConversation = useMemo(() => { return rearrangeCodeElements(conversation); }, [conversation]); const threadId = new URLSearchParams(window.location.search).get("thread_id"); if (threadId && !rearrangedConversation.length) { return ( <div className="text-center text-muted mt-5"> <h5>This chat does not exist</h5> <p>It may have been deleted or you don’t have access.</p> </div> ); }
Alternatively, this could be implemented as a toast notification (e.g. a red warning toast) followed by navigation to the new chat page.
Also there is another issue that when you are on an non-existing thread you would get TypeError: Invalid attempt to iterate non-iterable instance. In order to be iterable, non-array objects must have a [Symbol.iterator]() method. on console. it seems via evaluating the conversation on the rearrangedConversation here you can get rid of this.
const rearrangedConversation = useMemo(() => {
if (!Array.isArray(conversation)) {
return [];
}
return rearrangeCodeElements(conversation);
}, [conversation]);| if (response.ok) { | ||
| toastColor.current = "success"; | ||
| toastMessage.current = "Deleted chat."; | ||
| updateThreadList(mode, element); |
There was a problem hiding this comment.
here, when we remove one thread, it still shows the content of the removed thread and stays on the removed thread. To keep it clear, how about to force to go to the new message page as soon as it's removed. as same as what OpenAI does.
| updateThreadList(mode, element); | |
| updateThreadList(mode, element); | |
| window.location.assign("/chatbot/"); |
|
Hi @mo-dkrz |
tropicrainforest
left a comment
There was a problem hiding this comment.
One minor not directly relevant comment and then LGTM!
when i create a new message and then press on history, I don't get the current thread on the history side panel. It seems, we can fetch the backend each time on panel open to have an updated history.
here https://github.com/freva-org/freva-web/blob/thread-options/assets/js/Containers/FrevaGPT/customHooks/useGeneralThreads.js if we first Pass showThreadHistory prop to the hook from useGeneralThreads in SidePanel and then add this useEffect, it should fetch:
export default function useGeneralThreads(showThreadHistory) {
useEffect(() => {
if (showThreadHistory) {
setPageNumber(0);
handleThreadsRequest(0, undefined, setThreadsLoading, setThreads, setHasMore);
}
}, [showThreadHistory]);
I would put this into a new issue since this isn't a trivial task. I have to think about how to handle a filtered thread list as well as multi-page lists. |
Added option menu to threads history.
Now each thread can be renamed or deleted.
I didn't really managed to find a clean/general solution to multiple options without touching the whole list implementation. Maybe I have to have a deeper look into it in the future.
Expected behavior:
Note: On deletion of a thread react complains rightfully about a state update on an unmounted component (due to deleting thread from list). @Karinon do you have an idea how I could prevent this?