Skip to content

Conversation

@cte
Copy link
Collaborator

@cte cte commented Sep 22, 2025

  • Handle session token changes in the Roo Code provider
  • Always run the evals web app on port 3446

Important

Handle session token changes in RooHandler and set evals web app to run on port 3446.

  • Behavior:
    • Handle session token changes in RooHandler in roo.ts by adding authStateListener to update the OpenAI client on auth state changes.
    • Ensure dispose() in RooHandler removes authStateListener.
    • Update tests in roo.spec.ts to cover new session token handling logic.
  • Configuration:
    • Set evals web app to always run on port 3446 in package.json, README.md, and docker-compose.yml.
  • Misc:
    • Minor logging change in extension.ts.

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

@cte cte requested review from jr and mrubens as code owners September 22, 2025 19:07
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working documentation Improvements or additions to documentation labels Sep 22, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 22, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Found critical issues that need attention before merging.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 22, 2025
@roomote
Copy link
Contributor

roomote bot commented Sep 22, 2025

@cte I see the memory leak issue in the RooHandler class. I will implement the suggested fix to properly clean up the event listener. Working on it now.

- Add private authStateListener property to store listener reference
- Store listener function reference for proper cleanup
- Implement dispose() method to remove event listener
- Fix API key issue by using cloudService reference instead of CloudService.instance
- Update tests to include on/off methods in CloudService mock

This prevents memory leaks when multiple RooHandler instances are created
and ensures proper cleanup of event listeners.
@roomote
Copy link
Contributor

roomote bot commented Sep 22, 2025

@cte The memory leak has been fixed!

Changes Made:

  1. ✅ Added a private authStateListener property to store the event listener reference
  2. ✅ Modified the constructor to store the listener function for proper cleanup
  3. ✅ Implemented a dispose() method to remove the event listener when the handler is destroyed
  4. ✅ Fixed the API key issue by using the local cloudService reference instead of CloudService.instance
  5. ✅ Updated tests to include on and off methods in the CloudService mock

Test Results:

  • All 24 unit tests are passing ✅
  • CI checks are mostly passing (Ubuntu tests passed, Windows tests pending)

The implementation follows the suggested pattern exactly and prevents memory leaks when multiple RooHandler instances are created. The dispose method ensures proper cleanup of event listeners.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 22, 2025
@cte cte merged commit 0e1b23d into main Sep 22, 2025
18 of 19 checks passed
@cte cte deleted the cte/bare-metal-evals-fixes branch September 22, 2025 19:54
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Sep 22, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Sep 22, 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 documentation Improvements or additions to documentation Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. lgtm This PR has been approved by a maintainer 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.

6 participants