fix(core): add error logging for IDE fetch failures#17981
fix(core): add error logging for IDE fetch failures#17981Adib234 merged 4 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @yuvrajangadsingh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a diagnostic improvement by adding error logging to network requests made to the IDE. By catching and logging exceptions during Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
gemini-cli
bot
added
area/core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds valuable error logging for fetch failures in the IDE client, which will aid in diagnosing connection problems. The implementation is a good, simple fix. I have one suggestion to improve the logging by using the existing logger instance and the error log level for better consistency and semantics, aligning with our guidelines for detailed error logging.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds valuable error logging for fetch requests to the IDE, which will greatly aid in diagnosing connection issues. The implementation is straightforward and correct. I've suggested an improvement to the logging call to pass the full error object, aligning with our guidelines to provide richer debugging information, including the stack trace, when catching exceptions.
yuvrajangadsingh
force-pushed
the
fix/ide-fetch-error-handling
branch
from
195f474 to
f23bac3
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of lacking error visibility during IDE fetch failures by introducing a try-catch block around the fetchFn call in createProxyAwareFetch. The added logging provides crucial context, including the failed URL and the error object, which will significantly aid in diagnosing IDE connection problems. The implementation is clean and correctly re-throws the error, preserving the original error propagation flow. This is a valuable improvement for debugging and overall system maintainability.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds valuable error logging for IDE fetch failures, which will aid in diagnosing connection issues. The approach of using a try-catch block to log and re-throw errors is sound. I've identified an issue where the URL context might not be logged correctly for Request objects, potentially hindering debugging. The provided suggestion ensures the URL is always accurately captured, aligning with best practices for detailed error logging.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds error logging for fetch failures within createProxyAwareFetch, which is a good improvement for diagnosing IDE connection issues. The implementation correctly uses a try-catch block to log the error with the URL and then re-throws it. I've found one area for improvement in how the URL string is constructed for logging, which appears to contain a logical flaw.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces error logging for fetch failures within createProxyAwareFetch, which is a valuable addition for diagnosing IDE connection problems. The implementation correctly wraps the fetch call in a try-catch block to log errors. The suggested improvement to make the URL logging more robust by handling Request objects in addition to strings and URL objects has been retained as it is a valid and useful suggestion.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds error logging for failed fetch requests to the IDE, which is a great addition for debugging connection issues. I've reviewed the changes and found one potential issue in how the URL is stringified for logging. The current implementation doesn't handle Request objects correctly, which could lead to unhelpful log messages. I've suggested a more robust way to handle all possible URL types. Otherwise, the change is good.
|
Hi @NTaylorMullen, this PR is ready for review — all Gemini bot feedback has been addressed and resolved. Would appreciate a look when you get a chance. Thanks! |
yuvrajangadsingh
force-pushed
the
fix/ide-fetch-error-handling
branch
from
9d38c34 to
21527cd
Compare
|
Hey, just checking in — any chance someone could take a look at this? All feedback's been addressed and CI is green. Happy to make any changes if needed. |
| }); | ||
| try { | ||
| const response = await fetchFn(url, options); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion |
There was a problem hiding this comment.
Is it possible to not disable this lint rule?
There was a problem hiding this comment.
hey, that disable was already there before my changes — I just wrapped the existing code in a try/catch. the cast is needed because Node's RequestInit and undici's RequestInit don't play nice with each other, so going through unknown is the only option there.
yuvrajangadsingh
force-pushed
the
fix/ide-fetch-error-handling
branch
from
21527cd to
236e411
Compare
Fixes google-gemini#17513 Add try-catch block around the fetch call in createProxyAwareFetch to log errors before re-throwing. This helps diagnose IDE connection issues like "TypeError: fetch failed" by providing more context in debug logs. The error is logged with the URL that failed, making it easier to troubleshoot network connectivity issues between the CLI and IDE.
The url parameter is typed as string | URL, so the url.url fallback for Request objects is unreachable. Simplified to a straightforward ternary.
yuvrajangadsingh
force-pushed
the
fix/ide-fetch-error-handling
branch
from
236e411 to
f5df8ac
Compare
b79e5ce
2 participants
Summary
Fixes #17513
Add try-catch block around the fetch call in
createProxyAwareFetchto log errors before re-throwing. This helps diagnose IDE connection issues like "TypeError: fetch failed" by providing more context in debug logs.The error is logged with the URL that failed, making it easier to troubleshoot network connectivity issues between the CLI and IDE.
Test plan
npx vitest run packages/core/src/ide/ide-client.test.ts- all 56 tests pass