feat: duplicate contexts #96
Conversation
feloy
requested review from
SoniaSandler and
gastoner
and removed request for
a team
feloy
force-pushed
the
feat-18/duplicate-context
branch
from
b209939 to
a533976
Compare
|
Warning Rate limit exceeded@feloy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a context-duplication feature: new Changes
Sequence DiagramsequenceDiagram
participant User
participant ContextCard
participant DuplicateAction
participant ContextsApi
participant ContextsManager
participant KubeConfig
User->>ContextCard: views context list
ContextCard->>DuplicateAction: render(name)
User->>DuplicateAction: click duplicate
DuplicateAction->>ContextsApi: duplicateContext(name)
ContextsApi->>ContextsManager: duplicateContext(name)
ContextsManager->>KubeConfig: read current config
ContextsManager->>ContextsManager: findNewContextName(config, name)
ContextsManager->>ContextsManager: clone context, cluster, user entries with new name
ContextsManager->>KubeConfig: update contexts/clusters/users/currentContext
ContextsManager->>KubeConfig: persist (saveKubeConfig)
ContextsManager-->>ContextsApi: resolve
ContextsApi-->>DuplicateAction: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
packages/extension/src/manager/contexts-manager.ts (1)
77-86: Consider making findNewContextName private.The method is only used internally by duplicateContext and doesn't need to be part of the public API. Making it private would better encapsulate the implementation detail.
Apply this diff:
- findNewContextName(kubeConfig: KubeConfig, contextName: string): string { + #findNewContextName(kubeConfig: KubeConfig, contextName: string): string { let counter = 1; let newName = `${contextName}-${counter}`; // Keep creating new name by adding 1 to name until not existing name is found while (kubeConfig.contexts.find(context => context.name === newName)) { counter += 1; newName = `${contextName}-${counter}`; } return newName; }And update line 91:
- const newName = this.findNewContextName(kubeConfig, contextName); + const newName = this.#findNewContextName(kubeConfig, contextName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/channels/src/interface/contexts-api.ts(1 hunks)packages/extension/src/manager/contexts-manager.spec.ts(1 hunks)packages/extension/src/manager/contexts-manager.ts(1 hunks)packages/webview/src/component/ContextCard.svelte(2 hunks)packages/webview/src/component/actions/DuplicateContextAction.svelte(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build / ubuntu-24.04
packages/webview/src/component/actions/DuplicateContextAction.svelte
[failure] 14-14: src/component/ContextCard.spec.ts > ContextCard should render with no current context
TypeError: Cannot read properties of undefined (reading 'getProxy')
in <unknown>
❯ DuplicateContextAction src/component/actions/DuplicateContextAction.svelte:14:28
❯ ContextCard.add_svelte_meta.componentTag src/component/ContextCard.svelte:35:30
❯ ContextCard src/component/ContextCard.svelte:117:24
[failure] 14-14: src/component/ContextCard.spec.ts > ContextCard should render with current context
TypeError: Cannot read properties of undefined (reading 'getProxy')
in <unknown>
❯ DuplicateContextAction src/component/actions/DuplicateContextAction.svelte:14:28
❯ ContextCard.add_svelte_meta.componentTag src/component/ContextCard.svelte:35:30
❯ ContextCard src/component/ContextCard.svelte:117:24
⏰ 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). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (4)
packages/channels/src/interface/contexts-api.ts (1)
24-24: LGTM!The new API method follows the existing interface pattern and integrates cleanly with the other context management methods.
packages/extension/src/manager/contexts-manager.spec.ts (1)
595-645: LGTM!The test comprehensively validates the duplication behavior, including incremental name generation and context count verification.
packages/webview/src/component/ContextCard.svelte (1)
6-6: LGTM!The integration of DuplicateContextAction is clean and follows the existing pattern. Rendering it unconditionally (unlike SetCurrentContextAction) makes sense since any context can be duplicated.
Also applies to: 35-35
packages/webview/src/component/actions/DuplicateContextAction.svelte (1)
1-21: Component implementation looks good.The DuplicateContextAction component follows the same pattern as other action components (SetCurrentContextAction, DeleteContextAction) and correctly delegates to the API. Once the test failures are fixed, this component will work as expected.
feloy
force-pushed
the
feat-18/duplicate-context
branch
2 times, most recently
from
d8f634c to
5eea75f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/webview/src/component/ContextCard.spec.ts (1)
36-90: Verify that tests are passing without the Remote context fix.The past review flagged failing tests because the Remote context was not provided when rendering ContextCard. The suggested fix was to create a mock Remote object with
getProxyand pass it via the context map. This fix has not been applied in the current version.While the component is mocked at line 30 (which should prevent its code from executing), the previous build logs showed concrete test failures. Please verify whether the tests are passing now, or if the suggested fix still needs to be applied.
If tests are still failing, apply the fix from the previous review:
// Add this to test setup const remote = { getProxy: vi.fn().mockReturnValue({ duplicateContext: vi.fn(), }), }; // Update render calls at lines 37 and 65 render(ContextCard, { context: new Map([[Remote, remote]]), props: { // ... existing props }, });You can verify by running the tests:
#!/bin/bash # Run the ContextCard tests to check for failures cd packages/webview npm test -- ContextCard.spec.ts
🧹 Nitpick comments (1)
packages/webview/src/component/actions/DuplicateContextAction.svelte (1)
16-18: Consider adding error handling for better user experience.The
duplicateContextfunction doesn't handle potential errors from the API call. If the duplication fails (e.g., due to invalid context name or persistence issues), the user receives no feedback.Consider wrapping the call in a try-catch block to handle errors gracefully:
async function duplicateContext(): Promise<void> { - await contextsApi.duplicateContext(name); + try { + await contextsApi.duplicateContext(name); + } catch (error) { + console.error('Failed to duplicate context:', error); + // Optionally show user notification + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/channels/src/interface/contexts-api.ts(1 hunks)packages/extension/src/manager/contexts-manager.spec.ts(1 hunks)packages/extension/src/manager/contexts-manager.ts(1 hunks)packages/webview/src/component/ContextCard.spec.ts(3 hunks)packages/webview/src/component/ContextCard.svelte(2 hunks)packages/webview/src/component/actions/DuplicateContextAction.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/extension/src/manager/contexts-manager.ts
- packages/webview/src/component/ContextCard.svelte
- packages/extension/src/manager/contexts-manager.spec.ts
- packages/channels/src/interface/contexts-api.ts
⏰ 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). (2)
- GitHub Check: Build / windows-2022
- GitHub Check: Build extension OCI image
🔇 Additional comments (2)
packages/webview/src/component/actions/DuplicateContextAction.svelte (1)
1-21: Clean implementation following project patterns.The component structure is well-organized and follows the established patterns for action buttons in this codebase. The use of Svelte 5's
$props()rune and the Remote context pattern are appropriate.packages/webview/src/component/ContextCard.spec.ts (1)
26-30: Test coverage for DuplicateContextAction integration looks good.The tests appropriately verify that the DuplicateContextAction component is rendered with the correct context name prop in both test scenarios. The mocking approach and assertions follow the established patterns in this test file.
Also applies to: 61-61, 89-89
Signed-off-by: Philippe Martin <[email protected]>
feloy
force-pushed
the
feat-18/duplicate-context
branch
from
5eea75f to
4812e46
Compare
2 participants
Fixes #18
Adds a 'Duplicate' button to the contexts