Skip to content

[Chat] Support stop agent when it is streaming#11328

Open
SuZhou-Joe wants to merge 16 commits into
opensearch-project:mainfrom
SuZhou-Joe:feature/stop-agent
Open

[Chat] Support stop agent when it is streaming#11328
SuZhou-Joe wants to merge 16 commits into
opensearch-project:mainfrom
SuZhou-Joe:feature/stop-agent

Conversation

@SuZhou-Joe

@SuZhou-Joe SuZhou-Joe commented Feb 11, 2026

Copy link
Copy Markdown
Member

Description

Support stop agent execution when it is streaming.

Issues Resolved

Screenshot

20260211161050192.mp4

Testing the changes

Changelog

  • feat: '[Chat] Support stop agent when it is streaming'

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

@github-actions

Copy link
Copy Markdown
Contributor

❌ Changelog Entry Missing Hyphen

Changelog entries must begin with a hyphen (-).

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@coderabbitai

coderabbitai Bot commented Feb 11, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds support for stopping agent execution during streaming in the Chat component. It introduces an activity messaging system with ActivityType and ActivityMessage types, a new ActivityRow component, a stop button with timing management in ChatInput, abort handling in ChatWindow for stream cancellation, server-side disconnect handling, and activity message filtering to prevent UI-only messages from reaching the agent.

Changes

Cohort / File(s) Summary
Activity Type System
changelogs/fragments/11328.yml, src/core/public/chat/types.ts, src/core/public/chat/index.ts, src/core/public/index.ts, src/plugins/chat/common/types.ts
Introduces ActivityType enum with STOP value and ActivityMessage interface (role: 'activity') for system-level messaging. Extends Message union, Role type, and exports across public API surfaces to expose these new types.
Activity Rendering
src/plugins/chat/public/components/activity_row.tsx, src/plugins/chat/public/components/activity_row.test.tsx, src/plugins/chat/public/components/chat_messages.tsx, src/plugins/chat/public/components/chat_messages.test.tsx
Adds new ActivityRow component that renders activities as EUI callouts with color/icon mapping (STOP → warning). Integrates activity rendering into ChatMessages before system-message handling. Comprehensive test coverage for activity rendering with various content shapes.
Stop Button UI & Logic
src/plugins/chat/public/components/chat_input.tsx, src/plugins/chat/public/components/chat_input.test.tsx, src/plugins/chat/public/hooks/use_stop_button_timing.ts, src/plugins/chat/public/hooks/index.ts
Adds onStopExecution callback to ChatInputProps. Introduces useStopButtonTiming hook to manage stop button visibility with 50ms show delay and 200ms hide delay for smooth UX. Conditionally renders stop button when streaming. Exports hooks via new index barrel.
Stream Abort & Filtering
src/plugins/chat/public/components/chat_window.tsx, src/plugins/chat/public/components/chat_window.test.tsx
Implements handleStopExecution to abort active subscriptions, call chatService.abort(), append stop-activity message, and reset streaming state. Filters out activity messages from send/resend flows to prevent UI-only messages reaching the agent. Stores subscription reference for abort capability.
Server & Agent Integration
src/plugins/chat/server/routes/index.ts, packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts
Server: adds AbortController and client-disconnect handling via request.events.aborted$. Agent: filters out activity messages (role: 'activity') before sending to Bedrock, keeping them UI-only.

Sequence Diagram

sequenceDiagram
    actor User
    participant ChatUI as Chat Input
    participant ChatWindow as Chat Window
    participant ChatService as Chat Service
    participant Agent as Agent/Bedrock
    participant Server

    User->>ChatUI: Clicks Stop Button
    ChatUI->>ChatWindow: onStopExecution()
    ChatWindow->>ChatWindow: Abort activeSubscriptionRef
    ChatWindow->>ChatService: abort()
    ChatService->>Server: Send abort signal
    Server->>Agent: Cancel request (AbortController)
    ChatWindow->>ChatWindow: Reset streaming state
    ChatWindow->>ChatWindow: Append ActivityMessage (type: STOP)
    ChatWindow->>ChatUI: Update message list
    ChatUI->>User: Render ActivityRow (Stop activity)
    
    Note over ChatWindow: Activity messages filtered<br/>before sending to agent
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

ai, v3.4.0

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive PR description includes main sections but lacks critical details. 'Issues Resolved' and 'Testing the changes' sections are empty, and no explicit testing steps are provided. Fill in the 'Issues Resolved' section with issue numbers/URLs, provide detailed testing steps in 'Testing the changes', and confirm checklist items are addressed before merge.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and directly summarizes the main change: adding support to stop agent execution when streaming, which matches the primary objective across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Feb 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.75862% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.30%. Comparing base (b1260be) to head (25e8d7e).

Files with missing lines Patch % Lines
src/plugins/chat/public/components/chat_window.tsx 66.66% 3 Missing ⚠️
src/plugins/chat/server/routes/index.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11328      +/-   ##
==========================================
+ Coverage   60.29%   60.30%   +0.01%     
==========================================
  Files        4664     4665       +1     
  Lines      130372   130399      +27     
  Branches    22233    22238       +5     
==========================================
+ Hits        78603    78633      +30     
+ Misses      46161    46154       -7     
- Partials     5608     5612       +4     
Flag Coverage Δ
Linux_1 24.80% <ø> (ø)
Linux_2 38.36% <ø> (ø)
Linux_3 ?
Linux_4 33.61% <ø> (-0.01%) ⬇️
Windows_1 24.82% <ø> (+0.01%) ⬆️
Windows_2 38.34% <ø> (ø)
Windows_3 40.22% <82.75%> (+<0.01%) ⬆️
Windows_4 33.61% <ø> (+<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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🤖 Fix all issues with AI agents
In `@changelogs/fragments/11328.yml`:
- Around line 1-2: The YAML value in the feat list is being parsed as a flow
sequence because of unquoted brackets and a `#` character; update the list item
(the string starting with [Chat] Support stop agent when it is streaming) to be
a quoted scalar (e.g., wrap the entire message in double or single quotes) so
YAML treats it as a single string; ensure you quote the whole value for the
array element (the line beginning with `- [Chat] Support stop agent when it is
streaming`) and escape any internal quotes if needed.

In `@src/core/public/index.ts`:
- Line 449: You added ActivityType to the public API (exported in
src/core/public/index.ts) but didn't update the generated API docs; run the docs
generator and commit the changes by executing yarn docs:acceptApiChanges, review
the updated ./src/core/public/public.api.md to ensure ActivityType is
documented, and include those updated .api.md files in your commit alongside the
export change.

In `@src/plugins/chat/public/components/chat_input.tsx`:
- Around line 86-107: The stop button can be clicked during the 200ms linger
window causing handleStopExecution to append a spurious "Execution stopped by
user" activity; to fix this, guard the early exit in handleStopExecution (in
chat_window.tsx) by checking the streaming flag ref (isStreamingRef.current)
before calling chatService.abort() and appending the activity—if
isStreamingRef.current is false, simply return and do nothing; update any caller
(onStopExecution/shouldShowStopButton) to rely on this guard so clicks during
the linger window are ignored.

In `@src/plugins/chat/public/components/chat_window.test.tsx`:
- Around line 691-825: Tests in this block never invoke handleStopExecution;
update them to actually click the stop control so the stop flow is exercised:
render ChatWindow (ref: ChatWindowInstance) and call ref.current?.sendMessage to
start streaming (uses mockChatService.sendMessage and observable), ensure
mockChatService includes an abort: jest.fn() (or equivalent) and that the
observable's subscribe returns an unsubscribe mock, then find the DOM element
with test id/class chatStopExecutionButton and simulate a click to trigger
handleStopExecution, and finally assert that unsubscribe (or
mockChatService.abort) was called, an activity message with ActivityType.STOP
was appended, and loading/streaming state on ChatWindow was cleared; reference
functions/classes: handleStopExecution, chatStopExecutionButton,
mockChatService.sendMessage, ChatWindow, ChatWindowInstance,
observable.subscribe/unsubscribe.

In `@src/plugins/chat/public/components/chat_window.tsx`:
- Around line 69-70: activeSubscriptionRef (used by subscribeToMessageStream)
isn't cleaned up on component unmount which can leave the Observable
subscription active; add a useEffect with a cleanup return that checks
activeSubscriptionRef.current and calls unsubscribe() (or aborts via the
Subscription API) and sets it to null to avoid updates after unmount; place this
effect in the chat_window component alongside existing hooks so it runs once
(empty deps) and references activeSubscriptionRef and any abort logic used in
subscribeToMessageStream to ensure the stream is torn down on unmount.
- Around line 266-304: The mockChatService in chat_window.test.tsx is missing
the abort method, so handleStopExecution's abort flow isn't exercised; update
the mock (mockChatService) used by the chat window tests to include abort:
jest.fn() so the test invokes ChatService.abort() just like the real ChatService
(see ChatService.abort and the handleStopExecution function) and the
stop-execution tests can validate the full abort/cleanup path.
🧹 Nitpick comments (6)
src/core/public/chat/types.ts (1)

108-112: Use Record<string, unknown> instead of Record<string, any>.

The content field uses any, which bypasses type checking. Per project guidelines, prefer unknown over any.

Proposed fix
 export interface ActivityMessage extends Omit<BaseMessage, 'content'> {
   role: 'activity';
   activityType: ActivityType;
-  content: Record<string, any>;
+  content: Record<string, unknown>;
 }

As per coding guidelines, "Use TypeScript for all new code; avoid any type (use unknown or generics instead)".

src/plugins/chat/common/types.ts (1)

91-95: Use z.record(z.unknown()) instead of z.record(z.any()).

Same concern as in types.tsz.any() disables validation for the record values. Use z.record(z.unknown()) to match the recommended Record<string, unknown> type.

Proposed fix
 export const ActivityMessageSchema = BaseMessageSchema.extend({
   role: z.literal('activity'),
   activityType: z.nativeEnum(ActivityType),
-  content: z.record(z.any()),
+  content: z.record(z.unknown()),
 });

As per coding guidelines, "Use TypeScript for all new code; avoid any type (use unknown or generics instead)".

src/plugins/chat/public/components/activity_row.tsx (1)

48-57: Consider using a CSS class instead of inline styles for margins.

Inline styles bypass the stylesheet cascade and are harder to override or maintain. A small scss file (or an existing one) with a class like .actRow__callout { margin: 8px 0; } would be more consistent with the project's pattern of importing .scss files in components. As per coding guidelines, import .scss files at the top of the component file and use 3-letter prefix on class names for scoping.

src/plugins/chat/public/components/activity_row.test.tsx (1)

75-122: Duplicate test cases that don't verify icons.

The tests "should render with cross icon for STOP activity" (Lines 75-90) and "should render with iInCircle icon for unknown activity types" (Lines 107-122) are effectively duplicates of the tests on Lines 60-73 and 92-105. They claim to verify icons but only check the callout color class — the same assertion as the preceding tests. Either remove these duplicates or add actual icon verification (e.g., querying for [data-euiicon-type="cross"]).

src/plugins/chat/public/components/chat_window.tsx (1)

280-294: Multiple setTimeline calls may cause an intermediate render without the cancel message.

Lines 282 and 294 both call setTimeline. In React 18 with automatic batching, synchronous calls within the same event handler are batched, so this should be safe. However, combining them into a single update would be more explicit and avoid any edge-case with future async boundaries.

Combined timeline update
-      // 4. Remove loading message
-      setTimeline((prev) => prev.filter((msg) => !msg.id.startsWith('loading-')));
-
-      // 5. Add cancellation feedback message
-      // Use 'activity' role (AG-UI standard) for system activities
-      const cancelMessage: Message = {
+      // 4. Remove loading messages and add cancellation feedback
+      const cancelMessage: Message = {
         id: `cancelled-${Date.now()}`,
         role: 'activity',
         activityType: ActivityType.STOP,
         content: {
           message: 'Execution stopped by user',
         },
       };
-      setTimeline((prev) => [...prev, cancelMessage]);
+      setTimeline((prev) => [
+        ...prev.filter((msg) => !msg.id.startsWith('loading-')),
+        cancelMessage,
+      ]);
src/plugins/chat/public/hooks/use_stop_button_timing.ts (1)

26-64: Including shouldShowStopButton in the dependency array causes redundant effect executions.

When the show timer fires and shouldShowStopButton becomes true, the effect re-runs while isStreaming is still true, creating another unnecessary 50ms timer. Similarly, when the hide timer fires, the effect re-runs one extra time. This is functionally harmless (React bails out on identical state) but wasteful.

An alternative is to read the current visibility from a ref inside the effect, removing it from the dependency array — then the effect only re-runs when isStreaming changes, which is the true trigger.

Refactor to eliminate redundant effect runs
 export const useStopButtonTiming = (isStreaming: boolean): boolean => {
   const [shouldShowStopButton, setShouldShowStopButton] = useState(false);
   const showTimerRef = useRef<NodeJS.Timeout | null>(null);
   const hideTimerRef = useRef<NodeJS.Timeout | null>(null);
+  const isVisibleRef = useRef(false);
 
+  // Keep ref in sync
+  useEffect(() => {
+    isVisibleRef.current = shouldShowStopButton;
+  }, [shouldShowStopButton]);
+
   useEffect(() => {
     if (isStreaming) {
       if (hideTimerRef.current) {
         clearTimeout(hideTimerRef.current);
         hideTimerRef.current = null;
       }
       showTimerRef.current = setTimeout(() => {
         setShouldShowStopButton(true);
       }, 50);
     } else {
       if (showTimerRef.current) {
         clearTimeout(showTimerRef.current);
         showTimerRef.current = null;
       }
-      if (shouldShowStopButton) {
+      if (isVisibleRef.current) {
         hideTimerRef.current = setTimeout(() => {
           setShouldShowStopButton(false);
         }, 200);
       } else {
         setShouldShowStopButton(false);
       }
     }
 
     return () => {
       if (showTimerRef.current) clearTimeout(showTimerRef.current);
       if (hideTimerRef.current) clearTimeout(hideTimerRef.current);
     };
-  }, [isStreaming, shouldShowStopButton]);
+  }, [isStreaming]);
 
   return shouldShowStopButton;
 };
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7538a07 and 72f0687.

📒 Files selected for processing (17)
  • changelogs/fragments/11328.yml
  • packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts
  • src/core/public/chat/index.ts
  • src/core/public/chat/types.ts
  • src/core/public/index.ts
  • src/plugins/chat/common/types.ts
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/activity_row.tsx
  • src/plugins/chat/public/components/chat_input.test.tsx
  • src/plugins/chat/public/components/chat_input.tsx
  • src/plugins/chat/public/components/chat_messages.test.tsx
  • src/plugins/chat/public/components/chat_messages.tsx
  • src/plugins/chat/public/components/chat_window.test.tsx
  • src/plugins/chat/public/components/chat_window.tsx
  • src/plugins/chat/public/hooks/index.ts
  • src/plugins/chat/public/hooks/use_stop_button_timing.ts
  • src/plugins/chat/server/routes/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,json,scss,css,html,yml,yaml}

📄 CodeRabbit inference engine (CLAUDE.md)

Use snake_case for all filenames

Files:

  • src/plugins/chat/common/types.ts
  • src/plugins/chat/public/components/chat_messages.test.tsx
  • src/core/public/index.ts
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/chat_input.test.tsx
  • src/plugins/chat/public/components/activity_row.tsx
  • src/plugins/chat/public/components/chat_window.test.tsx
  • changelogs/fragments/11328.yml
  • src/core/public/chat/index.ts
  • src/core/public/chat/types.ts
  • src/plugins/chat/public/hooks/use_stop_button_timing.ts
  • src/plugins/chat/public/components/chat_input.tsx
  • src/plugins/chat/public/components/chat_window.tsx
  • src/plugins/chat/server/routes/index.ts
  • src/plugins/chat/public/hooks/index.ts
  • packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts
  • src/plugins/chat/public/components/chat_messages.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Use TypeScript for all new code; avoid any type (use unknown or generics instead)
Avoid non-null assertions (!.) and @ts-ignore comments
HTML attributes: id and data-test-subj values should be camelCase
Import .scss files at the top of the component file and use 3-letter prefix on class names for scoping (e.g., plgComponent)
Prefer functional components in React and name action props as on<Subject><Change>

Files:

  • src/plugins/chat/common/types.ts
  • src/plugins/chat/public/components/chat_messages.test.tsx
  • src/core/public/index.ts
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/chat_input.test.tsx
  • src/plugins/chat/public/components/activity_row.tsx
  • src/plugins/chat/public/components/chat_window.test.tsx
  • src/core/public/chat/index.ts
  • src/core/public/chat/types.ts
  • src/plugins/chat/public/hooks/use_stop_button_timing.ts
  • src/plugins/chat/public/components/chat_input.tsx
  • src/plugins/chat/public/components/chat_window.tsx
  • src/plugins/chat/server/routes/index.ts
  • src/plugins/chat/public/hooks/index.ts
  • packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts
  • src/plugins/chat/public/components/chat_messages.tsx
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx,js,jsx}: Use named exports, not default exports
Use ES2015 import/export syntax; only import top-level module APIs, never reach into internal paths
Use single quotes for strings with 100-character print width and ES5 trailing commas (enforced by Prettier)

Files:

  • src/plugins/chat/common/types.ts
  • src/plugins/chat/public/components/chat_messages.test.tsx
  • src/core/public/index.ts
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/chat_input.test.tsx
  • src/plugins/chat/public/components/activity_row.tsx
  • src/plugins/chat/public/components/chat_window.test.tsx
  • src/core/public/chat/index.ts
  • src/core/public/chat/types.ts
  • src/plugins/chat/public/hooks/use_stop_button_timing.ts
  • src/plugins/chat/public/components/chat_input.tsx
  • src/plugins/chat/public/components/chat_window.tsx
  • src/plugins/chat/server/routes/index.ts
  • src/plugins/chat/public/hooks/index.ts
  • packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts
  • src/plugins/chat/public/components/chat_messages.tsx
**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests must achieve 80%+ code coverage (enforced by Codecov); use react-testing-library for components, not enzyme snapshots

Files:

  • src/plugins/chat/public/components/chat_messages.test.tsx
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/chat_input.test.tsx
  • src/plugins/chat/public/components/chat_window.test.tsx
src/core/{public,server}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

When modifying src/core/public/ or src/core/server/ APIs, run yarn docs:acceptApiChanges and commit the updated .api.md files

Files:

  • src/core/public/index.ts
  • src/core/public/chat/index.ts
  • src/core/public/chat/types.ts
**/server/routes/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

API routes must start with /api/ and use snake_case for paths, parameters, and body fields

Files:

  • src/plugins/chat/server/routes/index.ts
