Conversation
![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 introduces a mechanism to detect and handle Google Front End (GFE) error pages by throwing a specific FirebaseError when the response contains characteristic HTML markers. The feedback provided addresses a typo and capitalization issue in the error message, suggests using a more robust regular expression for error detection to avoid brittleness with special characters, and ensures the associated unit tests are updated to match the corrected error string.
| if (options.responseType === "json") { | ||
| const text = await res.text(); | ||
| if (isGfeError(text)) { | ||
| throw new FirebaseError("the service you are calling doesnt exist or is misconfigured", { status: res.status }); |
There was a problem hiding this comment.
The error message contains a typo ("doesnt") and should be properly capitalized for consistency with other user-facing errors in the codebase.
| throw new FirebaseError("the service you are calling doesnt exist or is misconfigured", { status: res.status }); | |
| throw new FirebaseError("The service you are calling doesn't exist or is misconfigured.", { status: res.status }); |
References
- The repository style guide recommends using FirebaseError for user-facing errors; ensuring these messages are clear and correctly formatted maintains professional quality. (link)
| } | ||
|
|
||
| function isGfeError(text: string): boolean { | ||
| return text.includes("That’s all we know.") && text.includes("<!DOCTYPE html>"); |
There was a problem hiding this comment.
Using a specific smart quote (’) in text.includes makes the detection brittle, as the response might use a standard apostrophe or different casing. A regular expression is more robust for identifying these error pages.
| return text.includes("That’s all we know.") && text.includes("<!DOCTYPE html>"); | |
| return /That['’]s all we know/i.test(text) && /<!DOCTYPE html>/i.test(text); |
| method: "GET", | ||
| path: "/path/to/foo", | ||
| }); | ||
| await expect(r).to.eventually.be.rejectedWith(FirebaseError, "the service you are calling doesnt exist or is misconfigured"); |
There was a problem hiding this comment.
Updating the test expectation to match the corrected error message.
| await expect(r).to.eventually.be.rejectedWith(FirebaseError, "the service you are calling doesnt exist or is misconfigured"); | |
| await expect(r).to.eventually.be.rejectedWith(FirebaseError, "The service you are calling doesn't exist or is misconfigured."); |
2 participants
Description
Jetski took a crack at handling these errors that show up when you try to hit a API and your request gets blackholed.
Open to discussion on exactly how this should behave to best help users/developers understand and fix the issue.