Skip to content

[Chat] load latest conversation#11396

Merged
wanglam merged 11 commits into
opensearch-project:mainfrom
wanglam:add-load-latest-conversation
Mar 5, 2026
Merged

[Chat] load latest conversation#11396
wanglam merged 11 commits into
opensearch-project:mainfrom
wanglam:add-load-latest-conversation

Conversation

@wanglam

@wanglam wanglam commented Feb 26, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR implements automatic restoration of the latest conversation when the Chat window is opened. On mount, ChatWindow now calls the async restoreLatestConversation() to fetch and restore the most recent thread. A loading spinner is shown during restoration, and if it fails, an error state with a Retry button is displayed.

On the persistence side, updateCurrentMessages is replaced by saveConversation, triggered on every timeline update after a message is sent. Test coverage is updated throughout to reflect the new async flow and validate loading, error, and retry UI states.

Screenshots

Screen.Recording.2026-03-04.at.14.39.14.mov

Changelog

  • feat: [Chat] load latest conversation

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

github-actions Bot commented Feb 26, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit afb5dde)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: ChatService: replace sessionStorage persistence with saveConversation/restoreLatestConversation

Relevant files:

  • src/plugins/chat/public/services/chat_service.ts
  • src/plugins/chat/public/services/chat_service.test.ts

Sub-PR theme: ChatWindow: loading/error UI and abort controller for conversation restoration

Relevant files:

  • src/plugins/chat/public/components/chat_window.tsx
  • src/plugins/chat/public/components/chat_window.test.tsx
  • src/plugins/chat/public/components/chat_window_conversation_name.test.tsx
  • src/plugins/chat/public/components/chat_container.scss

⚡ Recommended focus areas for review

Save on Load

The saveConversation effect triggers whenever timeline changes and isLoading is false. When restoreConversationTimeline sets the timeline from the restored messages, isLoading is set to false in the finally block after setTimeline is called. Depending on React's batching behavior, this could cause saveConversation to be called immediately after restoration with the just-restored messages, resulting in an unnecessary write-back to persistence on every mount.

useEffect(() => {
  if (timeline.length > 0 && !isLoading) {
    chatService.saveConversation(timeline);
  }
}, [timeline, chatService, isLoading]);
Inline Styles

The loading and error state containers use inline style objects rather than CSS classes. This is inconsistent with the rest of the codebase which uses SCSS modules, and inline styles are recreated on every render. These should be moved to the existing SCSS file.

{isLoading ? (
  <div style={{
    display: 'flex',
    flexDirection: 'column',
    alignItems: 'center',
    justifyContent: 'center',
    height: '100%',
    gap: '16px'
  }}>
    <EuiLoadingSpinner size="xl" />
    <EuiText color="subdued">
      {i18n.translate('chat.window.loadingMessage', {
        defaultMessage: 'Loading conversation...',
      })}
    </EuiText>
  </div>
) : restoreError ? (
  <div style={{
    display: 'flex',
    flexDirection: 'column',
    alignItems: 'center',
    justifyContent: 'center',
    height: '100%',
    gap: '16px',
    padding: '24px'
  }}>
    <EuiText color="danger" textAlign="center">
      <h3>
        {i18n.translate('chat.window.restoreErrorTitle', {
          defaultMessage: 'Failed to restore conversation',
        })}
      </h3>
      <p>{restoreError}</p>
    </EuiText>
    <EuiButton
      onClick={handleRetryRestore}
      iconType="refresh"
    >
      {i18n.translate('chat.window.retryButton', {
        defaultMessage: 'Retry',
      })}
    </EuiButton>
  </div>
Pagination Index

restoreLatestConversation calls getConversations with page: 0, but if the API uses 1-based pagination (as suggested by the mock in tests returning page: 1), this may return no results or an incorrect page. The test mocks use page: 1 and pageSize: 10, which is inconsistent with the implementation using page: 0.

const result = await this.conversationHistoryService.getConversations({
  page: 0,
  pageSize: 1,
});
Missing setShowHistory

In handleSelectConversation, after successfully loading a conversation, setShowHistory(false) is called, but setIsLoading(false) is only called in the finally block when not aborted. If the conversation loads successfully but the abort check passes, the loading state is correctly cleared. However, if setShowHistory(false) is called inside the try block before finally, there could be a brief flash where history is hidden but loading spinner is still shown. Consider the ordering of state updates.

    // Reset UI state
    setCurrentRunId(null);
    setIsStreaming(false);
    setPendingConfirmation(null);
    setShowHistory(false);
  }
} catch (error: any) {
  // Don't show error if aborted
  if (!abortController.signal.aborted) {
    toasts?.addWarning({
      title: i18n.translate('chat.window.loadConversationErrorTitle', {
        defaultMessage: 'Failed to load conversation',
      }),
      text:
        error instanceof Error
          ? error.message
          : i18n.translate('chat.window.loadConversationErrorMessage', {
              defaultMessage: 'An unexpected error occurred while loading the conversation.',
            }),
    });
  }
} finally {
  // Only update loading state if not aborted
  if (!abortController.signal.aborted) {
    setIsLoading(false);
  }
  // Clear the abort controller reference
  if (loadingAbortControllerRef.current === abortController) {
    loadingAbortControllerRef.current = null;
  }
}