🧠 Learnings (3)
📚 Learning: 2026-02-04T00:52:23.917Z
Learnt from: CR
Repo: opensearch-project/OpenSearch-Dashboards PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T00:52:23.917Z
Learning: Applies to **/*.test.{ts,tsx} : Unit tests must achieve 80%+ code coverage (enforced by Codecov); use react-testing-library for components, not enzyme snapshots

Applied to files:

  • src/plugins/chat/public/components/chat_messages.test.tsx
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/chat_input.test.tsx
📚 Learning: 2026-02-04T00:52:23.917Z
Learnt from: CR
Repo: opensearch-project/OpenSearch-Dashboards PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T00:52:23.917Z
Learning: Applies to **/integration_tests/**/*.test.{ts,tsx} : Integration tests should be placed in `**/integration_tests/**/*.test.ts` and run with `yarn test:jest_integration`

Applied to files:

  • src/plugins/chat/public/components/activity_row.test.tsx
📚 Learning: 2026-02-04T00:52:23.917Z
Learnt from: CR
Repo: opensearch-project/OpenSearch-Dashboards PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T00:52:23.917Z
Learning: Applies to **/*.{ts,tsx} : Prefer functional components in React and name action props as `on<Subject><Change>`

Applied to files:

  • src/plugins/chat/public/components/chat_input.tsx
🧬 Code graph analysis (9)
src/plugins/chat/common/types.ts (2)
src/core/public/index.ts (1)
  • ActivityType (449-449)
src/core/public/chat/index.ts (1)
  • ActivityType (18-18)
src/plugins/chat/public/components/chat_messages.test.tsx (3)
src/core/public/chat/types.ts (1)
  • Message (117-123)
src/plugins/chat/common/types.ts (1)
  • Message (13-13)
src/plugins/chat/public/components/chat_messages.tsx (1)
  • ChatMessages (67-254)
src/plugins/chat/public/components/activity_row.test.tsx (4)
src/core/public/chat/types.ts (1)
  • ActivityMessage (108-112)
src/plugins/chat/common/types.ts (2)
  • ActivityMessage (19-19)
  • ActivityType (24-24)
src/core/public/chat/index.ts (2)
  • ActivityMessage (17-17)
  • ActivityType (18-18)
src/plugins/chat/public/components/activity_row.tsx (1)
  • ActivityRow (45-58)
src/plugins/chat/public/components/chat_input.test.tsx (1)
src/plugins/chat/public/components/chat_input.tsx (1)
  • ChatInput (24-111)
src/plugins/chat/public/components/activity_row.tsx (2)
src/core/public/chat/types.ts (1)
  • ActivityMessage (108-112)
src/core/public/chat/index.ts (2)
  • ActivityMessage (17-17)
  • ActivityType (18-18)
src/plugins/chat/public/components/chat_window.test.tsx (4)
src/plugins/chat/public/components/chat_window.tsx (2)
  • ChatWindow (46-48)
  • ChatWindowInstance (32-35)
src/core/public/index.ts (1)
  • ActivityType (449-449)
src/plugins/chat/common/types.ts (1)
  • ActivityType (24-24)
src/core/public/chat/index.ts (1)
  • ActivityType (18-18)
src/plugins/chat/public/hooks/use_stop_button_timing.ts (1)
src/plugins/chat/public/hooks/index.ts (1)
  • useStopButtonTiming (7-7)
src/plugins/chat/public/components/chat_input.tsx (2)
src/plugins/chat/public/hooks/index.ts (1)
  • useStopButtonTiming (7-7)
src/plugins/chat/public/hooks/use_stop_button_timing.ts (1)
  • useStopButtonTiming (21-67)
src/plugins/chat/public/components/chat_messages.tsx (1)
src/plugins/chat/public/components/activity_row.tsx (1)
  • ActivityRow (45-58)
🪛 YAMLlint (1.38.0)
changelogs/fragments/11328.yml

[error] 2-2: syntax error: expected , but found ''

(syntax)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (64)
  • GitHub Check: Run cypress tests (osd:ciGroup16Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup15Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup13)
  • GitHub Check: Run cypress tests (osd:ciGroup13Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup17Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup11)
  • GitHub Check: Run cypress tests (osd:ciGroup14Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup12Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup10Slow)
  • GitHub Check: Run cypress tests (osd:ciGroup12)
  • GitHub Check: Run functional tests on Windows (ciGroup10)
  • GitHub Check: Run cypress tests (osd:ciGroup10Explore)
  • GitHub Check: Run cypress tests (osd:ciGroup7)
  • GitHub Check: Run cypress tests (osd:ciGroup2)
  • GitHub Check: Run cypress tests (osd:ciGroup5)
  • GitHub Check: Run functional tests on Windows (ciGroup9)
  • GitHub Check: Run cypress tests (osd:ciGroup8)
  • GitHub Check: Run functional tests on Linux (ciGroup7)
  • GitHub Check: Run cypress tests (osd:ciGroup10Fast)
  • GitHub Check: Run cypress tests (osd:ciGroup6)
  • GitHub Check: Run cypress tests (osd:ciGroup1)
  • GitHub Check: Run functional tests on Windows (ciGroup8)
  • GitHub Check: Run cypress tests (osd:ciGroup4)
  • GitHub Check: Run functional tests on Windows (ciGroup12)
  • GitHub Check: Run functional tests on Windows (ciGroup11)
  • GitHub Check: Run cypress tests (osd:ciGroup9)
  • GitHub Check: Run functional tests on Linux (ciGroup1)
  • GitHub Check: Run functional tests on Windows (ciGroup4)
  • GitHub Check: Run cypress tests (osd:ciGroup3)
  • GitHub Check: Run functional tests on Windows (ciGroup1)
  • GitHub Check: Run functional tests on Windows (ciGroup5)
  • GitHub Check: Run functional tests on Linux (ciGroup10)
  • GitHub Check: Run functional tests on Windows (ciGroup7)
  • GitHub Check: Run functional tests on Windows (ciGroup2)
  • GitHub Check: Run functional tests on Linux (ciGroup4)
  • GitHub Check: Run functional tests on Linux (ciGroup5)
  • GitHub Check: Run functional tests on Linux (ciGroup9)
  • GitHub Check: Run functional tests on Linux (ciGroup8)
  • GitHub Check: Run functional tests on Windows (ciGroup6)
  • GitHub Check: Run functional tests on Linux (ciGroup3)
  • GitHub Check: Run functional tests on Linux (ciGroup13)
  • GitHub Check: Run functional tests on Linux (ciGroup2)
  • GitHub Check: Run functional tests on Linux (ciGroup6)
  • GitHub Check: Run functional tests on Windows (ciGroup3)
  • GitHub Check: Run functional tests on Linux (ciGroup11)
  • GitHub Check: Run functional tests on Linux (ciGroup12)
  • GitHub Check: Build and Verify on Windows (ciGroup4)
  • GitHub Check: Build and Verify on Linux (ciGroup2)
  • GitHub Check: Build and Verify on Linux (ciGroup3)
  • GitHub Check: Build and Verify on Linux (ciGroup1)
  • GitHub Check: Build and Verify on Linux (ciGroup4)
  • GitHub Check: Build and Verify on Windows (ciGroup1)
  • GitHub Check: Build and Verify on Windows (ciGroup3)
  • GitHub Check: lighthouse
  • GitHub Check: Build min release artifacts on Linux x64
  • GitHub Check: Run plugin functional tests on Windows
  • GitHub Check: Build min release artifacts on Linux ARM64
  • GitHub Check: Run plugin functional tests on Linux
  • GitHub Check: Build and Verify on Windows (ciGroup2)
  • GitHub Check: Build min release artifacts on macOS ARM64
  • GitHub Check: Build min release artifacts on Windows x64
  • GitHub Check: bundle-analyzer
  • GitHub Check: Build min release artifacts on macOS x64
  • GitHub Check: Lint and validate
🔇 Additional comments (10)
packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts (1)

454-457: LGTM — activity messages correctly filtered before reaching Bedrock.

The early-return guard is well-placed at the top of the filter chain, ensuring activity messages are excluded before any content normalization runs.

src/plugins/chat/server/routes/index.ts (1)

33-50: LGTM! Abort controller wired correctly for client disconnect.

The AbortController + request.events.aborted$ subscription correctly propagates cancellation to the upstream fetch. The aborted$ observable completes with the request lifecycle, so no explicit unsubscribe is needed.

src/core/public/chat/index.ts (1)

17-18: LGTM!

New ActivityMessage and ActivityType exports are correctly added to the barrel.

src/plugins/chat/public/components/chat_messages.tsx (1)

223-226: LGTM!

The activity message branch correctly leverages TypeScript's discriminated union narrowing on message.role === 'activity' and delegates rendering to ActivityRow. Placement before the system-message fallback is appropriate.

src/plugins/chat/public/hooks/index.ts (1)

1-7: LGTM!

Clean barrel file with named exports.

src/plugins/chat/public/components/activity_row.tsx (1)

1-57: LGTM! Clean component with good separation of concerns between content rendering, styling logic, and the main component. The fallback to JSON.stringify is a reasonable safety net.

src/plugins/chat/public/components/chat_input.test.tsx (1)

1-225: Well-structured tests with good coverage of the new stop button functionality. The mock for useStopButtonTiming transparently passing through isStreaming is clean, and the button consolidation tests with rerender effectively verify state transitions.

src/plugins/chat/public/components/chat_messages.test.tsx (1)

1-350: Solid test coverage for activity message rendering and message type handling. Good use of mocked child components and clear test organization. The timeline context tests effectively validate the integration scenario of user cancellation.

src/plugins/chat/public/components/chat_window.test.tsx (1)

862-947: Activity message filtering tests are well-constructed. These effectively verify that activity messages are excluded from the messages array sent to the agent, covering both the send and resend flows with proper assertions on the filtered output.

src/plugins/chat/public/components/chat_input.tsx (1)

11-36: Good integration of the timing hook and new prop.

The useStopButtonTiming hook cleanly encapsulates the debounce logic, and the optional onStopExecution prop maintains backward compatibility. The conditional rendering logic is straightforward.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread changelogs/fragments/11328.yml Outdated
Comment on lines +1 to +2
feat:
- [Chat] Support stop agent when it is streaming ([#11328](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11328)) No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

YAML syntax error: value must be quoted.

The [Chat] substring is parsed as a YAML flow sequence and # starts a comment, causing a parse error. Wrap the string in quotes.

Proposed fix
 feat:
-- [Chat] Support stop agent when it is streaming ([`#11328`](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11328))
+- '[Chat] Support stop agent when it is streaming ([`#11328`](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11328))'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
feat:
- [Chat] Support stop agent when it is streaming ([#11328](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11328))
feat:
- '[Chat] Support stop agent when it is streaming ([`#11328`](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11328))'
🧰 Tools
🪛 YAMLlint (1.38.0)

[error] 2-2: syntax error: expected , but found ''

(syntax)

🤖 Prompt for AI Agents
In `@changelogs/fragments/11328.yml` around lines 1 - 2, The YAML value in the
feat list is being parsed as a flow sequence because of unquoted brackets and a
`#` character; update the list item (the string starting with [Chat] Support
stop agent when it is streaming) to be a quoted scalar (e.g., wrap the entire
message in double or single quotes) so YAML treats it as a single string; ensure
you quote the whole value for the array element (the line beginning with `-
[Chat] Support stop agent when it is streaming`) and escape any internal quotes
if needed.

Comment thread src/core/public/index.ts
ChatImplementationFunctions,
Message,
ChatWindowState,
ActivityType,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if any .api.md files exist in the repository
echo "=== Finding .api.md files ==="
find . -name "*.api.md" -type f | head -20

echo ""
echo "=== Checking if .api.md files were modified in git ==="
git diff --name-only HEAD~1 2>/dev/null | grep -i '\.api\.md' || echo "No .api.md files changed"

echo ""
echo "=== Looking at files in src/core/public ==="
find ./src/core/public -name "*.api.md" -o -name ".api.md"

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 668


🏁 Script executed:

#!/bin/bash
# Check the public.api.md file for ActivityType
echo "=== Checking if ActivityType is in public.api.md ==="
grep -n "ActivityType" ./src/core/public/public.api.md || echo "ActivityType NOT found in public.api.md"

echo ""
echo "=== Checking the export statement in index.ts ==="
grep -A 2 -B 2 "ActivityType" ./src/core/public/index.ts

echo ""
echo "=== Checking what ActivityType is (from chat module) ==="
grep -r "export.*ActivityType" ./src/core/public/chat* 2>/dev/null || echo "Searching chat directory..."
find ./src/core/public -name "*.ts" -type f | xargs grep -l "ActivityType" 2>/dev/null | head -5

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 499


Commit updated .api.md files after running yarn docs:acceptApiChanges.

Adding ActivityType to src/core/public/index.ts extends the core public API surface. Per coding guidelines, this requires running yarn docs:acceptApiChanges and committing the updated .api.md files. Currently, ActivityType is not documented in ./src/core/public/public.api.md.

🤖 Prompt for AI Agents
In `@src/core/public/index.ts` at line 449, You added ActivityType to the public
API (exported in src/core/public/index.ts) but didn't update the generated API
docs; run the docs generator and commit the changes by executing yarn
docs:acceptApiChanges, review the updated ./src/core/public/public.api.md to
ensure ActivityType is documented, and include those updated .api.md files in
your commit alongside the export change.

Comment on lines +86 to +107
{shouldShowStopButton && onStopExecution ? (
<EuiButtonIcon
iconType="cross"
onClick={onStopExecution}
aria-label="Stop agent execution"
data-test-subj="chatStopExecutionButton"
size="m"
color="danger"
display="fill"
/>
) : (
<EuiButtonIcon
iconType="sortUp"
onClick={onSend}
isDisabled={input.trim().length === 0 || isStreaming}
aria-label="Send message"
data-test-subj="chatSendButton"
size="m"
color="primary"
display="fill"
/>
)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Clicking the stop button during the 200ms linger window after natural completion adds a spurious cancellation message.

When streaming completes naturally, isStreaming becomes false but shouldShowStopButton remains true for up to 200ms (per useStopButtonTiming). If the user clicks the stop button during this window, handleStopExecution in chat_window.tsx will still execute: chatService.abort() is called (potentially a no-op), and an "Execution stopped by user" activity message is appended to the timeline — even though execution already completed.

Consider guarding handleStopExecution with a check on isStreamingRef.current to prevent the spurious message:

Proposed guard in chat_window.tsx handleStopExecution
  const handleStopExecution = useCallback(() => {
+   // Don't stop if streaming already ended (e.g., during linger window)
+   if (!isStreamingRef.current) return;
+
    try {
      // 1. Unsubscribe from observable if active
🤖 Prompt for AI Agents
In `@src/plugins/chat/public/components/chat_input.tsx` around lines 86 - 107, The
stop button can be clicked during the 200ms linger window causing
handleStopExecution to append a spurious "Execution stopped by user" activity;
to fix this, guard the early exit in handleStopExecution (in chat_window.tsx) by
checking the streaming flag ref (isStreamingRef.current) before calling
chatService.abort() and appending the activity—if isStreamingRef.current is
false, simply return and do nothing; update any caller
(onStopExecution/shouldShowStopButton) to rely on this guard so clicks during
the linger window are ignored.

Comment on lines +691 to +825
describe('stop execution functionality', () => {
it('should create activity message when execution is stopped', async () => {
const ref = React.createRef<ChatWindowInstance>();
renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);

// Setup a streaming scenario with an abort controller
const observableWithAbort = {
subscribe: jest.fn((callbacks) => {
// Long-running stream
return { unsubscribe: jest.fn() };
}),
};

mockChatService.sendMessage.mockResolvedValue({
observable: observableWithAbort,
userMessage: { id: 'user-1', content: 'test', role: 'user' },
});

// Send a message to start streaming
await act(async () => {
await ref.current?.sendMessage({ content: 'test message' });
});

// At this point the component should be streaming
// In real usage, the ChatWindow would have an abort controller set
// and the stop button would call handleStopExecution

// Since we can't directly test the internal handleStopExecution,
// we verify the observable subscription was created
expect(observableWithAbort.subscribe).toHaveBeenCalled();
});

it('should abort request when stop execution is triggered', async () => {
const ref = React.createRef<ChatWindowInstance>();
renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);

const unsubscribeMock = jest.fn();
const observableWithAbort = {
subscribe: jest.fn(() => ({
unsubscribe: unsubscribeMock,
})),
};

mockChatService.sendMessage.mockResolvedValue({
observable: observableWithAbort,
userMessage: { id: 'user-1', content: 'test', role: 'user' },
});

// Send a message to create subscription
await act(async () => {
await ref.current?.sendMessage({ content: 'test message' });
});

expect(observableWithAbort.subscribe).toHaveBeenCalled();
// In actual implementation, stopping would call unsubscribe
});

it('should add activity message with STOP type after stopping', async () => {
// This test verifies the structure of activity messages added
// when execution is stopped
const ref = React.createRef<ChatWindowInstance>();
renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);

// The actual handleStopExecution creates a message like:
// {
// id: `cancelled-${Date.now()}`,
// role: 'activity',
// activityType: ActivityType.STOP,
// content: { message: 'Execution stopped by user' }
// }

// This would be verified through integration tests or manual testing
// as the stop functionality is triggered by user interaction
expect(mockChatService.sendMessage).toBeDefined();
});

it('should reset streaming state after stop', async () => {
const ref = React.createRef<ChatWindowInstance>();
renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);

const observableWithCompletion = {
subscribe: jest.fn((callbacks) => {
// Simulate immediate completion (like a stop)
setTimeout(() => callbacks.complete(), 10);
return { unsubscribe: jest.fn() };
}),
};

mockChatService.sendMessage.mockResolvedValue({
observable: observableWithCompletion,
userMessage: { id: 'user-1', content: 'test', role: 'user' },
});

await act(async () => {
await ref.current?.sendMessage({ content: 'test message' });
});

// Wait for completion
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 20));
});

// After completion, component should no longer be streaming
expect(observableWithCompletion.subscribe).toHaveBeenCalled();
});

it('should remove loading messages after stop', async () => {
const ref = React.createRef<ChatWindowInstance>();
renderWithContext(<ChatWindow ref={ref} onClose={jest.fn()} />);

const observableWithCompletion = {
subscribe: jest.fn((callbacks) => {
// Simulate stop by completing without messages
setTimeout(() => callbacks.complete(), 10);
return { unsubscribe: jest.fn() };
}),
};

mockChatService.sendMessage.mockResolvedValue({
observable: observableWithCompletion,
userMessage: { id: 'user-1', content: 'test', role: 'user' },
});

await act(async () => {
await ref.current?.sendMessage({ content: 'test message' });
});

// Wait for completion which should remove loading messages
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 20));
});

expect(observableWithCompletion.subscribe).toHaveBeenCalled();
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Stop execution tests don't exercise handleStopExecution — they're effectively duplicates of existing streaming tests.

None of these tests actually trigger the stop flow. They verify observable subscription creation and completion — behavior already covered by the "loading message functionality" and "streaming state management" sections. In particular:

  • Lines 748-765: Asserts mockChatService.sendMessage is defined — this is a trivially-passing no-op.
  • Lines 767-824: Test observable completion, not the stop action.

To meaningfully test stop execution, render the component, start streaming, then find and click the chatStopExecutionButton to trigger handleStopExecution. You'll also need to add abort: jest.fn() to mockChatService (it's currently missing).

🤖 Prompt for AI Agents
In `@src/plugins/chat/public/components/chat_window.test.tsx` around lines 691 -
825, Tests in this block never invoke handleStopExecution; update them to
actually click the stop control so the stop flow is exercised: render ChatWindow
(ref: ChatWindowInstance) and call ref.current?.sendMessage to start streaming
(uses mockChatService.sendMessage and observable), ensure mockChatService
includes an abort: jest.fn() (or equivalent) and that the observable's subscribe
returns an unsubscribe mock, then find the DOM element with test id/class
chatStopExecutionButton and simulate a click to trigger handleStopExecution, and
finally assert that unsubscribe (or mockChatService.abort) was called, an
activity message with ActivityType.STOP was appended, and loading/streaming
state on ChatWindow was cleared; reference functions/classes:
handleStopExecution, chatStopExecutionButton, mockChatService.sendMessage,
ChatWindow, ChatWindowInstance, observable.subscribe/unsubscribe.

Comment on lines +69 to +70
// Track active subscription for abort functionality
const activeSubscriptionRef = useRef<Subscription | null>(null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Active subscription is not cleaned up on component unmount — potential resource leak.

activeSubscriptionRef is set in subscribeToMessageStream (Line 255) and cleared on error/complete (Lines 241, 248), but there's no cleanup in a useEffect return callback for when the component unmounts mid-stream. This means the observable subscription can remain active after unmount, potentially causing state updates on an unmounted component and wasted network resources.

Proposed fix: add cleanup effect
   // Clean up event handler on component unmount
   useEffect(() => {
     return () => {
       eventHandler.clearState();
+      // Clean up active subscription on unmount
+      if (activeSubscriptionRef.current) {
+        activeSubscriptionRef.current.unsubscribe();
+        activeSubscriptionRef.current = null;
+      }
     };
   }, [eventHandler]);

Also applies to: 254-255

🤖 Prompt for AI Agents
In `@src/plugins/chat/public/components/chat_window.tsx` around lines 69 - 70,
activeSubscriptionRef (used by subscribeToMessageStream) isn't cleaned up on
component unmount which can leave the Observable subscription active; add a
useEffect with a cleanup return that checks activeSubscriptionRef.current and
calls unsubscribe() (or aborts via the Subscription API) and sets it to null to
avoid updates after unmount; place this effect in the chat_window component
alongside existing hooks so it runs once (empty deps) and references
activeSubscriptionRef and any abort logic used in subscribeToMessageStream to
ensure the stream is torn down on unmount.

Comment on lines +266 to +304
const handleStopExecution = useCallback(() => {
try {
// 1. Unsubscribe from observable if active
if (activeSubscriptionRef.current) {
activeSubscriptionRef.current.unsubscribe();
activeSubscriptionRef.current = null;
}

// 2. Abort fetch request
chatService.abort();

// 3. Reset streaming state
setIsStreaming(false);
isStreamingRef.current = false;

// 4. Remove loading message
setTimeline((prev) => prev.filter((msg) => !msg.id.startsWith('loading-')));

// 5. Add cancellation feedback message
// Use 'activity' role (AG-UI standard) for system activities
const cancelMessage: Message = {
id: `cancelled-${Date.now()}`,
role: 'activity',
activityType: ActivityType.STOP,
content: {
message: 'Execution stopped by user',
},
};
setTimeline((prev) => [...prev, cancelMessage]);

// 6. Clear event handler state
eventHandler.clearState();
} catch (error) {
console.error('Error stopping execution:', error);
// Ensure state cleanup even if abort fails
setIsStreaming(false);
isStreamingRef.current = false;
}
}, [chatService, eventHandler]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find ChatService definition and check for abort method
echo "=== Searching for ChatService class/interface definition ==="
fd -e ts -e tsx -g '*chat*service*' | head -20

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 368


🏁 Script executed:

# Search for abort method in service files
echo "=== Searching for 'abort' in chat-related service files ==="
rg -n 'abort' --type=ts -g '**/*chat*service*'

