Skip to content

Conversation

@a8trejo
Copy link
Contributor

@a8trejo a8trejo commented Nov 19, 2024

Added changes to allow Cline to take over a chrome browser opened with the remote-debugging-port flag, this way it can help with tasks on the same browser we're using, it also makes it easier for it to get past authentication since in our browser we'll already be authenticated to the systems we normally use. Ex: cypress cloud, posthog, etc.

You'll need to specify on the extension settings the following
image

In order for it to work it requires a browser instance running on the specified port, ex: /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --remote-debugging-port=7333, otherwise it'll just prompt a new puppeteer browser

Example prompt:

Could you navigate to this URL
https://cloud.cypress.io/projects/hkawvm/runs/22055/overview/b890f243-ce56-4047-9252-c99469dfbbea?roarHideRunsWithDiffGroupsAndTags=1&interactive=true

Please analyze the failure and let me know if you can fix it.

Other changes

  • Added changeset to manage versions in package.json
  • Added a docs folder to share useful prompts
  • Added a browser_action snapshot to allow cline to connect to your current URL instead of having to launch a new one

Demo video:
https://www.loom.com/share/cda6fbbe93454265abf79731e139fb82


Important

Adds interactive mode for Puppeteer to connect to existing Chrome instances, updates setup, release workflows, and UI for new settings.

  • Behavior:
    • Adds interactive mode in Cline.ts and BrowserSession.ts for Puppeteer to connect to existing Chrome instances via remote-debugging-port.
    • Requires specifying interactive mode and browserPort in prompts.
    • Defaults to non-interactive mode if not specified.
  • Setup and Packaging:
    • Adds changeset for version management in package.json.
    • Updates README.md with new setup and packaging instructions.
    • Adds install:all script in package.json.
  • Release Workflow:
    • Adds release.yml for automated versioning and publishing.
    • Updates code-qa.yml to cache node modules.
  • Misc:
    • Replaces puppeteer-core with puppeteer in BrowserSession.ts and UrlContentFetcher.ts.
    • Removes ensureChromiumExists() method from BrowserSession and UrlContentFetcher.
    • Updates SettingsView.tsx to include interactive mode settings.
    • Updates ExtensionStateContext.tsx to manage new state properties.

This description was created by Ellipsis for 59e976a. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to a0654dd in 37 seconds

More details
  • Looked at 288 lines of code in 5 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. src/services/browser/BrowserSession.ts:27
  • Draft comment:
    Switching from puppeteer-core with puppeteer-chromium-resolver to puppeteer means the default Chromium bundled with Puppeteer will be used. Ensure this is the intended behavior, as it might affect the browser version used.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR removes the use of puppeteer-chromium-resolver and directly uses puppeteer. This change simplifies the code by removing the need to ensure Chromium exists separately. However, it also means that the default Chromium bundled with Puppeteer will be used, which might not be the desired behavior if a specific Chromium version is needed.
2. src/services/browser/UrlContentFetcher.ts:21
  • Draft comment:
    Switching from puppeteer-core with puppeteer-chromium-resolver to puppeteer means the default Chromium bundled with Puppeteer will be used. Ensure this is the intended behavior, as it might affect the browser version used.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR removes the use of puppeteer-chromium-resolver and directly uses puppeteer. This change simplifies the code by removing the need to ensure Chromium exists separately. However, it also means that the default Chromium bundled with Puppeteer will be used, which might not be the desired behavior if a specific Chromium version is needed.
3. src/services/browser/BrowserSession.ts:20
  • Draft comment:
    Ensure to handle errors properly in asynchronous functions using try/catch blocks. This is required by our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180bc982fd2d560354e47
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about error handling in asynchronous functions, which is a valid concern. However, the main asynchronous function, doAction, already has error handling. The other functions, launchBrowser and closeBrowser, are simpler and may not need additional error handling. The comment might be unnecessary given the current code structure.
    I might be underestimating the need for error handling in the launchBrowser and closeBrowser methods. They could potentially throw errors that should be caught.
    While it's possible that launchBrowser and closeBrowser could benefit from error handling, the current implementation of doAction already covers the main asynchronous operations. The comment may not be necessary unless specific issues are identified in those methods.
    The comment about adding try/catch blocks for error handling in asynchronous functions should be deleted, as the main asynchronous function already has error handling, and the other functions are simpler and may not require it.
4. src/services/browser/UrlContentFetcher.ts:17
5. README.md:2

Workflow ID: wflow_BNGOOpIFSJHSJUJ2


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@stea9499
Copy link
Contributor

Can you build a new executable as part of this PR?

@a8trejo
Copy link
Contributor Author

a8trejo commented Nov 19, 2024

Can you build a new executable as part of this PR?

I think i'll even add changeset

@ColemanRoo
Copy link
Contributor

Can you build a new executable as part of this PR?

@a8trejo We shouldn't need to, once these changes are ready, just bump the version number as part of the PR; once my small PR is merged, we will be able to get the latest version of Roo-Cline installed from AWS CodeArtifact

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Skipped PR review on 3630fe3 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

@a8trejo
Copy link
Contributor Author

a8trejo commented Nov 19, 2024

Can you build a new executable as part of this PR?

@a8trejo We shouldn't need to, once these changes are ready, just bump the version number as part of the PR; once my small PR is merged, we will be able to get the latest version of Roo-Cline installed from AWS CodeArtifact

@ColemanRoo could you give it another review? I think these changeset updates can go hand by hand with the npm publish updates

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on ffd3562 in 57 seconds

More details
  • Looked at 194 lines of code in 5 files
  • Skipped 2 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/release.yml:1

Workflow ID: wflow_2bIH36qNSP33vwoR


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ColemanRoo
Copy link
Contributor

Can you build a new executable as part of this PR?

@a8trejo We shouldn't need to, once these changes are ready, just bump the version number as part of the PR; once my small PR is merged, we will be able to get the latest version of Roo-Cline installed from AWS CodeArtifact

@ColemanRoo could you give it another review? I think these changeset updates can go hand by hand with the npm publish updates

@a8trejo Can you bump the version number in the package.json file where it is used? Or will that be handled by the GitHub Actions?

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on fe1d823 in 1 minute and 12 seconds

More details
  • Looked at 609 lines of code in 5 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. src/services/browser/BrowserSession.ts:45
  • Draft comment:
    Consider providing more context in the error message when failing to connect to the browser. This can help with debugging issues related to browser connection.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The error handling in the launchBrowser method should be improved to provide more context about the failure.
2. src/services/browser/BrowserSession.ts:142
  • Draft comment:
    Ensure that actions are not performed when the browser is in interactive mode, as users have manual control in this mode.
  • Reason this comment was not posted:
    Marked as duplicate.
3. src/services/browser/BrowserSession.ts:112
  • Draft comment:
    Ensure that actions are not performed when the browser is in interactive mode, as users have manual control in this mode. This comment also applies to other action methods like click, scrollDown, and scrollUp.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. src/services/browser/BrowserSession.ts:44
  • Draft comment:
    Using async/await makes asynchronous code more readable and easier to maintain than chaining .then() calls. Please refactor this to use async/await. This is from our Development Standards: Development Standards.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_R0aY9OKvGHK2dOjt


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on b0e48c5 in 49 seconds

More details
  • Looked at 171 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/services/browser/BrowserSession.ts:51
  • Draft comment:
    The error message should dynamically reflect the browserPort being used instead of hardcoding port 7333.
				throw new Error(`BrowserSession.ts :: launchBrowser :: Failed to connect to browser: ${error.message}, make sure you have a running browser with --remote-debugging-port=${this.browserPort}`)
  • Reason this comment was not posted:
    Marked as duplicate.
2. src/core/Cline.ts:722

Workflow ID: wflow_IgXYB7qXRW1NQjqC


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on 3ad3bdc in 21 seconds

More details
  • Looked at 26 lines of code in 2 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. package.json:137
  • Draft comment:
    The install:all script was updated to remove co:install, but this change is not mentioned in the PR description. Consider updating the description for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The install:all script in package.json was updated, but the change was not reflected in the PR description.
2. .npmrc:1

Workflow ID: wflow_AytEytIoajhijQX9


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

# If quiet mode is enabled, Ellipsis will only leave reviews when it has comments, so “Looks good to me” reviews
# will be skipped. This can reduce clutter.
quiet: false
quiet: true
Copy link
Contributor Author

@a8trejo a8trejo Dec 7, 2024

Choose a reason for hiding this comment

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

let me know if anyone has strong objections against this, but I find it to be too much spam every time I get a "Looks good to me" email, not to mention how long it makes a PR conversation

@a8trejo
Copy link
Contributor Author

a8trejo commented Dec 7, 2024

@ellipsis-dev can you remove all your "Looks good to me" comments?

@ellipsis-dev
Copy link
Contributor

ellipsis-dev bot commented Dec 7, 2024

I can't remove comments from the pull request. Please let me know if there's anything else I can assist with.

@a8trejo a8trejo requested a review from mrubens December 7, 2024 02:06
const wasInteractiveBrowser = (lastRelevantMessageIndex > 0) ? modifiedClineMessages[lastRelevantMessageIndex - 1].text?.includes("interactive mode") : false
let hadBrowserPort = '';
if ( wasInteractiveBrowser ) {
const match = modifiedClineMessages[lastRelevantMessageIndex - 1].text?.match(/\(browserPort\s*=\s*(\d+)\)/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Parsing the string doesn't feel that with the grain of how the rest of this code works. Could we make it a checkbox or an option on the settings page instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried having Cline find a better solution and spent like $30 and its solutions never worked, this was the only way I got this to work

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try the settings page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow

Copy link
Collaborator

@mrubens mrubens Dec 8, 2024

Choose a reason for hiding this comment

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

Can we think of this as an option on the settings screen where you choose to either use the built in browser or your own that’s accessible on a specific port? And then we just have conditional logic in the implementation of the browser tool based on your settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the suggestion to add this feature to the settings page. A text input to accept the port your browser is listening seems ideal. Also probably need so error handling in case it can't connect (and/or maybe a fallback to the built-in browser).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR to use settings and also to launch a new browser if a remote debugging browser is not already running 👍
image

})
}
} catch (error) {
console.log(`BrowserSession.ts :: launchBrowser :: Failed to connect to browser, falling back to regular launch mode. Error: ${error.message}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use structured logging and proper error handling instead of console.log for errors. Implement try/catch blocks and use appropriate logging levels (info, warn, error) for better debugging. This is from our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180bc982fd2d560354e47

@changeset-bot
Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: 4e0cc31

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
roo-cline Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@a8trejo a8trejo marked this pull request as draft December 17, 2024 23:36
@a8trejo a8trejo closed this Jan 6, 2025
@a8trejo
Copy link
Contributor Author

a8trejo commented Jan 6, 2025

Closing since MCP servers can do this

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.

6 participants