@github-actions

github-actions Bot commented Feb 26, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to afb5dde

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add error handling to async save operation

The saveConversation method now awaits the save operation but has no error handling.
If the save fails, the error will propagate to the caller (the useEffect in
chat_window.tsx) which doesn't handle it, potentially causing unhandled promise
rejections. Add a try-catch block to handle errors gracefully.

src/plugins/chat/public/services/chat_service.ts [554-559]

 public async saveConversation(messages: Message[]): Promise<void> {
   if (messages.length > 0) {
     const threadId = this.getThreadId();
-    await this.conversationHistoryService.saveConversation(threadId, messages);
+    try {
+      await this.conversationHistoryService.saveConversation(threadId, messages);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn('Failed to save conversation to history:', error);
+    }
   }
 }
Suggestion importance[1-10]: 6

__

Why: The saveConversation method lacks error handling, which could cause unhandled promise rejections if the save fails. Adding a try-catch with a warning log is a reasonable improvement, consistent with the old code's pattern.

Low
Handle unhandled promise from async save call

The saveConversation call is not awaited and its returned promise is not handled. If
the save fails, the error will be silently swallowed. The promise should be handled
to avoid unhandled rejections, especially since saveConversation is now async.

src/plugins/chat/public/components/chat_window.tsx [232-236]

 // Save conversation to history whenever timeline changes
 useEffect(() => {
   if (timeline.length > 0 && !isLoading) {
-    chatService.saveConversation(timeline);
+    chatService.saveConversation(timeline).catch((error) => {
+      console.error('Failed to save conversation:', error);
+    });
   }
 }, [timeline, chatService, isLoading]);
Suggestion importance[1-10]: 5

__

Why: The saveConversation call in the useEffect is not awaited and its promise is unhandled. Adding a .catch() handler prevents silent failures and potential unhandled promise rejections.

Low
General
Clear stale abort controller reference after aborting

When handleShowHistory aborts the loading and sets isLoading to false, it does not
clear loadingAbortControllerRef.current. This means the stale aborted controller
remains in the ref, and subsequent operations that check
loadingAbortControllerRef.current === abortController in the finally block of
restoreConversationTimeline may not correctly clear the ref.

src/plugins/chat/public/components/chat_window.tsx [498-503]

 const handleShowHistory = useCallback(() => {
   loadingAbortControllerRef.current?.abort();
+  loadingAbortControllerRef.current = null;
   setIsLoading(false);
   setShowHistory(true);
   setRestoreError(null);
 }, []);
Suggestion importance[1-10]: 5

__

Why: After aborting in handleShowHistory, the stale aborted controller remains in loadingAbortControllerRef.current. While the finally block in restoreConversationTimeline checks identity before clearing, leaving a stale reference could cause subtle issues. Setting it to null after abort is cleaner and consistent with handleCloseHistory.

