Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix phind #793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asyncButNeverAwaits
Copy link
Contributor

@asyncButNeverAwaits asyncButNeverAwaits commented Apr 11, 2024

make mainWindow as global to use in other file
phind need to generate challenge seed which is dynamic only can open the phind window and get the seend

Close: #763
Close: #701
Close: #690
Close: #623

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new dependency raw-loader for enhanced functionality.
    • Added a mechanism for managing multiple chat windows, improving user experience.
    • Implemented a new LoginSetting component for simplified bot login configuration.
    • Added functions for dynamic JavaScript injection and element waiting to enhance interactivity.
    • Established a method for intercepting and processing network requests related to the Phind API.
  • Bug Fixes

    • Improved error handling for text insertion and form submission processes.
  • Refactor

    • Streamlined the PhindBot class for better API interaction and chat context management.

Copy link

vercel bot commented Apr 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
chatall ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2024 10:43pm

@sunner
Copy link
Member

sunner commented Apr 18, 2024

Thank you for contribution.

The diff is huge and I can't understand the code in windows well. Could you kindly explain it?

@asyncButNeverAwaits
Copy link
Contributor Author

This is a different approach to sending a prompt to Phind because Phind requires the challenge value in the request.

Image

I found that the challenge value is calculated in their script using a function. This function takes challengeSeeds, which change regularly within the script, so they can't be hardcoded.

Image

So, one way to get the challenge value is to intercept the network request, get all the POST data, and then send it.

When the Phind _sendPrompt function is called, it triggers the background function create-chat-window. This function opens a Phind chat page window with a debugger attached and Fetch.enable enabled to intercept network requests.

Once the Phind chat window is open, we need to run a script to send the prompt, so that the network request is sent out and we can intercept it.

For Phind, this is the script that needs to be run in the Phind chat window:

send.js

Before running this script, we need to inject the waitForElement script first. I've made it into a separate script so that it can be shared:

waitForElement.js

These scripts are run when the request to https://www.phind.com/api/stripe/subscriptions is intercepted. When this request is intercepted, meaning the Phind chat window is ready to send prompt.

Then, after running the send.js script, the request to https://https.api.phind.com/agent/ will be intercepted, and we'll run global.mainWindow.webContents.send("phind-request", data);, the data will have all POST data.

After that, it will go back to the renderer, and PhindBot will send a request with the POST data received from phind-request.


I also done this way for https://yiyan.baidu.com/

after this is merge i will open another PR for yiyan because some codes is shared in this PR

@asyncButNeverAwaits
Copy link
Contributor Author

asyncButNeverAwaits commented Apr 19, 2024

make mainWindow as global to use in other file
phind need to generate challenge seed which is dynamic only can open the phind window and get the seend
@sunner
Copy link
Member

sunner commented Apr 24, 2024

I tested it on macOS. Unfortunately, I can't get correct result.

The chat window was created but the prompt was not send. I need more time to find the reason. Have you ever met this?

@PeterDaveHello
Copy link
Collaborator

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Sep 26, 2024

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

coderabbitai bot commented Sep 26, 2024

Walkthrough

The pull request introduces several significant changes across multiple files. A new dependency, raw-loader, is added to the project. The src/background.js file has been modified to use a global variable for managing the main application window and to support multiple chat windows. The PhindBot class has been updated to improve API interaction and chat context management. Additionally, the PhindBotSettings.vue component has undergone a structural change, focusing on login rather than settings. New utility functions for script injection and element observation have also been introduced.

Changes

File Change Summary
package.json Added new dependency: "raw-loader": "^4.0.2".
src/background.js Transitioned mainWindow to global.mainWindow, added support for multiple chat windows, and updated IPC handlers for window management.
src/bots/PhindBot.js Modified _checkAvailability and _sendPrompt methods for improved API interaction, simplified context management, and updated return structure for createChatContext.
src/components/BotSettings/PhindBotSettings.vue Removed CommonBotSettings component, added LoginSetting, and updated data initialization to focus on login instead of settings.
src/store/index.js Modified phind state property from { model: "Phind Model" } to {}.
src/windows/helper/inject.js Added getInjectScript function for dynamic script injection.
src/windows/helper/waitForElement.js Introduced waitForElement function to observe DOM changes for a specific selector.
src/windows/phind/phind.js Added sendPhind function for intercepting and handling network requests in the Electron application.
src/windows/phind/send.js Implemented functionality to automate text insertion into a textarea and form submission.

Assessment against linked issues

Objective Addressed Explanation
Provide a login interface for Phind (Issue #763)
Resolve 404 errors when sending prompts (Issues #701, #690) Changes do not directly address the 404 errors.
Fix bad parameter errors in API requests (Issue #623) Unclear if changes resolve parameter issues.

🐇 In the meadow, changes bloom bright,
New scripts and windows take flight.
PhindBot's journey, now more clear,
With login flow, we cheer and cheer!
A hop and a skip, the code aligns,
In the world of tech, all is fine! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 16

🧹 Outside diff range and nitpick comments (7)
src/windows/phind/send.js (1)

6-6: Remove the trailing empty line.

There's an empty line at the end of the file. While some style guides recommend an empty line at the end of files, it's not necessary and can be removed for consistency if it's not a part of your project's style guide.

Consider removing this empty line if it doesn't align with your project's coding standards.

src/windows/helper/inject.js (1)

1-7: Improve documentation and provide usage context

The purpose and usage context of this function are not immediately clear from its implementation. Adding proper documentation would greatly improve maintainability and help prevent misuse.

Consider adding a JSDoc comment to explain:

  1. The function's purpose
  2. Parameters and their expected types
  3. Return value
  4. Any security considerations or usage restrictions

Example:

/**
 * Generates a string containing JavaScript code to inject a custom script into a web page.
 * 
 * @param {string} script - The JavaScript code to be injected.
 * @returns {string} A string containing JavaScript code that, when executed, will inject the provided script.
 * 
 * @warning This function should be used with caution as it can pose security risks if not properly sanitized.
 * Only use with trusted input and consider alternatives to dynamic script injection where possible.
 */
export function getInjectScript(script) {
  // ... (existing implementation)
}
src/components/BotSettings/PhindBotSettings.vue (1)

Line range hint 5-19: LGTM: Script section updated to support new login functionality.

The changes in the script section align well with the PR objectives:

  1. The Bot import and usage of Bot.getInstance() support the goal of making the mainWindow global and accessible across files.
  2. The focus on the LoginSetting component addresses the login interface issues mentioned in [BUG] phind为什么没有登录界面的选项 #763.
  3. Removal of settings and brandId simplifies the component, focusing it on login functionality.

These changes should help resolve the Phind-related issues mentioned in the PR description.

Consider adding a brief comment explaining the purpose of using Bot.getInstance() for better code documentation:

 data() {
   return {
-    bot: Bot.getInstance(),
+    // Use singleton instance to ensure consistent global state
+    bot: Bot.getInstance(),
   };
 },
src/background.js (3)

160-160: Consider the implications of using a global variable for mainWindow

The change from a local mainWindow to global.mainWindow allows access from other modules, which aligns with the PR objective. However, using global variables can lead to potential issues:

  1. It may make the code harder to test and reason about.
  2. It could introduce unexpected side effects if the global state is modified elsewhere.

To mitigate these risks:

  • Consider using a singleton pattern or a centralized state management solution.
  • Ensure that only necessary modules have access to modify global.mainWindow.
  • Add safeguards to prevent accidental modifications of the global state.

Also applies to: 298-298, 302-302, 307-310, 313-313, 316-316, 321-321, 324-324, 328-331, 339-339


355-373: New chat window management functionality looks good, consider future extensibility

The introduction of the windows object and the new IPC handlers for creating and closing chat windows is a good improvement for managing multiple windows. Some observations and suggestions:

  1. The create-chat-window handler currently only handles the "phind" case. Consider adding a default case to handle unknown window types or prepare for future window types.

  2. The close-chat-window handler correctly closes the window and removes the reference. Good practice!

  3. For better type checking and autocomplete, consider using JSDoc for the windows object:

/** @type {Record<string, Electron.BrowserWindow | undefined>} */
const windows = {};
  1. To improve error handling, consider adding a check in the close-chat-window handler:
if (!windows[winName]) {
  console.warn(`Attempted to close non-existent window: ${winName}`);
  return;
}
🧰 Tools
🪛 Biome

[error] 361-361: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)


Line range hint 298-339: Refactor token handling in createNewWindow for better maintainability and security

The createNewWindow function now handles token extraction for multiple websites. While this achieves the desired functionality, there are some concerns:

  1. Maintainability: As more websites are added, this function will grow and become harder to maintain. Consider refactoring this into a separate module with a map of website-specific handlers.

  2. Security: Sending tokens directly through IPC messages might pose a security risk. Consider encrypting sensitive data before sending it through IPC.

  3. Error Handling: Add try-catch blocks for each website-specific token extraction to prevent a failure in one from affecting others.

  4. Consistency: Some sites use getLocalStorage, others use getCookie. Consider standardizing this approach if possible.

Suggested refactor:

const tokenExtractors = {
  'https://moss.fastnlp.top/': async () => ({ secret: await getLocalStorage('flutter.token') }),
  'https://qianwen.aliyun.com/': async () => ({ token: await getCookie('XSRF-TOKEN') }),
  // ... other extractors ...
};

async function extractTokens(url) {
  const domain = new URL(url).origin;
  const extractor = tokenExtractors[domain];
  if (extractor) {
    try {
      const tokens = await extractor();
      // Encrypt tokens here before sending
      global.mainWindow.webContents.send(`${domain}-TOKENS`, tokens);
    } catch (error) {
      console.error(`Failed to extract tokens for ${domain}:`, error);
    }
  }
}

// In createNewWindow
newWin.on('close', async (e) => {
  e.preventDefault();
  await extractTokens(url);
  newWin.destroy();
  global.mainWindow.webContents.send("CHECK-AVAILABILITY", url);
});

This approach improves maintainability, error handling, and prepares for future additions.

src/windows/phind/phind.js (1)

48-48: Handle possible exceptions when detaching the debugger.

When detaching the debugger, an exception might occur if the debugger is not attached. It's advisable to check if the debugger is attached before detaching.

Apply this diff to safely detach the debugger:

 win.on("closed", () => {
-  _debugger.detach();
+  if (_debugger.isAttached()) {
+    try {
+      _debugger.detach();
+    } catch (err) {
+      console.error('Failed to detach debugger:', err);
+    }
+  }
 });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a19540d and c56cf39.

📒 Files selected for processing (9)
  • package.json (1 hunks)
  • src/background.js (7 hunks)
  • src/bots/PhindBot.js (3 hunks)
  • src/components/BotSettings/PhindBotSettings.vue (1 hunks)
  • src/store/index.js (1 hunks)
  • src/windows/helper/inject.js (1 hunks)
  • src/windows/helper/waitForElement.js (1 hunks)
  • src/windows/phind/phind.js (1 hunks)
  • src/windows/phind/send.js (1 hunks)
🧰 Additional context used
🪛 Biome
src/background.js

[error] 361-361: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

src/bots/PhindBot.js

[error] 61-61: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 63-63: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (6)
src/windows/helper/inject.js (1)

1-7: ⚠️ Potential issue

Security concern: Potential XSS vulnerability in script injection

The getInjectScript function creates a string that, when executed, injects a provided script into the document. This approach can lead to cross-site scripting (XSS) vulnerabilities if the script parameter is not properly sanitized.

Consider the following improvements:

  1. Implement strict input validation and sanitization for the script parameter.
  2. Use a Content Security Policy (CSP) to restrict script execution.
  3. If possible, avoid dynamic script injection altogether and use safer alternatives like event-driven communication between different parts of your application.

To assess the usage and potential impact of this function, let's search for its occurrences in the codebase:

src/components/BotSettings/PhindBotSettings.vue (1)

1-3: LGTM: Template simplified and focused on login functionality.

The template has been streamlined to use a single login-setting component, which aligns with the PR objectives of addressing login-related issues (e.g., #763). This change centralizes the login logic and simplifies the component structure.

package.json (1)

83-83: LGTM: Addition of raw-loader is appropriate.

The addition of raw-loader (version ^4.0.2) to the devDependencies is appropriate and aligns with the project's needs. This loader is commonly used to import files as raw strings in webpack, which could be beneficial for the Phind integration mentioned in the PR objectives.

A few points to note:

  1. raw-loader is correctly placed in devDependencies as it's a build-time dependency.
  2. The version specifier ^4.0.2 allows for compatible updates, which is a good practice.
  3. This addition suggests that the project now needs to import raw content, possibly related to the Phind feature implementation.
src/store/index.js (1)

115-115: LGTM. Verify impact and update documentation if needed.

The simplification of the phind property in the state object aligns with the PR objectives related to the Phind feature. This change provides more flexibility in how the phind state is used.

To ensure this change doesn't introduce any regressions:

  1. Verify that no components or other parts of the application directly access state.phind.model.
  2. Update any relevant documentation or comments that might reference the removed model property.
✅ Verification successful

Verification Successful. No references to state.phind.model found.

The removal of the model property from phind does not impact existing functionality. Please ensure that any related documentation or comments are updated accordingly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any direct references to state.phind.model
rg "state\.phind\.model" --type vue --type js

Length of output: 75


Script:

#!/bin/bash
# Search for any direct references to state.phind.model in .vue and .js files
rg "state\.phind\.model" --glob "*.vue" --glob "*.js"

Length of output: 55

src/windows/phind/phind.js (1)

38-39: Verify that 'global.mainWindow' is defined before using it.

The code assumes that global.mainWindow is defined and accessible. If global.mainWindow is undefined or not properly initialized, this could lead to runtime errors.

Ensure that global.mainWindow is correctly defined and initialized before this line.

Run the following script to check if global.mainWindow is defined:

✅ Verification successful

Verified: global.mainWindow is defined in src/background.js, ensuring it is properly initialized before its usage in phind.js.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that 'global.mainWindow' is defined in the codebase.

# Test: Search for the definition of 'global.mainWindow'. Expect: At least one occurrence where it's assigned.
rg --type javascript --word-regexp 'global\.mainWindow\s*='

Length of output: 96


Script:

#!/bin/bash
# Description: Verify that 'global.mainWindow' is defined in the codebase.

# Test: Search for the definition of 'global.mainWindow'. Expect: At least one occurrence where it's assigned.
rg --type js --word-regexp 'global\.mainWindow\s*='

Length of output: 98

src/bots/PhindBot.js (1)

74-76: Verify the use of 'readystatechange' event in SSE

In lines 74-76, the code adds an event listener for 'readystatechange' on the SSE source and checks event.readyState. However, 'readystatechange' is not a standard event for EventSource in SSE, and event.readyState may not be defined. Ensure that the SSE library (sse.js) you are using supports the 'readystatechange' event and that event.readyState provides the expected values. If not, consider using standard SSE events such as 'open', 'message', or 'error'.

Comment on lines +1 to +5
waitForElement("textarea").then((e) => {
e.focus();
document.execCommand("insertText", false, "{prompt}");
waitForElement("button[type=submit]").then((e) => e.click(), console.error);
}, console.error);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider modernizing the code and improving error handling.

The current implementation uses promises and the deprecated document.execCommand method. Consider the following improvements:

  1. Replace waitForElement with a more modern approach, such as using MutationObserver or async/await with a polling mechanism.
  2. Instead of document.execCommand, use the more modern Element.insertAdjacentText() or directly set the value property of the textarea.
  3. Enhance error handling to provide more specific error messages and potentially implement retry logic.

Here's a suggested refactor using async/await and modern DOM manipulation:

async function waitForElement(selector, timeout = 5000) {
  const start = Date.now();
  while (Date.now() - start < timeout) {
    const element = document.querySelector(selector);
    if (element) return element;
    await new Promise(resolve => setTimeout(resolve, 100));
  }
  throw new Error(`Element ${selector} not found within ${timeout}ms`);
}

async function insertTextAndSubmit(prompt) {
  try {
    const textarea = await waitForElement("textarea");
    textarea.focus();
    textarea.value = prompt;

    const submitButton = await waitForElement("button[type=submit]");
    submitButton.click();
  } catch (error) {
    console.error("Error in insertTextAndSubmit:", error.message);
  }
}

insertTextAndSubmit("{prompt}");

This refactored version provides better error handling, uses modern DOM manipulation, and is more readable.

@@ -0,0 +1,6 @@
waitForElement("textarea").then((e) => {
e.focus();
document.execCommand("insertText", false, "{prompt}");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid hardcoding the prompt value.

The current implementation uses a hardcoded "{prompt}" value. This might be a placeholder, but it's better to make it configurable.

Consider passing the prompt as a parameter or retrieving it from a configuration object. For example:

function insertTextAndSubmit(prompt) {
  // ... existing code ...
  document.execCommand("insertText", false, prompt);
  // ... existing code ...
}

// Usage
insertTextAndSubmit(actualPromptValue);

This change would make the function more flexible and reusable.

Comment on lines +1 to +7
export function getInjectScript(script) {
return `
var script = document.createElement('script');
var customScript = ${script};
script.innerHTML = customScript;
document.head.appendChild(script);`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring for improved robustness and maintainability

The current implementation could be improved to make it more robust and easier to maintain.

Consider the following improvements:

  1. Use template literals consistently:
export function getInjectScript(script) {
  return `
    (() => {
      const script = document.createElement('script');
      const customScript = ${JSON.stringify(script)};
      script.textContent = customScript;
      document.head.appendChild(script);
    })();
  `;
}
  1. Add error handling:
export function getInjectScript(script) {
  if (typeof script !== 'string' || script.trim() === '') {
    throw new Error('Invalid script provided');
  }
  
  return `
    (() => {
      try {
        const script = document.createElement('script');
        const customScript = ${JSON.stringify(script)};
        script.textContent = customScript;
        document.head.appendChild(script);
      } catch (error) {
        console.error('Failed to inject script:', error);
      }
    })();
  `;
}

These changes would make the function more resilient to errors and easier to debug.

Comment on lines +1 to +19
function waitForElement(selector) {
return new Promise((resolve) => {
if (document.querySelector(selector)) {
return resolve(document.querySelector(selector));
}

const observer = new MutationObserver((mutations) => {
if (document.querySelector(selector)) {
observer.disconnect();
resolve(document.querySelector(selector));
}
});

observer.observe(document.body, {
childList: true,
subtree: true,
});
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance robustness and reusability of waitForElement function

The waitForElement function is a good implementation for dynamically waiting for elements to appear in the DOM. However, there are a few improvements that could make it more robust and reusable:

  1. Add a timeout mechanism to prevent indefinite waiting, which could lead to memory leaks if the element never appears.
  2. Refactor repeated document.querySelector(selector) calls to improve maintainability.
  3. Export the function to make it available for use in other modules.
  4. Add error handling for invalid selectors.

Here's a suggested implementation incorporating these improvements:

export function waitForElement(selector, timeout = 5000) {
  return new Promise((resolve, reject) => {
    const checkElement = () => document.querySelector(selector);
    const element = checkElement();
    if (element) {
      return resolve(element);
    }

    const observer = new MutationObserver(() => {
      const element = checkElement();
      if (element) {
        observer.disconnect();
        resolve(element);
      }
    });

    observer.observe(document.body, {
      childList: true,
      subtree: true,
    });

    setTimeout(() => {
      observer.disconnect();
      reject(new Error(`Timeout waiting for element: ${selector}`));
    }, timeout);
  });
}

// Usage example:
// waitForElement('#my-element', 10000)
//   .then(element => console.log('Element found:', element))
//   .catch(error => console.error(error));

This implementation:

  • Adds a configurable timeout.
  • Refactors the repeated querySelector calls.
  • Exports the function.
  • Rejects the promise if the element isn't found within the timeout period.
  • Includes a usage example for clarity.

Consider adding this utility function to a shared utilities module if it's likely to be used across multiple components or scripts in your application.

@@ -157,7 +157,7 @@ async function createWindow() {
},
});

mainWindow = win;
global.mainWindow = win;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Refactor createWindow for better separation of concerns and security

The createWindow function has been updated with new functionality, but there are some concerns:

  1. Cookie Handling: Forcing SameSite to 'None' for all cookies can have security implications. This should be done selectively only for cookies that absolutely require it.

  2. Function Length: The function is handling multiple concerns (window creation, cookie handling, request interception). Consider breaking these into separate functions for better maintainability.

  3. Security: Modifying cookies and headers can potentially introduce security vulnerabilities. Ensure that these modifications are absolutely necessary and well-documented.

Suggestions:

  1. Extract cookie handling logic into a separate function:
function setupCookieHandling(win) {
  win.webContents.session.cookies.on('changed', async (event, cookie, cause, removed) => {
    // ... existing cookie handling logic ...
  });
}
  1. Extract request interception logic into a separate function:
function setupRequestInterception(win) {
  win.webContents.session.webRequest.onBeforeSendHeaders((details, callback) => {
    // ... existing request interception logic ...
  });
}
  1. Update createWindow:
async function createWindow() {
  const win = new BrowserWindow({
    // ... existing options ...
  });

  global.mainWindow = win;

  setupCookieHandling(win);
  setupRequestInterception(win);

  // ... rest of the function ...
}
  1. Add comments explaining why certain security-sensitive operations (like modifying SameSite attributes) are necessary.

Also applies to: 164-209

if (event.data) {
/** @type {{ "created": number, "model": string, "choices": [ { "index": number, "delta": { "role": string, "content": string } } ] }} */
const response = JSON.parse(event.data);
if (response && response.choices && response.choices.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify conditions using optional chaining

The conditions in lines 61 and 63 can be simplified using optional chaining for better readability and conciseness.

Line 61:

-            if (response && response.choices && response.choices.length) {
+            if (response?.choices?.length) {

Line 63:

-                if (choice.delta && choice.delta.content) {
+                if (choice.delta?.content) {

Also applies to: 63-63

🧰 Tools
🪛 Biome

[error] 61-61: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Comment on lines +98 to +99
console.error(event);
reject(this.getSSEDisplayError(event));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure getSSEDisplayError method is implemented

In line 99, reject(this.getSSEDisplayError(event)); calls getSSEDisplayError(event), but it's unclear if this method is defined in the class or its parent classes. Ensure that getSSEDisplayError is implemented to correctly handle and display SSE error events.

Comment on lines +44 to 134
done: true,
});
resolve();
}
});
source.addEventListener("error", (event) => {
console.error(event);
reject(this.getSSEDisplayError(event));
});
source.stream();
} catch (err) {
reject(err);
}
};

let text = "";
return new Promise((resolve, reject) => {
ipcRenderer.once("phind-request", (_, postData) =>
onPhindRequest(_, postData, text, resolve, reject),
);
})
.then(async () => {
ipcRenderer.invoke("close-chat-window", PhindBot._brandId);
let response;
if (context.chatId) {
response = await axios.put("https://www.phind.com/api/db/chat", {
chatId: context.chatId,
messages: context.message_history?.slice(-2),
});
source.addEventListener("error", (event) => {
console.error(event);
reject(this.getSSEDisplayError(event));
} else {
response = await axios.post("https://www.phind.com/api/db/chat", {
title: prompt,
messages: context.message_history?.slice(-2),
});

// override default _onStreamProgress to fix missing new line in response due to trimming
source._onStreamProgress = function (e) {
if (!source.xhr) {
return;
}

if (source.xhr.status !== 200) {
source._onStreamFailure(e);
return;
}

if (source.readyState == source.CONNECTING) {
source.dispatchEvent(new CustomEvent("open"));
source._setReadyState(source.OPEN);
}

var data = source.xhr.responseText.substring(source.progress);

source.progress += data.length;
var parts = (source.chunk + data).split(/\r\n\r\n/);
var lastPart = parts.pop();
for (let part of parts) {
// skip if data is empty
if (part === "data: ") {
continue;
}

// newline
if (part === "data: \r\ndata: ") {
let event = new CustomEvent("message");
event.data = "\n";
source.dispatchEvent(event);
continue;
}

const event = source._parseEventChunk(part);
source.dispatchEvent(event);
}
source.chunk = lastPart;
};
source.stream();
} catch (err) {
reject(err);
context.chatId = response.data;
this.setChatContext(context);
}
})
.catch((error) => {
console.error("Operation failed:", error);
ipcRenderer.invoke("close-chat-window", PhindBot._brandId);
});
} catch (error) {
if (error.request.status === 403) {
throw new Error(
`${error.request.status} ${error.request.responseText}`,
);
} else {
console.error("Error PhindBot _sendPrompt:", error);
throw error;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor _sendPrompt method for improved maintainability

The _sendPrompt method is quite lengthy and contains nested functions and multiple asynchronous operations. For better readability and maintainability, consider refactoring this method into smaller, reusable helper functions. This will make the code easier to understand, test, and debug.

🧰 Tools
🪛 Biome

[error] 61-61: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 63-63: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

});
const onPhindRequest = (_, postData, text, resolve, reject) => {
try {
const source = new SSE("https://https.api.phind.com/agent/", {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the URL in the SSE connection

In line 53, the SSE connection is initialized with an incorrect URL "https://https.api.phind.com/agent/". The domain seems to have an extra "https." prefix. The correct URL should likely be "https://api.phind.com/agent/". Verify and update the URL to prevent connection issues.

Apply this diff to fix the URL:

-        const source = new SSE("https://https.api.phind.com/agent/", {
+        const source = new SSE("https://api.phind.com/agent/", {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const source = new SSE("https://https.api.phind.com/agent/", {
const source = new SSE("https://api.phind.com/agent/", {

Comment on lines +24 to +33
try {
const response = await axios.get(
"https://www.phind.com/api/auth/session",
);
if (response?.data?.user?.userId) {
return true;
}
} catch (error) {
console.error("Error PhindBot check login:", error);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure _checkAvailability consistently returns a boolean

In the _checkAvailability method, the try block returns true when the user is logged in. However, if an error occurs, the catch block logs the error but doesn't return a value. This can result in the method returning undefined instead of false, which may cause unexpected behavior. Consider returning false at the end of the method to ensure it always returns a boolean.

Apply this diff to fix the issue:

    } catch (error) {
      console.error("Error PhindBot check login:", error);
+     return false;
    }
+   return false;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
const response = await axios.get(
"https://www.phind.com/api/auth/session",
);
if (response?.data?.user?.userId) {
return true;
}
} catch (error) {
console.error("Error PhindBot check login:", error);
}
try {
const response = await axios.get(
"https://www.phind.com/api/auth/session",
);
if (response?.data?.user?.userId) {
return true;
}
} catch (error) {
console.error("Error PhindBot check login:", error);
return false;
}
return false;

Copy link
Collaborator

@PeterDaveHello PeterDaveHello left a comment

Choose a reason for hiding this comment

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

@asyncButNeverAwaits would you like to continue working on this PR? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants