fix: split workspace label to respect 63-char limit - #2223
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 (7)📚 Learning: 2026-03-17T11:49:39.964ZApplied to files:
📚 Learning: 2026-04-15T08:04:32.031ZApplied to files:
📚 Learning: 2026-04-28T13:34:51.610ZApplied to files:
📚 Learning: 2026-04-29T11:54:22.423ZApplied to files:
📚 Learning: 2026-05-12T17:14:02.153ZApplied to files:
📚 Learning: 2026-06-10T15:22:39.639ZApplied to files:
📚 Learning: 2026-03-09T08:47:09.657ZApplied to files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughThe PR fixes a sandbox creation failure when the source path exceeds OpenShell's 63-character label value limit. Instead of hashing, it introduces ChangesWorkspace Path Encoding, Decoding, and Display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
I am not sure to understand: the idea of this label is to retrieve the path used by Kaiden when the workspace was created so that we can locate the workspace.json when we need to display the details. If this is a hash how do we retrieve the full path ?
![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/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts`:
- Line 423: The hashPath function lacks dedicated unit tests to verify its
length constraint requirement. Add a new unit test specifically for the hashPath
function that validates it produces consistent output with a length of 43
characters (or ≤63 characters as per the constraint) for various input path
lengths, including both short paths like '/tmp/a' and longer paths like
'/Users/fbricon/Dev/souk/ideas/stock-trading-ai/backend'. This test should
verify that the hashPath function meets the length requirements regardless of
the input path length.
🪄 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: 3d110c80-a4f7-405a-a3b5-bfd45094a914
📒 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 (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.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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/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.spec.tspackages/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.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🧠 Learnings (2)
📚 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
📚 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.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.ts
🔇 Additional comments (3)
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (2)
19-19: LGTM!Also applies to: 56-58
175-175: LGTM!packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
46-46: LGTM!
|
so I guess we have 2 options: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
88318ee to
2baa442
Compare
|
Decoded path now added as a tooltip on the sandbox 
|
fbricon
changed the title
fbricon
changed the title
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/api/src/openshell-gateway-info.ts`:
- Around line 52-63: The chunk label validation in the filter and sort logic
accepts non-numeric suffixes and non-contiguous indices, which can cause
incorrect decoding of the sourcePath. In the section where
Object.entries(labels) is filtered to match the WORKSPACE_LABEL prefix, add
validation to ensure that each chunk suffix is strictly numeric and that the
chunk indices form a contiguous sequence starting from zero (0, 1, 2, etc.). If
any chunk has a non-numeric suffix or if the indices are not contiguous, the
function should return undefined instead of concatenating the values together.
🪄 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: 4fd10107-ffb9-4f1d-a441-bf3b6189e925
📒 Files selected for processing (4)
packages/api/src/openshell-gateway-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/renderer/src/lib/agent-workspaces/columns/SandboxName.svelte
📜 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). (11)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: typecheck
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: unit tests / macos-15
- GitHub Check: Detect pnpm lock or pr-check files changes
- GitHub Check: unit tests / windows-2022
- 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/api/src/openshell-gateway-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.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/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/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/agent-workspace/agent-workspace-manager.spec.ts
🧠 Learnings (7)
📚 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/columns/SandboxName.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/columns/SandboxName.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/columns/SandboxName.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/columns/SandboxName.svelte
📚 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/columns/SandboxName.sveltepackages/api/src/openshell-gateway-info.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.tspackages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts
📚 Learning: 2026-06-10T15:22:39.639Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 2115
File: packages/renderer/src/lib/models/SemanticRouterCreate.svelte:134-137
Timestamp: 2026-06-10T15:22:39.639Z
Learning: In multi-step wizard UIs, it’s acceptable for “Next step” buttons to be temporarily non-interactive during incremental work delivered across multiple PRs. When reviewing Svelte files under `packages/renderer/src/lib/**`, do not flag these UX issues if the button lacks an onClick handler *and* there is an explicit TODO comment indicating deferred wiring, and the PR author confirms (in the PR description) that this is an intentional staged implementation. If no TODO/deferred-wiring marker (or no author confirmation) is present, treat missing handlers as a potential UX issue.
Applied to files:
packages/renderer/src/lib/agent-workspaces/columns/SandboxName.svelte
📚 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/agent-workspace/agent-workspace-manager.spec.ts
🔇 Additional comments (4)
packages/api/src/openshell-gateway-info.ts (1)
41-50: LGTM!Also applies to: 64-72
packages/main/src/plugin/agent-workspace/agent-workspace-manager.ts (1)
52-53: LGTM!Also applies to: 56-69, 186-187, 484-492
packages/main/src/plugin/agent-workspace/agent-workspace-manager.spec.ts (1)
44-47: LGTM!Also applies to: 402-402, 425-425, 1606-1639
packages/renderer/src/lib/agent-workspaces/columns/SandboxName.svelte (1)
2-2: LGTM!Also applies to: 14-16
Whart about having a Kaiden specific folder inside the VM that where we would store:
|
but then you'd have to peek inside the sandbox, I don't know how costly that will be |
2baa442 to
6dff980
Compare
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
LGTM but WORKSPACE_LABEL is defined twice should be defined in a central place
The workspace label stores a base64url-encoded source path so it can be decoded and displayed in the UI. Long paths can exceed the 63-char label value limit, causing sandbox creation to fail. Add encodeWorkspaceLabels/decodeWorkspaceLabels functions that use a single label when the value fits, and split into indexed labels (workspace.0, workspace.1, ...) when it exceeds the limit. The decoded workspace path is shown as a tooltip on the sandbox name in the workspace list. Closes openkaiden#2218 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
6dff980 to
feb527f
Compare
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
2 participants
The workspace label stores a base64url-encoded source path so it can
be decoded and displayed in the UI. Long paths can exceed the 63-char
label value limit, causing sandbox creation to fail.
Add encodeWorkspaceLabels/decodeWorkspaceLabels functions that use a
single label when the value fits, and split into indexed labels
(workspace.0, workspace.1, ...) when it exceeds the limit.
The decoded workspace path is shown as a tooltip on the sandbox name
in the workspace list.
Closes #2218
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Signed-off-by: Fred Bricon fbricon@gmail.com