Skip to content

fix(web): open the client host environment's editor locally - #34046

Closed
bernikr wants to merge 1 commit into
pingdotgg:mainfrom
bernikr:t3/fix-host-environment-editor-open
Closed

bernikr wants to merge 1 commit into
pingdotgg:mainfrom
bernikr:t3/fix-host-environment-editor-open

Conversation

@bernikr

@bernikr bernikr commented Nov 10, 2026

Copy link
Copy Markdown

Summary

A desktop app paired with a background service on the same machine keeps that service as a plain BearerConnectionTarget in the catalog. resolveRemoteOpenState only returned local-exec for a primary connection target or a desktop-managed local backend, so a paired service with no SSH profile and no advertised hosts fell through to remote-unavailable and "Open in" dead-ended at "No SSH route to …".

This makes editor resolution use the same "this machine" notion as chat file links:

  • resolveRemoteOpenState takes an optional isClientHostEnvironment and returns local-exec after the null-target check.
  • remoteOpenResolutionAtom computes that flag with isClientHostEnvironment(environmentId, hostEnvironmentIdAtom, entry.target, isDesktopRenderer).
  • Existing primary/loopback and desktop-managed local backend checks are unchanged for callers that do not pass the flag.

Testing

  • pnpm install in the worktree.
  • vp test run apps/web/src/remoteOpen.test.ts — 18 passed.
  • vp test run apps/web/src/remoteOpen.test.ts apps/web/src/state/primaryEnvironment.test.ts — 22 passed.
  • vp run --filter @t3tools/web typecheck — passed (Effect schemaClassMissingBrand suggestions only, all pre-existing).
  • vp run check:changed — format, lint, knip, typecheck, tests, and React compiler checks all passed.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 10, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — Client-host environments now automatically use local editor execution instead of remote links or an unavailable action. A human must confirm this default routing change, including its precedence over SSH and advertised remote routes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The resolver now accepts client-host status and returns local execution for client-host environments. The resolution atom computes that status. Tests cover bearer and SSH target outcomes.

Changes: Client-host resolution

Layer / File(s) Summary
Client-host resolution and coverage
apps/web/src/remoteOpen.ts, apps/web/src/remoteOpen.test.ts
The resolver checks client-host status after the null-target check and returns local-exec when it is true. The resolution atom computes and passes that status. Tests cover local execution for client-host bearer and SSH targets, plus unavailable and SSH-link results for non-host environments.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related issues

  • pingdotgg/t3code#30992 — Reports that Open in treats a paired background service on the desktop’s machine as remote. This change selects local execution for client-host environments.
  • pingdotgg/t3code#25781 — Reports that Open in creates an SSH remote link for a server on the desktop machine. This change selects local execution for client-host environments.

Possibly related PRs

  • pingdotgg/t3code#27236: Adds saved SSH route selection for remote editor links. This change makes client-host resolution take precedence over that route.
  • pingdotgg/t3code#31358: Also addresses local editor opening for a service paired from the desktop’s T3 home, using a different client-host identification method.
  • pingdotgg/t3code#33015: Adds device-local state for opted-in bearer routes. This change selects local execution for client-host environments before target classification.

Suggested labels: bugfix

Suggested reviewers: juliusmarminge, macroscopeapp

Merge Risk: Minimal · up to 00a2d

Editor resolution should update when the host environment is identified. No issue requiring a fix before merge was established.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly identifies the main change: opening the client host environment’s editor locally.
Description check Passed The description explains the problem, the code change, and focused verification results. It does not include the required scope and approval information or explain why this focused fix qualifies witho…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, the code change, and focused verification results. It does not include the required scope and approval information or explain why this focused fix qualifies without prior approval, but the remaining description is mostly complete.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between d756fed and 00a2df0.

Files selected for processing: 2

  • apps/web/src/remoteOpen.test.ts
  • apps/web/src/remoteOpen.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@binjamshid binjamshid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using the same client-host test as chat file links for "Open in" makes sense; for a desktop renderer whose host environment id matches, the service is on this machine, so local exec is correct even when an SSH profile also exists. Behaviour for non-desktop renderers is unchanged because isClientHostEnvironment only returns true there for a loopback PrimaryConnectionTarget, which the resolver already treated as local.

  1. low, apps/web/src/remoteOpen.ts:129: this re-derives the flag inline with the same four arguments that clientHostEnvironmentAtom(environmentId) in state/clientHostEnvironment.ts already computes. Using get(clientHostEnvironmentAtom(environmentId)) would keep the two call sites from drifting if the host test changes later.
  2. low: the new tests cover resolveRemoteOpenState with the flag passed in, but nothing covers the atom wiring (host id arriving asynchronously from getHostEnvironmentId, so the state starts as remote-unavailable and flips to local-exec). Not a blocker given remoteOpenStatesEqual handles the flip, but worth a sentence in the PR about having checked it in the desktop app, since there is no before/after evidence of the actual "Open in" menu.

@juliusmarminge

Copy link
Copy Markdown
Member

Thanks for this. Main has the change since #21661.

Compared both changed files and all four added resolver cases with main: matching desktop-host bearer and SSH environments now resolve to local execution before SSH routing, while other environments retain their unavailable or SSH-link behavior. Main therefore already provides this PR's behavior.

Closing as superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants