Skip to content

Conversation

louis-menlo
Copy link
Contributor

@louis-menlo louis-menlo commented May 22, 2025

Describe Your Changes

This PR fixes an issue where users close any side windows of the app could cause background services to be terminated (cortex, llama.cpp). It should just kill these processes on exit.

This include a small fix when running tool using uvx
CleanShot 2025-05-22 at 23 02 58@2x

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Fix window event handling to only terminate background processes when the main window is closed, and update command argument in run_mcp_commands().

  • Behavior:
    • Modify on_window_event in src-tauri/src/lib.rs to only terminate background processes when the main window is closed.
    • Change command argument in run_mcp_commands() in src-tauri/src/core/mcp.rs from "tool run" to "tool".
  • Misc:
    • Fixes a comment typo in src-tauri/src/lib.rs.

This description was created by Ellipsis for ea7b200. You can customize this summary. 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.

Important

Looks good to me! 👍

Reviewed everything up to 588de66 in 55 seconds. Click for details.
  • Reviewed 36 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/lib.rs:112
  • Draft comment:
    Clarify why we check for 'main' window to only kill background processes when the main window is closed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. src-tauri/src/lib.rs:115
  • Draft comment:
    Consider handling errors from the client.delete call (e.g., log if the request fails) and possibly using an async approach to avoid blocking.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. src-tauri/src/lib.rs:117
  • Draft comment:
    Avoid using unwrap() on window.emit; consider handling the error gracefully to prevent potential panics during shutdown.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. src-tauri/src/lib.rs:114
  • Draft comment:
    Consider avoiding hardcoding the kill endpoint URL; using a configurable constant would improve maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_8IrgXZnbMAzF14ja

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Important

Looks good to me! 👍

Reviewed ea7b200 in 41 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/core/mcp.rs:66
  • Draft comment:
    Changed from a single argument "tool run" to two separate arguments "tool" and "run". This likely ensures proper parsing by the binary. Consider adding a brief comment explaining this change for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_odXJIanjLeHEg3ok

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

This is the build for this pull request. You can download it from the Artifacts section here: Build URL.

Copy link
Contributor

This is the build for this pull request. You can download it from the Artifacts section here: Build URL.

Copy link
Contributor

This is the build for this pull request. You can download it from the Artifacts section here: Build URL.

@louis-menlo louis-menlo merged commit 2dda663 into release/v0.5.18 May 23, 2025
38 checks passed
@louis-menlo louis-menlo deleted the fix/close-other-app-windows-should-not-kill-cortex branch May 23, 2025 02:34
@github-project-automation github-project-automation bot moved this to QA in Jan May 23, 2025
@github-actions github-actions bot added this to the v0.5.18 milestone May 23, 2025
@david-menloai david-menloai moved this from QA to Done in Jan Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants