Skip to content

Conversation

@markijbema
Copy link
Contributor

@markijbema markijbema commented Jun 18, 2025

When we configure kilo to use the remote browser, but none is available, it falls back to a local browser. Administrate this in isUsingRemoteBrowser and use this to determine to close which browser when closing the browser.

Related GitHub Issue

Closes: #4822

Description

Instead of looking at what browser should have been used, look at what browser is actually used and close that

Test Procedure

Enable remote browser connection (checkbox)
(but do not actually start one)

Open the activity monitor (on macosx, or the equivalent on your OS)

Ask a simple browser request, for instance "give me the most important headline from cnn.com"

You can see chrome helpers starting
And when Roo says it closes the browser you can see them stopping/disappearing

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint).
    • All debug code (e.g., console.log) has been removed.
  • Testing:
    • New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

N/A

Documentation Updates

Additional Notes

Get in Touch


Important

Fixes browser closing logic in BrowserSession by tracking actual browser usage with isUsingRemoteBrowser.

  • Behavior:
    • Introduces isUsingRemoteBrowser in BrowserSession to track if a remote browser is used.
    • Updates closeBrowser() to use isUsingRemoteBrowser to determine whether to disconnect or close the browser.
  • Misc:
    • Updates launchLocalBrowser() and connectWithChromeHostUrl() to set isUsingRemoteBrowser appropriately.

This description was created by Ellipsis for fd8cec8b4c786e7d2aabf043f36fafa05e82ad47. You can customize this summary. It will automatically update as commits are pushed.

@markijbema markijbema force-pushed the close-local-browser-properly branch from 162ae55 to fd8cec8 Compare June 18, 2025 10:28
@markijbema markijbema marked this pull request as ready for review June 18, 2025 10:29
@markijbema markijbema requested review from cte, jr and mrubens as code owners June 18, 2025 10:29
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jun 18, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 18, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

Hey @markijbema, thank you for contributing to fix the issue.

I left a couple of suggestions that might be worth taking a look at, let me know what you think!

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Jun 18, 2025
@hannesrudolph hannesrudolph added PR - Changes Requested and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jun 18, 2025
When we configure Roo to use the remote browser, but none is available, it falls back to a local browser. Administrate this in isUsingRemoteBrowser and use this to determine to close which browser when closing the browser.
@markijbema markijbema force-pushed the close-local-browser-properly branch from fd8cec8 to 112b38c Compare June 20, 2025 09:04
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 20, 2025
@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Prelim Review] in Roo Code Roadmap Jun 21, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

Looks good!

Thank you @markijbema!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 21, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 21, 2025
@mrubens mrubens merged commit 9750d86 into RooCodeInc:main Jun 23, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 23, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 23, 2025
Alorse pushed a commit to Alorse/Roo-Code that referenced this pull request Jun 27, 2025
Alorse pushed a commit to Alorse/Roo-Code that referenced this pull request Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

When remote browser connection is used, but no remote browser is available, browser does not get closed

4 participants