feat: enable renaming providers#358
Merged
skevetter merged 15 commits intoskevetter:mainfrom Mar 8, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a transactional provider-rename feature: CLI subcommand and wiring, backend provider move and workspace/machine rebinding helpers with rollback, desktop client + UI rename integration and optimistic updates, docs, and extensive e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Desktop UI
participant CLI as DevPod CLI
participant Config as DevPod Config
participant FS as Filesystem
participant WS as Workspaces/Machines
participant State as Rename State
User->>UI: request rename(old -> new)
UI->>CLI: devpod provider rename old new
CLI->>Config: load config & validate names
CLI->>FS: move provider dir old → new
alt move fails
CLI->>State: record failure
State->>FS: attempt move back
CLI-->>User: error
else move succeeds
CLI->>WS: locate workspaces & machines bound to old
CLI->>WS: switch references to new
alt switch fails
CLI->>State: record changes
State->>WS: revert refs
State->>FS: move back new → old
CLI-->>User: error
else
CLI->>Config: update default provider if needed and persist
CLI-->>User: success
UI->>UI: optimistic cache update to new name
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
53-53: Regex bug: double backslash creates incorrect pattern.The regex
/^[a-z0-9\\-]+$/uses a double backslash, which in a regex literal matches a literal backslash character followed by a hyphen—not just a hyphen. This will reject valid names likemy-providerand accept invalid names likemy\-provider.Additionally, this duplicates
ALLOWED_NAMES_REGEXalready exported fromhelpers.ts. Import it instead.🐛 Proposed fix
Remove the local definition and import from helpers:
-import { exists, isError, randomString, useFormErrors } from "../../../lib" +import { exists, isError, randomString, useFormErrors, ALLOWED_NAMES_REGEX } from "../../../lib"Then remove line 53:
-const ALLOWED_NAMES_REGEX = /^[a-z0-9\\-]+$/Alternatively, if
ALLOWED_NAMES_REGEXis not re-exported from../../../lib, import directly from./helpers:-import { FieldName, TFormValues, TSetupProviderResult } from "./types" +import { FieldName, TFormValues, TSetupProviderResult } from "./types" +import { ALLOWED_NAMES_REGEX } from "./helpers"
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 43-47: Update the help/warning text for the "provider rename"
command (the help block in cmd/provider/rename.go used by the provider rename
command) to remove the instruction that users must manually edit workspaces and
instead state that the command will attempt to automatically rebind workspaces
to the new provider name, while advising users to verify workspace configs and
note any potential edge cases/failures that may require manual intervention;
ensure the revised wording replaces the existing WARNING paragraph so it clearly
reflects automatic rebinding and suggests verification steps.
- Around line 86-100: The loop that updates workspaces currently logs failures
from provider.SaveWorkspaceConfig and continues, then calls
provider.RenameProvider which can orphan workspaces; change the logic in the
block iterating workspaces so that any error returned by
provider.SaveWorkspaceConfig for a workspace causes the command to abort (or
accumulate and return an aggregated error) before calling
provider.RenameProvider; specifically, in the loop over workspaces where you
check ws.Provider.Name == oldName, stop and return the error (or collect it and
return after the loop) instead of only logging in the
ws.Provider.SaveWorkspaceConfig error path so provider.RenameProvider is only
called when all workspace rebinds succeeded.
In `@cmd/workspace/rebind.go`:
- Around line 36-46: The code is passing the raw workspace name to
provider.LoadWorkspaceConfig (and later to the save path) but
provider.LoadWorkspaceConfig expects a workspace ID; convert the name to an ID
first using workspace.ToID and use that ID when calling
provider.LoadWorkspaceConfig(devPodConfig.DefaultContext, workspaceID) and when
later saving (e.g., provider.SaveWorkspaceConfig). Replace usages of
workspaceName in LoadWorkspaceConfig/SaveWorkspaceConfig calls with the
normalized workspaceID returned by workspace.ToID, and propagate any conversion
errors appropriately.
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`:
- Around line 105-118: The current handleSave sequence renames before
configuring which can leave the UI on the old route if configure fails; change
the sequence to call client.providers.configure(finalProviderID/config) first
(using providerID), then call client.providers.rename(providerID, newName) if
needed, update finalProviderID accordingly, invalidate QueryKeys.PROVIDERS after
both succeed, and call navigate(Routes.toProvider(newName), { replace: true })
only after rename succeeds; ensure you reference handleSave,
client.providers.configure, client.providers.rename, QueryKeys.PROVIDERS and
Routes.toProvider when making the change.
In `@desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx`:
- Around line 377-381: The validation message in SetupProviderSourceForm using
ALLOWED_NAMES_REGEX is misleading because the regex only allows lowercase
letters; update the user-facing error text to accurately reflect the rule (e.g.,
"Name can only contain lowercase letters, numbers and -") or alternatively
broaden ALLOWED_NAMES_REGEX to allow uppercase if uppercase should be permitted;
modify the message string next to the rules.pattern block (the rule inside
SetupProviderSourceForm.tsx) and ensure it matches the actual
ALLOWED_NAMES_REGEX behavior.
In `@e2e/tests/provider/provider.go`:
- Line 21: The test uses an incorrect relative path when calling
framework.CopyToTempDir, so update the string argument from
"testdata/simple-k8s-provider" to "tests/provider/testdata/simple-k8s-provider"
wherever it is used in this file (all four occurrences that call
framework.CopyToTempDir, e.g., the calls that assign tempDir and err); keep the
rest of the call and error handling intact.
In `@e2e/tests/workspace/rebind_test.go`:
- Around line 50-59: The test derives workspaceID with filepath.Base(tempDir)
but DevPod uses workspace.ToID() (see ToID in pkg/workspace) so Stop/Rebind/SSH
may target the wrong workspace; update the test to consistently transform the
derived ID using workspace.ToID() before passing it to f.DevPodStop and
f.DevPodWorkspaceRebind (or alternatively pass tempDir directly like
f.DevPodWorkspaceDelete does), ensuring the same identifier normalization used
by ToID() is applied when calling DevPodStop, DevPodWorkspaceRebind, and any
subsequent workspace operations.
In `@pkg/provider/dir.go`:
- Around line 453-466: Currently the code saves the new provider config then
deletes the old provider directory, which discards binaries/state; instead,
obtain oldProviderDir via GetProviderDir(context, oldName) and move it to the
new provider directory using os.Rename (or shutil-equivalent on cross-filesystem
if needed), then call SaveProviderConfig(context, providerConfig) to write
provider.json into the moved directory (or update provider.json in its new
path). Ensure you replace the final os.RemoveAll(oldProviderDir) with the rename
operation and update any path references so the provider daemon/binaries remain
intact.
- Around line 435-442: The current check treats any os.Stat error other than
IsNotExist as "already exists"; change the logic in the rename/create flow that
uses GetProviderDir, newProviderDir and os.Stat so you explicitly handle three
cases: if os.Stat returns nil then return the "provider with name %s already
exists" error; if os.IsNotExist(err) continue (allowed); otherwise return the
underlying filesystem error (wrap or propagate) so permission/I/O errors are
surfaced instead of being misclassified as "already exists".
♻️ Duplicate comments (1)
e2e/tests/provider/rename_test.go (1)
143-145: Workspace identifier mismatch risk (path vs ID).Line 144 passes
tempDirtoDevPodSSH. If the helper expects a workspace ID (as used in other tests), this will be flaky. Align with the identifier the framework expects.
🧹 Nitpick comments (4)
e2e/framework/helper.go (1)
8-10: Consider differentiating from the existingDevPodDescribein integration tests.A
DevPodDescribefunction already exists ine2e/tests/integration/framework.gothat adds an"[integration] "prefix to the description. This new generic version in the sharedframeworkpackage has no prefix, which could cause confusion when both packages are imported or when developers expect consistent test output formatting.Consider either:
- Adding a distinguishing prefix (e.g.,
"[workspace] ") for workspace tests- Renaming this function to clarify its generic nature
- Removing the integration-specific version and having all tests use this generic one with explicit prefixes
e2e/framework/command.go (1)
391-395: Add context to rebind failures for easier triage.Returning the raw error drops command context in test logs. Consider wrapping the error (and optionally stderr) to match other helpers.
♻️ Suggested tweak
func (f *Framework) DevPodWorkspaceRebind(ctx context.Context, workspaceName, newProvider string, extraArgs ...string) error { args := []string{"workspace", "rebind", workspaceName, newProvider} args = append(args, extraArgs...) - _, _, err := f.ExecCommandCapture(ctx, args) - return err + _, _, err := f.ExecCommandCapture(ctx, args) + if err != nil { + return fmt.Errorf("devpod workspace rebind failed: %s", err.Error()) + } + return nil }desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
150-151: Hide the name field when configuring workspace-specific options.In the workspace flow,
onSaveis a noop, so editing the name currently does nothing. Consider rendering the name field only for provider-level configuration to avoid misleading the user.♻️ Suggested UI guard
- <FormControl> - <FormLabel>Provider Name</FormLabel> - <Input {...formMethods.register(FieldName.NAME)} /> - </FormControl> + {workspace === undefined && ( + <FormControl> + <FormLabel>Provider Name</FormLabel> + <Input {...formMethods.register(FieldName.NAME)} /> + </FormControl> + )}Also applies to: 166-167, 270-273
docs/pages/managing-providers/rename-provider.md (1)
39-39: Consider adding a warning about workspace impact.The CLI help text in
cmd/provider/rename.goincludes an important warning: "WARNING: Renaming a provider will cause all workspaces bound to it to fail. You will have to manually edit the workspace configurations to use the new provider name."This documentation should include a similar warning or note so users are aware of the potential impact on existing workspaces, particularly for CLI users who may not see the help text.
📝 Suggested addition
After renaming, DevPod will automatically update the provider's configuration. + +:::warning +Renaming a provider will cause all workspaces bound to it to fail. You may need to rebind workspaces to use the new provider name using `devpod workspace rebind`. +:::
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
53-53: Bug: Double backslash allows literal backslash characters in names.The regex
/^[a-z0-9\\-]+$/uses\\-which in a JS regex literal matches a literal backslash plus hyphen. This differs from the existingALLOWED_NAMES_REGEXinhelpers.tswhich correctly uses/^[a-z0-9\-]+$/.Import from
helpers.tsinstead of duplicating and introducing this bug:🔧 Suggested fix
-const ALLOWED_NAMES_REGEX = /^[a-z0-9\\-]+$/Then update the import on line 45 to include
ALLOWED_NAMES_REGEX:import { exists, isError, randomString, useFormErrors, ALLOWED_NAMES_REGEX } from "../../../lib"Or import directly from the helpers file if
ALLOWED_NAMES_REGEXisn't re-exported throughlib:import { ALLOWED_NAMES_REGEX } from "./helpers"
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 90-112: The provider is being renamed before workspaces are
rebound which can orphan workspaces if any rebind fails; change the sequence so
you iterate workspacesToRebind and call provider.SaveWorkspaceConfig (updating
ws.Provider.Name = newName) for each workspace first, collecting rebindErrors
and successfulRebinds, and only if all required rebinding succeeds call
provider.RenameProvider(devPodConfig.DefaultContext, oldName, newName);
additionally, if you keep partial-rebind behavior, implement rollback logic to
restore any successfulRebinds back to oldName (using SaveWorkspaceConfig) when
provider.RenameProvider fails so no workspace is left referencing a non-existent
provider.
🧹 Nitpick comments (2)
cmd/provider/rename.go (1)
22-51: Excessive blank lines reduce readability.Multiple unnecessary blank lines between statements (e.g., lines 23, 25, 30, 45, 47) create visual clutter. Consider removing the extra blank lines for a cleaner structure.
♻️ Suggested cleanup
func NewRenameCmd(globalFlags *flags.GlobalFlags) *cobra.Command { - cmd := &RenameCmd{ - GlobalFlags: globalFlags, } return &cobra.Command{ - Use: "rename", - Short: "Rename a provider", - Long: `Renames a provider and automatically rebinds all workspaces that are bound to it to use the new provider name. Example: devpod provider rename my-provider my-new-provider `, - Args: cobra.ExactArgs(2), - RunE: func(cobraCmd *cobra.Command, args []string) error { - return cmd.Run(cobraCmd, args) - }, } - }desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
272-275: Consider defaulting to providerID for better UX.When
nameprop is undefined, the input renders empty. Users editing an existing provider may expect to see the current provider name. Consider defaulting toproviderIDwhennameis not provided.♻️ Suggested default
const formMethods = useForm<TFieldValues>({ defaultValues: { reuseMachine, useAsDefault: isDefault, - name: name, + name: name ?? providerID, }, })
skevetter
requested changes
Jan 25, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 145-150: When renaming a provider, if oldName is currently the
default, switch the default to newName before calling DeleteProviderConfig to
avoid clearing the user's default selection; detect this by checking the current
default provider (e.g., compare stored defaultName to oldName) and call the
appropriate setter to update the default to newName, then proceed to call
DeleteProviderConfig(devPodConfig, oldName, true) and handle errors as before
(use DeleteProviderConfig, oldName, newName to locate the logic).
Owner
|
Please fix failing checks first |
skevetter
requested changes
Jan 27, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 103-116: The rollback function currently returns immediately if
rebindWorkspaces(workspacesTouched, oldName) fails, leaving the cloned provider
(newName) orphaned; change rollback to always attempt
DeleteProviderConfig(devPodConfig, newName, true) even when rebindWorkspaces
returns an error, capture both errors (the rebind error and any delete error),
and return an aggregated error (or combined message) so callers know both
failures; reference the rollback function, the rebindWorkspaces call, and
DeleteProviderConfig invocation when making the change.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
52-52: Regex allows unintended backslash characters in names.The double backslash
\\-in the character class matches a literal backslash character plus a hyphen. This means names likemy\providerwould pass validation, which could cause issues with file paths or command-line operations.🐛 Suggested fix
-const ALLOWED_NAMES_REGEX = /^[a-z0-9\\-]+$/ +const ALLOWED_NAMES_REGEX = /^[a-z0-9-]+$/A hyphen at the end of a character class doesn't require escaping.
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
150-167: Hide/guard Provider Name in workspace mode to avoid a no‑op rename.In workspace mode,
onSaveis a no‑op, so the new Provider Name field gives the impression a rename will happen when it won’t. Also, passing an empty/whitespace name can lead to confusing UX or backend errors. Consider hiding (or disabling) the field whenworkspaceis set and only forwarding a trimmed non-emptynewName.✅ Suggested fix
const handleSubmit: FormEventHandler<HTMLFormElement> = (event) => { // make sure we don't bubble up the event to the parent event.stopPropagation() event.preventDefault() formMethods.handleSubmit((data) => { const { useAsDefault, reuseMachine, name } = data + const trimmedName = typeof name === "string" ? name.trim() : undefined + const newName = trimmedName && trimmedName !== providerID ? trimmedName : undefined configureProvider({ providerID, config: { reuseMachine: reuseMachine ?? false, useAsDefaultProvider: useAsDefault, options: filterOptions(data, allOptions), }, - newName: name as string, + newName, }) })(event) } @@ - <VStack align="start" spacing={8} position="relative" width="full"> - <FormControl> - <FormLabel>Provider Name</FormLabel> - <Input {...formMethods.register(FieldName.NAME)} /> - </FormControl> + <VStack align="start" spacing={8} position="relative" width="full"> + {!workspace && ( + <FormControl> + <FormLabel>Provider Name</FormLabel> + <Input {...formMethods.register(FieldName.NAME)} /> + </FormControl> + )}Also applies to: 221-236, 270-273
🧹 Nitpick comments (1)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
376-389: Consider using*quantifier for optional field pattern validation.The pattern uses
+(one or more), but this field is optional. While react-hook-form typically skips pattern validation for empty non-required fields, using*(zero or more) would make the intent explicit and guard against edge cases where the field value is an empty string.♻️ Suggested refinement
rules={{ pattern: { - value: ALLOWED_NAMES_REGEX, + value: /^[a-z0-9-]*$/, message: "Name can only contain lowercase letters, numbers and hyphens", },Alternatively, update
ALLOWED_NAMES_REGEXto use*if other usages also need to allow empty strings.
skevetter
requested changes
Jan 29, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 119-121: The error message returned in the provider rename
validation (where provider.ProviderNameRegEx.MatchString(newName) is checked)
uses "smaller case letters"; update that error string to use standard
terminology "lowercase letters" (e.g., change "provider name can only include
smaller case letters, numbers or dashes" to "provider name can only include
lowercase letters, numbers, or dashes") so the message is clearer and
consistent.
🧹 Nitpick comments (2)
pkg/workspace/provider.go (1)
443-460: Consider usingdeferfor unlock to prevent resource leaks.The current lock/unlock pattern doesn't use
defer, which could leave the workspace locked if a panic occurs betweenLock()andUnlock(). Additionally, errors fromUnlock()are silently ignored.♻️ Proposed refactor using defer
// Lock the workspace before switching provider err = client.Lock(ctx) if err != nil { return fmt.Errorf("failed to lock workspace %s: %w", workspace.ID, err) } + defer client.Unlock() // Update the provider name in the workspace configuration workspace.Provider.Name = newProviderName // Save the updated configuration err = providerpkg.SaveWorkspaceConfig(workspace) if err != nil { - client.Unlock() // Unlock before returning error return fmt.Errorf("failed to save workspace config: %w", err) } - // Unlock after successful save - client.Unlock() - return nile2e/framework/command.go (1)
391-399: Wrap the error for consistency with other helper methods.Most other methods in this file wrap errors with context (e.g.,
DevPodProviderRenameuses"devpod provider rename failed: %s"). Returning the raw error makes test failures harder to diagnose.♻️ Proposed fix
func (f *Framework) DevPodWorkspaceRebind( ctx context.Context, workspaceName, newProvider string, extraArgs ...string) error { args := []string{"workspace", "rebind", workspaceName, newProvider} args = append(args, extraArgs...) - _, _, err := f.ExecCommandCapture(ctx, args) - return err + _, stderr, err := f.ExecCommandCapture(ctx, args) + if err != nil { + return fmt.Errorf("devpod workspace rebind failed: %s", stderr) + } + return nil }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 424-454: The status check in SwitchProvider is done before
acquiring the client lock, creating a TOCTOU race; acquire the lock first and
then re-check status while holding it. Change the order in SwitchProvider to
call client.Lock() immediately after Get(...), defer client.Unlock() only after
successful Lock(), then call client.Status(...) and validate status
(client2.StatusStopped or StatusNotFound) before modifying
workspace.Provider.Name and calling providerpkg.SaveWorkspaceConfig(workspace);
preserve the same error wrapping for Lock/Status/Save failures.
🧹 Nitpick comments (1)
cmd/provider/rename.go (1)
114-124: Consider extracting the max name length as a constant.The magic number
32on line 122 could be extracted to a named constant (e.g.,MaxProviderNameLength) for improved maintainability and consistency with other validation constants in the codebase.♻️ Optional refactor
+const MaxProviderNameLength = 32 + // Run executes the command func (cmd *RenameCmd) Run(cobraCmd *cobra.Command, args []string) error { - oldName := args[0] newName := args[1] if provider.ProviderNameRegEx.MatchString(newName) { return fmt.Errorf("provider name can only include lowercase letters, numbers or dashes") } - if len(newName) > 32 { - return fmt.Errorf("provider name cannot be longer than 32 characters") + if len(newName) > MaxProviderNameLength { + return fmt.Errorf("provider name cannot be longer than %d characters", MaxProviderNameLength) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/workspace/rebind.go`:
- Around line 37-66: Before calling workspace.SwitchProvider, validate that the
requested provider exists in the loaded devPodConfig: check devPodConfig for
newProviderName (e.g., lookup devPodConfig.Providers[newProviderName] or call an
existing helper like devPodConfig.FindProvider/HasProvider) and return a clear
error if it does not exist; update RebindCmd.Run to perform this check after
loading devPodConfig and before workspace.SwitchProvider so you never persist a
workspace config pointing to a non-existent provider.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 123-131: The validateProviderName function currently allows empty
strings because ProviderNameRegEx doesn't catch them; add an explicit check at
the start of validateProviderName (the function named validateProviderName) to
return an error when newName == "" (e.g., "provider name cannot be empty")
before running ProviderNameRegEx.MatchString and the length check so empty names
are rejected and downstream filesystem/key issues are avoided.
In `@e2e/tests/workspace/rebind_test.go`:
- Around line 118-127: The test calls f.DevPodUp then immediately attempts
rebind, which can make the test flaky; update the test to wait until the
workspace is confirmed running before calling DevPodWorkspaceRebind. After
computing workspaceID (workspace.ToID(...)), poll the workspace status (e.g.,
via an existing helper like f.WaitForWorkspaceRunning or by repeatedly calling
f.GetWorkspaceStatus/GetWorkspace until status == "running" with a timeout) and
only then call f.DevPodWorkspaceRebind and framework.ExpectError; reference
DevPodUp, workspace.ToID, and DevPodWorkspaceRebind when adding the wait.
🧹 Nitpick comments (1)
e2e/tests/workspace/rebind_test.go (1)
29-36: Consider registering cleanup immediately to avoid resource leaks.Cleanup only happens at the end, so any earlier failure can leave providers/workspaces behind and make later runs flaky. Prefer
ginkgo.DeferCleanup(ordefer) right after creation.Also applies to: 83-89
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/provider/provider.go`:
- Around line 349-358: The test calls DevPodProviderRename immediately after
DevPodUp, causing flakiness because the workspace may not yet be in
State.Running; add an explicit wait for the workspace to reach Running (using
the same helper used in provider_docker.go — e.g., the test helper that waits
for State.Running or WaitForWorkspaceState/WaitForState on f) after DevPodUp and
before DevPodProviderRename so the rename assertion always runs against a fully
Running workspace.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 89-99: The function adjustDefaultProvider mutates
devPodConfig.Current().DefaultProvider to newName before calling
config.SaveConfig; if SaveConfig returns an error this leaves the in-memory
config pointing to newName and can break rollback logic—change the function so
that after a failed config.SaveConfig you restore
devPodConfig.Current().DefaultProvider back to oldName (do the restore before
returning the error), while keeping the existing logging via log.Default.Errorf
and log.Default.Infof; reference adjustDefaultProvider,
devPodConfig.Current().DefaultProvider, and config.SaveConfig to locate and
implement the rollback.
In `@e2e/tests/provider/provider.go`:
- Around line 328-333: After calling f.DevPodUp(ctx, tempDir) but before
f.DevPodSSH(...), add an explicit wait for the workspace to reach the Running
state (the same pattern used in the RENAME-5 test) to avoid flakiness; use the
existing helper that polls the workspace state (e.g., WaitForDevPodState or
whatever pattern is used elsewhere in this file) to wait until the dev pod is
"Running" and only then call f.DevPodSSH(ctx, tempDir, "echo 'hello'"), and make
sure to propagate and check the returned error with framework.ExpectNoError.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/provider/provider.go`:
- Around line 361-364: The code discards the first deletion error and then
asserts a stale err (from CopyToTempDir); update the two calls to
f.DevPodProviderDelete so each return value is assigned (e.g., err :=
f.DevPodProviderDelete(...)) and immediately checked with
framework.ExpectNoError(err) for providerName and renamedProviderName
respectively, ensuring you reference f.DevPodProviderDelete, providerName,
renamedProviderName, and framework.ExpectNoError rather than reusing the old
err.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@e2e/tests/provider/provider.go`:
- Around line 449-455: The cleanup races because DevPodProviderDelete is invoked
before the workspace teardown finishes; update the cleanup sequence around
f.DevPodWorkspaceDelete(ctx, tempDir) and f.DevPodProviderDelete(ctx,
providerName) to wait for the workspace to be fully deleted before deleting the
provider—implement a short retry/poll loop (with timeout) that checks for
workspace absence (mirror the RENAME-4 deletion wait logic used elsewhere) and
only call f.DevPodProviderDelete after the workspace deletion check succeeds;
keep the existing error checks (framework.ExpectNoError) and surface timeout as
a test failure if the workspace does not disappear in time.
- Around line 338-342: After calling DevPodProviderRename, immediately check and
handle its error (ensure the framework.ExpectNoError(err) is placed right after
DevPodProviderRename) before calling DevPodStatus; then call DevPodStatus(ctx,
tempDir), capture its error and only access or log status.State after verifying
the DevPodStatus error is nil (use framework.ExpectNoError for the DevPodStatus
error) so that failures from DevPodStatus are not masked when logging.
In `@pkg/workspace/provider.go`:
- Around line 595-599: The code currently mutates workspace.Provider.Name to
newProviderName and then calls providerpkg.SaveWorkspaceConfig(workspace); if
SaveWorkspaceConfig returns an error, revert workspace.Provider.Name back to its
original value before returning the error to avoid leaving the in-memory
workspace in a mutated state. To fix, capture the original name (e.g., oldName
:= workspace.Provider.Name) before assignment, assign newProviderName, call
providerpkg.SaveWorkspaceConfig(workspace), and on any non-nil err set
workspace.Provider.Name = oldName and then return the wrapped error.
- Around line 584-599: After acquiring the workspace config client (Get) and
successfully calling client.Lock(), validate the runtime state before changing
provider: call the client's status method (e.g., client.Status(ctx) or
client.GetStatus(ctx)) and if the returned state is neither "Stopped" nor
"NotFound" return an error (ensuring the defer client.Unlock() still runs) to
prevent switching providers while the workspace is running; only then set
workspace.Provider.Name = newProviderName and call
providerpkg.SaveWorkspaceConfig(workspace).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 595-598: The error message returned when client.Status(...) fails
contains a typo ("sataus"); update the fmt.Errorf in the Status call handling
(the block that calls client.Status(ctx, client2.StatusOptions{ContainerStatus:
true})) to read "failed to get status for workspace %s: %w" and keep the same
workspace.ID and wrapped err values.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 600-633: The code uses the undefined alias providerpkg (e.g.,
providerpkg.Workspace and providerpkg.SaveWorkspaceConfig) while the package is
already imported as provider; update SwitchProvider and any other symbols to use
the existing provider import: change the parameter type from
*providerpkg.Workspace to *provider.Workspace and replace
providerpkg.SaveWorkspaceConfig with provider.SaveWorkspaceConfig (and any other
providerpkg.* references) so the code builds and goimports no longer tries to
add a conflicting alias.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 612-627: The lock is acquired with client.Lock(...) but defer
client.Unlock() is registered too late, causing a lock leak on early returns;
move the defer client.Unlock() immediately after the successful client.Lock(ctx)
call so the unlock is guaranteed even if subsequent calls (client.Status(ctx,
client2.StatusOptions{ContainerStatus: true}) or the status checks) return
early; update the block around client.Lock, client.Unlock, client.Status and the
workspace state checks (referencing client.Lock, client.Unlock, client.Status,
client2.StatusOptions and workspace.ID) so Unlock is always deferred right after
Lock succeeds.
This comment was marked as outdated.
This comment was marked as outdated.
blstjohnson
force-pushed
the
feature/274-rename
branch
from
cc29619 to
b3b2aa0
Compare
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- command to rename provider - command to rebind workspace to another provider - pkg function to switch provider - UI support for provider rename
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
skevetter
force-pushed
the
feature/274-rename
branch
from
b3b2aa0 to
70fd82a
Compare
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/tests/provider/provider.go (1)
379-384:⚠️ Potential issue | 🟡 MinorAdd explicit wait for Running state before SSH.
Similar to RENAME-5 which waits for workspace state, this test calls
DevPodSSHimmediately afterDevPodUpwithout verifying the workspace has reached Running state. This can cause test flakiness if the workspace isn't fully ready.⏱️ Suggested wait pattern
// Start the workspace with the new provider and verify it's accessible. err = f.DevPodUp(ctx, tempDir) framework.ExpectNoError(err) + // Wait for the workspace to reach running state. + gomega.Eventually(func() string { + status, err := f.DevPodStatus(ctx, tempDir) + if err != nil { + return "error" + } + return string(status.State) + }).WithTimeout(30 * time.Second). + WithPolling(1 * time.Second). + Should(gomega.Equal("Running")) + _, err = f.DevPodSSH(ctx, tempDir, "echo 'hello'") framework.ExpectNoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/provider/provider.go` around lines 379 - 384, The test calls DevPodSSH immediately after DevPodUp which can race before the workspace reaches Running; add an explicit wait for the Running state after f.DevPodUp and before f.DevPodSSH. Use the existing helper that waits for workspace state (e.g. f.WaitForWorkspaceState or f.WaitForDevPodState) or implement a short poll loop that checks the workspace status until it equals "Running" (or workspacev1alpha1.StateRunning) and only then call f.DevPodSSH(ctx, tempDir, ...). Ensure the wait returns an error on timeout and propagate it to framework.ExpectNoError so the test fails cleanly if the workspace never becomes Running.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`:
- Around line 114-121: The rename operation is not awaited, so navigation via
navigate(Routes.toProvider(newName)) can occur before rename.run({
oldProviderID: providerID, newProviderID: newName }) completes; update the
ConfigureProviderOptionsForm logic to await rename.run and handle errors (e.g.,
wrap in try/catch), only calling navigate(...) after the awaited rename
succeeds, and on failure invalidate or refresh via
queryClient.invalidateQueries(QueryKeys.PROVIDERS) or surface an error to the
user.
In `@e2e/tests/provider/provider.go`:
- Around line 538-539: The unchecked type assertions for beforeVal and afterVal
(assigned to beforeMap and afterMap) can panic if the JSON shape changes;
replace them with safe comma-ok assertions (e.g., v, ok :=
beforeVal.(map[string]any) and handle !ok by failing the test with a clear
message) or use Gomega matchers
(Expect(beforeVal).To(BeAssignableToTypeOf(map[string]any{})) then cast) so the
test reports a readable failure instead of panicking; apply the same change for
afterVal/afterMap and ensure subsequent code uses the guarded variables.
---
Duplicate comments:
In `@e2e/tests/provider/provider.go`:
- Around line 379-384: The test calls DevPodSSH immediately after DevPodUp which
can race before the workspace reaches Running; add an explicit wait for the
Running state after f.DevPodUp and before f.DevPodSSH. Use the existing helper
that waits for workspace state (e.g. f.WaitForWorkspaceState or
f.WaitForDevPodState) or implement a short poll loop that checks the workspace
status until it equals "Running" (or workspacev1alpha1.StateRunning) and only
then call f.DevPodSSH(ctx, tempDir, ...). Ensure the wait returns an error on
timeout and propagate it to framework.ExpectNoError so the test fails cleanly if
the workspace never becomes Running.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ac9b3d2-a3e5-477e-81e5-4f3f0bad2eed
📒 Files selected for processing (20)
cmd/provider/add.gocmd/provider/provider.gocmd/provider/rename.godesktop/src/client/constants.tsdesktop/src/client/providers/client.tsdesktop/src/client/providers/providerCommands.tsdesktop/src/contexts/DevPodContext/useProviderManager.tsdesktop/src/lib/validation.tsdesktop/src/types.tsdesktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsxdesktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsxdesktop/src/views/Providers/AddProvider/helpers.tsdesktop/src/views/Providers/Provider.tsxdocs/pages/managing-providers/rename-provider.mddocs/sidebars.jse2e/e2e_suite_test.goe2e/framework/command.goe2e/tests/provider/provider.goe2e/tests/workspace/workspace.goe2e/tests/workspace/workspace_suite_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/provider/add.go
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/pages/managing-providers/rename-provider.md
- desktop/src/lib/validation.ts
- docs/sidebars.js
- e2e/tests/workspace/workspace.go
- desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx
- desktop/src/views/Providers/AddProvider/helpers.ts
- e2e/framework/command.go
- desktop/src/types.ts
- e2e/e2e_suite_test.go
- desktop/src/client/constants.ts
- desktop/src/client/providers/providerCommands.ts
- e2e/tests/workspace/workspace_suite_test.go
Comment on lines
+538
to
+539
| beforeMap := beforeVal.(map[string]any) | ||
| afterMap := afterVal.(map[string]any) |
There was a problem hiding this comment.
Guard type assertions to prevent test panics.
These type assertions will panic if the JSON structure differs from expected, causing cryptic test failures. Use type assertion with ok check or gomega matchers for safer handling.
🛡️ Suggested defensive approach
// Verify option values are preserved
for key, beforeVal := range beforeOpts {
afterVal, exists := afterOpts[key]
gomega.Expect(exists).
To(gomega.BeTrue(), "option %s should exist after rename", key)
- beforeMap := beforeVal.(map[string]any)
- afterMap := afterVal.(map[string]any)
+ beforeMap, ok := beforeVal.(map[string]any)
+ gomega.Expect(ok).To(gomega.BeTrue(), "option %s should be a map", key)
+ afterMap, ok := afterVal.(map[string]any)
+ gomega.Expect(ok).To(gomega.BeTrue(), "option %s after rename should be a map", key)
beforeV, hasBefore := beforeMap["value"]
afterV, hasAfter := afterMap["value"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/provider/provider.go` around lines 538 - 539, The unchecked type
assertions for beforeVal and afterVal (assigned to beforeMap and afterMap) can
panic if the JSON shape changes; replace them with safe comma-ok assertions
(e.g., v, ok := beforeVal.(map[string]any) and handle !ok by failing the test
with a clear message) or use Gomega matchers
(Expect(beforeVal).To(BeAssignableToTypeOf(map[string]any{})) then cast) so the
test reports a readable failure instead of panicking; apply the same change for
afterVal/afterMap and ensure subsequent code uses the guarded variables.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/tests/provider/provider.go (1)
212-221: Consider using the test contextctxconsistently.Several calls here use
context.Background()instead of the providedctx. Usingctxwould ensure these operations respect the test's timeout and cancellation, making the test more predictable under CI timeout conditions.♻️ Suggested consistency fix
- err = f.DevPodProviderUse(context.Background(), providerName) + err = f.DevPodProviderUse(ctx, providerName) framework.ExpectNoError(err) - err = f.DevPodProviderRename(context.Background(), providerName, renamedProviderName) + err = f.DevPodProviderRename(ctx, providerName, renamedProviderName) framework.ExpectNoError(err) - err = f.DevPodProviderUse(context.Background(), providerName) + err = f.DevPodProviderUse(ctx, providerName) framework.ExpectError(err) - err = f.DevPodProviderUse(context.Background(), renamedProviderName) + err = f.DevPodProviderUse(ctx, renamedProviderName) framework.ExpectNoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/provider/provider.go` around lines 212 - 221, Replace usages of context.Background() with the test-provided ctx so the provider operations respect the test timeout/cancellation; specifically update the calls to f.DevPodProviderUse and f.DevPodProviderRename (the three spots currently passing context.Background()) to pass ctx instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`:
- Around line 230-239: The provider rename is silently dropped when an empty
name is submitted because name is coerced to undefined; add a required
validation rule for the name field so the form cannot submit an empty string.
Update the validation for the "name" input used to build data (the same field
referenced where configureProvider is called) to require non-empty trimmed text,
and ensure the submit handler (where configureProvider is invoked) only uses
newName when the validated name is present; apply the same change to the other
submit block around the 273-299 region that also reads `name` and calls
`configureProvider`.
- Around line 109-119: handleSave currently calls client.providers.configure
then await rename.run, but if configure succeeds and rename.run fails the UI is
left in an error state while the backend persisted the config; catch errors from
rename.run, refresh provider data and surface a partial-success result instead
of treating it as a full rollback. Concretely, wrap the rename.run call in its
own try/catch inside handleSave: on rename failure call
queryClient.invalidateQueries(QueryKeys.PROVIDERS) (and/or refetch the specific
provider), navigate to Routes.toProvider(providerID) or keep the form on the
updated provider, and surface a user-facing partial-success message; if rename
succeeds behave as before (navigate to Routes.toProvider(newName) and replace).
Ensure you reference client.providers.configure, rename.run,
queryClient.invalidateQueries and Routes.toProvider when making the changes.
In `@e2e/tests/provider/provider.go`:
- Around line 295-297: The test title for the ginkgo.It case "should rename a
provider with an associated stopped workspace" is inaccurate because the
workspace is started via DevPodUp before the rename; either change the test
description to "should rename a provider with an associated running workspace"
or insert a call to f.DevPodStop (the helper used to stop the dev pod)
immediately after DevPodUp and before the provider rename so the workspace is
actually stopped; update assertions accordingly if you choose to stop the
workspace.
---
Nitpick comments:
In `@e2e/tests/provider/provider.go`:
- Around line 212-221: Replace usages of context.Background() with the
test-provided ctx so the provider operations respect the test
timeout/cancellation; specifically update the calls to f.DevPodProviderUse and
f.DevPodProviderRename (the three spots currently passing context.Background())
to pass ctx instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5fbe9f7-8c29-43af-bbdb-5c492201676c
📒 Files selected for processing (2)
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsxe2e/tests/provider/provider.go
Comment on lines
109
to
+119
| const handleSave: TConfigureOptionsFormProps["onSave"] = useCallback( | ||
| async ({ providerID, config }) => { | ||
| async ({ providerID, config, newName }) => { | ||
| // Configure first, then rename to prevent UI route inconsistencies | ||
| ;(await client.providers.configure(providerID, config)).unwrap() | ||
| await queryClient.invalidateQueries(QueryKeys.PROVIDERS) | ||
|
|
||
| if (newName && newName !== providerID) { | ||
| await rename.run({ oldProviderID: providerID, newProviderID: newName }) | ||
| navigate(Routes.toProvider(newName), { replace: true }) | ||
| } else { | ||
| await queryClient.invalidateQueries(QueryKeys.PROVIDERS) | ||
| } |
There was a problem hiding this comment.
Handle the configure-succeeded / rename-failed path explicitly.
This save flow is now non-atomic: if configure() succeeds and rename.run() fails, the mutation rejects even though the provider config was already persisted. That leaves the form in an error state while the backend has partially applied the user's changes. Cross-file, useProviders().rename only rolls back the optimistic provider list update; it does not undo the earlier configure write.
At minimum, catch the rename failure here, refresh provider data, and surface a partial-success message instead of treating it like a full rollback.
💡 Minimal mitigation
const handleSave: TConfigureOptionsFormProps["onSave"] = useCallback(
async ({ providerID, config, newName }) => {
- // Configure first, then rename to prevent UI route inconsistencies
;(await client.providers.configure(providerID, config)).unwrap()
- if (newName && newName !== providerID) {
- await rename.run({ oldProviderID: providerID, newProviderID: newName })
- navigate(Routes.toProvider(newName), { replace: true })
- } else {
- await queryClient.invalidateQueries(QueryKeys.PROVIDERS)
+ try {
+ if (newName && newName !== providerID) {
+ await rename.run({ oldProviderID: providerID, newProviderID: newName })
+ navigate(Routes.toProvider(newName), { replace: true })
+ } else {
+ await queryClient.invalidateQueries(QueryKeys.PROVIDERS)
+ }
+ } catch {
+ await queryClient.invalidateQueries(QueryKeys.PROVIDERS)
+ throw new Error("Provider options were saved, but renaming the provider failed.")
}
},
[queryClient, navigate, rename]
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`
around lines 109 - 119, handleSave currently calls client.providers.configure
then await rename.run, but if configure succeeds and rename.run fails the UI is
left in an error state while the backend persisted the config; catch errors from
rename.run, refresh provider data and surface a partial-success result instead
of treating it as a full rollback. Concretely, wrap the rename.run call in its
own try/catch inside handleSave: on rename failure call
queryClient.invalidateQueries(QueryKeys.PROVIDERS) (and/or refetch the specific
provider), navigate to Routes.toProvider(providerID) or keep the form on the
updated provider, and surface a user-facing partial-success message; if rename
succeeds behave as before (navigate to Routes.toProvider(newName) and replace).
Ensure you reference client.providers.configure, rename.run,
queryClient.invalidateQueries and Routes.toProvider when making the changes.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
109-122:⚠️ Potential issue | 🟠 MajorHandle the configure-succeeded / rename-failed path explicitly.
The save flow is non-atomic: if
configure()succeeds butrename.run()throws, the mutation rejects even though provider config was already persisted. The form shows an error state while the backend has partially applied changes. Users won't know their config was saved.Catch rename failure separately, refresh provider data, and surface a partial-success message.
🛡️ Minimal mitigation
const handleSave: TConfigureOptionsFormProps["onSave"] = useCallback( async ({ providerID, config, newName }) => { // Configure first, then rename to prevent UI route inconsistencies ;(await client.providers.configure(providerID, config)).unwrap() - if (newName && newName !== providerID) { - await rename.run({ oldProviderID: providerID, newProviderID: newName }) - navigate(Routes.toProvider(newName), { replace: true }) - } else { - await queryClient.invalidateQueries({ queryKey: QueryKeys.PROVIDERS }) + try { + if (newName && newName !== providerID) { + await rename.run({ oldProviderID: providerID, newProviderID: newName }) + navigate(Routes.toProvider(newName), { replace: true }) + } else { + await queryClient.invalidateQueries({ queryKey: QueryKeys.PROVIDERS }) + } + } catch { + await queryClient.invalidateQueries({ queryKey: QueryKeys.PROVIDERS }) + throw new Error("Provider options were saved, but renaming failed.") } }, [queryClient, navigate, rename] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx` around lines 109 - 122, The handleSave flow in ConfigureProviderOptionsForm is non-atomic: if client.providers.configure(providerID, config) succeeds but rename.run(...) throws, the UI treats it as a full failure even though the config persisted; to fix, wrap the rename step in its own try/catch inside handleSave (after the successful configure call), on rename failure call queryClient.invalidateQueries({ queryKey: QueryKeys.PROVIDERS }) and re-fetch the specific provider (or call the same provider read query) to refresh persisted config, then surface a partial-success notification/error message indicating config saved but rename failed (use existing navigate(Routes.toProvider(...)) only on successful rename), and ensure you still return/resolve the save handler appropriately so the form doesn't stay in error state.e2e/tests/provider/provider.go (2)
328-332:⚠️ Potential issue | 🟡 MinorWait for
Runningbefore the rename step.This case is meant to cover a running workspace, but the rename happens immediately after
DevPodUp(). Without the sameEventually(... == "Running")guard you use later in the spec, this can run against a transitional workspace and flake.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/provider/provider.go` around lines 328 - 332, After calling f.DevPodUp(ctx, tempDir) and before f.DevPodProviderRename(...), add the same wait used later in the spec to assert the pod reached the "Running" phase (i.e., use the existing Eventually check that waits for phase == "Running"); this ensures DevPodProviderRename operates on a fully running workspace rather than a transitional state. Locate the sequence around DevPodUp and DevPodProviderRename in provider.go and insert the identical Eventually/expectation logic used elsewhere in the file to wait for "Running" before proceeding with the rename.
431-437:⚠️ Potential issue | 🟡 MinorGuard the option-map type assertions.
beforeVal.(map[string]any)andafterVal.(map[string]any)will panic on any schema drift in the JSON output, which makes this test much harder to diagnose than a normal assertion failure.Suggested change
for key, beforeVal := range beforeOpts { afterVal, exists := afterOpts[key] gomega.Expect(exists). To(gomega.BeTrue(), "option %s should exist after rename", key) - beforeMap := beforeVal.(map[string]any) - afterMap := afterVal.(map[string]any) + beforeMap, ok := beforeVal.(map[string]any) + gomega.Expect(ok).To(gomega.BeTrue(), "option %s should be an object", key) + afterMap, ok := afterVal.(map[string]any) + gomega.Expect(ok).To(gomega.BeTrue(), "option %s after rename should be an object", key) beforeV, hasBefore := beforeMap["value"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/provider/provider.go` around lines 431 - 437, The test currently uses direct type assertions beforeVal.(map[string]any) and afterVal.(map[string]any) which will panic on schema drift; change these to safe type checks using the "ok" idiom and gomega.Expect to assert the value is a map[string]any before casting (e.g., check that beforeVal is a map[string]any and afterVal is a map[string]any, fail the test with a clear message if not), then use the safely-cast variables for beforeMap and afterMap; reference the beforeOpts/afterOpts loop and the beforeVal/afterVal variables to locate where to add the guarded assertions.
🧹 Nitpick comments (1)
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
556-589: Update comments to reflect expanded dependency arrays.The comments state "only rerun when suggestedOptions changes" and "only rerun when workspace options change," but the dependency arrays now include additional values (
formMethods,refreshSubOptionsMutation,setIsEditingWorkspaceOptions). While these references are typically stable in react-hook-form and react-query, the comments are misleading.📝 Update comments
if (changedOptions.length > 0) { refreshSubOptionsMutation({ options: changedOptions.reduce((acc, o) => { const option = { value: opts[o] } as unknown as TProviderOption return { ...acc, [o]: option } }, {} as TProviderOptions), }) } - // only rerun when suggestedOptions changes + // primarily reruns when suggestedOptions changes (other deps are stable references) }, [suggestedOptions, formMethods, refreshSubOptionsMutation])if (Object.keys(changedOptions).length > 0) { setIsEditingWorkspaceOptions(true) refreshSubOptionsMutation({ options: changedOptions }) } - // only rerun when workspace options change + // primarily reruns when workspace options change (other deps are stable references) }, [ workspace?.provider?.options, formMethods, refreshSubOptionsMutation, setIsEditingWorkspaceOptions, ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx` around lines 556 - 589, Update the trailing comments on both useEffect blocks to accurately reflect their dependency arrays: for the first useEffect mention that it reruns when suggestedOptions or any of the listed stable references change (suggestedOptions, formMethods, refreshSubOptionsMutation), and for the second useEffect mention it reruns when workspace?.provider?.options or any of the listed stable references change (workspace?.provider?.options, formMethods, refreshSubOptionsMutation, setIsEditingWorkspaceOptions); reference the surrounding useEffect blocks and variable names (suggestedOptions, workspace?.provider?.options, formMethods, refreshSubOptionsMutation, setIsEditingWorkspaceOptions) so the comments are no longer misleading about dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/provider/provider.go`:
- Around line 212-220: Replace uses of context.Background() with the
spec-provided ctx for all provider CLI calls so they respect test
timeouts/cancellation: update calls to DevPodProviderUse, DevPodProviderRename
(and other provider calls in this file that currently pass context.Background())
to pass ctx instead; keep DevPodProviderDelete behavior as-is if already using
ctx but ensure consistency across f.DevPodProviderUse, f.DevPodProviderRename,
f.DevPodProviderDelete, and any other f.DevPod* invocations so they all accept
the spec ctx for proper cancellation.
---
Duplicate comments:
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`:
- Around line 109-122: The handleSave flow in ConfigureProviderOptionsForm is
non-atomic: if client.providers.configure(providerID, config) succeeds but
rename.run(...) throws, the UI treats it as a full failure even though the
config persisted; to fix, wrap the rename step in its own try/catch inside
handleSave (after the successful configure call), on rename failure call
queryClient.invalidateQueries({ queryKey: QueryKeys.PROVIDERS }) and re-fetch
the specific provider (or call the same provider read query) to refresh
persisted config, then surface a partial-success notification/error message
indicating config saved but rename failed (use existing
navigate(Routes.toProvider(...)) only on successful rename), and ensure you
still return/resolve the save handler appropriately so the form doesn't stay in
error state.
In `@e2e/tests/provider/provider.go`:
- Around line 328-332: After calling f.DevPodUp(ctx, tempDir) and before
f.DevPodProviderRename(...), add the same wait used later in the spec to assert
the pod reached the "Running" phase (i.e., use the existing Eventually check
that waits for phase == "Running"); this ensures DevPodProviderRename operates
on a fully running workspace rather than a transitional state. Locate the
sequence around DevPodUp and DevPodProviderRename in provider.go and insert the
identical Eventually/expectation logic used elsewhere in the file to wait for
"Running" before proceeding with the rename.
- Around line 431-437: The test currently uses direct type assertions
beforeVal.(map[string]any) and afterVal.(map[string]any) which will panic on
schema drift; change these to safe type checks using the "ok" idiom and
gomega.Expect to assert the value is a map[string]any before casting (e.g.,
check that beforeVal is a map[string]any and afterVal is a map[string]any, fail
the test with a clear message if not), then use the safely-cast variables for
beforeMap and afterMap; reference the beforeOpts/afterOpts loop and the
beforeVal/afterVal variables to locate where to add the guarded assertions.
---
Nitpick comments:
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`:
- Around line 556-589: Update the trailing comments on both useEffect blocks to
accurately reflect their dependency arrays: for the first useEffect mention that
it reruns when suggestedOptions or any of the listed stable references change
(suggestedOptions, formMethods, refreshSubOptionsMutation), and for the second
useEffect mention it reruns when workspace?.provider?.options or any of the
listed stable references change (workspace?.provider?.options, formMethods,
refreshSubOptionsMutation, setIsEditingWorkspaceOptions); reference the
surrounding useEffect blocks and variable names (suggestedOptions,
workspace?.provider?.options, formMethods, refreshSubOptionsMutation,
setIsEditingWorkspaceOptions) so the comments are no longer misleading about
dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc8d1140-ef96-4d24-a691-9219fdb7adf9
📒 Files selected for processing (3)
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsxe2e/tests/provider/provider.gopkg/workspace/provider.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/workspace/provider.go
Signed-off-by: Samuel K <skevetter@pm.me>
skevetter
force-pushed
the
feature/274-rename
branch
from
748e6e6 to
d165cf7
Compare
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
skevetter
changed the title
skevetter
approved these changes
Mar 8, 2026
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.
New feature to rename providers #274
new command for provider - rename accepting old name and new name, that will validate new name and change the name of the provider
new command workspace rebind - to rebind workspace to a new provider, runs inside provider rename, but can also be used to transfer workspace to a different provider
Summary by CodeRabbit
New Features
User Experience
Documentation
Tests