feat(Workspaces): added connecting to workspace using kortex-cli terminal#1222
Open
gastoner wants to merge 3 commits intoopenkaiden:mainfrom
Open
feat(Workspaces): added connecting to workspace using kortex-cli terminal#1222gastoner wants to merge 3 commits intoopenkaiden:mainfrom
gastoner wants to merge 3 commits intoopenkaiden:mainfrom
Conversation
gastoner
requested review from
bmahabirbu and
fbricon
and removed request for
a team
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds interactive terminal support for agent workspaces: main process spawns per-workspace CLI terminals and exposes IPC for lifecycle and I/O; preload bridges terminal APIs to the renderer; renderer adds a terminal component, routes, and persistent terminal store plus tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (AgentWorkspaceTerminal)
participant Preload as Preload Bridge
participant Main as Main Process (AgentWorkspaceManager)
participant CLI as CLI Process (kortex-cli)
Renderer->>Preload: shellInAgentWorkspace(workspaceId, onData,onError,onEnd)
Preload->>Main: ipcInvoke('agent-workspace:terminal', workspaceId, callbackId)
Main->>CLI: spawn('kortex-cli', ['terminal', workspaceId])
Main->>Main: store callbacks & process keyed by callbackId
Main-->>Preload: return callbackId
Preload-->>Renderer: return callbackId
CLI->>Main: stdout/stderr data
Main->>Preload: webContents.send('agent-workspace:terminal-onData', {callbackId, data})
Preload->>Renderer: dispatch onData(callbackId, data)
Renderer->>Renderer: update xterm display
Renderer->>Preload: shellInAgentWorkspaceSend(callbackId, input)
Preload->>Main: ipcInvoke('agent-workspace:terminalSend', callbackId, input)
Main->>CLI: process.stdin.write(input)
Renderer->>Preload: shellInAgentWorkspaceResize(callbackId, w,h)
Preload->>Main: ipcInvoke('agent-workspace:terminalResize', callbackId, w, h)
CLI->>Main: process close
Main->>Preload: webContents.send('agent-workspace:terminal-onEnd', {callbackId})
Preload->>Renderer: dispatch onEnd(callbackId)
Renderer->>Renderer: cleanup or re-register session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. 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 |
![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: 7
🧹 Nitpick comments (2)
packages/main/src/plugin/util/exec.ts (1)
77-89: Extract the Flatpak wrapping into a shared helper.
createProcess()now duplicates the command/args rewrite thatexec()already does a few lines below, and the two branches are already diverging (exec()forwards extra env with--env=..., whilecreateProcess()cannot). Pulling that logic into one helper will keep the terminal path and the regular exec path from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/util/exec.ts` around lines 77 - 89, create a single shared helper (e.g., wrapForFlatpak or prepareCommandForFlatpak) that encapsulates the Flatpak rewrite logic currently duplicated between createProcess() and exec(): accept the original command, args and env, return the finalCommand, finalArgs and adjusted env; move the '--host' wrapping and also the env forwarding behavior (injecting '--env=KEY=VALUE' entries that exec() uses) into this helper so both createProcess() and exec() call it and then spawn/exec the returned finalCommand/finalArgs with the returned env; update references in createProcess() and exec() to use the helper and remove duplicated rewrite code so terminal and regular exec paths stay consistent.packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
358-375: Assertkill()in thedispose()test.This currently passes even if
dispose()stops terminating tracked terminals, so the new cleanup path is effectively untested.✅ Tighten the assertion
await manager.shellInAgentWorkspace('ws-1', vi.fn(), vi.fn(), vi.fn()); manager.dispose(); - expect(true).toBe(true); + expect(mockProcess.kill).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts` around lines 358 - 375, The test for dispose() doesn't assert that tracked terminal processes are terminated; update the test that uses shellInAgentWorkspace and the mocked process (returned by exec.createProcess) to assert that mockProcess.kill was called when manager.dispose() is invoked (e.g., replace the placeholder expect(true).toBe(true) with an assertion like expect(mockProcess.kill).toHaveBeenCalled()), ensuring the mocked ChildProcessWithoutNullStreams instance created in the test is the same one tracked and killed by dispose().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Around line 256-260: The mock parameter types in createMockProcess are using
the factory type `typeof vi.fn` instead of the mock instance type; update the
parameter types for stdoutOn, stderrOn, and processOn to `ReturnType<typeof
vi.fn>` and also change any casts that use `as typeof vi.fn` (the casts at the
spots corresponding to lines with the mocks referenced in the diff: the casts
around the stdoutOn, stderrOn, and processOn usages) to `as ReturnType<typeof
vi.fn>` so the function signature and all mock usages (createMockProcess and its
mocked stdoutOn/stderrOn/processOn instances) use the correct mock instance
type.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 174-215: Add an IPC handler to close individual terminal sessions:
implement a new ipcHandle (e.g. 'agent-workspace:terminalClose') that takes
onDataId, looks up this.terminalProcesses and this.terminalCallbacks, calls an
orderly shutdown on the stored process (invocation.process.kill()/terminate or
invocation.process.close() depending on how shellInAgentWorkspace exposes it),
removes entries from this.terminalCallbacks and this.terminalProcesses, and
emits the same 'agent-workspace:terminal-onEnd' notification to the renderer;
update shellInAgentWorkspace usage sites if needed to expose a safe close
method. Also call this new IPC from AgentWorkspaceTerminal.svelte in onDestroy
before clearing sendCallbackId so the child process is not leaked.
In `@packages/preload/src/index.ts`:
- Around line 346-394: The bridge currently registers global ipcRenderer
handlers and only deletes entries from onDataCallbacksShellInAgentWorkspace on
'agent-workspace:terminal-onEnd', which leaks closures if sessions fail or are
rebound; add an explicit dispose/unbind API and per-callback listener cleanup:
expose a new method (e.g. 'shellInAgentWorkspaceDispose' or
'shellInAgentWorkspaceUnbind') that takes the callbackId, calls ipcInvoke to
tell the main process to close the terminal session, deletes the entry from
onDataCallbacksShellInAgentWorkspace, and removes any per-callback ipcRenderer
listeners; to support removal, change the global ipcRenderer.on usage for
'agent-workspace:terminal-onData' / '-onError' / '-onEnd' to either store and
register per-callback bound listener functions (so you can call
ipcRenderer.removeListener with those functions when disposing) or wrap handlers
to no-op when the callbackId is missing; update usages tied to
onDataCallbacksShellInAgentWorkspaceId, onDataCallbacksShellInAgentWorkspace,
and the exposed 'shellInAgentWorkspace' so the new dispose/unbind flow is
invoked on unmount or on invoke failure.
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.ts`:
- Around line 57-80: AgentWorkspaceTerminal.svelte currently derives isRunning
from agentWorkspaceStatuses.get(workspaceId) and early-returns before calling
shellInAgentWorkspace, which blocks manual/external starts; change the logic in
AgentWorkspaceTerminal.svelte to not exclusively gate shell initialization on
agentWorkspaceStatuses.get — add a fallback health check or external-status sync
(e.g., call a new checkWorkspaceRunning(workspaceId) or subscribe to an external
event) when agentWorkspaceStatuses.get(...) === 'stopped' and only return after
that check confirms stopped; update the initialization flow around isRunning and
the shellInAgentWorkspace call so shellInAgentWorkspace(workspaceId, ...) is
invoked if the health check or synced status indicates running, and ensure the
code references/updates agentWorkspaceStatuses when the external status is
discovered.
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte`:
- Around line 34-40: The startup race and duplicate input-forwarding happen
because executeShellInWorkspace() runs concurrently with refreshTerminal() and
each start/reconnect adds another input handler; fix by serializing session
startup (make executeShellInWorkspace await refreshTerminal() before reading
shellTerminal.cols/rows and set a "starting" mutex/flag so concurrent calls
queue or return) and ensure a single input forwarder by tracking and removing
the previous handler or guarding with an attached boolean (e.g.,
inputForwarderAttached) in the code paths that add the input-forwarding listener
inside executeShellInWorkspace/refreshTerminal/shellTerminal so you only add the
listener once and remove or reuse it on reconnect.
- Around line 121-130: The resize listener is added anonymously which prevents
removal on component teardown; change the anonymous arrow to a named handler
(e.g., const onWindowResize = () => { ... }) that calls fitAddon.fit() and
invokes window.shellInAgentWorkspaceResize(sendCallbackId, shellTerminal.cols,
shellTerminal.rows) as before, register it with
window.addEventListener('resize', onWindowResize) and then remove it in the
component cleanup (onDestroy or equivalent) via
window.removeEventListener('resize', onWindowResize); reference symbols:
fitAddon, sendCallbackId, shellTerminal, currentRouterPath, workspaceId, and
window.shellInAgentWorkspaceResize when making the change.
In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts`:
- Around line 53-58: registerTerminal mutates the updater parameter and
reassigns it, which violates the noParameterAssign rule and mixes mutation with
reassignment; change the update to return a fresh array instead of mutating
`list` (e.g., use the result of filter combined with the new terminal via array
spread or concat) so `agentWorkspaceTerminals.update` receives an immutable new
array containing all filtered entries plus the `terminal`.
---
Nitpick comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Around line 358-375: The test for dispose() doesn't assert that tracked
terminal processes are terminated; update the test that uses
shellInAgentWorkspace and the mocked process (returned by exec.createProcess) to
assert that mockProcess.kill was called when manager.dispose() is invoked (e.g.,
replace the placeholder expect(true).toBe(true) with an assertion like
expect(mockProcess.kill).toHaveBeenCalled()), ensuring the mocked
ChildProcessWithoutNullStreams instance created in the test is the same one
tracked and killed by dispose().
In `@packages/main/src/plugin/util/exec.ts`:
- Around line 77-89: create a single shared helper (e.g., wrapForFlatpak or
prepareCommandForFlatpak) that encapsulates the Flatpak rewrite logic currently
duplicated between createProcess() and exec(): accept the original command, args
and env, return the finalCommand, finalArgs and adjusted env; move the '--host'
wrapping and also the env forwarding behavior (injecting '--env=KEY=VALUE'
entries that exec() uses) into this helper so both createProcess() and exec()
call it and then spawn/exec the returned finalCommand/finalArgs with the
returned env; update references in createProcess() and exec() to use the helper
and remove duplicated rewrite code so terminal and regular exec paths stay
consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 596e2335-61e5-492a-837a-66faa5765fa4
📒 Files selected for processing (11)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/util/exec.spec.tspackages/main/src/plugin/util/exec.tspackages/preload/src/index.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.sveltepackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.sveltepackages/renderer/src/stores/agent-workspace-terminal-store.spec.tspackages/renderer/src/stores/agent-workspace-terminal-store.ts
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
Show resolved
Hide resolved
Comment on lines
+174
to
+215
| this.ipcHandle( | ||
| 'agent-workspace:terminal', | ||
| async (_listener: unknown, id: string, onDataId: number): Promise<number> => { | ||
| const invocation = await this.shellInAgentWorkspace( | ||
| id, | ||
| (content: string) => { | ||
| this.webContents.send('agent-workspace:terminal-onData', onDataId, content); | ||
| }, | ||
| (error: string) => { | ||
| this.webContents.send('agent-workspace:terminal-onError', onDataId, error); | ||
| }, | ||
| () => { | ||
| this.webContents.send('agent-workspace:terminal-onEnd', onDataId); | ||
| this.terminalCallbacks.delete(onDataId); | ||
| this.terminalProcesses.delete(onDataId); | ||
| }, | ||
| ); | ||
| this.terminalCallbacks.set(onDataId, { write: invocation.write, resize: invocation.resize }); | ||
| this.terminalProcesses.set(onDataId, invocation.process); | ||
| return onDataId; | ||
| }, | ||
| ); | ||
|
|
||
| this.ipcHandle( | ||
| 'agent-workspace:terminalSend', | ||
| async (_listener: unknown, onDataId: number, content: string): Promise<void> => { | ||
| const callback = this.terminalCallbacks.get(onDataId); | ||
| if (callback) { | ||
| callback.write(content); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| this.ipcHandle( | ||
| 'agent-workspace:terminalResize', | ||
| async (_listener: unknown, onDataId: number, width: number, height: number): Promise<void> => { | ||
| const callback = this.terminalCallbacks.get(onDataId); | ||
| if (callback) { | ||
| callback.resize(width, height); | ||
| } | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Expose a close IPC for individual terminal sessions.
Lines 174-215 start and retain kortex-cli terminal children, but the only cleanup paths are the process exiting on its own and the manager-wide dispose() in Lines 218-227. AgentWorkspaceTerminal.svelte only disposes the local xterm instance on destroy, so leaving the terminal tab leaks the child process and reopening the tab starts another shell.
🧹 Suggested handler
this.ipcHandle(
'agent-workspace:terminalResize',
async (_listener: unknown, onDataId: number, width: number, height: number): Promise<void> => {
const callback = this.terminalCallbacks.get(onDataId);
if (callback) {
callback.resize(width, height);
}
},
);
+
+ this.ipcHandle(
+ 'agent-workspace:terminalClose',
+ async (_listener: unknown, onDataId: number): Promise<void> => {
+ const proc = this.terminalProcesses.get(onDataId);
+ if (proc && !proc.killed) {
+ proc.kill();
+ }
+ this.terminalProcesses.delete(onDataId);
+ this.terminalCallbacks.delete(onDataId);
+ },
+ );
}Please pair this with a renderer-side call from AgentWorkspaceTerminal.svelte's onDestroy before clearing sendCallbackId.
Also applies to: 218-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts` around
lines 174 - 215, Add an IPC handler to close individual terminal sessions:
implement a new ipcHandle (e.g. 'agent-workspace:terminalClose') that takes
onDataId, looks up this.terminalProcesses and this.terminalCallbacks, calls an
orderly shutdown on the stored process (invocation.process.kill()/terminate or
invocation.process.close() depending on how shellInAgentWorkspace exposes it),
removes entries from this.terminalCallbacks and this.terminalProcesses, and
emits the same 'agent-workspace:terminal-onEnd' notification to the renderer;
update shellInAgentWorkspace usage sites if needed to expose a safe close
method. Also call this new IPC from AgentWorkspaceTerminal.svelte in onDestroy
before clearing sendCallbackId so the child process is not leaked.
Comment on lines
+346
to
+394
| contextBridge.exposeInMainWorld( | ||
| 'shellInAgentWorkspace', | ||
| async ( | ||
| id: string, | ||
| onData: (data: string) => void, | ||
| onError: (error: string) => void, | ||
| onEnd: () => void, | ||
| ): Promise<number> => { | ||
| onDataCallbacksShellInAgentWorkspaceId++; | ||
| onDataCallbacksShellInAgentWorkspace.set(onDataCallbacksShellInAgentWorkspaceId, { onData, onError, onEnd }); | ||
| return ipcInvoke('agent-workspace:terminal', id, onDataCallbacksShellInAgentWorkspaceId); | ||
| }, | ||
| ); | ||
|
|
||
| contextBridge.exposeInMainWorld( | ||
| 'shellInAgentWorkspaceSend', | ||
| async (dataId: number, content: string): Promise<void> => { | ||
| return ipcInvoke('agent-workspace:terminalSend', dataId, content); | ||
| }, | ||
| ); | ||
|
|
||
| contextBridge.exposeInMainWorld( | ||
| 'shellInAgentWorkspaceResize', | ||
| async (dataId: number, width: number, height: number): Promise<void> => { | ||
| return ipcInvoke('agent-workspace:terminalResize', dataId, width, height); | ||
| }, | ||
| ); | ||
|
|
||
| ipcRenderer.on('agent-workspace:terminal-onData', (_, callbackId: number, data: string) => { | ||
| const callback = onDataCallbacksShellInAgentWorkspace.get(callbackId); | ||
| if (callback) { | ||
| callback.onData(data); | ||
| } | ||
| }); | ||
|
|
||
| ipcRenderer.on('agent-workspace:terminal-onError', (_, callbackId: number, error: string) => { | ||
| const callback = onDataCallbacksShellInAgentWorkspace.get(callbackId); | ||
| if (callback) { | ||
| callback.onError(error); | ||
| } | ||
| }); | ||
|
|
||
| ipcRenderer.on('agent-workspace:terminal-onEnd', (_, callbackId: number) => { | ||
| const callback = onDataCallbacksShellInAgentWorkspace.get(callbackId); | ||
| if (callback) { | ||
| callback.onEnd(); | ||
| onDataCallbacksShellInAgentWorkspace.delete(callbackId); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Allow terminal sessions to be rebound or explicitly disposed.
The renderer now persists callbackId per workspace in packages/renderer/src/stores/agent-workspace-terminal-store.ts, but this bridge only ever registers handlers once and only removes them on agent-workspace:terminal-onEnd. If the terminal view unmounts or the initial ipcInvoke('agent-workspace:terminal', ...) fails, those closures stay live and the next mount has no way to replace or close that session. That can leak handlers and lose terminal output after leaving and returning to the tab.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/preload/src/index.ts` around lines 346 - 394, The bridge currently
registers global ipcRenderer handlers and only deletes entries from
onDataCallbacksShellInAgentWorkspace on 'agent-workspace:terminal-onEnd', which
leaks closures if sessions fail or are rebound; add an explicit dispose/unbind
API and per-callback listener cleanup: expose a new method (e.g.
'shellInAgentWorkspaceDispose' or 'shellInAgentWorkspaceUnbind') that takes the
callbackId, calls ipcInvoke to tell the main process to close the terminal
session, deletes the entry from onDataCallbacksShellInAgentWorkspace, and
removes any per-callback ipcRenderer listeners; to support removal, change the
global ipcRenderer.on usage for 'agent-workspace:terminal-onData' / '-onError' /
'-onEnd' to either store and register per-callback bound listener functions (so
you can call ipcRenderer.removeListener with those functions when disposing) or
wrap handlers to no-op when the callbackId is missing; update usages tied to
onDataCallbacksShellInAgentWorkspaceId, onDataCallbacksShellInAgentWorkspace,
and the exposed 'shellInAgentWorkspace' so the new dispose/unbind flow is
invoked on unmount or on invoke failure.
Comment on lines
+57
to
+80
| test('shows empty screen when workspace is not running', async () => { | ||
| render(AgentWorkspaceTerminal, { workspaceId: 'ws-1', screenReaderMode: true }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Workspace is not running')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| test('calls shellInAgentWorkspace when workspace is running', async () => { | ||
| agentWorkspaceStatuses.set('ws-1', 'running'); | ||
|
|
||
| const sendCallbackId = 42; | ||
| shellInAgentWorkspaceMock.mockResolvedValue(sendCallbackId); | ||
|
|
||
| render(AgentWorkspaceTerminal, { workspaceId: 'ws-1', screenReaderMode: true }); | ||
|
|
||
| await waitFor(() => expect(shellInAgentWorkspaceMock).toHaveBeenCalled()); | ||
| expect(shellInAgentWorkspaceMock).toHaveBeenCalledWith( | ||
| 'ws-1', | ||
| expect.any(Function), | ||
| expect.any(Function), | ||
| expect.any(Function), | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd AgentWorkspaceTerminal.svelte | xargs -r rg -n -C3 'agentWorkspaceStatuses|shellInAgentWorkspace|Workspace is not running'
fd agent-workspaces.svelte | xargs -r rg -n -C3 'agentWorkspaceStatuses|startAgentWorkspace|stopAgentWorkspace'Repository: kortex-hub/kortex
Length of output: 13022
The component gates shellInAgentWorkspace exclusively on the local status map, breaking manual-start scenarios.
AgentWorkspaceTerminal.svelte derives isRunning from agentWorkspaceStatuses.get(workspaceId) ?? 'stopped' (line 30–31), then gates the shellInAgentWorkspace call with if (!isRunning) { return; } (line 68–70). A workspace started outside the app remains 'stopped' in the map and never reaches the API call, showing "Workspace is not running" instead. To support manually or externally started workspaces, add a mechanism to detect and sync running status from the system (e.g., event listener or health check) before blocking terminal initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.ts`
around lines 57 - 80, AgentWorkspaceTerminal.svelte currently derives isRunning
from agentWorkspaceStatuses.get(workspaceId) and early-returns before calling
shellInAgentWorkspace, which blocks manual/external starts; change the logic in
AgentWorkspaceTerminal.svelte to not exclusively gate shell initialization on
agentWorkspaceStatuses.get — add a fallback health check or external-status sync
(e.g., call a new checkWorkspaceRunning(workspaceId) or subscribe to an external
event) when agentWorkspaceStatuses.get(...) === 'stopped' and only return after
that check confirms stopped; update the initialization flow around isRunning and
the shellInAgentWorkspace call so shellInAgentWorkspace(workspaceId, ...) is
invoked if the health check or synced status indicates running, and ensure the
code references/updates agentWorkspaceStatuses when the external status is
discovered.
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte
Show resolved
Hide resolved
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte
Outdated
Show resolved
Hide resolved
gastoner
force-pushed
the
connecting_to_workspace
branch
from
76ce7ff to
b7b6534
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
packages/renderer/src/stores/agent-workspace-terminal-store.ts (1)
53-59:⚠️ Potential issue | 🟡 MinorReturn a fresh array from the store update.
Line 55 reassigns the
listparameter, which violates thenoParameterAssignlint rule and mixes reassignment with mutation.🔧 Suggested fix
export function registerTerminal(terminal: TerminalOfAgentWorkspace): void { agentWorkspaceTerminals.update(list => { - list = list.filter(t => t.workspaceId !== terminal.workspaceId); - list.push(terminal); - return list; + return [...list.filter(t => t.workspaceId !== terminal.workspaceId), terminal]; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts` around lines 53 - 59, The registerTerminal function currently reassigns the update callback parameter and mutates the existing array, violating noParameterAssign; change the update to return a new array instance instead of reassigning or mutating `list` — for example, use the result of filtering by `t.workspaceId !== terminal.workspaceId` and then append the new `terminal` via spread/concat so the update callback returns a fresh array (refer to registerTerminal and agentWorkspaceTerminals.update).packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte (2)
53-65:⚠️ Potential issue | 🟠 MajorPrevent duplicate input forwarders on reconnect.
Both
receiveEndCallback()(line 59-61) andexecuteShellInWorkspace()(line 78-80) register newonDatahandlers. After the first reconnect, each keypress is sent multiple times since xterm'sonDatadoesn't replace existing handlers.Consider tracking the disposable returned by
onDataand disposing it before registering a new handler:let inputDisposable: IDisposable | undefined; // In both locations: inputDisposable?.dispose(); inputDisposable = shellTerminal.onData(async data => { await window.shellInAgentWorkspaceSend(id, data); });Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte` around lines 53 - 65, receiveEndCallback and executeShellInWorkspace both add xterm onData handlers causing duplicate input forwarding after reconnects; fix by storing the IDisposable returned by shellTerminal.onData in a shared variable (e.g., inputDisposable), call inputDisposable?.dispose() before registering a new handler, assign the new disposable to inputDisposable, and also dispose it when the terminal is torn down; update the code paths that call window.shellInAgentWorkspace and window.shellInAgentWorkspaceSend (and references to sendCallbackId and shellTerminal) to reuse this disposable management so only one onData handler is active.
121-130:⚠️ Potential issue | 🟠 MajorRemove the window resize handler on destroy.
Lines 121-130 register an anonymous resize listener that's never removed in
onDestroy. After navigating away and back, stale handlers accumulate and callfit()/shellInAgentWorkspaceResize()against disposed terminal state.♻️ Suggested cleanup
let sendCallbackId: number | undefined; let serializeAddon: SerializeAddon; +let handleResize: (() => void) | undefined; // In refreshTerminal(): - window.addEventListener('resize', () => { + handleResize = () => { if (currentRouterPath.includes(`/agent-workspaces/${encodeURIComponent(workspaceId)}/terminal`)) { fitAddon.fit(); if (sendCallbackId) { window .shellInAgentWorkspaceResize(sendCallbackId, shellTerminal.cols, shellTerminal.rows) .catch((err: unknown) => console.error(`Error resizing terminal for workspace ${workspaceId}`, err)); } } - }); + }; + window.addEventListener('resize', handleResize); // In onDestroy: onDestroy(() => { + if (handleResize) { + window.removeEventListener('resize', handleResize); + } const terminalContent = serializeAddon?.serialize() ?? '';Also applies to: 139-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte` around lines 121 - 130, The anonymous window 'resize' listener registering fitAddon.fit() and calling shellInAgentWorkspaceResize(sendCallbackId, shellTerminal.cols, shellTerminal.rows) should be converted to a named handler so it can be removed in onDestroy; create a const/let resizeHandler = () => { ... } that contains the currentRouterPath check, fitAddon.fit(), and the sendCallbackId call, register it with window.addEventListener('resize', resizeHandler), and call window.removeEventListener('resize', resizeHandler) inside onDestroy to avoid stale handlers (apply the same change for the other similar listener around the 139-149 block). Ensure the handler references workspaceId, shellTerminal, sendCallbackId, and fitAddon from the same scope so removal works correctly.packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
174-215:⚠️ Potential issue | 🟠 MajorExpose a close IPC for individual terminal sessions.
The only cleanup paths are the process exiting on its own and the manager-wide
dispose().AgentWorkspaceTerminal.svelteonly disposes the local xterm instance on destroy, so leaving the terminal tab leaks the child process and reopening the tab starts another shell.🧹 Suggested handler
this.ipcHandle( 'agent-workspace:terminalResize', async (_listener: unknown, onDataId: number, width: number, height: number): Promise<void> => { const callback = this.terminalCallbacks.get(onDataId); if (callback) { callback.resize(width, height); } }, ); + + this.ipcHandle( + 'agent-workspace:terminalClose', + async (_listener: unknown, onDataId: number): Promise<void> => { + const proc = this.terminalProcesses.get(onDataId); + if (proc && !proc.killed) { + proc.kill(); + } + this.terminalProcesses.delete(onDataId); + this.terminalCallbacks.delete(onDataId); + }, + ); }Pair this with a renderer-side call from
AgentWorkspaceTerminal.svelte'sonDestroybefore clearingsendCallbackId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts` around lines 174 - 215, Add an IPC handler to close a single terminal session: create a new ipcHandle (e.g., 'agent-workspace:terminalClose') that accepts onDataId, looks up the corresponding entry in terminalProcesses and terminalCallbacks (the same maps populated in the 'agent-workspace:terminal' handler), terminates/kills the child process obtained from terminalProcesses.get(onDataId) (or call a close method on the process returned by shellInAgentWorkspace), and then delete both terminalProcesses and terminalCallbacks entries and emit the same 'agent-workspace:terminal-onEnd' event to webContents to mirror the existing cleanup path; ensure this handler coordinates with existing shellInAgentWorkspace semantics so no resources remain.packages/preload/src/index.ts (1)
340-394:⚠️ Potential issue | 🟠 MajorAllow terminal sessions to be explicitly disposed from renderer.
The preload bridge only cleans up callbacks on
agent-workspace:terminal-onEnd. If the initialipcInvoke('agent-workspace:terminal', ...)fails or the terminal view unmounts beforeonEnd, those closures remain inonDataCallbacksShellInAgentWorkspace. Subsequent mounts have no way to unbind the stale session.Consider adding a dispose API:
contextBridge.exposeInMainWorld( 'shellInAgentWorkspaceDispose', async (dataId: number): Promise<void> => { onDataCallbacksShellInAgentWorkspace.delete(dataId); return ipcInvoke('agent-workspace:terminalClose', dataId); }, );This pairs with the suggested
agent-workspace:terminalClosehandler inagent-workspace-manager.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preload/src/index.ts` around lines 340 - 394, The bridge never exposes a dispose API so stale handlers in onDataCallbacksShellInAgentWorkspace can leak if ipcInvoke('agent-workspace:terminal', ...) fails or the view unmounts; add a new contextBridge.exposeInMainWorld entry named 'shellInAgentWorkspaceDispose' that takes the dataId, deletes the entry from onDataCallbacksShellInAgentWorkspace, and calls ipcInvoke('agent-workspace:terminalClose', dataId) to notify main to clean up; ensure the new API mirrors existing patterns used by 'shellInAgentWorkspace', 'shellInAgentWorkspaceSend', and 'shellInAgentWorkspaceResize' so renderers can explicitly unbind stale sessions.
🧹 Nitpick comments (2)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
358-375: Strengthen the dispose test assertion.The test spawns a process but only asserts
expect(true).toBe(true), which doesn't verify thatdispose()actually kills the process.♻️ Suggested improvement
describe('dispose', () => { test('kills active terminal processes', async () => { const mockProcess = { stdout: { on: vi.fn() }, stderr: { on: vi.fn() }, stdin: { write: vi.fn() }, on: vi.fn(), killed: false, kill: vi.fn(), } as unknown as ChildProcessWithoutNullStreams; vi.spyOn(exec, 'createProcess').mockReturnValue(mockProcess); await manager.shellInAgentWorkspace('ws-1', vi.fn(), vi.fn(), vi.fn()); manager.dispose(); - expect(true).toBe(true); + expect(mockProcess.kill).toHaveBeenCalled(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts` around lines 358 - 375, The test currently spawns a mock process via exec.createProcess but only asserts a no-op; update the test to verify that manager.dispose() actually terminates the spawned process by asserting that the mock process's kill method was called (or its killed flag became true) after calling manager.dispose(); locate the mockProcess created in the test and the calls to manager.shellInAgentWorkspace and manager.dispose to add an assertion like expect(mockProcess.kill).toHaveBeenCalled() (or check mockProcess.killed) so the test validates disposal behavior.packages/renderer/src/stores/agent-workspace-terminal-store.ts (1)
32-51: Consider immutable update in subscription cleanup.The current implementation mutates the array in place with
splice. While functional, returning a new array would be more consistent with immutable patterns:♻️ Immutable cleanup
agentWorkspaces.subscribe(workspaces => { - const terminals = get(agentWorkspaceTerminals); - const toRemove: TerminalOfAgentWorkspace[] = []; - for (const terminal of terminals) { - const found = workspaces.find(ws => ws.id === terminal.workspaceId); - if (!found) { - toRemove.push(terminal); - } - } - - for (const terminal of toRemove) { - agentWorkspaceTerminals.update(list => { - const index = list.indexOf(terminal); - if (index > -1) { - list.splice(index, 1); - } - return list; - }); - } + agentWorkspaceTerminals.update(terminals => + terminals.filter(t => workspaces.some(ws => ws.id === t.workspaceId)), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts` around lines 32 - 51, The subscription cleanup mutates the terminals array in place using list.splice; change it to an immutable update by replacing the update callback in agentWorkspaceTerminals.update to return a new filtered array (e.g., filter out TerminalOfAgentWorkspace items collected in toRemove or directly filter by checking workspaces existence) instead of splicing the existing list—modify the logic in the agentWorkspaces.subscribe block where terminals, toRemove, and agentWorkspaceTerminals.update are used so the update returns a new array rather than mutating the original.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 174-215: Add an IPC handler to close a single terminal session:
create a new ipcHandle (e.g., 'agent-workspace:terminalClose') that accepts
onDataId, looks up the corresponding entry in terminalProcesses and
terminalCallbacks (the same maps populated in the 'agent-workspace:terminal'
handler), terminates/kills the child process obtained from
terminalProcesses.get(onDataId) (or call a close method on the process returned
by shellInAgentWorkspace), and then delete both terminalProcesses and
terminalCallbacks entries and emit the same 'agent-workspace:terminal-onEnd'
event to webContents to mirror the existing cleanup path; ensure this handler
coordinates with existing shellInAgentWorkspace semantics so no resources
remain.
In `@packages/preload/src/index.ts`:
- Around line 340-394: The bridge never exposes a dispose API so stale handlers
in onDataCallbacksShellInAgentWorkspace can leak if
ipcInvoke('agent-workspace:terminal', ...) fails or the view unmounts; add a new
contextBridge.exposeInMainWorld entry named 'shellInAgentWorkspaceDispose' that
takes the dataId, deletes the entry from onDataCallbacksShellInAgentWorkspace,
and calls ipcInvoke('agent-workspace:terminalClose', dataId) to notify main to
clean up; ensure the new API mirrors existing patterns used by
'shellInAgentWorkspace', 'shellInAgentWorkspaceSend', and
'shellInAgentWorkspaceResize' so renderers can explicitly unbind stale sessions.
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte`:
- Around line 53-65: receiveEndCallback and executeShellInWorkspace both add
xterm onData handlers causing duplicate input forwarding after reconnects; fix
by storing the IDisposable returned by shellTerminal.onData in a shared variable
(e.g., inputDisposable), call inputDisposable?.dispose() before registering a
new handler, assign the new disposable to inputDisposable, and also dispose it
when the terminal is torn down; update the code paths that call
window.shellInAgentWorkspace and window.shellInAgentWorkspaceSend (and
references to sendCallbackId and shellTerminal) to reuse this disposable
management so only one onData handler is active.
- Around line 121-130: The anonymous window 'resize' listener registering
fitAddon.fit() and calling shellInAgentWorkspaceResize(sendCallbackId,
shellTerminal.cols, shellTerminal.rows) should be converted to a named handler
so it can be removed in onDestroy; create a const/let resizeHandler = () => {
... } that contains the currentRouterPath check, fitAddon.fit(), and the
sendCallbackId call, register it with window.addEventListener('resize',
resizeHandler), and call window.removeEventListener('resize', resizeHandler)
inside onDestroy to avoid stale handlers (apply the same change for the other
similar listener around the 139-149 block). Ensure the handler references
workspaceId, shellTerminal, sendCallbackId, and fitAddon from the same scope so
removal works correctly.
In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts`:
- Around line 53-59: The registerTerminal function currently reassigns the
update callback parameter and mutates the existing array, violating
noParameterAssign; change the update to return a new array instance instead of
reassigning or mutating `list` — for example, use the result of filtering by
`t.workspaceId !== terminal.workspaceId` and then append the new `terminal` via
spread/concat so the update callback returns a fresh array (refer to
registerTerminal and agentWorkspaceTerminals.update).
---
Nitpick comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Around line 358-375: The test currently spawns a mock process via
exec.createProcess but only asserts a no-op; update the test to verify that
manager.dispose() actually terminates the spawned process by asserting that the
mock process's kill method was called (or its killed flag became true) after
calling manager.dispose(); locate the mockProcess created in the test and the
calls to manager.shellInAgentWorkspace and manager.dispose to add an assertion
like expect(mockProcess.kill).toHaveBeenCalled() (or check mockProcess.killed)
so the test validates disposal behavior.
In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts`:
- Around line 32-51: The subscription cleanup mutates the terminals array in
place using list.splice; change it to an immutable update by replacing the
update callback in agentWorkspaceTerminals.update to return a new filtered array
(e.g., filter out TerminalOfAgentWorkspace items collected in toRemove or
directly filter by checking workspaces existence) instead of splicing the
existing list—modify the logic in the agentWorkspaces.subscribe block where
terminals, toRemove, and agentWorkspaceTerminals.update are used so the update
returns a new array rather than mutating the original.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bd3d55e-1138-4945-98fb-0ac885bda3b7
📒 Files selected for processing (11)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/util/exec.spec.tspackages/main/src/plugin/util/exec.tspackages/preload/src/index.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.sveltepackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.sveltepackages/renderer/src/stores/agent-workspace-terminal-store.spec.tspackages/renderer/src/stores/agent-workspace-terminal-store.ts
✅ Files skipped from review due to trivial changes (2)
- packages/main/src/plugin/util/exec.spec.ts
- packages/main/src/plugin/util/exec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.ts
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.svelte
- packages/renderer/src/stores/agent-workspace-terminal-store.spec.ts
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.ts
gastoner
force-pushed
the
connecting_to_workspace
branch
from
b7b6534 to
d5c3729
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte (1)
36-43:⚠️ Potential issue | 🟠 MajorWait for
refreshTerminal()before attaching.The
starting -> runningeffect can fire whilerefreshTerminal()is still awaiting configuration reads inonMount(). In that windowshellTerminalis still unset, so Lines 80-83 dereference it and the first terminal attach can fail. SerializeexecuteShellInWorkspace()behind terminal initialization, or bail out until the terminal instance exists.Also applies to: 70-86, 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte` around lines 36 - 43, The effect watching lastStatus/status can call executeShellInWorkspace before the terminal is ready (refreshTerminal/onMount), causing shellTerminal dereference errors; update the $effect (the block referencing lastStatus, status, executeShellInWorkspace) to wait for terminal initialization by either (a) checking a ready flag or that shellTerminal is non-null before calling executeShellInWorkspace, or (b) awaiting a promise that refreshTerminal resolves, so executeShellInWorkspace only runs after refreshTerminal/onMount completes; reference the symbols lastStatus, status, executeShellInWorkspace, refreshTerminal, shellTerminal and ensure the early-bail check prevents terminal attach until shellTerminal is set.
🧹 Nitpick comments (1)
packages/renderer/src/stores/agent-workspace-terminal-store.spec.ts (1)
25-28: ResetagentWorkspacesinbeforeEach.Later cases mutate the shared workspace store, but this setup only clears
agentWorkspaceTerminals. That makes the suite order-dependent once another test assumes the default empty workspace list.♻️ Small isolation fix
beforeEach(() => { vi.resetAllMocks(); + agentWorkspaces.set([]); agentWorkspaceTerminals.set([]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/stores/agent-workspace-terminal-store.spec.ts` around lines 25 - 28, The beforeEach currently resets mocks and clears agentWorkspaceTerminals but not the shared workspace store; add a reset of agentWorkspaces in the same beforeEach so each test starts with an empty workspace list (e.g., call agentWorkspaces.set([]) alongside agentWorkspaceTerminals.set([])) to prevent test-order dependencies when tests mutate the workspace store.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte`:
- Around line 103-116: The code creates a fresh shell session via
shellInAgentWorkspace() and ignores any persisted sendCallbackId from
getExistingTerminal(workspaceId), which leaves prior callbacks/processes alive;
fix by checking the saved sendCallbackId from getExistingTerminal and either
reuse it to reattach/resume the session (call shellInAgentWorkspace with the
existing callbackId or an attach API) or, if resuming is unsupported, explicitly
close/detach the prior session in onDestroy by emitting the
agent-workspace:terminal-onEnd/terminal-close with that sendCallbackId; update
getExistingTerminal, shellInAgentWorkspace usage and add onDestroy cleanup to
ensure the saved callbackId is either reused or terminated.
- Around line 55-67: receiveEndCallback currently uses the shared sendCallbackId
and can reopen a shell for an older session; change to capture the session's
callback id/token in a per-invocation closure so only the session that actually
ended can trigger reconnection. Specifically, when calling
window.shellInAgentWorkspace(...) and wiring the onEnd handler, replace the
shared receiveEndCallback usage with an inline function that closes over the
local id (the value returned by the promise) and only proceeds to call
window.shellInAgentWorkspace(...) again if the closed-over id matches the
session that ended; apply the same pattern for the other block (lines 74-85)
where inputDisposable, shellTerminal?.onData and
window.shellInAgentWorkspaceSend are set so each session's onEnd handler
references its own id/token rather than the module-level sendCallbackId.
---
Duplicate comments:
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte`:
- Around line 36-43: The effect watching lastStatus/status can call
executeShellInWorkspace before the terminal is ready (refreshTerminal/onMount),
causing shellTerminal dereference errors; update the $effect (the block
referencing lastStatus, status, executeShellInWorkspace) to wait for terminal
initialization by either (a) checking a ready flag or that shellTerminal is
non-null before calling executeShellInWorkspace, or (b) awaiting a promise that
refreshTerminal resolves, so executeShellInWorkspace only runs after
refreshTerminal/onMount completes; reference the symbols lastStatus, status,
executeShellInWorkspace, refreshTerminal, shellTerminal and ensure the
early-bail check prevents terminal attach until shellTerminal is set.
---
Nitpick comments:
In `@packages/renderer/src/stores/agent-workspace-terminal-store.spec.ts`:
- Around line 25-28: The beforeEach currently resets mocks and clears
agentWorkspaceTerminals but not the shared workspace store; add a reset of
agentWorkspaces in the same beforeEach so each test starts with an empty
workspace list (e.g., call agentWorkspaces.set([]) alongside
agentWorkspaceTerminals.set([])) to prevent test-order dependencies when tests
mutate the workspace store.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4371bfe3-904f-463a-8f4d-ff21b261ba34
📒 Files selected for processing (9)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.sveltepackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.sveltepackages/renderer/src/stores/agent-workspace-terminal-store.spec.tspackages/renderer/src/stores/agent-workspace-terminal-store.ts
✅ Files skipped from review due to trivial changes (2)
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.ts
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.svelte
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/renderer/src/stores/agent-workspace-terminal-store.ts
- packages/preload/src/index.ts
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.ts
- packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
- packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
Comment on lines
+55
to
+67
| function receiveEndCallback(): void { | ||
| if (sendCallbackId && isRunning) { | ||
| window | ||
| .shellInAgentWorkspace(workspaceId, createDataCallback(), () => {}, receiveEndCallback) | ||
| .then(id => { | ||
| sendCallbackId = id; | ||
| inputDisposable?.dispose(); | ||
| inputDisposable = shellTerminal?.onData(async data => { | ||
| await window.shellInAgentWorkspaceSend(id, data); | ||
| }); | ||
| }) | ||
| .catch((err: unknown) => console.error(`Error reopening terminal for workspace ${workspaceId}`, err)); | ||
| } |
Contributor
There was a problem hiding this comment.
Reconnect only for the session that actually ended.
receiveEndCallback() is reused across every shellInAgentWorkspace() call and only checks the current sendCallbackId. If an older session ends after a newer one was already started, this callback opens another shell and leaves both sessions live. The preload onEnd callback has no payload, so the active callback id or a session token needs to be closed over per invocation.
Also applies to: 74-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte`
around lines 55 - 67, receiveEndCallback currently uses the shared
sendCallbackId and can reopen a shell for an older session; change to capture
the session's callback id/token in a per-invocation closure so only the session
that actually ended can trigger reconnection. Specifically, when calling
window.shellInAgentWorkspace(...) and wiring the onEnd handler, replace the
shared receiveEndCallback usage with an inline function that closes over the
local id (the value returned by the promise) and only proceeds to call
window.shellInAgentWorkspace(...) again if the closed-over id matches the
session that ended; apply the same pattern for the other block (lines 74-85)
where inputDisposable, shellTerminal?.onData and
window.shellInAgentWorkspaceSend are set so each session's onEnd handler
references its own id/token rather than the module-level sendCallbackId.
Comment on lines
+103
to
+116
| const existingTerminal = getExistingTerminal(workspaceId); | ||
|
|
||
| shellTerminal = new Terminal({ | ||
| fontSize, | ||
| lineHeight, | ||
| screenReaderMode, | ||
| theme: getTerminalTheme(), | ||
| scrollback, | ||
| }); | ||
|
|
||
| if (existingTerminal) { | ||
| shellTerminal.options = { fontSize, lineHeight }; | ||
| shellTerminal.write(existingTerminal.terminal); | ||
| } |
Contributor
There was a problem hiding this comment.
The saved callbackId is never reused or closed.
Lines 149-153 persist sendCallbackId, but Line 141 always starts a fresh shellInAgentWorkspace() session and the restore path only replays serialized text. With the current preload/main lifecycle, navigating away and back can leave the previous callback/process alive until agent-workspace:terminal-onEnd. Either resume the saved callback id or add an explicit close/detach path in onDestroy().
Also applies to: 139-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.svelte`
around lines 103 - 116, The code creates a fresh shell session via
shellInAgentWorkspace() and ignores any persisted sendCallbackId from
getExistingTerminal(workspaceId), which leaves prior callbacks/processes alive;
fix by checking the saved sendCallbackId from getExistingTerminal and either
reuse it to reattach/resume the session (call shellInAgentWorkspace with the
existing callbackId or an attach API) or, if resuming is unsupported, explicitly
close/detach the prior session in onDestroy by emitting the
agent-workspace:terminal-onEnd/terminal-close with that sendCallbackId; update
getExistingTerminal, shellInAgentWorkspace usage and add onDestroy cleanup to
ensure the saved callbackId is either reused or terminated.
d5c3729 to
b2ac27f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/renderer/src/stores/agent-workspace-terminal-store.ts (1)
42-50: Prefer immutable update pattern for consistency.Lines 43-48 mutate the array with
spliceinsideupdate(), whileregisterTerminal(line 55) correctly uses an immutable spread pattern. For consistency and to avoid potential Svelte reactivity edge cases, consider using the same immutable pattern here.Suggested change
- for (const terminal of toRemove) { - agentWorkspaceTerminals.update(list => { - const index = list.indexOf(terminal); - if (index > -1) { - list.splice(index, 1); - } - return list; - }); - } + if (toRemove.length > 0) { + agentWorkspaceTerminals.update(list => + list.filter(t => !toRemove.includes(t)) + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts` around lines 42 - 50, The current loop mutates the agentWorkspaceTerminals list with splice; instead, follow the immutable pattern used in registerTerminal by producing a new array. Convert toRemove into a Set for O(1) checks (or use item equality), then call agentWorkspaceTerminals.update(list => list.filter(item => !toRemoveSet.has(item))) so you return a new array rather than splicing in-place; this also lets you perform a single update instead of one per terminal.packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
80-89: Consider handling malformed JSON gracefully.If
workspace.jsonexists but contains invalid JSON,JSON.parsethrows aSyntaxErrorthat propagates to the caller. Depending on expected usage, you may want to catch parse errors and return a fallback or wrap with a clearer error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts` around lines 80 - 89, The current try/catch around reading and parsing workspace.json can let a SyntaxError from JSON.parse bubble up; update the error handling in the block that reads configPath (using readFile and JSON.parse) inside agent-workspace-manager so that a JSON parse failure is handled explicitly: catch SyntaxError and either return a safe fallback (e.g., { name: workspace.name } as AgentWorkspaceConfiguration) or throw a new Error that clearly indicates "malformed workspace.json" and includes the original error message; keep the existing ENOENT branch unchanged. Ensure you reference the same symbols (configPath, readFile, JSON.parse, AgentWorkspaceConfiguration, workspace.paths.configuration, workspace.name) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 80-89: The current try/catch around reading and parsing
workspace.json can let a SyntaxError from JSON.parse bubble up; update the error
handling in the block that reads configPath (using readFile and JSON.parse)
inside agent-workspace-manager so that a JSON parse failure is handled
explicitly: catch SyntaxError and either return a safe fallback (e.g., { name:
workspace.name } as AgentWorkspaceConfiguration) or throw a new Error that
clearly indicates "malformed workspace.json" and includes the original error
message; keep the existing ENOENT branch unchanged. Ensure you reference the
same symbols (configPath, readFile, JSON.parse, AgentWorkspaceConfiguration,
workspace.paths.configuration, workspace.name) when making the change.
In `@packages/renderer/src/stores/agent-workspace-terminal-store.ts`:
- Around line 42-50: The current loop mutates the agentWorkspaceTerminals list
with splice; instead, follow the immutable pattern used in registerTerminal by
producing a new array. Convert toRemove into a Set for O(1) checks (or use item
equality), then call agentWorkspaceTerminals.update(list => list.filter(item =>
!toRemoveSet.has(item))) so you return a new array rather than splicing
in-place; this also lets you perform a single update instead of one per
terminal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab06aed9-d4d9-48af-8d5e-80bcdebef3dc
📒 Files selected for processing (9)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.sveltepackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.sveltepackages/renderer/src/stores/agent-workspace-terminal-store.spec.tspackages/renderer/src/stores/agent-workspace-terminal-store.ts
✅ Files skipped from review due to trivial changes (3)
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.spec.ts
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceDetails.svelte
- packages/renderer/src/lib/agent-workspaces/AgentWorkspaceTerminal.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/preload/src/index.ts
- packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
jeffmaury
reviewed
Apr 2, 2026
| const childProcess = spawn('kortex-cli', [ | ||
| 'terminal', | ||
| id, | ||
| '--', |
Contributor
There was a problem hiding this comment.
question: why not using the default agent command (I would expect to see claude running ?)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
168-209:⚠️ Potential issue | 🟠 MajorExpose an IPC handler to close individual terminal sessions.
The terminal IPC handlers register processes but only clean up when the process exits on its own or when
dispose()is called. If the user closes the terminal tab in the UI, the child process continues running because there's noagent-workspace:terminalClosehandler. Reopening the tab spawns another shell, leaking processes.🧹 Suggested handler
this.ipcHandle( 'agent-workspace:terminalResize', async (_listener: unknown, onDataId: number, width: number, height: number): Promise<void> => { const callback = this.terminalCallbacks.get(onDataId); if (callback) { callback.resize(width, height); } }, ); + + this.ipcHandle( + 'agent-workspace:terminalClose', + async (_listener: unknown, onDataId: number): Promise<void> => { + const proc = this.terminalProcesses.get(onDataId); + if (proc && !proc.killed) { + proc.kill(); + } + this.terminalProcesses.delete(onDataId); + this.terminalCallbacks.delete(onDataId); + }, + ); }Pair this with a renderer-side call from
AgentWorkspaceTerminal.svelte'sonDestroybefore clearingsendCallbackId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts` around lines 168 - 209, Add a new IPC handler named 'agent-workspace:terminalClose' that accepts onDataId, looks up the associated process in terminalProcesses and the callbacks in terminalCallbacks (the same maps populated by shellInAgentWorkspace), attempts to terminate the child process (e.g., call process.kill() or an appropriate close method on the stored process object), and then removes the entries from terminalProcesses and terminalCallbacks; ensure the handler is no-op if onDataId is not found and send any error back via webContents if kill fails. Also call this IPC from AgentWorkspaceTerminal.svelte onDestroy before clearing sendCallbackId so closing the UI tab triggers the 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 `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Around line 302-317: Add a new test mirroring the stdout test to verify stderr
is also forwarded to the onData callback: create a Map for stderr listeners,
mock a process via createMockProcess with stderrOn set to a vi.fn that stores
callbacks in that Map, have vi.mocked(spawn).mockReturnValue(mockProcess), call
manager.shellInAgentWorkspace('ws-1', onData, vi.fn(), vi.fn()), retrieve the
'data' callback from the stderr listeners and invoke it with Buffer.from('error
output'), then expect(onData).toHaveBeenCalledWith('error output') to confirm
stderr wiring.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 104-142: shellInAgentWorkspace's returned write implementation
currently calls childProcess.stdin.write without handling failures; update the
write function in shellInAgentWorkspace to guard and report write errors by (1)
checking childProcess.stdin.writable and !childProcess.stdin.destroyed before
writing, (2) invoking childProcess.stdin.write(param, callback) and in the
callback call onError(err.message) if err is provided, and (3) wrapping the
write call in try/catch to call onError for synchronous exceptions — reference
the shellInAgentWorkspace function and its returned write method and ensure you
do not change the existing onError/onData/onEnd callbacks' signatures.
---
Duplicate comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 168-209: Add a new IPC handler named
'agent-workspace:terminalClose' that accepts onDataId, looks up the associated
process in terminalProcesses and the callbacks in terminalCallbacks (the same
maps populated by shellInAgentWorkspace), attempts to terminate the child
process (e.g., call process.kill() or an appropriate close method on the stored
process object), and then removes the entries from terminalProcesses and
terminalCallbacks; ensure the handler is no-op if onDataId is not found and send
any error back via webContents if kill fails. Also call this IPC from
AgentWorkspaceTerminal.svelte onDestroy before clearing sendCallbackId so
closing the UI tab triggers the 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d456502b-be5a-4b94-a39d-e1c0bd9ad1c5
📒 Files selected for processing (2)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases (e.g.,'/@/plugin/provider-registry.js') instead of relative paths (e.g.,'../plugin/provider-registry.js') for imports outside the current directory's module group. Relative imports are only used for sibling modules within the same directory (e.g.,'./chat-manager.js').
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
**/*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.ts: Usetest()function instead ofit()for test cases in Vitest
Usevi.mock(import('...'))for auto-mocking modules. Avoid manual mock factories (vi.mock('...', () => ({...}))) when possible.
Usevi.resetAllMocks()inbeforeEachhooks, notvi.clearAllMocks(), for resetting mocks between tests.
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...). For class methods, use the prototype pattern:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...).
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
packages/main/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
IPC handlers must follow the naming convention
<registry-name>:<action>(e.g.,container-provider-registry:listContainers) when exposed viaipcHandle()
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🧠 Learnings (7)
📚 Learning: 2026-04-08T15:23:51.417Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T15:23:51.417Z
Learning: Applies to **/*.spec.ts : Use `vi.resetAllMocks()` in `beforeEach` hooks, not `vi.clearAllMocks()`, for resetting mocks between tests.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-08T15:23:51.417Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T15:23:51.417Z
Learning: Applies to **/*.spec.ts : Use `vi.mock(import('...'))` for auto-mocking modules. Avoid manual mock factories (`vi.mock('...', () => ({...}))`) when possible.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-08T15:23:51.417Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T15:23:51.417Z
Learning: Applies to **/*.spec.ts : When an auto-mocked function or class method needs a real implementation, use `vi.mocked(...)`. For class methods, use the prototype pattern: `vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)`.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-08T15:23:51.417Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T15:23:51.417Z
Learning: Applies to **/*.spec.ts : Use `test()` function instead of `it()` for test cases in Vitest
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-08T15:23:51.417Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T15:23:51.417Z
Learning: Applies to tests/playwright/src/**/*.ts : E2E tests should follow the Page Object Model pattern. See `.agents/skills/playwright-testing/` for detailed guidance on fixtures, locator conventions, and examples.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-04-08T15:23:51.417Z
Learnt from: CR
Repo: openkaiden/kaiden PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-08T15:23:51.417Z
Learning: Applies to packages/main/src/**/*.ts : IPC handlers must follow the naming convention `<registry-name>:<action>` (e.g., `container-provider-registry:listContainers`) when exposed via `ipcHandle()`
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-03-09T08:47:09.657Z
Learnt from: benoitf
Repo: kortex-hub/kortex PR: 1077
File: packages/main/src/plugin/skill/skill-manager.ts:80-109
Timestamp: 2026-03-09T08:47:09.657Z
Learning: In the kortex-hub/kortex repository, IPC handlers (via ipcHandle()) may be registered directly inside feature manager/service classes (e.g., SkillManager in packages/main/src/plugin/skill/skill-manager.ts) rather than exclusively in packages/main/src/plugin/index.ts. Treat this as an accepted design pattern for files under the plugin directory. Reviewers should not require centralization in index.ts; allow IPC registration proximity to the feature that owns the handler. When reviewing code, accept direct ipcHandle() registrations inside feature managers and ensure the pattern is consistently applied across similar feature-manager modules.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🔇 Additional comments (9)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (3)
41-56: LGTM!The new terminal state management properties (
terminalCallbacks,terminalProcesses) andWebContentsinjection are well-structured for tracking spawned processes and their callbacks.
74-90: LGTM!The configuration loading correctly handles the
ENOENTcase by returning a fallback configuration with the workspace name, while re-throwing other errors. The error type narrowing viaNodeJS.ErrnoExceptionis appropriate.
212-221: LGTM!The
dispose()method correctly iterates over all tracked terminal processes, kills those that haven't already been killed, and clears both maps. This ensures proper cleanup on manager destruction.packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (6)
36-37: LGTM!Correctly uses
vi.mock(import('...'))for auto-mocking modules as per coding guidelines.
73-77: LGTM!Uses
vi.resetAllMocks()inbeforeEachas required by coding guidelines, and correctly passes the newwebContentsdependency to the constructor.
99-110: LGTM!Good coverage for verifying the new terminal-related IPC handlers are registered with the correct naming convention.
189-205: LGTM!Excellent test coverage for the
ENOENTfallback behavior and non-ENOENTerror propagation. The error construction with explicitcodeproperty correctly simulatesNodeJS.ErrnoException.
258-272: LGTM!The
createMockProcesshelper is well-structured with proper typing (ReturnType<typeof vi.fn>) and sensible defaults for all required properties.
354-380: LGTM!Good test for verifying that
dispose()kills active terminal processes. The approach of extracting the IPC handler from the mock calls and invoking it to set up state before testingdispose()is appropriate.
Comment on lines
+302
to
+317
| test('calls onData when stdout emits data', () => { | ||
| const stdoutListeners = new Map<string, (data: Buffer) => void>(); | ||
| const mockProcess = createMockProcess({ | ||
| stdoutOn: vi.fn((event: string, cb: (data: Buffer) => void) => stdoutListeners.set(event, cb)), | ||
| }); | ||
| vi.mocked(spawn).mockReturnValue(mockProcess); | ||
|
|
||
| const onData = vi.fn(); | ||
| manager.shellInAgentWorkspace('ws-1', onData, vi.fn(), vi.fn()); | ||
|
|
||
| const dataCallback = stdoutListeners.get('data'); | ||
| expect(dataCallback).toBeDefined(); | ||
| dataCallback!(Buffer.from('output')); | ||
|
|
||
| expect(onData).toHaveBeenCalledWith('output'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add test coverage for stderr forwarding to onData.
The implementation forwards both stdout and stderr data to the onData callback (lines 117-123 in the implementation), but only stdout is tested here. Consider adding a test for stderr to ensure both streams are properly wired.
✅ Suggested test
test('calls onData when stderr emits data', () => {
const stderrListeners = new Map<string, (data: Buffer) => void>();
const mockProcess = createMockProcess({
stderrOn: vi.fn((event: string, cb: (data: Buffer) => void) => stderrListeners.set(event, cb)),
});
vi.mocked(spawn).mockReturnValue(mockProcess);
const onData = vi.fn();
manager.shellInAgentWorkspace('ws-1', onData, vi.fn(), vi.fn());
const dataCallback = stderrListeners.get('data');
expect(dataCallback).toBeDefined();
dataCallback!(Buffer.from('error output'));
expect(onData).toHaveBeenCalledWith('error output');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`
around lines 302 - 317, Add a new test mirroring the stdout test to verify
stderr is also forwarded to the onData callback: create a Map for stderr
listeners, mock a process via createMockProcess with stderrOn set to a vi.fn
that stores callbacks in that Map, have
vi.mocked(spawn).mockReturnValue(mockProcess), call
manager.shellInAgentWorkspace('ws-1', onData, vi.fn(), vi.fn()), retrieve the
'data' callback from the stderr listeners and invoke it with Buffer.from('error
output'), then expect(onData).toHaveBeenCalledWith('error output') to confirm
stderr wiring.
Comment on lines
+104
to
+142
| shellInAgentWorkspace( | ||
| id: string, | ||
| onData: (data: string) => void, | ||
| onError: (error: string) => void, | ||
| onEnd: () => void, | ||
| ): { | ||
| write: (param: string) => void; | ||
| resize: (w: number, h: number) => void; | ||
| process: ChildProcessWithoutNullStreams; | ||
| } { | ||
| // eslint-disable-next-line sonarjs/no-os-command-from-path | ||
| const childProcess = spawn('kortex-cli', ['terminal', id]); | ||
|
|
||
| childProcess.stdout.on('data', (chunk: Buffer) => { | ||
| onData(chunk.toString('utf-8')); | ||
| }); | ||
|
|
||
| childProcess.stderr.on('data', (chunk: Buffer) => { | ||
| onData(chunk.toString('utf-8')); | ||
| }); | ||
|
|
||
| childProcess.on('error', (error: Error) => { | ||
| onError(error.message); | ||
| }); | ||
|
|
||
| childProcess.on('close', () => { | ||
| onEnd(); | ||
| }); | ||
|
|
||
| return { | ||
| write: (param: string): void => { | ||
| childProcess.stdin.write(param); | ||
| }, | ||
| resize: (_w: number, _h: number): void => { | ||
| // no-op: resize requires a PTY (e.g. node-pty); can be added later | ||
| }, | ||
| process: childProcess, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling for stdin.write failures.
stdin.write can fail (e.g., if the child process has already exited or the stream is closed), but the error is silently ignored. This could lead to data loss without any indication to the caller.
♻️ Suggested improvement
return {
write: (param: string): void => {
- childProcess.stdin.write(param);
+ childProcess.stdin.write(param, err => {
+ if (err) {
+ onError(err.message);
+ }
+ });
},
resize: (_w: number, _h: number): void => {📝 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
| shellInAgentWorkspace( | |
| id: string, | |
| onData: (data: string) => void, | |
| onError: (error: string) => void, | |
| onEnd: () => void, | |
| ): { | |
| write: (param: string) => void; | |
| resize: (w: number, h: number) => void; | |
| process: ChildProcessWithoutNullStreams; | |
| } { | |
| // eslint-disable-next-line sonarjs/no-os-command-from-path | |
| const childProcess = spawn('kortex-cli', ['terminal', id]); | |
| childProcess.stdout.on('data', (chunk: Buffer) => { | |
| onData(chunk.toString('utf-8')); | |
| }); | |
| childProcess.stderr.on('data', (chunk: Buffer) => { | |
| onData(chunk.toString('utf-8')); | |
| }); | |
| childProcess.on('error', (error: Error) => { | |
| onError(error.message); | |
| }); | |
| childProcess.on('close', () => { | |
| onEnd(); | |
| }); | |
| return { | |
| write: (param: string): void => { | |
| childProcess.stdin.write(param); | |
| }, | |
| resize: (_w: number, _h: number): void => { | |
| // no-op: resize requires a PTY (e.g. node-pty); can be added later | |
| }, | |
| process: childProcess, | |
| }; | |
| } | |
| shellInAgentWorkspace( | |
| id: string, | |
| onData: (data: string) => void, | |
| onError: (error: string) => void, | |
| onEnd: () => void, | |
| ): { | |
| write: (param: string) => void; | |
| resize: (w: number, h: number) => void; | |
| process: ChildProcessWithoutNullStreams; | |
| } { | |
| // eslint-disable-next-line sonarjs/no-os-command-from-path | |
| const childProcess = spawn('kortex-cli', ['terminal', id]); | |
| childProcess.stdout.on('data', (chunk: Buffer) => { | |
| onData(chunk.toString('utf-8')); | |
| }); | |
| childProcess.stderr.on('data', (chunk: Buffer) => { | |
| onData(chunk.toString('utf-8')); | |
| }); | |
| childProcess.on('error', (error: Error) => { | |
| onError(error.message); | |
| }); | |
| childProcess.on('close', () => { | |
| onEnd(); | |
| }); | |
| return { | |
| write: (param: string): void => { | |
| childProcess.stdin.write(param, err => { | |
| if (err) { | |
| onError(err.message); | |
| } | |
| }); | |
| }, | |
| resize: (_w: number, _h: number): void => { | |
| // no-op: resize requires a PTY (e.g. node-pty); can be added later | |
| }, | |
| process: childProcess, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts` around
lines 104 - 142, shellInAgentWorkspace's returned write implementation currently
calls childProcess.stdin.write without handling failures; update the write
function in shellInAgentWorkspace to guard and report write errors by (1)
checking childProcess.stdin.writable and !childProcess.stdin.destroyed before
writing, (2) invoking childProcess.stdin.write(param, callback) and in the
callback call onError(err.message) if err is provided, and (3) wrapping the
write call in try/catch to call onError for synchronous exceptions — reference
the shellInAgentWorkspace function and its returned write method and ensure you
do not change the existing onError/onData/onEnd callbacks' signatures.
a01d75d to
538d759
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…inal command Co-Authored-By: Claude Sonnet 4.6 <[email protected]> Signed-off-by: Evzen Gasta <[email protected]>
Signed-off-by: Evzen Gasta <[email protected]>
gastoner
force-pushed
the
connecting_to_workspace
branch
2 times, most recently
from
91f37bf to
d12b446
Compare
Co-Authored-By: Claude Sonnet 4.6 <[email protected]> Signed-off-by: Evzen Gasta <[email protected]>
gastoner
force-pushed
the
connecting_to_workspace
branch
from
d12b446 to
184020e
Compare
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
2 participants
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.
This PR adds support for opening the running workspace terminal in Kortex
To test this, create a workspace using cli
kortex-cli init --runtime podman --agent claudeRun kortex open the workspace terminal, start the workspace (you have to do it manually even if the workspace is actually running from CLI) - kortex is not detecting if the workspace is running or not...
Closes #1125
Screen.Recording.2026-04-02.at.7.55.04.mov