fix(web,paths,server): WSL2-aware Open in IDE link#1504
Open
blankse wants to merge 2 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds WSL detection helpers to ChangesWSL Integration
Sequence DiagramsequenceDiagram
participant Browser as Browser (Windows)
participant ArchonServer as Archon Server (WSL2)
participant WebClient as Web Client
participant VSCode as VS Code
Browser->>ArchonServer: GET /api/health
ArchonServer->>ArchonServer: isWSL(), getWSLDistroName()
ArchonServer-->>Browser: { is_wsl: true, wsl_distro: "Ubuntu" }
Browser->>WebClient: receive health
WebClient->>WebClient: ideUri(path, { is_wsl, wsl_distro })
WebClient->>VSCode: window.open(vscode://vscode-remote/wsl+Ubuntu/<path>)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. ✨ 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: 1
🧹 Nitpick comments (1)
packages/web/src/lib/ide-uri.ts (1)
21-21: ⚡ Quick winExtract
envto a named interface instead of inline object typing.Please define a dedicated interface (e.g.,
IdeUriEnv) and use it in the function signature for consistency with the TypeScript guidelines.Proposed change
+interface IdeUriEnv { + is_wsl?: boolean; + wsl_distro?: string; +} + -export function ideUri(path: string, env?: { is_wsl?: boolean; wsl_distro?: string }): string { +export function ideUri(path: string, env?: IdeUriEnv): string {As per coding guidelines: "Use
interfacefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/ide-uri.ts` at line 21, The inline env object type in the ideUri function should be extracted to a named interface: create and export an interface (e.g., IdeUriEnv) with properties is_wsl?: boolean and wsl_distro?: string, replace the inline typing in the ideUri(path: string, env?: { ... }) signature with env?: IdeUriEnv, and update any imports/usage accordingly to keep types consistent across the module and follow TypeScript interface conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paths/src/archon-paths.test.ts`:
- Around line 95-101: The test title says "returns false when WSL_DISTRO_NAME is
unset and not on a WSL kernel" but only asserts a boolean; update the test to be
deterministic by mocking the kernel-check branch and then assert false (or
change the title to match the current assertion). Specifically, in the test that
references isWSL(), unset process.env.WSL_DISTRO_NAME, stub/mock the
kernel-release check used by isWSL() so it returns a non-WSL value (e.g.,
contains no "microsoft"), then assert expect(isWSL()).toBe(false);
alternatively, if you prefer not to mock, rename the test to reflect that it
only verifies a boolean return.
---
Nitpick comments:
In `@packages/web/src/lib/ide-uri.ts`:
- Line 21: The inline env object type in the ideUri function should be extracted
to a named interface: create and export an interface (e.g., IdeUriEnv) with
properties is_wsl?: boolean and wsl_distro?: string, replace the inline typing
in the ideUri(path: string, env?: { ... }) signature with env?: IdeUriEnv, and
update any imports/usage accordingly to keep types consistent across the module
and follow TypeScript interface conventions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c86ebc30-1858-405e-a025-c72816fb8ed0
📒 Files selected for processing (12)
packages/paths/src/archon-paths.test.tspackages/paths/src/archon-paths.tspackages/paths/src/index.tspackages/server/src/routes/api.tspackages/web/src/components/chat/ChatInterface.tsxpackages/web/src/components/dashboard/WorkflowRunCard.tsxpackages/web/src/components/dashboard/WorkflowRunGroup.tsxpackages/web/src/components/layout/Header.tsxpackages/web/src/lib/api.tspackages/web/src/lib/ide-uri.test.tspackages/web/src/lib/ide-uri.tspackages/web/src/routes/DashboardPage.tsx
Collaborator
Open
4 tasks
blankse
force-pushed
the
fix/1502-wsl-vscode-uri
branch
3 times, most recently
from
5e88c15 to
8a9c3b3
Compare
) The Open-in-IDE buttons emit a `vscode://file/<path>` URI. When Archon runs inside WSL2 and the browser is on the Windows host, that URI hands a Linux path to Windows VS Code, which can't resolve it — the IDE launches with an empty / "file not found" window. The correct form for that case is: vscode://vscode-remote/wsl+<distro>/<path> This change wires the detection from the server through to the link construction so each setup gets the right URI: paths - isWSL(): two-signal detection (WSL_DISTRO_NAME env var, then /proc/sys/kernel/osrelease containing "microsoft"). - getWSLDistroName(): exposes WSL_DISTRO_NAME for the URI. - Both exported from @archon/paths and tested. server - GET /api/health gains `is_wsl` (always present) and `wsl_distro` (only when known). Schema updated accordingly. web - HealthResponse type in src/lib/api.ts mirrors the server fields. - New helper src/lib/ide-uri.ts builds the right URI based on {is_wsl, wsl_distro}, with tests for each branch (default, WSL2 path, broken inputs, distro-name encoding). - Header, WorkflowRunCard, WorkflowRunGroup, ChatInterface and DashboardPage now plumb the two new health fields the same way they already plumb is_docker, and call ideUri() instead of inlining a vscode://file/... template literal. Tests: paths and web tests pass; the existing api.health tests still pass (the new fields don't break the response-schema assertions — HealthResponse fields are optional except is_docker / is_wsl). Note: api.generated.d.ts not regenerated here because the manual HealthResponse interface in src/lib/api.ts is what the components consume; running `bun --filter @archon/web generate:types` against a live dev server will refresh the generated types in a follow-up.
blankse
force-pushed
the
fix/1502-wsl-vscode-uri
branch
from
8a9c3b3 to
a80f30c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/lib/api.ts (1)
427-456:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOut-of-scope changes:
saveWorkflowanddeleteWorkflowmodifications.The additions of the optional
source?: WorkflowSourceparameter and the conditional query-string logic forsource === 'global'are not mentioned in the PR objectives (issue#1502) or the current layer description ("thread props into UI components"). No components in this layer use the newsourceparameter, and the PR focuses exclusively on WSL detection and IDE URI generation.These changes should either be removed or split into a separate PR with clear justification and usage examples.
🤖 Prompt for 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. In `@packages/web/src/lib/api.ts` around lines 427 - 456, The additions of the optional source?: WorkflowSource parameter and the source === 'global' query logic are out-of-scope; revert saveWorkflow and deleteWorkflow to their original signatures and behavior by removing the source parameter and any conditional query-string code in both functions (restore building params only from cwd), and update any touched call sites/tests accordingly; alternatively, if you intend to keep this feature, extract the source changes into a separate PR with a clear description, usage examples, and updates to any callers that need the new source argument (reference functions saveWorkflow and deleteWorkflow).
♻️ Duplicate comments (1)
packages/server/src/routes/api.ts (1)
445-457:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame path issue in delete workflow handler.
The delete handler has the same potential nested path bug as the save handler (see comment on lines 384-386, 406-410). When
targetSource !== 'global'and no project context exists,workingDirfalls back togetArchonHome(), producingjoin(getArchonHome(), getWorkflowFolderSearchPaths()[0], ...), which may create a nested.archon/.archon/workflowspath.Apply the same fix here: default to global scope or handle the no-project-context case explicitly.
Also applies to: 459-462
🤖 Prompt for 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. In `@packages/server/src/routes/api.ts` around lines 445 - 457, The delete workflow handler constructs workingDir using targetSource, getArchonHome() and getWorkflowFolderSearchPaths(), which can produce a nested .archon/.archon/workflows when targetSource !== 'global' and project context is missing; fix it in the delete workflow handler by using the same approach as the save handler: if no project context exists default the scope to 'global' (or explicitly branch to build workingDir from getArchonHome() alone) instead of joining getArchonHome() with getWorkflowFolderSearchPaths()[0]; update the code that computes workingDir (and the analogous code around lines 459-462) to check for project context before using getWorkflowFolderSearchPaths() so paths aren’t nested.
🧹 Nitpick comments (2)
packages/web/src/routes/DashboardPage.tsx (1)
378-379: 💤 Low valueConsider defaulting
isWslfor consistency.
ChatInterface.tsxexplicitly defaultsisWsltofalsewhenhealthis unavailable:const isWsl = health?.is_wsl ?? false;Here, you pass
health?.is_wsldirectly, which may beundefineduntil the health query resolves. Both approaches work (the props andideUrishould handleundefined), but applying the same defensive default across all call sites would improve consistency and make the fallback behavior explicit.Suggested change for consistency
+ const isWsl = health?.is_wsl ?? false; + const wslDistro = health?.wsl_distro; + // Split into active and history (from server-filtered results) ... <WorkflowRunCard key={run.id} run={run} isDocker={health?.is_docker} - isWsl={health?.is_wsl} - wslDistro={health?.wsl_distro} + isWsl={isWsl} + wslDistro={wslDistro}(Apply similarly at lines 397–398.)
Also applies to: 397-398
🤖 Prompt for 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. In `@packages/web/src/routes/DashboardPage.tsx` around lines 378 - 379, The DashboardPage is passing health?.is_wsl directly to the ChatInterface, which can be undefined until the health query resolves; make this consistent with ChatInterface.tsx by defaulting the prop when calling the component (use health?.is_wsl ?? false for the isWsl prop) and apply the same defensive default at the other call site noted (the second occurrence around where wslDistro/isWsl are passed). Update the prop expression(s) for isWsl while leaving wslDistro as-is, so both call sites explicitly pass a boolean fallback for isWsl to match ChatInterface's expected behavior.packages/server/src/routes/api.ts (1)
2760-2761: ⚡ Quick winEliminate duplicate call to
getWSLDistroName().Line 2761 calls
getWSLDistroName()twice—once in the condition and once in the value. Store the result in a variable to follow DRY and avoid redundant filesystem/env reads.♻️ Refactor to eliminate duplicate call
const stats = lockManager.getStats(); const runningWorkflowRows = await workflowDb.getRunningWorkflows(); + const wslDistro = getWSLDistroName(); // Merge lock-based and DB-based active tracking. ... return c.json({ status: 'ok', ... is_docker: isDocker(), is_wsl: isWSL(), - ...(getWSLDistroName() ? { wsl_distro: getWSLDistroName() } : {}), + ...(wslDistro ? { wsl_distro: wslDistro } : {}), activePlatforms: activePlatforms ? [...activePlatforms] : ['Web'], });🤖 Prompt for 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. In `@packages/server/src/routes/api.ts` around lines 2760 - 2761, The code calls getWSLDistroName() twice when building the object (condition and value); cache the result in a local variable (e.g., const wslDistro = getWSLDistroName()) before the object literal and then use isWSL() for is_wsl and wsl_distro: wslDistro only when wslDistro is truthy so you remove the duplicate call and preserve the same conditional behavior around getWSLDistroName().
🤖 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.
Outside diff comments:
In `@packages/web/src/lib/api.ts`:
- Around line 427-456: The additions of the optional source?: WorkflowSource
parameter and the source === 'global' query logic are out-of-scope; revert
saveWorkflow and deleteWorkflow to their original signatures and behavior by
removing the source parameter and any conditional query-string code in both
functions (restore building params only from cwd), and update any touched call
sites/tests accordingly; alternatively, if you intend to keep this feature,
extract the source changes into a separate PR with a clear description, usage
examples, and updates to any callers that need the new source argument
(reference functions saveWorkflow and deleteWorkflow).
---
Duplicate comments:
In `@packages/server/src/routes/api.ts`:
- Around line 445-457: The delete workflow handler constructs workingDir using
targetSource, getArchonHome() and getWorkflowFolderSearchPaths(), which can
produce a nested .archon/.archon/workflows when targetSource !== 'global' and
project context is missing; fix it in the delete workflow handler by using the
same approach as the save handler: if no project context exists default the
scope to 'global' (or explicitly branch to build workingDir from getArchonHome()
alone) instead of joining getArchonHome() with
getWorkflowFolderSearchPaths()[0]; update the code that computes workingDir (and
the analogous code around lines 459-462) to check for project context before
using getWorkflowFolderSearchPaths() so paths aren’t nested.
---
Nitpick comments:
In `@packages/server/src/routes/api.ts`:
- Around line 2760-2761: The code calls getWSLDistroName() twice when building
the object (condition and value); cache the result in a local variable (e.g.,
const wslDistro = getWSLDistroName()) before the object literal and then use
isWSL() for is_wsl and wsl_distro: wslDistro only when wslDistro is truthy so
you remove the duplicate call and preserve the same conditional behavior around
getWSLDistroName().
In `@packages/web/src/routes/DashboardPage.tsx`:
- Around line 378-379: The DashboardPage is passing health?.is_wsl directly to
the ChatInterface, which can be undefined until the health query resolves; make
this consistent with ChatInterface.tsx by defaulting the prop when calling the
component (use health?.is_wsl ?? false for the isWsl prop) and apply the same
defensive default at the other call site noted (the second occurrence around
where wslDistro/isWsl are passed). Update the prop expression(s) for isWsl while
leaving wslDistro as-is, so both call sites explicitly pass a boolean fallback
for isWsl to match ChatInterface's expected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf295104-dff2-4228-be5e-4abf665dc0b3
📒 Files selected for processing (12)
packages/paths/src/archon-paths.test.tspackages/paths/src/archon-paths.tspackages/paths/src/index.tspackages/server/src/routes/api.tspackages/web/src/components/chat/ChatInterface.tsxpackages/web/src/components/dashboard/WorkflowRunCard.tsxpackages/web/src/components/dashboard/WorkflowRunGroup.tsxpackages/web/src/components/layout/Header.tsxpackages/web/src/lib/api.tspackages/web/src/lib/ide-uri.test.tspackages/web/src/lib/ide-uri.tspackages/web/src/routes/DashboardPage.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/web/src/lib/ide-uri.ts
- packages/web/src/lib/ide-uri.test.ts
The fallback test asserted only `typeof isWSL() === 'boolean'`, which trivially passes regardless of the actual /proc detection result. Address CodeRabbit feedback by deriving the expected value from the same source the implementation reads (/proc/sys/kernel/osrelease), so the assertion is pinned to real behavior on both Linux CI and WSL2 hosts. Bun's mock.module() is process-global and irreversible per repo policy, so reading the source file directly is the cleanest deterministic option. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Review SummaryVerdict: minor-fixes-needed Your WSL detection feature is clean and well-scoped. Tests cover happy paths, error paths, and edge cases thoroughly. The Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality. |
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.
Summary
vscode://file/<path>directly. When Archon runs inside WSL2 and the user is browsing from a Windows host, that URI hands a Linux path to Windows VS Code, which can't resolve it. The IDE launches with an empty / "file not found" window./api/health, and have the Web UI build the WSL-flavouredvscode://vscode-remote/wsl+<distro>/<path>URI when the server reports it. Plainvscode://file/...is preserved for every other environment.is_dockerplumbing).UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
@archon/pathsexportsisWSL,getWSLDistroNameisDocker/api/healthresponseis_wsl,wsl_distro@archon/web/lib/ide-uri.tsHeader.tsx,WorkflowRunCard.tsx,WorkflowRunGroup.tsx,ChatInterface.tsx,DashboardPage.tsxideUri()vscode://file/...→ helper callLabel Snapshot
risk: lowsize: Mweb(primary),paths,serverweb:dashboard,web:layout,paths:archon-paths,server:routes/apiChange Metadata
bugmultiLinked Issue
Validation Evidence
ide-uri.test.tscovers each branch: default URI form, WSL2 form with various distro inputs, missing-distro fallback to default URI, Windows-style backslash normalisation, and URL-encoding of distro names containing special characters.Security Impact
process.env.WSL_DISTRO_NAME(already in env) and/proc/sys/kernel/osrelease(read-only, no privilege required)./proc/sys/kernel/osreleaseper/api/healthrequest (~30 s polling cadence in the UI).Compatibility / Migration
is_wsldefaults tofalseoutside WSL,wsl_distrois optional. Existingis_docker-based UI behaviour is unchanged.Human Verification
vscode://vscode-remote/wsl+Ubuntu/.... VS Code opens the workspace correctly via the WSL remote.WSL_DISTRO_NAMEunset (simulating a non-WSL run), the link falls back tovscode://file/...— the existing behaviour.encodeURIComponent'd (covered by tests).~/.vscode-server/...path: detection path uses env-var first, so any in-distro VS Code Server session is also covered, but I haven't manually clicked the button from VS Code's built-in browser.api.generated.d.tswas not regenerated (the manually-maintainedHealthResponseinterface inweb/src/lib/api.tsis what the components consume); a follow-upbun --filter @archon/web generate:typesagainst a live dev server can refresh the OpenAPI-derived types.Side Effects / Blast Radius
/api/healthresponse shape.is_dockerconsumers (the same components) are unchanged./api/healthresponse shape via strict equality would now see two extra fields. The schema already declaresversionandactivePlatformsas optional, so any well-formed client should be tolerant — but worth flagging.Rollback Plan
git revert <this commit>— single commit, clean revert. No data migrations.Risks and Mitigations
/proc/sys/kernel/osreleaseread might fail on some exotic environments (containers without/proc, frozen rootfs).try/catchand falls back tofalseon any error. The env-var path (WSL_DISTRO_NAME) is checked first and short-circuits without touching/procin the common case.encodeURIComponentis applied aroundwsl_distroinideUri(), with a test case that covers spaces.Summary by CodeRabbit
New Features
Tests