Show warning for toggling tracing button when running integration#2110
Open
dan-niles wants to merge 5 commits intowso2:release/bi-1.8.xfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a tracing-status notification path and propagation: extension emits TraceStatus notifications; RPC layer forwards them to webviews; integration-runner state tracks runs and can restart integrations to apply tracing changes; webviews subscribe to live updates and adjust UI state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Extension as Extension\n(Tracing)
participant StateMgr as Integration\nRunner State
participant RPC as RPCLayer
participant Webview as Webviews\n(Chat & Visualizer)
User->>Extension: Toggle tracing (enable/disable)
Extension->>Extension: update TracerMachine state
Extension->>RPC: sendTracingStatusChangedNotification(status)
RPC->>Webview: tracingStatusChanged notification (TraceStatus)
Webview->>Webview: onTracingStatusChanged -> update UI state
alt Integration running
Extension->>StateMgr: isIntegrationRunning()
StateMgr->>Extension: true
Extension->>User: prompt "Restart Integration?"
alt User accepts
Extension->>StateMgr: restartIntegration()
StateMgr->>StateMgr: terminate terminal/debug session
StateMgr->>Extension: restart trace server and relaunch run/debug
Extension->>Webview: (receives tracing update)
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
dan-niles
force-pushed
the
show-warning-for-tracing-button-when-running-integration
branch
from
53c820c to
f4b2eb7
Compare
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds tracing toggle UX improvements across the Ballerina Integrator extension + webviews, ensuring tracing state stays in sync between the diagram UI and command palette, and prompting users to restart a running integration when tracing is toggled.
Changes:
- Push tracing status updates from the extension to relevant webviews via a new
tracingStatusChangednotification. - Track integration run/debug lifecycle so the extension can offer a “Restart Integration” action when tracing is toggled mid-run.
- Improve trace/diagram UI behavior (message expansion behavior in span details, and title bar skeleton display logic).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/ballerina/trace-visualizer/src/components/SpanDetails.tsx | Adjusts message expansion logic based on message section type. |
| workspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsx | Listens for pushed tracing-status changes and refines title bar skeleton rendering. |
| workspaces/ballerina/ballerina-visualizer/src/views/AgentChatPanel/Components/ChatInterface.tsx | Listens for pushed tracing-status changes to keep toggle state in sync. |
| workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts | Exposes onTracingStatusChanged notification subscription. |
| workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts | Updates server stop behavior to terminate the trace-server task execution and await task end. |
| workspaces/ballerina/ballerina-extension/src/features/tracing/trace-server-task.ts | Minor formatting cleanup in pseudoterminal shutdown flow. |
| workspaces/ballerina/ballerina-extension/src/features/tracing/activate.ts | Sends tracing status change notifications; adds restart warning flow when toggling tracing during a run. |
| workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts | New module to track run/debug lifecycle and implement “restart integration” behavior. |
| workspaces/ballerina/ballerina-extension/src/features/project/cmds/cmd-runner.ts | Marks run-terminal runs as started so restart logic can detect “integration running”. |
| workspaces/ballerina/ballerina-extension/src/features/project/activator.ts | Activates integration runner state tracking during project feature activation. |
| workspaces/ballerina/ballerina-extension/src/RPCLayer.ts | Adds sendTracingStatusChangedNotification to broadcast tracing status to webviews. |
| workspaces/ballerina/ballerina-core/src/rpc-types/agent-chat/rpc-type.ts | Adds tracingStatusChanged notification type to core RPC types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts (1)
106-108: Expose a cleanup handle for the new tracing-status subscription.
ChatInterface.tsxandDiagramWrapper/index.tsxboth register this listener fromuseEffect(), but this wrapper returnsvoid, so neither caller can unsubscribe on unmount or whenrpcClientchanges. That will stack duplicate handlers across remounts and replay the same state update multiple times. Please return the underlying disposable/unsubscribe mechanism here and use it in both effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts` around lines 106 - 108, The onTracingStatusChanged wrapper currently returns void and must return the underlying unsubscribe/disposable from this._messenger.onNotification so callers can remove the listener; change onTracingStatusChanged to return the disposable (or an unsubscribe function) produced by this._messenger.onNotification(tracingStatusChanged, handler) instead of void, keeping the same types (TraceStatus) so callers like ChatInterface.tsx and DiagramWrapper/index.tsx can call the returned unsubscribe in their useEffect cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/ballerina/ballerina-extension/src/features/project/cmds/cmd-runner.ts`:
- Around line 181-189: The hook currently only stores terminal and filePath via
markTerminalRunStarted, so restarts replay as PALETTE_COMMANDS.RUN with only
lastRunPath and lose flags/args; update markTerminalRunStarted (and any storage)
to persist the full run invocation (e.g., command enum plus args/flags or the
exact invocation string) when isRunCommand detects
RUN/RUN_WITH_WATCH/RUN_WITH_EXPERIMENTAL, ensure callers such as
runCommandWithConf pass those args into markTerminalRunStarted, and update
restartIntegration to replay using that saved invocation (not just lastRunPath)
so restarts restore run --watch, run --experimental, config-file runs, and extra
args.
In
`@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts`:
- Around line 38-59: The restartIntegration function unconditionally calls
TracerMachine.startServer() (both in the debug and non-debug paths) which
restarts tracing even when tracing was just disabled; change restartIntegration
to accept or check the current tracing-enabled state (passed from
notifyTracingToggle or a similar caller) and only call
TracerMachine.startServer() when tracing is enabled; update the calls to
restartIntegration (where notifyTracingToggle invokes it) to pass the toggle
state, and remove or gate the two TracerMachine.startServer() invocations so
terminateTraceServerTask() and TracerMachine.startServer() are only paired when
tracing should be restarted.
In `@workspaces/ballerina/ballerina-extension/src/features/tracing/activate.ts`:
- Line 120: The restart prompt is not scoped to the integration that was toggled
because notifyTracingToggle() is called without the targetPath; update
notifyTracingToggle to accept a targetPath (or integration identifier) and use
it to compare against lastRunPath / active debug session before offering a
restart, then propagate the new parameter to all callers (the places currently
calling notifyTracingToggle() around the tracing toggle and the block handling
restarts). Ensure the logic in notifyTracingToggle checks equality with
lastRunPath and any active debug session identifiers so the restart prompt only
appears for the integration that was actually changed.
In
`@workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts`:
- Around line 133-145: stopServer() currently terminates the TaskExecution
(context.taskExecution.terminate()) while TraceServerPseudoterminal may still be
starting (TraceServerPseudoterminal.serverStarted === false), which can leave
the OTLP server running; change stopServer() to check the pseudoterminal/server
start state (TraceServerPseudoterminal.serverStarted or an existing start
promise) and either wait for that startup to complete before calling
taskExecution.terminate(), or if startup is pending call TraceServer.stop()
directly and skip/avoid relying on taskExecution.terminate(); locate the logic
around stopServer(), context.taskExecution,
TraceServerPseudoterminal.serverStarted, TraceServer.stop() and ensure
termination always triggers TraceServer.stop() (or waits for serverStarted) so
the server cannot be left running without a task.
---
Nitpick comments:
In
`@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts`:
- Around line 106-108: The onTracingStatusChanged wrapper currently returns void
and must return the underlying unsubscribe/disposable from
this._messenger.onNotification so callers can remove the listener; change
onTracingStatusChanged to return the disposable (or an unsubscribe function)
produced by this._messenger.onNotification(tracingStatusChanged, handler)
instead of void, keeping the same types (TraceStatus) so callers like
ChatInterface.tsx and DiagramWrapper/index.tsx can call the returned unsubscribe
in their useEffect cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9436b470-2404-4376-95e1-0b201519d538
📒 Files selected for processing (12)
workspaces/ballerina/ballerina-core/src/rpc-types/agent-chat/rpc-type.tsworkspaces/ballerina/ballerina-extension/src/RPCLayer.tsworkspaces/ballerina/ballerina-extension/src/features/project/activator.tsworkspaces/ballerina/ballerina-extension/src/features/project/cmds/cmd-runner.tsworkspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/activate.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/trace-server-task.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.tsworkspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.tsworkspaces/ballerina/ballerina-visualizer/src/views/AgentChatPanel/Components/ChatInterface.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsxworkspaces/ballerina/trace-visualizer/src/components/SpanDetails.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts`:
- Around line 40-45: The current isIntegrationRunningAt() uses strict equality
between path.resolve(lastRunPath) and path.resolve(targetPath), which misses
cases where lastRunPath is a file inside targetPath; change the comparison to
containment: resolve both paths (using path.resolve) and return true if
resolvedLastRunPath equals resolvedTargetPath or resolvedLastRunPath is inside
resolvedTargetPath (e.g. check that
resolvedLastRunPath.startsWith(resolvedTargetPath + path.sep) or use
path.relative and ensure it does not start with '..'), keeping the early return
when isIntegrationRunning() or lastRunPath are falsy and referencing the
existing symbols isIntegrationRunning, lastRunPath, isIntegrationRunningAt, and
path.resolve.
In
`@workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts`:
- Around line 137-145: The Promise returned in stopServer() can remain pending
if a task ends between the earlier includes(...) check and the onDidEndTask
listener; add a fallback settle path: introduce a local "settled" flag and a
short timeout (e.g. 1–2s) that, if triggered, disposes the subscription and
calls TraceServer.stop().then(resolve, reject) (and sets settled), and in the
existing onDidEndTask handler check/flip the same settled flag so only one path
resolves/rejects; ensure taskExecution.terminate() still runs and that
subscription.dispose() is called in both paths to avoid leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbc42a9e-332b-4c6e-b943-46c5a0fce457
📒 Files selected for processing (3)
workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/activate.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts
Comment on lines
+40
to
+45
| export function isIntegrationRunningAt(targetPath: string): boolean { | ||
| if (!isIntegrationRunning() || !lastRunPath) { | ||
| return false; | ||
| } | ||
| return path.resolve(lastRunPath) === path.resolve(targetPath); | ||
| } |
Contributor
There was a problem hiding this comment.
Use containment matching instead of strict path equality in isIntegrationRunningAt().
On Line 44, exact equality can miss active debug runs where lastRunPath is a script file inside targetPath. That skips the restart warning for the integration that is actually running.
Proposed fix
export function isIntegrationRunningAt(targetPath: string): boolean {
if (!isIntegrationRunning() || !lastRunPath) {
return false;
}
- return path.resolve(lastRunPath) === path.resolve(targetPath);
+ const runningPath = path.resolve(lastRunPath);
+ const selectedPath = path.resolve(targetPath);
+ const relative = path.relative(selectedPath, runningPath);
+ return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative));
}📝 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
| export function isIntegrationRunningAt(targetPath: string): boolean { | |
| if (!isIntegrationRunning() || !lastRunPath) { | |
| return false; | |
| } | |
| return path.resolve(lastRunPath) === path.resolve(targetPath); | |
| } | |
| export function isIntegrationRunningAt(targetPath: string): boolean { | |
| if (!isIntegrationRunning() || !lastRunPath) { | |
| return false; | |
| } | |
| const runningPath = path.resolve(lastRunPath); | |
| const selectedPath = path.resolve(targetPath); | |
| const relative = path.relative(selectedPath, runningPath); | |
| return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts`
around lines 40 - 45, The current isIntegrationRunningAt() uses strict equality
between path.resolve(lastRunPath) and path.resolve(targetPath), which misses
cases where lastRunPath is a file inside targetPath; change the comparison to
containment: resolve both paths (using path.resolve) and return true if
resolvedLastRunPath equals resolvedTargetPath or resolvedLastRunPath is inside
resolvedTargetPath (e.g. check that
resolvedLastRunPath.startsWith(resolvedTargetPath + path.sep) or use
path.relative and ensure it does not start with '..'), keeping the early return
when isIntegrationRunning() or lastRunPath are falsy and referencing the
existing symbols isIntegrationRunning, lastRunPath, isIntegrationRunningAt, and
path.resolve.
Comment on lines
+137
to
+145
| return new Promise<void>((resolve, reject) => { | ||
| const subscription = vscode.tasks.onDidEndTask((endEvent) => { | ||
| if (endEvent.execution === taskExecution) { | ||
| subscription.dispose(); | ||
| TraceServer.stop().then(resolve, reject); | ||
| } | ||
| }); | ||
| taskExecution.terminate(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add a fallback for task-end race in stopServer().
If the task ends after the includes(...) check but before the listener effectively catches the event, this Promise can stay pending. A fallback settle path avoids stuck state transitions.
Proposed fix
- return new Promise<void>((resolve, reject) => {
+ return new Promise<void>((resolve, reject) => {
+ let settled = false;
+ const finish = (err?: unknown) => {
+ if (settled) {
+ return;
+ }
+ settled = true;
+ subscription.dispose();
+ if (err) {
+ reject(err);
+ } else {
+ resolve();
+ }
+ };
const subscription = vscode.tasks.onDidEndTask((endEvent) => {
if (endEvent.execution === taskExecution) {
- subscription.dispose();
- TraceServer.stop().then(resolve, reject);
+ TraceServer.stop().then(() => finish(), finish);
}
});
taskExecution.terminate();
+ // Fallback: task may have already ended around listener setup.
+ if (!vscode.tasks.taskExecutions.includes(taskExecution)) {
+ TraceServer.stop().then(() => finish(), finish);
+ }
});📝 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
| return new Promise<void>((resolve, reject) => { | |
| const subscription = vscode.tasks.onDidEndTask((endEvent) => { | |
| if (endEvent.execution === taskExecution) { | |
| subscription.dispose(); | |
| TraceServer.stop().then(resolve, reject); | |
| } | |
| }); | |
| taskExecution.terminate(); | |
| }); | |
| return new Promise<void>((resolve, reject) => { | |
| let settled = false; | |
| const finish = (err?: unknown) => { | |
| if (settled) { | |
| return; | |
| } | |
| settled = true; | |
| subscription.dispose(); | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }; | |
| const subscription = vscode.tasks.onDidEndTask((endEvent) => { | |
| if (endEvent.execution === taskExecution) { | |
| TraceServer.stop().then(() => finish(), finish); | |
| } | |
| }); | |
| taskExecution.terminate(); | |
| // Fallback: task may have already ended around listener setup. | |
| if (!vscode.tasks.taskExecutions.includes(taskExecution)) { | |
| TraceServer.stop().then(() => finish(), finish); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts`
around lines 137 - 145, The Promise returned in stopServer() can remain pending
if a task ends between the earlier includes(...) check and the onDidEndTask
listener; add a fallback settle path: introduce a local "settled" flag and a
short timeout (e.g. 1–2s) that, if triggered, disposes the subscription and
calls TraceServer.stop().then(resolve, reject) (and sets settled), and in the
existing onDidEndTask handler check/flip the same settled flag so only one path
resolves/rejects; ensure taskExecution.terminate() still runs and that
subscription.dispose() is called in both paths to avoid leaks.
…ervice is already running
dan-niles
force-pushed
the
show-warning-for-tracing-button-when-running-integration
branch
from
7769d48 to
b78309b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts (1)
40-45:⚠️ Potential issue | 🟠 MajorUse containment matching in
isIntegrationRunningAt()instead of strict equality.Line [44] can return false when the running path is a file within
targetPath, which suppresses the restart warning for the actually-running integration.Suggested fix
export function isIntegrationRunningAt(targetPath: string): boolean { if (!isIntegrationRunning() || !lastRunPath) { return false; } - return path.resolve(lastRunPath) === path.resolve(targetPath); + const runningPath = path.resolve(lastRunPath); + const selectedPath = path.resolve(targetPath); + const relative = path.relative(selectedPath, runningPath); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts` around lines 40 - 45, The function isIntegrationRunningAt currently checks strict equality between path.resolve(lastRunPath) and path.resolve(targetPath), which fails when lastRunPath is a file inside targetPath; update isIntegrationRunningAt (which calls isIntegrationRunning and reads lastRunPath) to perform containment matching: resolve both paths, normalize/trailing-separator them, and return true if resolvedLastRunPath === resolvedTargetPath or resolvedLastRunPath startsWith(resolvedTargetPath + path.sep) (or use path.relative to check that the relative path does not start with '..'), so a running integration inside targetPath is considered a match.workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts (1)
137-145:⚠️ Potential issue | 🟠 MajorPrevent
stopServer()from hanging on task-end race windows.At Line [137]-Line [145], resolution depends entirely on
onDidEndTask. If the task ends around listener setup/check timing, the Promise can stay pending and block the machine inserverStopping.Suggested fix
function stopServer(context: TracerMachineContext, event?: any): Promise<void> { const taskExecution = context.taskExecution; if (!taskExecution || !vscode.tasks.taskExecutions.includes(taskExecution)) { return TraceServer.stop(); } return new Promise<void>((resolve, reject) => { + let settled = false; + let timeout: NodeJS.Timeout | undefined; + const finish = (err?: unknown) => { + if (settled) { + return; + } + settled = true; + subscription.dispose(); + if (timeout) { + clearTimeout(timeout); + } + if (err) { + reject(err); + } else { + resolve(); + } + }; const subscription = vscode.tasks.onDidEndTask((endEvent) => { if (endEvent.execution === taskExecution) { - subscription.dispose(); - TraceServer.stop().then(resolve, reject); + TraceServer.stop().then(() => finish(), finish); } }); taskExecution.terminate(); + // Fallback if task already ended around listener registration. + if (!vscode.tasks.taskExecutions.includes(taskExecution)) { + TraceServer.stop().then(() => finish(), finish); + return; + } + timeout = setTimeout(() => { + TraceServer.stop().then(() => finish(), finish); + }, 1500); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts` around lines 137 - 145, The Promise relies solely on the onDidEndTask listener and can hang if the task already ended; after creating the subscription to vscode.tasks.onDidEndTask (and before calling taskExecution.terminate()), also check whether the execution is already absent from vscode.tasks.taskExecutions and, if so, dispose the subscription and call TraceServer.stop() immediately (wiring resolve/reject the same way); ensure subscription.dispose() is called in both the listener path and the immediate-path and that TraceServer.stop() errors propagate to reject. Reference: onDidEndTask, taskExecution, vscode.tasks.taskExecutions, subscription.dispose(), and TraceServer.stop().
🧹 Nitpick comments (1)
workspaces/ballerina/trace-visualizer/src/components/SpanDetails.tsx (1)
881-881: DecoupleexpandLastOnlyfrom display label textUsing
messagesLabel === 'Messages'to drive behavior is fragile. If UI text changes (e.g., copy updates/localization), expansion behavior can silently change. Prefer an explicit boolean derived from payload type (isToolPayload/isMessagePayload) and bindexpandLastOnlyto that.♻️ Suggested refactor
// inputData - return { + const isToolPayload = Boolean(toolArguments); + return { systemInstructions, messages: inputMessages || toolArguments, - messagesLabel: toolArguments ? 'Tool Arguments' : (operationName?.includes('invoke_agent') ? 'User' : 'Messages'), + messagesLabel: isToolPayload ? 'Tool Arguments' : (operationName?.includes('invoke_agent') ? 'User' : 'Messages'), + isToolPayload, tools: inputTools, inputContent, kbIngestChunks, kbRetrieveQuery }; // outputData - return { + const isToolPayload = Boolean(toolOutput); + return { messages: outputMessages || toolOutput, - messagesLabel: toolOutput ? 'Tool Output' : 'Messages', + messagesLabel: isToolPayload ? 'Tool Output' : 'Messages', + isToolPayload, error: errorMessage, kbRetrieveOutput }; // JsonViewer usage - expandLastOnly={inputData.messagesLabel === 'Messages'} + expandLastOnly={!inputData.isToolPayload} ... - expandLastOnly={outputData.messagesLabel === 'Messages'} + expandLastOnly={!outputData.isToolPayload}Also applies to: 973-973
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/trace-visualizer/src/components/SpanDetails.tsx` at line 881, The prop expandLastOnly is currently driven by UI text (inputData.messagesLabel === 'Messages'), which is fragile; change the logic to use an explicit boolean derived from the payload type (e.g., compute or use inputData.isMessagePayload or isToolPayload) and pass that boolean into expandLastOnly instead of comparing the label string, updating both occurrences (the one using expandLastOnly around inputData.messagesLabel and the other at the later occurrence) so behavior is based on payload type, not display text; locate usages of expandLastOnly and inputData.messagesLabel in SpanDetails.tsx and replace the string comparison with the new boolean property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts`:
- Around line 84-92: The current await in terminateTraceServerTask() waits only
for tasks.onDidEndTask(event) tied to traceTask, which can hang if the task ends
before the listener is registered; update terminateTraceServerTask() to race the
onDidEndTask subscription against a fallback settle path: either check the
current state of traceTask/execution (if available) immediately after calling
traceTask.terminate() and resolve if it's already finished, or add a short
timeout (e.g., 2–5s) that disposes the subscription and resolves to prevent
indefinite waiting; ensure the subscription is always disposed and resolve() is
only called once so restartIntegration() cannot hang.
In
`@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts`:
- Around line 106-108: Change the signature of onTracingStatusChanged to return
a Disposable (not void) and return the Disposable produced by
this._messenger.onNotification so callers can unsubscribe; specifically update
the method declaration onTracingStatusChanged(handler: (status: TraceStatus) =>
void): Disposable and change its body to return
this._messenger.onNotification(tracingStatusChanged, handler) so React effects
can call the disposable in their cleanup.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsx`:
- Line 126: The title bar state variable hasLoadedTitleBar is only ever set
true, leaving header action buttons (Configure/Chat) wired to stale
parentMetadata/currentPosition during subsequent diagram loads; update
DiagramWrapper so you reset setHasLoadedTitleBar(false) when a new diagram
starts loading (e.g., on diagram id/parentMetadata change or at load start),
only setHasLoadedTitleBar(true) from onReady, and pass a prop to the TitleBar or
disable its Configure/Chat handlers when hasLoadedTitleBar is false so the
header remains visible but inert until onReady updates parentMetadata and
currentPosition.
---
Duplicate comments:
In
`@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts`:
- Around line 40-45: The function isIntegrationRunningAt currently checks strict
equality between path.resolve(lastRunPath) and path.resolve(targetPath), which
fails when lastRunPath is a file inside targetPath; update
isIntegrationRunningAt (which calls isIntegrationRunning and reads lastRunPath)
to perform containment matching: resolve both paths,
normalize/trailing-separator them, and return true if resolvedLastRunPath ===
resolvedTargetPath or resolvedLastRunPath startsWith(resolvedTargetPath +
path.sep) (or use path.relative to check that the relative path does not start
with '..'), so a running integration inside targetPath is considered a match.
In
`@workspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.ts`:
- Around line 137-145: The Promise relies solely on the onDidEndTask listener
and can hang if the task already ended; after creating the subscription to
vscode.tasks.onDidEndTask (and before calling taskExecution.terminate()), also
check whether the execution is already absent from vscode.tasks.taskExecutions
and, if so, dispose the subscription and call TraceServer.stop() immediately
(wiring resolve/reject the same way); ensure subscription.dispose() is called in
both the listener path and the immediate-path and that TraceServer.stop() errors
propagate to reject. Reference: onDidEndTask, taskExecution,
vscode.tasks.taskExecutions, subscription.dispose(), and TraceServer.stop().
---
Nitpick comments:
In `@workspaces/ballerina/trace-visualizer/src/components/SpanDetails.tsx`:
- Line 881: The prop expandLastOnly is currently driven by UI text
(inputData.messagesLabel === 'Messages'), which is fragile; change the logic to
use an explicit boolean derived from the payload type (e.g., compute or use
inputData.isMessagePayload or isToolPayload) and pass that boolean into
expandLastOnly instead of comparing the label string, updating both occurrences
(the one using expandLastOnly around inputData.messagesLabel and the other at
the later occurrence) so behavior is based on payload type, not display text;
locate usages of expandLastOnly and inputData.messagesLabel in SpanDetails.tsx
and replace the string comparison with the new boolean property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a156c45-bcc9-4109-a633-fa44161d548c
📒 Files selected for processing (12)
workspaces/ballerina/ballerina-core/src/rpc-types/agent-chat/rpc-type.tsworkspaces/ballerina/ballerina-extension/src/RPCLayer.tsworkspaces/ballerina/ballerina-extension/src/features/project/activator.tsworkspaces/ballerina/ballerina-extension/src/features/project/cmds/cmd-runner.tsworkspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/activate.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/trace-server-task.tsworkspaces/ballerina/ballerina-extension/src/features/tracing/tracer-machine.tsworkspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.tsworkspaces/ballerina/ballerina-visualizer/src/views/AgentChatPanel/Components/ChatInterface.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsxworkspaces/ballerina/trace-visualizer/src/components/SpanDetails.tsx
✅ Files skipped from review due to trivial changes (3)
- workspaces/ballerina/ballerina-extension/src/features/tracing/trace-server-task.ts
- workspaces/ballerina/ballerina-core/src/rpc-types/agent-chat/rpc-type.ts
- workspaces/ballerina/ballerina-extension/src/features/tracing/activate.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/ballerina/ballerina-extension/src/features/project/activator.ts
Comment on lines
+84
to
+92
| await new Promise<void>((resolve) => { | ||
| const subscription = tasks.onDidEndTask((event) => { | ||
| if (event.execution === traceTask) { | ||
| subscription.dispose(); | ||
| resolve(); | ||
| } | ||
| }); | ||
| traceTask.terminate(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add a fallback settle path in terminateTraceServerTask() to avoid stuck awaits.
At Line [84]-Line [92], the Promise resolves only via onDidEndTask. If the task ends around registration, restartIntegration() can wait indefinitely.
Suggested fix
async function terminateTraceServerTask(): Promise<void> {
const traceTask = tasks.taskExecutions.find(
(execution) => execution.task.source === TRACE_SERVER_TASK_SOURCE
&& execution.task.name === TRACE_SERVER_TASK_NAME
);
if (!traceTask) {
return;
}
await new Promise<void>((resolve) => {
+ let settled = false;
+ let timeout: NodeJS.Timeout | undefined;
+ const finish = () => {
+ if (settled) {
+ return;
+ }
+ settled = true;
+ subscription.dispose();
+ if (timeout) {
+ clearTimeout(timeout);
+ }
+ resolve();
+ };
const subscription = tasks.onDidEndTask((event) => {
if (event.execution === traceTask) {
- subscription.dispose();
- resolve();
+ finish();
}
});
traceTask.terminate();
+ if (!tasks.taskExecutions.includes(traceTask)) {
+ finish();
+ return;
+ }
+ timeout = setTimeout(finish, 1500);
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-extension/src/features/project/integration-runner-state.ts`
around lines 84 - 92, The current await in terminateTraceServerTask() waits only
for tasks.onDidEndTask(event) tied to traceTask, which can hang if the task ends
before the listener is registered; update terminateTraceServerTask() to race the
onDidEndTask subscription against a fallback settle path: either check the
current state of traceTask/execution (if available) immediately after calling
traceTask.terminate() and resolve if it's already finished, or add a short
timeout (e.g., 2–5s) that disposes the subscription and resolves to prevent
indefinite waiting; ensure the subscription is always disposed and resolve() is
only called once so restartIntegration() cannot hang.
Comment on lines
+106
to
+108
| onTracingStatusChanged(handler: (status: TraceStatus) => void): void { | ||
| this._messenger.onNotification(tracingStatusChanged, handler); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AgentChatRpcClient listener API:"
sed -n '100,110p' workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts
echo
echo "ChatInterface consumer:"
sed -n '580,588p' workspaces/ballerina/ballerina-visualizer/src/views/AgentChatPanel/Components/ChatInterface.tsx
echo
echo "DiagramWrapper consumer:"
sed -n '230,238p' workspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsxRepository: wso2/vscode-extensions
Length of output: 1201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check Messenger type and onNotification signature ==="
rg "class Messenger|interface Messenger|onNotification" workspaces/ -A 2 -B 1 | head -80
echo
echo "=== All listener methods in rpc-client.ts ==="
rg "on[A-Z]\w+\(" workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts -A 2
echo
echo "=== Full useEffect hooks in ChatInterface.tsx around lines 580-590 ==="
sed -n '580,595p' workspaces/ballerina/ballerina-visualizer/src/views/AgentChatPanel/Components/ChatInterface.tsx
echo
echo "=== Full useEffect hooks in DiagramWrapper/index.tsx around lines 230-245 ==="
sed -n '230,250p' workspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsxRepository: wso2/vscode-extensions
Length of output: 11474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for Messenger type definition (likely from vscode-messenger-webview) ==="
rg "interface Messenger|class Messenger|type Messenger" workspaces/ -A 10 | head -60
echo
echo "=== Look for vscode-messenger-webview or messenger type imports ==="
rg "from.*messenger|import.*Messenger" workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts -B 2 -A 2
echo
echo "=== Check messenger instance setup in rpc-client.ts ==="
head -30 workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts
echo
echo "=== Look for onNotification type signature in package.json or elsewhere ==="
find workspaces -name "package.json" -exec grep -l "vscode-messenger\|messenger" {} \; 2>/dev/null | head -5
</parameter>Repository: wso2/vscode-extensions
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find messenger imports in rpc-client.ts ==="
head -40 workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts
echo
echo "=== Search for vscode-messenger in package.json files ==="
find workspaces/ballerina -name "package.json" -exec grep -l "vscode-messenger\|messenger-webview" {} \;
echo
echo "=== Show package.json from ballerina-rpc-client ==="
cat workspaces/ballerina/ballerina-rpc-client/package.json | grep -A 5 -B 5 "messenger\|dependencies"Repository: wso2/vscode-extensions
Length of output: 2203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if onNotification return value is ever captured in codebase ==="
rg "const .* = .*\.onNotification|let .* = .*\.onNotification|= .*\.onNotification\(" workspaces/ -A 2
echo
echo "=== Look for Disposable or unsubscribe patterns in useEffect ==="
rg "useEffect.*=>.*{" workspaces/ballerina/ballerina-visualizer/src/views/ -A 10 | grep -E "return|Disposable|unsubscribe|removeListener" | head -20
echo
echo "=== Check how Messenger is instantiated in rpc-client ==="
sed -n '30,80p' workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts
echo
echo "=== Search for any return statement in onTracingStatusChanged or similar methods ==="
rg "on[A-Z]\w+.*\(" workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts -A 3 | grep -E "return|Disposable"Repository: wso2/vscode-extensions
Length of output: 1717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for Messenger type in agent-chat rpc-client ==="
sed -n '1,100p' workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts | grep -A 3 "Messenger\|_messenger"
echo
echo "=== Look for Disposable type patterns in vscode-messenger imports ==="
rg "Disposable|import.*Messenger" workspaces/ballerina/ballerina-rpc-client/ -B 2 -A 2
echo
echo "=== Check if vscode-messenger-webview docs or types mention return ==="
find workspaces -name "*messenger*" -type f | head -10
echo
echo "=== Look at full implementation of onTracingStatusChanged and onActiveAgentChanged ==="
sed -n '103,120p' workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts
echo
echo "=== Check if other similar listener wrapper methods return anything ==="
grep -r "onNotification" workspaces/ballerina/ballerina-rpc-client/src/ -B 1 -A 2Repository: wso2/vscode-extensions
Length of output: 28654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check what langClient is (from pull-module-progress.ts) ==="
grep -r "StateMachine.langClient\|langClient\s*=" workspaces/ballerina/ballerina-extension/src/utils/pull-module-progress.ts -B 5 -A 2 | head -20
echo
echo "=== Check if langClient is VSCode's LanguageClient (different from vscode-messenger-webview) ==="
rg "import.*LanguageClient|from.*languageserver" workspaces/ballerina/ballerina-extension/src/ -B 1 -A 1 | head -30
echo
echo "=== Web search vscode-messenger-webview documentation for onNotification return type ==="
# Try to look for type definitions or see if there's a .d.ts file reference
find workspaces -name "*.d.ts" -o -name "vscode-messenger*" 2>/dev/null | head -10
echo
echo "=== Check if Disposable is used anywhere with vscode-messenger-webview ==="
rg "Disposable.*vscode-messenger|vscode-messenger.*Disposable" workspaces/ -B 2 -A 2Repository: wso2/vscode-extensions
Length of output: 4209
🌐 Web query:
vscode-messenger-webview onNotification return type Disposable
💡 Result:
The onNotification method in the vscode-messenger-webview library returns a Disposable object. This disposable allows for automatic cleanup by unregistering the handler when dispose is called. From the source code in Messenger.ts: typescript onNotification (type: NotificationType, handler: NotificationHandler): Disposable { this.handlerRegistry.set(type.method, handler as NotificationHandler); return { dispose: => { this.unregisterHandler(type.method); } }; } Usage example: typescript const notificationDisposable = messenger.onNotification(myNotification, handler); // Later: notificationDisposable.dispose; // Clean up when done This behavior is consistent across the library's documentation and examples, enabling proper resource management in VS Code webviews.
Citations:
- 1: https://cdn.jsdelivr.net/npm/vscode-messenger-webview@0.6.0/src/messenger.ts
- 2: https://github.com/TypeFox/vscode-messenger
- 3: https://www.typefox.io/blog/vs-code-messenger/
Return a disposable from onTracingStatusChanged() to enable cleanup in React effects.
onTracingStatusChanged() wraps Messenger.onNotification(), which returns a Disposable. Currently the method returns void, preventing cleanup. Both React effects in ChatInterface.tsx (lines 582–586) and DiagramWrapper/index.tsx (lines 232–236) subscribe without returning a cleanup function, so component remounts will accumulate handlers and trigger duplicate state updates.
Change the return type from void to Disposable and bubble up the return value so callers can clean up in their effect cleanup functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/agent-chat/rpc-client.ts`
around lines 106 - 108, Change the signature of onTracingStatusChanged to return
a Disposable (not void) and return the Disposable produced by
this._messenger.onNotification so callers can unsubscribe; specifically update
the method declaration onTracingStatusChanged(handler: (status: TraceStatus) =>
void): Disposable and change its body to return
this._messenger.onNotification(tracingStatusChanged, handler) so React effects
can call the disposable in their cleanup.
| const [showSequenceDiagram, setShowSequenceDiagram] = useState(false); | ||
| const [enableSequenceDiagram, setEnableSequenceDiagram] = useState(false); | ||
| const [loadingDiagram, setLoadingDiagram] = useState(true); | ||
| const [hasLoadedTitleBar, setHasLoadedTitleBar] = useState(false); |
Contributor
There was a problem hiding this comment.
Disable stale title-bar actions while the next diagram is still loading.
hasLoadedTitleBar is only ever flipped to true, so after the first successful render the previous header stays interactive during later reloads. That leaves Configure / Chat wired to the old parentMetadata and currentPosition until the next onReady arrives.
💡 One safe way to keep the header visible without exposing stale actions
- <TitleBar title={getTitle()} subtitleElement={getSubtitleElement} actions={getActions()} />
+ <TitleBar
+ title={getTitle()}
+ subtitleElement={getSubtitleElement}
+ actions={loadingDiagram ? null : getActions()}
+ />Also applies to: 279-280, 608-611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/DiagramWrapper/index.tsx`
at line 126, The title bar state variable hasLoadedTitleBar is only ever set
true, leaving header action buttons (Configure/Chat) wired to stale
parentMetadata/currentPosition during subsequent diagram loads; update
DiagramWrapper so you reset setHasLoadedTitleBar(false) when a new diagram
starts loading (e.g., on diagram id/parentMetadata change or at load start),
only setHasLoadedTitleBar(true) from onReady, and pass a prop to the TitleBar or
disable its Configure/Chat handlers when hasLoadedTitleBar is false so the
header remains visible but inert until onReady updates parentMetadata and
currentPosition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes wso2/product-integrator#1136, wso2/product-integrator#1135
This PR includes the following changes:
Screen.Recording.2026-04-27.at.5.30.29.PM.mov
Summary by CodeRabbit
New Features
Improvements