echo ""
echo "=== Searching for ChatService interface/class definition ==="
rg -n 'class ChatService|interface ChatService' --type=ts

echo ""
echo "=== Check the actual ChatService file content ==="
rg -n 'abort\(' --type=ts --context=3

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 50396


🏁 Script executed:

# Check the mock in chat_window.test.tsx around lines 54-63
sed -n '50,70p' src/plugins/chat/public/components/chat_window.test.tsx

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 824


🏁 Script executed:

# Search for tests related to handleStopExecution or stop execution in chat_window.test.tsx
grep -n "handleStopExecution\|stop.*execution\|onStopExecution" src/plugins/chat/public/components/chat_window.test.tsx | head -20

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 505


🏁 Script executed:

# View the stop execution tests in detail
sed -n '691,860p' src/plugins/chat/public/components/chat_window.test.tsx

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 6297


🏁 Script executed:

# Verify if any test in chat_window.test.tsx actually calls chatService.abort()
grep -n "\.abort()" src/plugins/chat/public/components/chat_window.test.tsx

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 66


🏁 Script executed:

# Check if 'abort' appears in the mockChatService definition in chat_window.test.tsx
grep -A 20 "mockChatService = {" src/plugins/chat/public/components/chat_window.test.tsx | head -30

Repository: opensearch-project/OpenSearch-Dashboards

