-
Notifications
You must be signed in to change notification settings - Fork 23
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
reset chat or reload history after data source change #194
Changes from 2 commits
a4954bf
adb38cc
07eae19
f3e4aab
dc465d2
7590f93
b84272b
418f84c
4a7328c
13f2b1d
7981746
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,6 +106,15 @@ export const useChatActions = (): AssistantActions => { | |
} | ||
}; | ||
|
||
const resetChat = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you simply call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
abortControllerRef?.abort(); | ||
core.services.conversationLoad.abortController?.abort(); | ||
chatContext.setConversationId(undefined); | ||
chatContext.setTitle(undefined); | ||
chatContext.setFlyoutComponent(null); | ||
chatStateDispatch({ type: 'reset' }); | ||
}; | ||
|
||
const openChatUI = () => { | ||
chatContext.setFlyoutVisible(true); | ||
chatContext.setSelectedTabId(TAB_ID.CHAT); | ||
|
@@ -225,5 +234,5 @@ export const useChatActions = (): AssistantActions => { | |
} | ||
}; | ||
|
||
return { send, loadChat, executeAction, openChatUI, abortAction, regenerate }; | ||
return { send, loadChat, resetChat, executeAction, openChatUI, abortAction, regenerate }; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,12 @@ import { | |
EuiFlexGroup, | ||
EuiFlexItem, | ||
} from '@elastic/eui'; | ||
import React, { useCallback, useEffect, useMemo, useState } from 'react'; | ||
import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; | ||
import { FormattedMessage } from '@osd/i18n/react'; | ||
import { useDebounce, useObservable } from 'react-use'; | ||
import { useDebounce, useObservable, useUpdateEffect } from 'react-use'; | ||
import cs from 'classnames'; | ||
import { skip } from 'rxjs/operators'; | ||
|
||
import { useChatActions, useChatState } from '../../hooks'; | ||
import { useChatContext, useCore } from '../../contexts'; | ||
import { TAB_ID } from '../../utils/constants'; | ||
|
@@ -61,6 +63,8 @@ export const ChatHistoryPage: React.FC<ChatHistoryPageProps> = React.memo((props | |
const chatHistories = useMemo(() => conversations?.objects || [], [conversations]); | ||
const hasNoConversations = | ||
!debouncedSearchName && !!conversations && conversations.total === 0 && !loading; | ||
const shouldRefreshRef = useRef(props.shouldRefresh); | ||
shouldRefreshRef.current = props.shouldRefresh; | ||
|
||
const handleSearchChange = useCallback((e) => { | ||
setSearchName(e.target.value); | ||
|
@@ -96,14 +100,29 @@ export const ChatHistoryPage: React.FC<ChatHistoryPageProps> = React.memo((props | |
[searchName] | ||
); | ||
|
||
useEffect(() => { | ||
if (props.shouldRefresh) services.conversations.reload(); | ||
useUpdateEffect(() => { | ||
if (!props.shouldRefresh) { | ||
return; | ||
} | ||
services.conversations.reload(); | ||
return () => { | ||
services.conversations.abortController?.abort(); | ||
}; | ||
}, [props.shouldRefresh, services.conversations]); | ||
|
||
useEffect(() => { | ||
services.conversations.load(bulkGetOptions); | ||
const subscription = services.dataSource | ||
.getDataSourceId$() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In current implementation, the same value will be emit after default data source change. Do you have any suggestions about that? How about add a |
||
.pipe(skip(1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you leave a comment to describe why it should skip the first value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, since the the conversations load will be called after mount. We skip the first value here to avoid duplicate load API request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we say that we are skipping the case that dataSource$ emits the same value? In that case we'd use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not the same value. Since |
||
.subscribe(() => { | ||
if (shouldRefreshRef.current) { | ||
services.conversations.reload(); | ||
} | ||
}); | ||
return () => { | ||
services.conversations.abortController?.abort(); | ||
subscription.unsubscribe(); | ||
}; | ||
}, [services.conversations, bulkGetOptions]); | ||
|
||
|
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.
Can we just use the
props.assistantActions.resetChat
without a ref?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.
I've tested in my local machine, the
props.assistantActions.resetChat
will be changed after chat button render. ThegetDataSourceId$
will be executed multi times. It's OK to change to useprops.assistantActions.resetChat
directly.