🛠️ Refactor: Split API handler into focused files#143
Conversation
Co-authored-by: lucasew <15693688+lucasew@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @lucasew, 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 significantly improves the Highlights
Changelog
Activity
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-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 does a great job of refactoring the main API handler into smaller, more focused files, which significantly improves modularity and maintainability. The separation of concerns into client, fetch, render, and request files is logical and follows the Single Responsibility Principle.
I've added a couple of suggestions for improvement: one to make the SSRF protection logic in the HTTP client more direct and another to ensure the user-agent rotation works reliably across different Go versions. Overall, this is a solid improvement to the codebase.
| ips, err := net.LookupIP(host) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, ip := range ips { | ||
| if ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() { | ||
| return errors.New("refusing to connect to private network address") | ||
| } | ||
| } |
There was a problem hiding this comment.
The address parameter in the Control function is already a resolved IP address string. Using net.ParseIP is more direct and efficient for this task than net.LookupIP, which is designed for DNS name resolution. This change avoids an unnecessary lookup and a loop.
ip := net.ParseIP(host)
if ip == nil {
return fmt.Errorf("invalid IP address: %s", host)
}
if ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() {
return errors.New("refusing to connect to private network address")
}| func getRandomUserAgent() string { | ||
| return userAgentPool[rand.Intn(len(userAgentPool))] | ||
| } |
There was a problem hiding this comment.
On Go versions prior to 1.20, the global math/rand source is deterministic unless seeded. This would cause rand.Intn to return the same value on every execution, meaning the same user agent would be used for every request, defeating the purpose of the user agent pool.
To ensure user agents are properly rotated across all Go versions and to follow best practices by avoiding stateful package-level functions, it's better to use a dedicated rand.Rand instance.
Here's how you can implement this:
// Add this at the package level in fetch.go.
// This will require importing the "time" package.
var userAgentRand = rand.New(rand.NewSource(time.Now().UnixNano()))
// Update getRandomUserAgent to use it.
func getRandomUserAgent() string {
return userAgentPool[userAgentRand.Intn(len(userAgentPool))]
}If you are certain the project is built with Go 1.20 or newer, the top-level rand.Intn is automatically seeded, and you can disregard this suggestion.
1 participant
Modularize api/index.go into separate files (client.go, fetch.go, render.go, request.go) to improve cohesion and maintainability.
api/client.go.api/fetch.go.api/render.go.api/request.go.api/index.goas the entry point and middleware orchestrator.AGENTS.mdto reflect the new location ofuserAgentPool.This refactoring adheres to the Single Responsibility Principle and makes the codebase easier to navigate and test.
PR created automatically by Jules for task 2307800879716948708 started by @lucasew