Length of output: 830


Add abort method to mockChatService in chat_window.test.tsx.

The abort() method exists on ChatService (src/plugins/chat/public/services/chat_service.ts:518) and is properly tested in its own test suite. However, the mock in chat_window.test.tsx (lines 54-63) omits it, preventing the stop-execution tests from fully exercising the abort flow. Add abort: jest.fn() to the mock so tests validate the complete stop flow.

🤖 Prompt for AI Agents
In `@src/plugins/chat/public/components/chat_window.tsx` around lines 266 - 304,
The mockChatService in chat_window.test.tsx is missing the abort method, so
handleStopExecution's abort flow isn't exercised; update the mock
(mockChatService) used by the chat window tests to include abort: jest.fn() so
the test invokes ChatService.abort() just like the real ChatService (see
ChatService.abort and the handleStopExecution function) and the stop-execution
tests can validate the full abort/cleanup path.

SuZhou-Joe and others added 3 commits February 11, 2026 16:40
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
SuZhou-Joe and others added 6 commits February 12, 2026 16:49
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: update chat-header look & feel

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>

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

---------

Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Co-authored-by: ZilongX <99905560+ZilongX@users.noreply.github.com>
…oject#11327)

* Improve chatbot input and message bubble layout

Signed-off-by: Owen Wang <owenwyk@amazon.com>

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