Low
Add explicit error propagation documentation to restore method

The restoreLatestConversation method has no error handling. If getConversations or
getConversation throws, the error will propagate to the caller in chat_window.tsx
which does handle it via setRestoreError. However, the method should be consistent
with the rest of the service's error handling patterns and document that callers
must handle errors.

src/plugins/chat/public/services/chat_service.ts [596-605]

 public async restoreLatestConversation(): Promise<{
   threadId: string;
   messages: Message[];
 } | null> {
-  // Get the latest conversation summary from conversation history service
-  const result = await this.conversationHistoryService.getConversations({
-    page: 0,
-    pageSize: 1,
-  });
+  try {
+    // Get the latest conversation summary from conversation history service
+    const result = await this.conversationHistoryService.getConversations({
+      page: 0,
+      pageSize: 1,
+    });
+    // ... rest of the method
+  } catch (error) {
+    // Re-throw so callers can handle and display appropriate error UI
+    throw error;
+  }
+}
Suggestion importance[1-10]: 1

__

Why: The suggestion adds a try-catch that just re-throws the error, which is functionally equivalent to no try-catch at all. The improved_code is incomplete (contains // ... rest of the method) and doesn't actually improve the code behavior.

Low

Previous suggestions

Suggestions up to commit f6e9aba
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add error handling to async save operation

The saveConversation method now awaits the save operation but has no error handling.
If the save fails, the error will propagate to the caller (a useEffect in the
component) which doesn't handle it, potentially causing unhandled promise
rejections. Add a try-catch block to handle errors gracefully, similar to the
previous implementation.

src/plugins/chat/public/services/chat_service.ts [554-559]

 public async saveConversation(messages: Message[]): Promise<void> {
   if (messages.length > 0) {
     const threadId = this.getThreadId();
-    await this.conversationHistoryService.saveConversation(threadId, messages);
+    try {
+      await this.conversationHistoryService.saveConversation(threadId, messages);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn('Failed to save conversation to history:', error);
+    }
   }
 }
Suggestion importance[1-10]: 7

__

Why: The saveConversation method awaits the operation without error handling, which could cause unhandled promise rejections when called from a useEffect. Adding a try-catch is a valid improvement for robustness.

Medium
General
Handle unhandled promise rejection in effect

The saveConversation call is not awaited and its returned promise is not handled.
Since saveConversation is now async, calling it without handling the promise means
any errors will be silently swallowed as unhandled promise rejections. Either add
.catch() to handle errors or wrap in an async IIFE.

src/plugins/chat/public/components/chat_window.tsx [232-236]

 // Save conversation to history whenever timeline changes
 useEffect(() => {
   if (timeline.length > 0 && !isLoading) {
-    chatService.saveConversation(timeline);
+    chatService.saveConversation(timeline).catch((error) => {
+      console.warn('Failed to save conversation:', error);
+    });
   }
 }, [timeline, chatService, isLoading]);
Suggestion importance[1-10]: 6

__

Why: The saveConversation call in useEffect is not awaited and its promise is not handled, which can lead to silent unhandled rejections. Adding .catch() is a valid and important fix, especially if suggestion 1 is not applied to saveConversation itself.

Low
Prevent race condition when aborting loading

When handleShowHistory aborts the loading and sets isLoading to false, the
restoreConversationTimeline's finally block will also attempt to set isLoading to
false after the abort check. However, if a new loading operation starts before the
old one's finally runs, the loadingAbortControllerRef.current === abortController
check should prevent issues. But directly setting isLoading(false) here bypasses the
abort controller's finally block logic and could cause a race condition if the abort
check in finally passes before the ref is cleared. The abort controller ref should
be explicitly nulled out here to prevent the finally block from clearing a new
controller.

src/plugins/chat/public/components/chat_window.tsx [498-503]

 const handleShowHistory = useCallback(() => {
-  loadingAbortControllerRef.current?.abort();
+  if (loadingAbortControllerRef.current) {
+    loadingAbortControllerRef.current.abort();
+    loadingAbortControllerRef.current = null;
+  }
   setIsLoading(false);
   setShowHistory(true);
   setRestoreError(null);
 }, []);
Suggestion importance[1-10]: 5

__

Why: The suggestion to null out loadingAbortControllerRef.current in handleShowHistory is a minor defensive improvement, but the existing finally block already checks loadingAbortControllerRef.current === abortController before clearing, so the race condition risk is limited.

Low
No change needed for this specific pattern

The restoreLatestConversation method has no error handling. If getConversations or
getConversation throws, the error will propagate to the component's
restoreConversationTimeline which does handle it, but the method itself should be
robust. More critically, if this.coreChatService is undefined when setThreadId is
called, it is guarded, but if getConversations throws due to network issues, the
component will show an error state with no way to distinguish transient vs.
permanent failures. This is acceptable, but the method should at minimum not
silently fail on partial errors.

src/plugins/chat/public/services/chat_service.ts [596-604]

 public async restoreLatestConversation(): Promise<{
   threadId: string;
   messages: Message[];
 } | null> {
-  // Get the latest conversation summary from conversation history service
   const result = await this.conversationHistoryService.getConversations({
     page: 0,
     pageSize: 1,
   });
Suggestion importance[1-10]: 1

__

Why: The improved_code is essentially identical to the existing_code with only a comment removed, and the suggestion doesn't propose a concrete actionable change. The component already handles errors from this method.

Low
Suggestions up to commit aa2c94e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle promise rejection from async save call

The saveConversation call inside useEffect is not awaited and its returned promise
is ignored, which can lead to unhandled promise rejections if the save fails. Either
wrap it in an async IIFE with error handling, or ensure errors are caught.

src/plugins/chat/public/components/chat_window.tsx [232-236]

 // Save conversation to history whenever timeline changes
 useEffect(() => {
   if (timeline.length > 0 && !isLoading) {
-    chatService.saveConversation(timeline);
+    chatService.saveConversation(timeline).catch((error) => {
+      console.error('Failed to save conversation:', error);
+    });
   }
 }, [timeline, chatService, isLoading]);
Suggestion importance[1-10]: 7

__

Why: The saveConversation call in useEffect ignores the returned promise, which can lead to unhandled rejections. Adding .catch() is a straightforward fix that prevents potential runtime errors, especially since suggestion 1 and 2 are independent and this directly addresses the call site.

Medium
Add error handling to prevent unhandled rejections

The saveConversation method now awaits the save operation but has no error handling.
If the save fails, the error will propagate to the caller (useEffect in the
component), which doesn't handle it, potentially causing unhandled promise
rejections. Add a try-catch to handle errors gracefully, similar to the previous
implementation.

src/plugins/chat/public/services/chat_service.ts [552-557]

 public async saveConversation(messages: Message[]): Promise<void> {
   if (messages.length > 0) {
     const threadId = this.getThreadId();
-    await this.conversationHistoryService.saveConversation(threadId, messages);
+    try {
+      await this.conversationHistoryService.saveConversation(threadId, messages);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn('Failed to save conversation to history:', error);
+    }
   }
 }
Suggestion importance[1-10]: 6

__

Why: The saveConversation method lacks error handling, which could cause unhandled promise rejections when the save fails. The suggestion is valid and improves robustness, though the impact is moderate since the caller could also handle errors.

Low
General
Abort previous operation before starting a new one

When restoreConversationTimeline is called (e.g., on retry), a new AbortController
is created and assigned to loadingAbortControllerRef.current, but any previously
running operation's abort controller is not aborted first. This could lead to a race
condition where the old operation completes after the new one starts. Abort the
previous controller before creating a new one.

src/plugins/chat/public/components/chat_window.tsx [138-144]

 const restoreConversationTimeline = useCallback(async () => {
+  // Abort any previous loading operation
+  if (loadingAbortControllerRef.current) {
+    loadingAbortControllerRef.current.abort();
+  }
+
   // Create abort controller for this loading operation
   const abortController = new AbortController();
   loadingAbortControllerRef.current = abortController;
 
   setIsLoading(true);
   setRestoreError(null);
Suggestion importance[1-10]: 6

__

Why: The race condition identified is valid - if restoreConversationTimeline is called again (e.g., on retry) while a previous call is still running, the old controller is overwritten without being aborted. The fix correctly aborts the previous operation before starting a new one.

Low
Suggestions up to commit 344255c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add error handling to prevent unhandled rejections

The saveConversation method now awaits the save operation but has no error handling.
Previously the code used .catch() to log warnings on failure. Without error
handling, a failed save will throw an unhandled promise rejection that could crash
the caller. Add a try/catch block to handle errors gracefully.

src/plugins/chat/public/services/chat_service.ts [552-557]

 public async saveConversation(messages: Message[]): Promise<void> {
   if (messages.length > 0) {
     const threadId = this.getThreadId();
-    await this.conversationHistoryService.saveConversation(threadId, messages);
+    try {
+      await this.conversationHistoryService.saveConversation(threadId, messages);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn('Failed to save conversation to history:', error);
+    }
   }
 }
Suggestion importance[1-10]: 7

__

Why: The new saveConversation method awaits the operation without any error handling, which could cause unhandled promise rejections. The old code used .catch() to log warnings, and this improvement restores that safety net.

Medium
Handle unhandled promise rejection in effect

The saveConversation call is not awaited and its returned promise is not handled. If
saveConversation throws (e.g., network error), it will result in an unhandled
promise rejection. The promise should be caught or the effect should handle errors
explicitly.

src/plugins/chat/public/components/chat_window.tsx [232-236]

 // Save conversation to history whenever timeline changes
 useEffect(() => {
   if (timeline.length > 0 && !isLoading) {
-    chatService.saveConversation(timeline);
+    chatService.saveConversation(timeline).catch((error) => {
+      console.error('Failed to save conversation:', error);
+    });
   }
 }, [timeline, chatService, isLoading]);
Suggestion importance[1-10]: 6

__

Why: The saveConversation call in the useEffect is not awaited and its promise is not handled, which could result in silent failures or unhandled rejections. Adding a .catch() handler improves robustness.

Low
Fix potential off-by-one pagination error

The restoreLatestConversation method in chat_service.ts uses page: 0 when calling
getConversations, but the test for "should restore timeline from persisted messages
on mount" mocks getConversations with page: 1. If the API uses 1-based pagination,
using page: 0 may return incorrect or empty results. Verify the pagination
convention used by getConversations and use the correct page index.

src/plugins/chat/public/components/chat_window.tsx [599-603]

 public async restoreLatestConversation(): Promise<{
   threadId: string;
   messages: Message[];
 } | null> {
   // Get the latest conversation summary from conversation history service
   const result = await this.conversationHistoryService.getConversations({
-    page: 0,
+    page: 1,
     pageSize: 1,
   });
Suggestion importance[1-10]: 3

__

Why: The suggestion points to chat_window.tsx but the relevant code is in chat_service.ts. Additionally, the test mock uses page: 1 but this is just a mock return value and doesn't necessarily indicate the API convention. The suggestion is speculative without clear evidence of a bug.

Low
General
Clear stale error state when closing history panel

When handleCloseHistory aborts the loading and sets isLoading to false, the
restoreConversationTimeline function's finally block also checks
abortController.signal.aborted before setting isLoading(false). However, if a new
restoreConversationTimeline call is triggered after abort (e.g., via retry), the
loadingAbortControllerRef.current is set to null here but the old abort controller's
finally block may still run and incorrectly clear the new controller reference. The
finally block already guards against this with if (loadingAbortControllerRef.current
=== abortController), so this is safe, but setRestoreError(null) is not reset on
close, which could leave a stale error visible if the user re-opens. Consider also
resetting restoreError when closing history.

src/plugins/chat/public/components/chat_window.tsx [503-513]

 const handleCloseHistory = useCallback(() => {
   // Abort any ongoing loading operation
   if (loadingAbortControllerRef.current) {
     loadingAbortControllerRef.current.abort();
     loadingAbortControllerRef.current = null;
   }
 
-  // Reset loading state and show history panel
+  // Reset loading state, error state, and hide history panel
   setIsLoading(false);
+  setRestoreError(null);
   setShowHistory(false);
 }, []);
Suggestion importance[1-10]: 4

__

Why: The handleCloseHistory function doesn't reset restoreError, which could leave a stale error visible if the user re-opens the panel. However, handleShowHistory already calls setRestoreError(null), so the impact is limited to edge cases.

Low
Suggestions up to commit a3ba182
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add error handling to prevent unhandled rejections

The saveConversation method now awaits the save operation but has no error handling.
Previously, errors were caught and logged as warnings. Without error handling, a
failed save will propagate an unhandled promise rejection to the caller, which could
crash the UI or cause unexpected behavior.

src/plugins/chat/public/services/chat_service.ts [552-557]

 public async saveConversation(messages: Message[]): Promise<void> {
   if (messages.length > 0) {
     const threadId = this.getThreadId();
-    await this.conversationHistoryService.saveConversation(threadId, messages);
+    try {
+      await this.conversationHistoryService.saveConversation(threadId, messages);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn('Failed to save conversation to history:', error);
+    }
   }
 }
Suggestion importance[1-10]: 7

__

Why: The new saveConversation method awaits the async operation without any error handling, which could cause unhandled promise rejections. The previous implementation had a .catch() handler that logged warnings, and this important safety net was lost in the refactor.

Medium
Handle async save errors in effect

chatService.saveConversation is now async but the returned promise is not handled in
the useEffect. If the save fails, the error will be silently swallowed or cause an
unhandled rejection. The promise should be caught to handle errors gracefully.

src/plugins/chat/public/components/chat_window.tsx [232-236]

 // Save conversation to history whenever timeline changes
 useEffect(() => {
   if (timeline.length > 0 && !isLoading) {
-    chatService.saveConversation(timeline);
+    chatService.saveConversation(timeline).catch((error) => {
+      console.error('Failed to save conversation:', error);
+    });
   }
 }, [timeline, chatService, isLoading]);
Suggestion importance[1-10]: 6

__

Why: The useEffect calls chatService.saveConversation which is async but the returned promise is not handled. If saveConversation itself doesn't catch errors (see suggestion 1), this could result in unhandled promise rejections in the component.

Low
General
Clear error state when aborting and closing history

When handleCloseHistory aborts an ongoing restoreConversationTimeline operation, it
sets isLoading to false. However, restoreConversationTimeline also sets isLoading to
false in its finally block only when not aborted. But if the initial mount
restoration is aborted, the user will see an empty chat with no error and no way to
retry, since restoreError is not cleared here. Consider also clearing restoreError
to ensure a consistent UI state.

src/plugins/chat/public/components/chat_window.tsx [503-513]

 const handleCloseHistory = useCallback(() => {
   // Abort any ongoing loading operation
   if (loadingAbortControllerRef.current) {
     loadingAbortControllerRef.current.abort();
     loadingAbortControllerRef.current = null;
   }
 
   // Reset loading state and show history panel
   setIsLoading(false);
+  setRestoreError(null);
   setShowHistory(false);
 }, []);
Suggestion importance[1-10]: 5

__

Why: When handleCloseHistory aborts an ongoing load, restoreError is not cleared, which could leave a stale error state visible to the user. The handleShowHistory callback already clears restoreError, but handleCloseHistory does not, creating an inconsistent UI state.

Low
Document that method may throw on network errors

The restoreLatestConversation method has no error handling. If getConversations or
getConversation throws (e.g., network error), the exception will propagate to the
caller. The chat_window.tsx does catch errors from this method and sets
restoreError, but the service itself should document or handle this, and callers
relying on a null return for "no conversation" may be surprised by thrown
exceptions.

src/plugins/chat/public/services/chat_service.ts [594-602]

+public async restoreLatestConversation(): Promise<{
+  threadId: string;
+  messages: Message[];
+} | null> {
+  // Get the latest conversation summary from conversation history service
+  const result = await this.conversationHistoryService.getConversations({
+    page: 1,
+    pageSize: 1,
+  });
 
-
Suggestion importance[1-10]: 1

__

Why: The suggestion only asks to add documentation/comments and the existing_code is identical to the improved_code, making this a zero-impact suggestion. The caller in chat_window.tsx already handles thrown exceptions properly.

Low
Suggestions up to commit 82fcd77
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add error handling to prevent unhandled rejections

The saveConversation method now awaits the save operation but has no error handling.
Previously, errors were caught and logged with console.warn. Without error handling,
a failed save will throw an unhandled promise rejection that could crash the caller
or produce unhandled rejection warnings.

src/plugins/chat/public/services/chat_service.ts [552-557]

 public async saveConversation(messages: Message[]): Promise<void> {
   if (messages.length > 0) {
     const threadId = this.getThreadId();
-    await this.conversationHistoryService.saveConversation(threadId, messages);
+    try {
+      await this.conversationHistoryService.saveConversation(threadId, messages);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn('Failed to save conversation to history:', error);
+    }
   }
 }
Suggestion importance[1-10]: 7

__

Why: The new saveConversation method awaits the operation without any error handling, which could cause unhandled promise rejections. The previous implementation had a .catch() handler with console.warn, and this improvement restores that safety net.

Medium
Handle async save errors in effect

chatService.saveConversation is now async but the returned promise is not handled
inside the useEffect. If the save fails, the error will be silently swallowed or
cause an unhandled rejection. The promise should be caught explicitly.

src/plugins/chat/public/components/chat_window.tsx [233-237]

 // Save conversation to history whenever timeline changes
 useEffect(() => {
   if (timeline.length > 0 && !isLoading) {
-    chatService.saveConversation(timeline);
+    chatService.saveConversation(timeline).catch((error) => {
+      console.error('Failed to save conversation:', error);
+    });
   }
 }, [timeline, chatService, isLoading]);
Suggestion importance[1-10]: 6

__

Why: The useEffect calls chatService.saveConversation (which is now async) without handling the returned promise. A rejection would be silently swallowed or cause an unhandled rejection warning, so adding a .catch() is a valid improvement.

Low
General
Always restore thread ID from latest conversation

restoreLatestConversation is defined in chat_service.ts, not chat_window.tsx. More
importantly, when no MESSAGES_SNAPSHOT event is found but a conversation does exist,
the thread ID is never set in coreChatService. This means subsequent messages would
be sent under a new thread ID rather than continuing the latest conversation. The
thread ID should be set regardless of whether a snapshot is found.

src/plugins/chat/public/components/chat_window.tsx [594-633]

 public async restoreLatestConversation(): Promise<{
   threadId: string;
   messages: Message[];
 } | null> {
-  // Get the latest conversation summary from conversation history service
   const result = await this.conversationHistoryService.getConversations({
     page: 1,
     pageSize: 1,
   });
 
   if (result.conversations.length > 0) {
-    // Found a latest conversation - get full details
     const latestConversationSummary = result.conversations[0];
 
-    // Get the full conversation with all events
+    // Always restore the thread ID so new messages continue the latest conversation
+    if (this.coreChatService) {
+      this.coreChatService.setThreadId(latestConversationSummary.threadId);
+    }
+
     const events = await this.conversationHistoryService.getConversation(
       latestConversationSummary.threadId
     );
 
     if (!events) {
       return null;
     }
 
-    // Extract messages from MESSAGES_SNAPSHOT event
     const snapshotEvent = events.find((e) => e.type === EventType.MESSAGES_SNAPSHOT);
     if (snapshotEvent && 'messages' in snapshotEvent) {
-      // Set the thread ID in core service
-      if (this.coreChatService) {
-        this.coreChatService.setThreadId(latestConversationSummary.threadId);
-      }
       return {
         threadId: latestConversationSummary.threadId,
         messages: snapshotEvent.messages,
       };
     }
   }
 
-  // No snapshot event found
   return null;
 }
Suggestion importance[1-10]: 7

__

Why: When a conversation exists but has no MESSAGES_SNAPSHOT event, the thread ID is never set in coreChatService, meaning subsequent messages would be sent under a new thread ID instead of continuing the latest conversation. Moving the setThreadId call before the snapshot check fixes this logical gap.

Medium
Clear error state when closing history panel

When handleCloseHistory aborts an ongoing load and sets isLoading(false), the
restoreConversationTimeline callback's finally block also checks
abortController.signal.aborted before calling setIsLoading(false). However,
loadingAbortControllerRef.current is set to null here before the async operation's
finally block runs, so the ref-equality check loadingAbortControllerRef.current ===
abortController in finally will be false and won't re-set loading to false — this is
correct. But setRestoreError is not cleared here, so a previous error state could
persist when the user closes history and reopens it. Consider also clearing
restoreError on close.

src/plugins/chat/public/components/chat_window.tsx [504-514]

 const handleCloseHistory = useCallback(() => {
   // Abort any ongoing loading operation
   if (loadingAbortControllerRef.current) {
     loadingAbortControllerRef.current.abort();
     loadingAbortControllerRef.current = null;
   }
 
-  // Reset loading state and show history panel
+  // Reset loading state and error, hide history panel
   setIsLoading(false);
+  setRestoreError(null);
   setShowHistory(false);
 }, []);
Suggestion importance[1-10]: 5

__

Why: The handleCloseHistory callback doesn't clear restoreError, so a previous error state could persist when the user closes and reopens the history panel. Adding setRestoreError(null) here is a valid UX improvement, though handleShowHistory already clears it.

Low

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0f16c17

@codecov

codecov Bot commented Feb 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.19512% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.02%. Comparing base (cde17bd) to head (afb5dde).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/plugins/chat/public/components/chat_window.tsx 58.82% 17 Missing and 11 partials ⚠️
src/plugins/chat/public/services/chat_service.ts 78.57% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11396      +/-   ##
==========================================
- Coverage   61.02%   61.02%   -0.01%     
==========================================
  Files        4897     4897              
  Lines      135545   135568      +23     
  Branches    23518    23533      +15     
==========================================
+ Hits        82717    82731      +14     
- Misses      46899    46901       +2     
- Partials     5929     5936       +7     
Flag Coverage Δ
Linux_1 24.80% <ø> (ø)
Linux_2 38.36% <ø> (ø)
Linux_3 42.60% <62.19%> (+<0.01%) ⬆️
Linux_4 33.61% <ø> (ø)
Windows_1 24.82% <ø> (ø)
Windows_2 38.34% <ø> (ø)
Windows_3 42.61% <62.19%> (+<0.01%) ⬆️
Windows_4 33.62% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e69f962

Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 1c9c37e

Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

github-actions Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 82fcd77

@github-actions

github-actions Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a3ba182

Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

github-actions Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 344255c

Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

github-actions Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit aa2c94e

wanglam added 2 commits March 4, 2026 13:59
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
@github-actions

github-actions Bot commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f6e9aba

@github-actions

github-actions Bot commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit afb5dde

{showHistory ? (
{isLoading ? (
<div style={{
display: 'flex',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move inline css to src/plugins/chat/public/components/chat_window.scss?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I will update this in the next PR.

@wanglam wanglam merged commit 54086ca into opensearch-project:main Mar 5, 2026
85 of 86 checks passed
markdboyd pushed a commit to cloud-gov/OpenSearch-Dashboards that referenced this pull request Mar 9, 2026
* Add load latest conversation

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Changeset file for PR opensearch-project#11396 created/updated

* Address PR comments

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Fix unit tests

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Abort conversation loading after go back

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Change to page 0

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Increase left padding to 12px

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Only pass toolMessage if not include full messages

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Abort loading after show history clicked

Signed-off-by: Lin Wang <wonglam@amazon.com>

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants