reset chat or reload history after data source change #194

wanglam · 2024-05-30T10:04:32Z

Description

Reset chat after data source change
Reload history and reset search params after data source change
Reset to chat tab when in trace tab

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test.
New functionality has user manual doc added.
Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Lin Wang <[email protected]>

ruanyl · 2024-06-03T07:13:29Z

public/tabs/history/chat_history_page.tsx

  }, [props.shouldRefresh, services.conversations]);

  useEffect(() => {
    services.conversations.load(bulkGetOptions);
+    const subscription = services.dataSource
+      .getDataSourceId$()


Would it be possible that dataSourceId$ emit the same value one after another? In such case, I think we don't want to reload.

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 distinctUntilChanged?

ruanyl · 2024-06-03T07:15:04Z

public/tabs/history/chat_history_page.tsx

  }, [props.shouldRefresh, services.conversations]);

  useEffect(() => {
    services.conversations.load(bulkGetOptions);
+    const subscription = services.dataSource
+      .getDataSourceId$()
+      .pipe(skip(1))


Could you leave a comment to describe why it should skip the first value?

Sure, since the the conversations load will be called after mount. We skip the first value here to avoid duplicate load API request.

Can we say that we are skipping the case that dataSource$ emits the same value? In that case we'd use distinctUntilChanged instead of skip the first value right?

Not the same value. Since dataSourceId$ and defaultDataSourceId$ are BehaviorSubject. The observer will be executed once first subscribed. The observer will reload conversations history. Since we already call conversations.load after mount, skip first value here to avoid duplicate API call.

ruanyl · 2024-06-03T07:20:24Z

public/hooks/use_chat_actions.tsx

@@ -106,6 +106,15 @@ export const useChatActions = (): AssistantActions => {
    }
  };

+  const resetChat = () => {


Can you simply call loadChat without any parameter instead of creating a new method? we can call loadChat() to start a new conversation

The loadChat only work when current tab is chat tab. The loadChat will set current tab to CHAT in this line. I think we can't change tab to CHAT if user is in history tab. So add a new resetChat here.

Signed-off-by: Lin Wang <[email protected]>

SuZhou-Joe · 2024-06-03T09:06:35Z

public/chat_header_button.tsx

@@ -54,6 +56,9 @@ export const HeaderChatButton = (props: HeaderChatButtonProps) => {
  const flyoutFullScreen = sidecarDockedMode === SIDECAR_DOCKED_MODE.TAKEOVER;
  const flyoutMountPoint = useRef(null);

+  const resetChatRef = useRef(props.assistantActions.resetChat);
+  resetChatRef.current = props.assistantActions.resetChat;


Can we just use the props.assistantActions.resetChat without a ref?

I've tested in my local machine, the props.assistantActions.resetChat will be changed after chat button render. The getDataSourceId$ will be executed multi times. It's OK to change to use props.assistantActions.resetChat directly.

raintygao · 2024-06-03T11:00:49Z

public/services/data_source_service.ts

-    if (this.dataSourceId$.getValue() === newDataSourceId) {
-      return;
-    }


Could you elaborate more on why we remove duplicate check in set function?

Since we already add distinctUntilChanged in getDataSourceId$, the next value won't be the same. Then we can remove the duplicate check here.

comment

Signed-off-by: Lin Wang <[email protected]>

codecov · 2024-06-05T04:14:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.50%. Comparing base (3991de2) to head (dc465d2).
Report is 6 commits behind head on main.

❗ Current head dc465d2 differs from pull request most recent head 7981746

Please upload reports for the commit 7981746 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
+ Coverage   90.02%   90.50%   +0.48%     
==========================================
  Files          54       57       +3     
  Lines        1433     1527      +94     
  Branches      347      361      +14     
==========================================
+ Hits         1290     1382      +92     
- Misses        141      143       +2     
  Partials        2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hailong-am · 2024-06-05T05:24:12Z

how about how was it generated(trace) page? I guess that page would need to reset if user in that page and datasource changed.

wanglam · 2024-06-05T05:47:02Z

how about how was it generated(trace) page? I guess that page would need to reset if user in that page and datasource changed.

Do you mean refresh how was it generated page when data source changed? I think it's a little bit confused. Because user won't get the trace info in another data source.

Hailong-am · 2024-06-05T07:33:20Z

how about how was it generated(trace) page? I guess that page would need to reset if user in that page and datasource changed.

Do you mean refresh how was it generated page when data source changed? I think it's a little bit confused. Because user won't get the trace info in another data source.

right, show existing trace data maybe confusing. how about reset to chat window?

Signed-off-by: Lin Wang <[email protected]>

…ot-after-data-source-change Signed-off-by: Lin Wang <[email protected]>

wanglam · 2024-06-06T03:52:44Z

how about how was it generated(trace) page? I guess that page would need to reset if user in that page and datasource changed.

Do you mean refresh how was it generated page when data source changed? I think it's a little bit confused. Because user won't get the trace info in another data source.

right, show existing trace data maybe confusing. how about reset to chat window?

Thanks for suggestions, I've already updated this part.

Signed-off-by: Lin Wang <[email protected]>

SuZhou-Joe · 2024-06-07T08:29:15Z

public/tabs/history/chat_history_page.tsx

+  const bulkGetOptionsRef = useRef(bulkGetOptions);
+  bulkGetOptionsRef.current = bulkGetOptions;


Not needed anymore I think?

Sure, can be removed.

Signed-off-by: Lin Wang <[email protected]>

* reset chat or reload history when data source change Signed-off-by: Lin Wang <[email protected]> * Add change log Signed-off-by: Lin Wang <[email protected]> * Address PR comments Signed-off-by: Lin Wang <[email protected]> * Set search and page after data source change Signed-off-by: Lin Wang <[email protected]> * Remove skip first value when subscribe dataSourceId$ Signed-off-by: Lin Wang <[email protected]> * Add dataSourceIdUdpates$ and finalDataSourceId Signed-off-by: Lin Wang <[email protected]> * Remove data source service mock in chat header button Signed-off-by: Lin Wang <[email protected]> * Remove no need useRef Signed-off-by: Lin Wang <[email protected]> * Refactor load history after data source change Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> (cherry picked from commit a2a98f6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* reset chat or reload history when data source change Signed-off-by: Lin Wang <[email protected]> * Add change log Signed-off-by: Lin Wang <[email protected]> * Address PR comments Signed-off-by: Lin Wang <[email protected]> * Set search and page after data source change Signed-off-by: Lin Wang <[email protected]> * Remove skip first value when subscribe dataSourceId$ Signed-off-by: Lin Wang <[email protected]> * Add dataSourceIdUdpates$ and finalDataSourceId Signed-off-by: Lin Wang <[email protected]> * Remove data source service mock in chat header button Signed-off-by: Lin Wang <[email protected]> * Remove no need useRef Signed-off-by: Lin Wang <[email protected]> * Refactor load history after data source change Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> (cherry picked from commit a2a98f6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

reset chat or reload history when data source change

a4954bf

Signed-off-by: Lin Wang <[email protected]>

wanglam added backport 2.x Trigger the backport flow to 2.x v2.15.0 labels May 30, 2024

Add change log

adb38cc

Signed-off-by: Lin Wang <[email protected]>

wanglam marked this pull request as ready for review May 30, 2024 10:19

wanglam requested review from lezzago, joshuali925, anirudha, ruanyl, SuZhou-Joe, xluo-aws, gaobinlong, Hailong-am and raintygao as code owners May 30, 2024 10:19

ruanyl reviewed Jun 3, 2024

View reviewed changes

wanglam added 2 commits June 3, 2024 16:24

Address PR comments

07eae19

Signed-off-by: Lin Wang <[email protected]>

Merge branch 'main' into feat-update-chat-bot-after-data-source-change

f3e4aab

Signed-off-by: Lin Wang <[email protected]>

SuZhou-Joe reviewed Jun 3, 2024

View reviewed changes

raintygao previously approved these changes Jun 3, 2024

View reviewed changes

raintygao self-requested a review June 3, 2024 11:00

raintygao reviewed Jun 3, 2024

View reviewed changes

raintygao self-requested a review June 3, 2024 11:01

Set search and page after data source change

dc465d2

Signed-off-by: Lin Wang <[email protected]>

wanglam added 2 commits June 5, 2024 18:19

Remove skip first value when subscribe dataSourceId$

7590f93

Signed-off-by: Lin Wang <[email protected]>

Add dataSourceIdUdpates$ and finalDataSourceId

b84272b

Signed-off-by: Lin Wang <[email protected]>

Merge remote-tracking branch 'os-origin/main' into feat-update-chat-b…

418f84c

…ot-after-data-source-change Signed-off-by: Lin Wang <[email protected]>

Remove data source service mock in chat header button

4a7328c

Signed-off-by: Lin Wang <[email protected]>

ruanyl approved these changes Jun 6, 2024

View reviewed changes

Hailong-am approved these changes Jun 6, 2024

View reviewed changes

raintygao approved these changes Jun 7, 2024

View reviewed changes

SuZhou-Joe reviewed Jun 7, 2024

View reviewed changes

wanglam added 2 commits June 7, 2024 16:33

Remove no need useRef

13f2b1d

Signed-off-by: Lin Wang <[email protected]>

Refactor load history after data source change

7981746

Signed-off-by: Lin Wang <[email protected]>

SuZhou-Joe approved these changes Jun 7, 2024

View reviewed changes

wanglam merged commit a2a98f6 into opensearch-project:main Jun 7, 2024
9 checks passed

opensearch-trigger-bot bot mentioned this pull request Jun 7, 2024

[Backport 2.x] reset chat or reload history after data source change #204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reset chat or reload history after data source change #194

reset chat or reload history after data source change #194

wanglam commented May 30, 2024 •

edited

Loading

ruanyl Jun 3, 2024

wanglam Jun 3, 2024

ruanyl Jun 3, 2024

wanglam Jun 3, 2024

SuZhou-Joe Jun 3, 2024

wanglam Jun 3, 2024

ruanyl Jun 3, 2024

wanglam Jun 3, 2024

SuZhou-Joe Jun 3, 2024 •

edited

Loading

wanglam Jun 3, 2024

raintygao Jun 3, 2024

wanglam Jun 4, 2024

codecov bot commented Jun 5, 2024 •

edited

Loading

Hailong-am commented Jun 5, 2024

wanglam commented Jun 5, 2024

Hailong-am commented Jun 5, 2024

wanglam commented Jun 6, 2024

SuZhou-Joe Jun 7, 2024

wanglam Jun 7, 2024

		const bulkGetOptionsRef = useRef(bulkGetOptions);
		bulkGetOptionsRef.current = bulkGetOptions;

reset chat or reload history after data source change #194

reset chat or reload history after data source change #194

Conversation

wanglam commented May 30, 2024 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2024 • edited Loading

Codecov Report

Hailong-am commented Jun 5, 2024

wanglam commented Jun 5, 2024

Hailong-am commented Jun 5, 2024

wanglam commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanglam commented May 30, 2024 •

edited

Loading

SuZhou-Joe Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 5, 2024 •

edited

Loading