* Update panel style

Signed-off-by: Owen Wang <owenwyk@amazon.com>

* Fix user message bubble alignment

Signed-off-by: Owen Wang <owenwyk@amazon.com>

---------

Signed-off-by: Owen Wang <owenwyk@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

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: Add ActivityMessage type definitions

Relevant files:

  • src/core/public/chat/types.ts
  • src/core/public/chat/index.ts
  • src/core/public/index.ts
  • src/plugins/chat/common/types.ts

Sub-PR theme: Implement ActivityRow UI component

Relevant files:

  • src/plugins/chat/public/components/activity_row.tsx
  • src/plugins/chat/public/components/activity_row.test.tsx
  • src/plugins/chat/public/components/activity_row.scss
  • src/plugins/chat/public/components/chat_messages.tsx

Sub-PR theme: Add stop functionality and activity filtering

Relevant files:

  • src/plugins/chat/public/components/chat_window.tsx
  • packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts

⚡ Recommended focus areas for review

Race Condition

The handleStop function checks isStreamingRef.current but the ref and state may be out of sync during rapid user interactions. Consider if the guard at line 388 is sufficient to prevent race conditions when stop is called multiple times or during state transitions.

  // Guard: only stop if actively streaming (prevents spurious stop messages during linger window)
  if (!isStreamingRef.current) return;

  // Abort the current streaming request
  chatService.abort();

  // Remove loading message if it exists
  if (loadingMessageIdRef.current) {
    setTimeline((prev) => prev.filter((msg) => msg.id !== loadingMessageIdRef.current));
    loadingMessageIdRef.current = null;
  }

  // Unsubscribe from current observable if exists
  if (currentSubscriptionRef.current) {
    currentSubscriptionRef.current.unsubscribe();
    currentSubscriptionRef.current = null;
  }

  // Update streaming state (both ref and state for React 18 compatibility)
  isStreamingRef.current = false;
  setIsStreaming(false);

  // Add cancellation feedback message
  // Use 'activity' role (AG-UI standard)
  const cancelMessage: Message = {
    id: `cancelled-${Date.now()}`,
    role: 'activity',
    activityType: ActivityType.STOP,
    content: {
      message: 'Execution stopped by user',
    },
  };
  setTimeline((prev) => [...prev, cancelMessage]);

  // 6. Clear event handler state
  eventHandler.clearState();
}, [chatService, eventHandler]);
Resource Leak

The abortController is created but the subscription to request.events.aborted$ is never cleaned up. If the request completes normally, the subscription may remain active. Consider unsubscribing after the fetch completes or errors.

const abortController = new AbortController();

// Listen for client disconnect event
request.events.aborted$.subscribe(() => {
  logger?.debug('Client disconnected, aborting AG-UI request');
  abortController.abort();
});
Incomplete Filter

The filter only removes 'activity' role messages but doesn't validate if other message types are compatible with Bedrock format. Consider if additional validation is needed for message content structure before sending to Bedrock.

// Filter out 'activity' role messages (AG-UI standard) - these are for UI display only
if (msg.role === 'activity') {
  return false;
}

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent memory leak from unsubscribed observable

The subscription to request.events.aborted$ is never cleaned up, which can lead to
memory leaks if the request completes normally before the client disconnects. Store
the subscription and unsubscribe after the AG-UI response completes or when the
abort signal fires.

src/plugins/chat/server/routes/index.ts [37-41]

 // Listen for client disconnect event
-request.events.aborted$.subscribe(() => {
+const abortSubscription = request.events.aborted$.subscribe(() => {
   logger?.debug('Client disconnected, aborting AG-UI request');
   abortController.abort();
 });
 
+// Clean up subscription after request completes
+try {
+  // ... existing fetch logic ...
+} finally {
+  abortSubscription.unsubscribe();
+}
+
Suggestion importance[1-10]: 8

__

Why: The subscription to request.events.aborted$ is never cleaned up, which can lead to memory leaks if the request completes normally. This is a significant issue that should be addressed to prevent resource leaks in production.

Medium
General
Remove unnecessary dependency from useCallback

The handleStop callback depends on eventHandler but only calls
eventHandler.clearState() at the end. If eventHandler reference changes between
renders, the callback will be recreated unnecessarily. Consider removing
eventHandler from dependencies or memoizing it properly.

src/plugins/chat/public/components/chat_window.tsx [386-423]

 const handleStop = useCallback(() => {
   // Guard: only stop if actively streaming (prevents spurious stop messages during linger window)
   if (!isStreamingRef.current) return;
 
   // Abort the current streaming request
   chatService.abort();
   ...
-}, [chatService, eventHandler]);
+  // 6. Clear event handler state
+  eventHandler.clearState();
+}, [chatService]);
Suggestion importance[1-10]: 5

__

Why: The eventHandler dependency in useCallback may cause unnecessary re-renders. However, the impact is moderate as it only affects performance optimization rather than correctness. The suggestion correctly identifies a potential optimization.

Low
Add null safety check for message role

The filter checks msg.role === 'activity' without verifying that msg.role exists. If
a message object lacks a role property, this could cause unexpected behavior. Add a
null/undefined check for msg.role before comparing it.

packages/osd-agents/src/agents/langgraph/react_graph_nodes.ts [453-461]

 .filter((msg) => {
   // Filter out 'activity' role messages (AG-UI standard) - these are for UI display only
-  if (msg.role === 'activity') {
+  if (msg?.role === 'activity') {
     return false;
   }
   // Keep all messages that have content (including empty arrays for assistant)
   // Bedrock needs to see the full conversation flow including tool use/result pairs
   if (msg.content === undefined || msg.content === null) {
     return false;
   }
Suggestion importance[1-10]: 3

__

Why: While adding optional chaining (msg?.role) is a defensive programming practice, the context suggests messages are already validated through TypeScript types. The suggestion is valid but has low impact since the code likely already ensures msg has a role property.

Low

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