Skip to content

Conversation

Wendong-Fan
Copy link
Contributor

Description

  1. Removed unused import - Deleted import e from 'express' in electron/main/index.ts:21
  2. Fixed type inconsistency - Updated code to handle checkToolInstalled() returning PromiseReturnType instead of boolean
  3. Improved error handling - Converted installCommandTool() from Promise constructor to async/await with proper try-catch
  4. Fixed UV path configuration - Updated uv() function path from ~/.eigent/bin/uv to ~/.local/bin/uv in
    backend/app/component/command.py:9

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@Wendong-Fan Wendong-Fan requested a review from a7m-1st September 28, 2025 17:10
@Wendong-Fan Wendong-Fan changed the title Fix #352 install enhanced PR404 enhance: Fix #352 install enhanced PR404 Sep 28, 2025
Comment on lines 102 to 104
export async function installCommandTool(): Promise<PromiseReturnType> {
return new Promise(async (resolve, reject) => {
try {
const ensureInstalled = async (toolName: 'uv' | 'bun', scriptName: string): Promise<PromiseReturnType> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great Catch. Looked into it & you are right, new Promise is unnecessary here & in many others. Its just a js way to create manual low-level async for non awaitable functions.

These days TypeScript automatically wraps the return value in a Promise for async funcs.

// Even though you return a string, the function returns Promise<string>
async function getName(): Promise<string> {
  return "John"; // TypeScript wraps this in Promise.resolve("John")
}

// This is equivalent to:
function getNameManual(): Promise<string> {
  return Promise.resolve("John"); //& ya, static function, no need to create class
}

Thanks @Wendong-Fan , LGTM.

@FooFindBar
Copy link
Collaborator

@a7m-1st I’m unclear about the logic in two places:
https://github.com/eigent-ai/eigent/blob/453800032e8e0b2b6e8bf40f1fd66d5a84aac8c6/src/hooks/useInstallationSetup.ts#L19C3-L47C52

useEffect(() => {
    const checkToolInstalled = async () => {
      try {
        console.log('[useInstallationSetup] Checking tool installation status...');
        const result = await window.ipcRenderer.invoke("check-tool-installed");
        
        if (result.success && initState === "done" && !result.isInstalled) {
          console.log('[useInstallationSetup] Tool not installed, setting initState to carousel');
          setInitState("carousel");
        }        
      } catch (error) {
        console.error("[useInstallationSetup] Tool installation check failed:", error);
      }
    };

    const checkBackendStatus = async() => {
      // Also check if installation is currently in progress
      const installationStatus = await window.electronAPI.getInstallationStatus();
      console.log('[useInstallationSetup] Installation status check:', installationStatus);
      
      if (installationStatus.success && installationStatus.isInstalling) {
        console.log('[useInstallationSetup] Installation in progress, starting frontend state');
        startInstallation();
      }
    }
    
    checkToolInstalled();
    checkBackendStatus();
  }, [initState, setInitState, startInstallation]);
  1. The checkToolInstalled method only calls setInitState("carousel"), but it doesn’t start installing dependencies or show the dependency installation page. It seems that dependency installation is controlled via startInstallation, not setInitState.

  2. The checkBackendStatus method calls startInstallation when the tool is not installed. However, after the tool is installed, the backend service does not seem to start.

@a7m-1st
Copy link
Collaborator

a7m-1st commented Sep 29, 2025

  1. As for checkToolInstalled(), yes you are right. It just sets the installation page to type of "carousel" rather than without it. I didn't change this code as it was there in Layout/index.tsx previously. Thus when installing without the carousel it shows:
Screenshot 2025-08-12 144606
  1. To be more exact, I didn't add any update logic to the backend. Its only a Ping status to see if the Installing.lock is free or no in the backend (Read only). That is because relying on the event listners to notify install-dependencies-start as the only source of truth is not reliable due to race conditions.

& Honestly this is the previous flow:
image

But in reality here is what is happening:

  • All the old logic in backend logic is the same, except that I removed the frontend-ready event which was trying to fix this frontend issue.
  • The only significant change I made is that now, the frontend doesn't wait for dependencies-start to show the Installation page, it can detect it anytime if the lock file is Installing & display the page.
  • Thus this has allowed for checking for uv sync installations on runtime by detectInstallationLogs(msg:string)
image

@a7m-1st
Copy link
Collaborator

a7m-1st commented Sep 29, 2025

@FooFindBar So to recap about 2:

  • I think its just a naming issue, all it does is show the frontend. I assume the backend always starts installation correctly.
//InstallationStore.ts
// Basic actions
startInstallation: () => 
  set({
    state: 'installing',
    progress: 20,
    logs: [],
    error: undefined,
    isVisible: true,
  }),
  • Most backend logic is the same. I just refactored it for easier maintenance & error response. So any updates to enhance it is welcome, as I couldn't find a reliable way / library to handle this for us, except for using logs.

Thanks for your questions !

@Wendong-Fan Wendong-Fan merged commit df10746 into fix-#352-install-enhanced Sep 30, 2025
1 check passed
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.

3 participants