Skip to content

refactor(ide/vscode): flavors to configuration struct#370

Merged
skevetter merged 4 commits intomainfrom
refactor-open-vscode
Jan 25, 2026
Merged

refactor(ide/vscode): flavors to configuration struct#370
skevetter merged 4 commits intomainfrom
refactor-open-vscode

Conversation

@skevetter
Copy link
Copy Markdown
Owner

@skevetter skevetter commented Jan 25, 2026

Signed-off-by: Samuel K skevetter@pm.me

Summary by CodeRabbit

  • Refactor
    • Centralized IDE open API to use a single parameter object and data-driven flavor configs.
    • Unified browser and CLI launch flows to try CLI first, then fallback to browser, with aggregated error reporting.
    • Consolidated per-flavor handling, reduced duplication, and added automatic extension checks and clearer success/error logging.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Samuel K <skevetter@pm.me>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 25, 2026

📝 Walkthrough

Walkthrough

Replaces the old per-argument VS Code open API with a single exported OpenParams struct and vscode.Open(ctx, params). Centralizes flavor handling via openConfig/openConfigs, consolidates CLI vs browser launch logic, and adds helpers for CLI path resolution, argument construction, extension checks/installation, and aggregated error/log handling.

Changes

Cohort / File(s) Summary
API Consumer
cmd/up.go
All IDE open calls now construct an OpenParams value (Workspace, Folder, NewWindow, Flavor, Log) and call vscode.Open(ctx, params) instead of the previous multi-argument Open(...).
IDE Integration Implementation
pkg/ide/vscode/open.go
Replaced Open(ctx, workspace, folder, newWindow, flavor, log) with Open(ctx, OpenParams) and added exported OpenParams. Added openConfig/openConfigs map for flavor-specific CLI/browser data, refactored launch flow to try CLI then browser, and introduced helpers: getCLIPath, buildOpenArgs, listInstalledExtensions, ensureSSHExtension plus related logging/error aggregation.

Sequence Diagram(s)

sequenceDiagram
    participant UpCmd as Up Command
    participant OpenAPI as vscode.Open (OpenParams)
    participant OS as Operating System
    participant CLI as VSCode CLI
    participant Browser as Web Browser
    participant ExtSvc as Extension Installer

    UpCmd->>OpenAPI: Call Open(ctx, OpenParams{Workspace, Folder, NewWindow, Flavor, Log})
    OpenAPI->>OpenAPI: lookup openConfigs[Flavor]
    alt CLI available
        OpenAPI->>CLI: resolve CLI path (getCLIPath)
        CLI-->>OpenAPI: cliPath
        OpenAPI->>CLI: listInstalledExtensions(cliPath)
        CLI-->>OpenAPI: installedExtensions
        alt SSH/Containers extension missing
            OpenAPI->>ExtSvc: install extension via CLI
            ExtSvc-->>OpenAPI: install result
        end
        OpenAPI->>CLI: spawn CLI with built args (buildOpenArgs)
        CLI-->>OS: process spawn
        CLI-->>OpenAPI: result
    else CLI not available
        OpenAPI->>Browser: construct/open protocol URL (openViaBrowser)
        Browser-->>OS: open URL
        Browser-->>OpenAPI: result
    end
    OpenAPI-->>UpCmd: return error/success
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor(ide/vscode): flavors to configuration struct' accurately summarizes the main change: refactoring the vscode IDE package to move from flavor-based parameters to a configuration struct approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/ide/vscode/open.go`:
- Around line 105-109: The log call in the error path after open.Run(openURL)
uses the incorrect printf verb `%w`; replace it with a logging-safe verb like
`%v` or `%s` in the params.Log.Errorf call so the error is formatted correctly
(e.g., params.Log.Errorf("flavor %s is not installed on host device: %v",
params.Flavor.DisplayName(), err)). Leave the open.Run(openURL) error handling
and return err unchanged.
🧹 Nitpick comments (1)
pkg/ide/vscode/open.go (1)

169-177: Consider using CombinedOutput() for better error diagnostics.

Using Output() discards stderr, which may contain useful error information when extension installation fails. Other exec calls in this file use CombinedOutput().

Proposed fix
 func ensureSSHExtension(ctx context.Context, cliPath, sshExtension string, log log.Logger) error {
 	args := []string{"--install-extension", sshExtension}
 	log.Debugf("%s %s", cliPath, strings.Join(args, " "))
-	out, err := exec.CommandContext(ctx, cliPath, args...).Output()
+	out, err := exec.CommandContext(ctx, cliPath, args...).CombinedOutput()
 	if err != nil {
 		return command.WrapCommandError(out, err)
 	}
 	return nil
 }

Comment thread pkg/ide/vscode/open.go
Signed-off-by: Samuel K <skevetter@pm.me>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/ide/vscode/open.go`:
- Around line 146-150: Change listInstalledExtensions to accept a
context.Context parameter (update its signature: listInstalledExtensions(ctx
context.Context, cliPath, sshExtension string) ...) and replace exec.Command
with exec.CommandContext(ctx, cliPath, "--list-extensions") so the call respects
cancellation; also use CombinedOutput() instead of Output() so both stdout and
stderr are captured and passed into command.WrapCommandError on failure. Update
the call site in openViaCLI to pass the existing ctx into
listInstalledExtensions. Ensure any tests or callers are adjusted accordingly.

Comment thread pkg/ide/vscode/open.go Outdated
Signed-off-by: Samuel K <skevetter@pm.me>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/ide/vscode/open.go`:
- Around line 78-91: The Open function currently always tries openViaBrowser
even if the context was canceled or timed out; update Open to short‑circuit if
ctx has been canceled or if cliErr is a context error by checking ctx.Err() (or
using errors.Is(cliErr, context.Canceled) / context.DeadlineExceeded) after
openViaCLI returns, and return the appropriate error without calling
openViaBrowser when cancellation/timeouts occur; keep use of params.Log.Infof
and preserve returning errors.Join for non‑context failures.

Comment thread pkg/ide/vscode/open.go
Signed-off-by: Samuel K <skevetter@pm.me>
@skevetter
Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 25, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@skevetter skevetter merged commit f7abf21 into main Jan 25, 2026
38 checks passed
@skevetter skevetter deleted the refactor-open-vscode branch January 25, 2026 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant