Skip to content

fix(server): preserve discovered editors when scanning times out - #10694

Closed
edemaine wants to merge 1 commit into
pingdotgg:mainfrom
edemaine:editor-timeout
Closed

edemaine wants to merge 1 commit into
pingdotgg:mainfrom
edemaine:editor-timeout

Conversation

@edemaine

@edemaine edemaine commented Sep 8, 2026 •

Copy link
Copy Markdown

What Changed

Return editors found so far when discovery times out. Timeout recovery now lives in ExternalLauncher; only completed scans are cached, so subsequent requests can retry incomplete discovery.

Why

Editor discovery could find an installed editor, then hit the five-second timeout during a later probe and return an empty list. This caused the Open menu to show “No installed editors found.”

This is especially common on Windows, where file probes are slow. But it could hit anyone with a large PATH.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Preserve discovered editors when scanning times out in externalLauncher

Updates the editor discovery timeout behavior so that a timed-out scan returns editors found before the timeout, rather than degrading to an empty list. Adds a regression test in externalLauncher.test.ts that runs discovery with a one-second timeout, confirms partial results are returned when a later probe blocks indefinitely, and verifies a subsequent uncapped scan finds additional editors.

Macroscope summarized 5e115dd.

Summary by CodeRabbit

  • Improvements

    • Editor discovery now supports a five-second timeout, allowing the server to continue using editors found before the timeout.
    • Editor discovery can complete successfully later when a temporarily blocked scan becomes available.
    • Remote open target discovery continues to return an empty result when it exceeds its timeout.
  • Bug Fixes

    • Improved handling of slow or stalled editor and file manager capability detection to prevent discovery from blocking server configuration.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 8, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 5e115dd

Macroscope's review found this PR approvable — This is a narrowly scoped server bug fix that preserves partial editor-discovery results on timeout without changing the existing timeout value or unrelated discovery paths. The new regression test covers both partial results and retrying an incomplete scan.

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

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: f5ab096a-433a-4c28-93ea-cd4e0a8f04a6

📥 Commits

Reviewing files that changed from the base of the PR and between 11601da and 5e115dd.

📒 Files selected for processing (4)
  • apps/server/src/process/externalLauncher.test.ts
  • apps/server/src/process/externalLauncher.ts
  • apps/server/src/server.test.ts
  • apps/server/src/ws.ts

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


📝 Walkthrough

Walkthrough

Editor discovery now supports per-call timeouts and returns editors found before timeout. Server configuration calls this API directly. Tests cover partial results, later completion, and file-manager reveal timeouts.

Changes

Editor discovery timeout

Layer / File(s) Summary
Accumulated discovery and timeout contract
apps/server/src/process/externalLauncher.ts
Editor discovery accumulates successful probe results and returns the accumulated editors when the optional timeout expires.
Server configuration integration
apps/server/src/ws.ts
Server configuration calls externalLauncher.resolveAvailableEditors with the configured timeout. Remote target discovery retains its empty-list fallback.
Timeout behavior tests
apps/server/src/process/externalLauncher.test.ts, apps/server/src/server.test.ts
Tests verify partial editor results, later discovery completion, and file-manager reveal timeout behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 5e115

Editor discovery now retains editors found before a timeout, preventing an empty Open menu when later probes stall. The timeout remains bounded and later requests can retry incomplete discovery.

Sequence Diagram(s)

sequenceDiagram
  participant loadServerConfig
  participant ExternalLauncher
  participant EditorDiscovery
  participant EditorProbes
  loadServerConfig->>ExternalLauncher: Request editors with CONFIG_DISCOVERY_TIMEOUT
  ExternalLauncher->>EditorDiscovery: Start cached discovery
  EditorDiscovery->>EditorProbes: Run editor probes
  EditorProbes-->>EditorDiscovery: Append discovered editors
  EditorDiscovery-->>ExternalLauncher: Return accumulated editors on timeout
  ExternalLauncher-->>loadServerConfig: Provide available editors
Loading

Suggested reviewers: t3dotgg, juliusmarminge, sunkenintime

🚥 Pre-merge checks | ✅ 4 | ❌ 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserving editors discovered before an editor scan times out.
Description check ✅ Passed The description includes the required What Changed, Why, and Checklist sections. It explains the timeout problem, the recovery behavior, and the cache behavior. The UI-related checklist items remain u…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

Closing for missing verification results. The partial-discovery regression test is present, but the PR provides no focused test command or observed run result, and the current checks contain no test execution. The verification rule requires evidence for the changed behavior. Report the test environment, command, and results showing that a timed-out scan preserves discovered editors and a later scan retries, then request reconsideration.

@edemaine

edemaine commented Oct 1, 2026

Copy link
Copy Markdown
Author

Sorry, I assumed you'd use CI to confirm yourself. Yes, I verified (again just now) on commit 5e115dd, on Windows x64 (build 26200), Node.js v24.11.0, Vitest v4.1.11.

Command:

vp test run apps/server/src/process/externalLauncher.test.ts -t "returns editors found before discovery times out"

Result: 1 test passed, 22 skipped by the name filter.

The test lets the first lookup succeed, stalls subsequent lookups, and advances the test clock through the timeout. It asserts that the discovered editor is retained. It then unblocks discovery and verifies that another call performs fresh filesystem probes and returns more editors, demonstrating that the partial result was not cached.

@juliusmarminge Could you reconsider reopening this PR?

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

Labels

size:M 30-99 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.

2 participants