fix(openshell-cli): auto-refresh sandbox list while deleting sandboxes - #2475
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (10)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.spec.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/main/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/{main,renderer,preload}/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-03-09T08:47:09.657ZApplied to files:
📚 Learning: 2026-05-12T17:14:02.153ZApplied to files:
📚 Learning: 2026-06-29T13:16:53.102ZApplied to files:
🪛 Biome (2.5.3)packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts[error] 770-770: This callback passed to forEach() iterable method should not return a value. (lint/suspicious/useIterableCallbackReturn) 🔇 Additional comments (6)
📝 WalkthroughWalkthroughSandbox deletion polling now detects relevant sandbox-list changes and emits an event. ChangesSandbox refresh notifications
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OpenshellCli
participant Gateway
participant AgentWorkspaceManager
participant apiSender
OpenshellCli->>Gateway: list sandboxes per gateway
Gateway-->>OpenshellCli: sandbox results
OpenshellCli->>OpenshellCli: schedule deleting-sandbox refresh
OpenshellCli->>Gateway: re-list sandboxes after 5 seconds
Gateway-->>OpenshellCli: refreshed sandbox results
OpenshellCli->>AgentWorkspaceManager: emit onDidSandboxListChange
AgentWorkspaceManager->>apiSender: send agent-workspace-update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Around line 745-748: Store the disposable returned by onDidSandboxListChange
in a private AgentWorkspaceManager property, and dispose it at the start of the
`@preDestroy`() dispose method before cleaning up other resources. Preserve the
existing agent-workspace-update callback behavior.
In `@packages/main/src/plugin/openshell-cli/openshell-cli.ts`:
- Around line 286-313: Introduce a private _disposed flag on the OpenshellCli
class, check it at the start of scheduleDeletingPollIfNeeded so no timer is
created after disposal, and set it to true at the beginning of dispose before
clearing the existing _deletingPollTimer.
- Around line 308-311: Update the polling error handler around
scheduleDeletingPollIfNeeded so its catch branch passes the previous results
back into scheduleDeletingPollIfNeeded, preserving the existing results while
restarting the timer after listSandboxesPerGateway fails.
🪄 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: 44336f38-d895-4b61-96b3-1af4140d8030
📒 Files selected for processing (4)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: Linux
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2022
- GitHub Check: linter, formatters
- GitHub Check: typecheck
- GitHub Check: macOS
- GitHub Check: Windows
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
**/*.spec.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx,js,jsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible
Usevi.resetAllMocks()inbeforeEachhooks instead ofvi.clearAllMocks()for resetting mocks between tests
When an auto-mocked function or class method needs a real implementation, usevi.mocked(...)with the prototype pattern for class methods:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🧠 Learnings (3)
📚 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.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-06-29T13:16:53.102Z
Learnt from: benoitf
Repo: openkaiden/kaiden PR: 2296
File: extensions/container/packages/extension/src/helper/socket-finder/_socket-finder-module.ts:28-29
Timestamp: 2026-06-29T13:16:53.102Z
Learning: When reviewing imports in openkaiden/kaiden TypeScript/JavaScript files, prefer the configured `/@/` path alias instead of relative imports that would require traversing out of the current directory/module group (i.e., paths containing `..` that cross boundaries).
Do not require alias conversion for descendant-path relative imports within the socket-finder module directory—for example, in `extensions/container/packages/extension/src/helper/socket-finder/**`, imports like `./podman/podman-version-detector` and `./podman/podman-windows-finder` are acceptable and should not be flagged.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/openshell-cli/openshell-cli.spec.tspackages/main/src/plugin/openshell-cli/openshell-cli.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
🔇 Additional comments (2)
packages/main/src/plugin/openshell-cli/openshell-cli.spec.ts (1)
769-925: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
262-270: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bmahabirbu
left a comment
bmahabirbu
left a comment
There was a problem hiding this comment.
LGTM codewise and tested this I think 5 seconds is a good time for polling!
Orthogonal, while testing I noticed that Kaiden currently treats these two situations the same:
- The gateway is reachable and has no sandboxes.
- The gateway cannot be reached, so Kaiden cannot determine whether it has sandboxes.
So would be a good follow up to have kaiden give an error for a gateway cant be reached
|
I think we could still improve the design. Right now the deletion is blocking, so this.apiSender.send('agent-workspace-update'); will only be triggered after the actual deletion happened. In the meantime, the UI still shows the sandbox in Ready mode. Then the sandbox just disappears. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts`:
- Line 780: Replace the this.disposables.forEach callback with a for...of loop
that iterates over this.disposables and calls disposable.dispose() without
returning its result.
🪄 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: 9ccf0793-906c-426e-99e4-2aa2a13c3d85
📒 Files selected for processing (1)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases instead of relative paths for imports outside the current directory's module group; use relative imports only for sibling modules within the same directory
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
packages/main/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/main/src/**/*.{ts,tsx}: UseipcHandle()to expose handlers in the main process with naming convention<registry-name>:<action>(e.g.,container-provider-registry:listContainers)
UseapiSender.send()to send events from main process to renderer for real-time updates
Long-running operations should useTaskManager.createTask()with title and action configuration
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
packages/{main,renderer,preload}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Container operations must include
engineIdparameter to identify the container engine
Files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🧠 Learnings (3)
📚 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.ts
📚 Learning: 2026-05-12T17:14:02.153Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1850
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte:66-70
Timestamp: 2026-05-12T17:14:02.153Z
Learning: When reviewing code that uses `AgentWorkspaceSummaryUI.runtime`, treat it as a required, non-null `string` per the `openkaiden/kdn-api` 0.12.0 schema. Therefore, code like `a.runtime.localeCompare(b.runtime)` is safe and should not trigger warnings about possible `undefined`/`null` values or suggestions to use nullish coalescing/optional chaining for `runtime` (unless the current local types still mark `runtime` as optional, indicating a schema/version mismatch).
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
📚 Learning: 2026-06-29T13:16:53.102Z
Learnt from: benoitf
Repo: openkaiden/kaiden PR: 2296
File: extensions/container/packages/extension/src/helper/socket-finder/_socket-finder-module.ts:28-29
Timestamp: 2026-06-29T13:16:53.102Z
Learning: When reviewing imports in openkaiden/kaiden TypeScript/JavaScript files, prefer the configured `/@/` path alias instead of relative imports that would require traversing out of the current directory/module group (i.e., paths containing `..` that cross boundaries).
Do not require alias conversion for descendant-path relative imports within the socket-finder module directory—for example, in `extensions/container/packages/extension/src/helper/socket-finder/**`, imports like `./podman/podman-version-detector` and `./podman/podman-windows-finder` are acceptable and should not be flagged.
Applied to files:
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🪛 Biome (2.5.3)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
[error] 780-780: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🔇 Additional comments (2)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (2)
92-92: LGTM!
742-752: LGTM!
When listing sandboxes, if any are in "Deleting" phase, schedule an automatic re-list after 5 seconds. The emitter only fires when the total sandbox count or deleting count changes, avoiding unnecessary UI refreshes. The polling chain self-terminates once no sandboxes remain in "Deleting" state. Fixes openkaiden#2330 Signed-off-by: Jeff MAURY <jmaury@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
7795624 to
0d7203c
Compare
Signed-off-by: Jeff MAURY <jmaury@redhat.com>
…penkaiden#2030, openkaiden#2033) Workspaces were not visible until fully created, and there was no progress indication during creation or deletion. Backend (OpenshellCli): - Add operation polling around create/delete CLI calls: an early poll at 500ms catches the transitional phase (Provisioning/Deleting), and an immediate refresh on CLI completion picks up the final state. - Keep the 5-second transitional poll (from openkaiden#2475) for sandboxes that enter transitional states outside of Kaiden (e.g. external CLI usage). It also activates indirectly after our operation polls: the emitter update triggers a renderer re-fetch via listSandboxesPerGateway(), which schedules the transitional poll if transitional sandboxes are still present — this is desirable as it keeps monitoring until the sandbox reaches its final state after the CLI completes. Renderer: - Make create and delete fire-and-forget: navigate to the workspace list immediately without waiting for the IPC call to complete. Draft is preserved on creation error. - Pin sandbox phase to Deleting in the store once observed, until the sandbox disappears. Works around a Podman driver race where derive_phase briefly returns Provisioning between the stop and remove events during deletion. - Disable interactions (name link, trash icon) on sandboxes in Deleting phase. - Auto-redirect the details page to the list when a workspace disappears from the store. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
4 participants
Summary
openshell sandbox list, if any sandbox is in "Deleting" phase, automatically re-poll after 5 seconds to detect when the deletion completesAgentWorkspaceManagersubscribes to the newonDidSandboxListChangeevent and forwardsagent-workspace-updateto the rendererTest plan
AgentWorkspaceManagersubscription wiringpnpm run typecheck:main)pnpm run lint:check)Fixes #2330
🤖 Generated with Claude Code