-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fixed aider echo and output error #198
Conversation
There was a problem hiding this 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 a3a5c0b in 1 minute and 20 seconds
More details
- Looked at
91
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. extensions/vscode/src/integrations/aider/aiderProcess.ts:47
- Draft comment:
TheshouldUpgrade
flag is set totrue
in the catch block, but the upgrade logic is outside the catch block. This means the upgrade attempt will always happen after an error, which might not be intended. Consider moving the upgrade logic inside the catch block. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. core/llm/llms/AiderLLM.ts:159
- Draft comment:
Theconsole.dir
statement in the constructor seems unnecessary for production code and might be a leftover from debugging. Consider removing it. - Reason this comment was not posted:
Confidence changes required:50%
Theconsole.dir
statement in the constructor of theAider
class seems unnecessary for production code and might be a leftover from debugging.
3. core/llm/llms/AiderLLM.ts:200
- Draft comment:
EnsurenewOutput
is defined before callingstartsWith
to avoid potential runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
ThestartedListening
flag is used to delay yielding messages until a newline-prefixed message is received. This logic seems correct, but it's important to ensure thatnewOutput
is correctly defined before callingstartsWith
.
Workflow ID: wflow_S2iw9X9pPjlaujO0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this 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 38e2eda in 19 seconds
More details
- Looked at
71
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. extensions/vscode/src/integrations/aider/aiderUtil.ts:166
- Draft comment:
Consider usingconsole.warn
instead ofconsole.error
for logging errors to maintain consistency with other parts of the code. - Reason this comment was not posted:
Confidence changes required:50%
The error handling in the installation process is inconsistent. It would be better to handle errors in a consistent manner across different platforms.
2. extensions/vscode/src/integrations/aider/aiderUtil.ts:188
- Draft comment:
Consider usingconsole.warn
instead ofconsole.error
for logging errors to maintain consistency with other parts of the code. - Reason this comment was not posted:
Confidence changes required:50%
The error handling in the installation process is inconsistent. It would be better to handle errors in a consistent manner across different platforms.
Workflow ID: wflow_vCESlNaO1dw4JxOO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this 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 646ec35 in 51 seconds
More details
- Looked at
80
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. extensions/vscode/src/integrations/aider/aiderProcess.ts:54
- Draft comment:
UsingexecSync
in an async function can block the event loop. Consider usingexec
orpromisify
to handle this asynchronously. - Reason this comment was not posted:
Marked as duplicate.
2. extensions/vscode/src/integrations/aider/aiderProcess.ts:59
- Draft comment:
UsingexecSync
in an async function can block the event loop. Consider usingexec
orpromisify
to handle this asynchronously. - Reason this comment was not posted:
Marked as duplicate.
3. extensions/vscode/src/integrations/aider/aiderProcess.ts:64
- Draft comment:
UsingexecSync
in an async function can block the event loop. Consider usingexec
orpromisify
to handle this asynchronously. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_QRjriSdweF5PuKzM
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.
{ | ||
name: 'pipx reinstall', | ||
command: async () => { | ||
await execSync(`pipx uninstall aider-chat`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using execSync
in an async function can block the event loop. Consider using exec
or promisify
to handle this asynchronously.
There was a problem hiding this 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 e0c423b in 34 seconds
More details
- Looked at
27
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. extensions/vscode/src/integrations/aider/aiderProcess.ts:121
- Draft comment:
Consider using an asynchronous method instead ofexecSync
to avoid blocking the event loop. This applies to other uses ofexecSync
inupdateUsingPackageManager
. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_FBIrhn7QvzU5xkxh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this 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 85dac88 in 19 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. extensions/vscode/src/integrations/aider/aiderProcess.ts:74
- Draft comment:
UsingexecSync
in an async function can block the event loop. Consider usingexec
orspawn
for non-blocking execution. This applies to other uses ofexecSync
in this file as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_jQTwcCjVpT5qVN1t
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* Fixed echo and output error * Fallback to brew install * Fallback to brew install * Added no detect urls * Added resetSession --------- Co-authored-by: nang-dev <[email protected]>
Description ✏️
Closes #xxx
What changed? Feel free to be brief.
Checklist ✅
Important
Improved Aider process output handling, error logging, version management, and installation logic across multiple files.
AiderLLM.ts
, addedstartedListening
flag to process output only after a newline-prefixed message.aiderProcess.ts
, added stderr listener to log errors from the Aider process.aiderProcess.ts
, added logic to attempt upgrading Aider if initial update fails.aiderUtil.ts
, improved installation logic for Aider on Windows and Unix systems, with fallback mechanisms.AiderLLM.ts
.This description was created by
for 85dac88. It will automatically update as commits are pushed.