fix(openshell): refresh sandbox list on page navigation#2114
Conversation
bmahabirbu
requested review from
benoitf and
jeffmaury
and removed request for
a team
|
Warning Review limit reached
More reviews will be available in 23 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughStore setup is exported; AgentWorkspaceList calls its fetch() on mount; tests mock window.listOpenshellSandboxes to return an empty array. ChangesOpenshell Sandboxes Initialization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
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/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte`:
- Around line 49-51: Wrap the manual call to
openshellSandboxesEventStoreInfo.fetch() inside a try/catch in the onMount
callback in AgentWorkspaceList.svelte so rejected promises from
EventStoreInfo.fetch/performUpdate don't become unhandled; catch the error, log
it (or surface it to component state/UI) and handle any UI state cleanup (e.g.,
stop loading flags) to avoid stale UI. Ensure the catch references
openshellSandboxesEventStoreInfo.fetch by name and leaves the
performUpdate/updater implementation unchanged.
🪄 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: 83f9ac82-3dd1-4112-94e2-9d1e68dd1236
📒 Files selected for processing (3)
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.spec.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.sveltepackages/renderer/src/stores/openshell-sandboxes.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: Windows
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: macOS
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Linux
- GitHub Check: typecheck
- GitHub Check: linter, formatters
🧰 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/renderer/src/lib/agent-workspaces/AgentWorkspaceList.spec.tspackages/renderer/src/stores/openshell-sandboxes.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/renderer/src/lib/agent-workspaces/AgentWorkspaceList.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/renderer/src/lib/agent-workspaces/AgentWorkspaceList.spec.tspackages/renderer/src/stores/openshell-sandboxes.ts
🧠 Learnings (8)
📚 Learning: 2026-04-15T08:51:08.199Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.spec.ts:64-69
Timestamp: 2026-04-15T08:51:08.199Z
Learning: For Svelte component test files under `packages/renderer/src/**` with names ending in `.spec.ts` that use `render` from `testing-library/svelte`, call `vi.useFakeTimers({ shouldAdvanceTime: true })` inside a `beforeEach`. Keep it in `beforeEach` for these tests to avoid flakiness from Svelte’s internal tick scheduling; do not remove or flag it based on the presence/absence of explicit timer manipulation in individual tests.
Applied to files:
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.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/renderer/src/lib/agent-workspaces/AgentWorkspaceList.spec.tspackages/renderer/src/stores/openshell-sandboxes.tspackages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte
📚 Learning: 2026-04-17T20:27:03.231Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1379
File: packages/renderer/src/stores/model-catalog.ts:5-21
Timestamp: 2026-04-17T20:27:03.231Z
Learning: In openkaiden/kaiden’s Electron renderer (single window, no shared localStorage origin across tabs/windows), do not suggest using `window.addEventListener('storage', ...)` for localStorage cross-tab synchronization. For store code like `packages/renderer/src/stores/model-catalog.ts`, treat `storage` events as not applicable for syncing state; use other intra-window state/update mechanisms instead.
Applied to files:
packages/renderer/src/stores/openshell-sandboxes.ts
📚 Learning: 2026-03-17T11:49:39.964Z
Learnt from: MarsKubeX
Repo: kortex-hub/kortex PR: 1111
File: packages/renderer/src/lib/agent-workspaces/AgentWorkspaceCard.svelte:46-52
Timestamp: 2026-03-17T11:49:39.964Z
Learning: In Svelte components like AgentWorkspaceCard.svelte and CustomPick.svelte, using a div with role="button" and tabindex="0" as the clickable card container with an inner native <button> (e.g., for a remove action) is acceptable because nested <button> elements are invalid per HTML spec. Ensure the inner button's events do not bubble by calling stopPropagation on both click and keydown handlers. Do not flag this pattern as an accessibility issue when implemented this way, but verify that keyboard activation (Enter/Space) and ARIA semantics are preserved and that focus management remains clear.
Applied to files:
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte
📚 Learning: 2026-04-15T08:04:32.031Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.svelte:9-11
Timestamp: 2026-04-15T08:04:32.031Z
Learning: For Svelte components in this repo, if a callback prop is typed as `() => void`, TypeScript idiomatically allows passing async functions (e.g., `() => Promise<void>`), because `() => void` indicates the caller ignores the return value rather than requiring `undefined`. Do not recommend changing these prop types to `() => void | Promise<void>` solely to “fix” async compatibility—unless there is an actual need for the caller to observe the returned value.
Applied to files:
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte
📚 Learning: 2026-04-17T08:04:22.761Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1362
File: packages/renderer/src/lib/secret-vault/SecretVaultList.svelte:23-29
Timestamp: 2026-04-17T08:04:22.761Z
Learning: In packages/renderer, the established list-page convention for Svelte components (e.g., *SkillsList.svelte, *SecretVaultList.svelte) is to use Svelte `$effect` blocks to sync local `$state` variables (like `searchTerm` or `screen`) into their corresponding Svelte stores (e.g., `*SearchPattern`, `*CategoryFilter`). During code review, do not treat this `$effect`-based state→store synchronization as an anti-pattern or recommend replacing it with direct store bindings or event-handler-only updates; it is intentional and consistent across these list pages.
Applied to files:
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte
📚 Learning: 2026-04-28T13:34:51.610Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1431
File: packages/renderer/src/lib/guided-setup/panels/OpenCodePanel.svelte:14-20
Timestamp: 2026-04-28T13:34:51.610Z
Learning: In this repo’s Svelte renderer (packages/renderer/src/**/*.svelte), `podman-desktop/ui-svelte`’s `Link` component does not accept an `href` prop. For opening external URLs, use the established pattern `on:click={() => window.openExternal(url)}` (optionally typed as `on:click={(): Promise<void> => window.openExternal(url)}`), consistent with existing components like `ProviderLinks.svelte`, `WelcomePage.svelte`, and `OpenCodePanel.svelte`. Do not treat `window.openExternal()` usage in renderer Svelte components as bypassing a security restriction, and do not recommend replacing it with `Link`/`href`-based navigation. (There is also no `setupSecurityRestrictionsOnLinks` utility in the renderer.)
Applied to files:
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte
📚 Learning: 2026-04-29T11:54:22.423Z
Learnt from: vancura
Repo: openkaiden/kaiden PR: 1494
File: packages/renderer/src/Loader.svelte:13-13
Timestamp: 2026-04-29T11:54:22.423Z
Learning: In this codebase’s renderer, timer variables in Svelte files (e.g., in `packages/renderer/src/Loader.svelte`) that store `setTimeout` handles are intentionally typed as `NodeJS.Timeout` (and not `ReturnType<typeof setTimeout>`). When reviewing, do not flag or recommend changing this to `ReturnType<typeof setTimeout>` for portability—treat `NodeJS.Timeout` as the established convention for renderer timer variables.
Applied to files:
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte
🔇 Additional comments (3)
packages/renderer/src/stores/openshell-sandboxes.ts (1)
53-53: LGTM!packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.svelte (1)
13-13: LGTM!Also applies to: 21-21
packages/renderer/src/lib/agent-workspaces/AgentWorkspaceList.spec.ts (1)
32-32: LGTM!
bmahabirbu
force-pushed
the
fix/openshell-sandbox-list-refresh
branch
from
2a7d57d to
c3a64ba
Compare
OpenShell sandboxes are remote resources with no local state file to watch, so deletions made outside Kaiden left the workspace list stale. Re-fetch the sandbox list on every mount of the Agentic Workspaces page. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
bmahabirbu
force-pushed
the
fix/openshell-sandbox-list-refresh
branch
from
c3a64ba to
30578bd
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
Should be managed by the back end component
|
Right now I beleive the backend has no way to know when OpenShell sandboxes change externally There is an api to watch one sandbox in openshell but not a fleet. This might be a good alternative till we have the ability to fully watch all sandboxes |
2 participants
Summary
~/.kdn/instances.json— so the existing file watcher cannot detect external deletionsEventStoreInfofrom the openshell sandboxes store so components can call.fetch()to force a re-fetch