[init] initial implementation - #1
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughIntroduces the ChangesHTTP client module
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant FxModule
participant Factory
participant newClient
participant HTTPTransport
Application->>FxModule: initialize httpfx.Module()
FxModule->>Factory: provide NewFactory(Config, Logger)
FxModule->>Factory: call NewClient()
Factory->>newClient: build client from Config
newClient->>HTTPTransport: configure connections and proxy routing
newClient-->>Factory: return *http.Client
Factory-->>FxModule: provide *http.Client
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client.go`:
- Around line 45-48: Update both invalid-URL error paths in factory.NewClient’s
URL parsing flow to return only the generic ErrInvalidProxyURL error, without
including rawURL or any embedded credentials; preserve error wrapping if needed
while ensuring neither path exposes the original URL.
- Around line 95-99: Update the bypass parsing loop in client.go around
perHost.AddHost to detect CIDR values with net.ParseCIDR and register
successfully parsed networks via perHost.AddNetwork, while retaining AddHost for
hostnames and literal IPs. Update the documented bypass behavior in README.md
lines 187-194 to reflect CIDR network matching; no other site changes are
required.
In `@README.md`:
- Line 119: Update the proxy configuration documentation to state the actual
precedence as ProxyURL → ProxyFromEnv → HTTPProxyURL, and document the
configuration needed to disable all proxying, including inherited environment
and HTTP proxy fallbacks when ProxyURL is cleared.
- Around line 83-87: Update the Go module setup import block in the README
example to include the standard-library time package, matching the existing
time.Second usage so the snippet compiles as shown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e58db6d-b753-401d-a8f7-425baf7dc241
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.mdclient.goconfig.goerrors.gofactory.gogo.modmodule.gooptions.go
capcom6
force-pushed
the
init/add-implementation
branch
2 times, most recently
from
July 25, 2026 09:28
61b9382 to
52bed96
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
client.go (1)
84-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep cancellation in the non-
ContextDialerfallback.The callback receives
ctxbut discards it and callsdialer.Dial. A custom registered proxy dialer can therefore continue dialing after the request or client timeout. Use a context-aware adapter, or reject dialers that do not implementproxy.ContextDialer;x/net/proxyuses this pattern inPerHost.DialContext. (github.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client.go` around lines 84 - 96, Update setTransportDialer’s non-ContextDialer fallback to preserve cancellation from the callback’s context, using a context-aware adapter consistent with proxy.PerHost.DialContext or rejecting unsupported dialers; do not ignore ctx while invoking dialer.Dial.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client.go`:
- Around line 59-64: The applyEnvProxy function currently treats all
proxy.Direct returns the same way, but it needs to distinguish between absent
proxy environment variables versus present-but-broken ones. Check whether proxy
environment variables are actually set in the environment before the
proxy.FromEnvironment() call. If proxy variables are present but
proxy.FromEnvironment() returns proxy.Direct, return an error to fail closed. If
no proxy variables are set, continue with the existing nil return. Preserve the
bypass parameter handling throughout this logic.
- Around line 42-50: Add validation in the applySOCKSProxy function to check
that the parsed URL contains a non-empty hostname before attempting to construct
the proxy dialer. After the existing url.Parse error check and before the
proxy.FromURL call, validate that u.Hostname() returns a non-empty string, and
return an error wrapping ErrInvalidProxyURL if the hostname is missing or empty.
This prevents malformed SOCKS URLs like socks5:// from reaching the dialer
construction step.
In `@config.go`:
- Around line 15-17: Update the documentation comment for the Bypass field to
state that it applies when any proxy is configured, covering both explicit SOCKS
proxies and ProxyFromEnv.
In `@README.md`:
- Line 47: Update the proxy environment-variable references in the README
overview, configuration table, and example to be consistent: either document
both ALL_PROXY and all_proxy in every occurrence, or remove all_proxy from the
overview and retain only ALL_PROXY.
- Around line 15-17: Update the “Report Bug” and “Request Feature” links in
README.md to point to the repository’s issue destinations rather than its root,
using the appropriate bug-report and feature-request URLs while leaving the link
labels unchanged.
---
Nitpick comments:
In `@client.go`:
- Around line 84-96: Update setTransportDialer’s non-ContextDialer fallback to
preserve cancellation from the callback’s context, using a context-aware adapter
consistent with proxy.PerHost.DialContext or rejecting unsupported dialers; do
not ignore ctx while invoking dialer.Dial.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc6e71ab-adee-436c-b273-1ca5ece499b0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.mdclient.goconfig.goerrors.gofactory.gogo.modmodule.gooptions.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- factory.go
- errors.go
capcom6
force-pushed
the
init/add-implementation
branch
from
July 27, 2026 01:48
253a5b3 to
f12dde3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 248: Update the README license-url reference to point to the repository’s
default master branch by replacing the current main branch segment with master,
while preserving the existing LICENSE target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ac0eb23-a1cf-4da4-b55c-51c2fe046e7e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.mdclient.goconfig.goerrors.gofactory.gogo.modmodule.gooptions.go
🚧 Files skipped from review as they are similar to previous changes (7)
- errors.go
- go.mod
- factory.go
- module.go
- options.go
- config.go
- client.go
capcom6
force-pushed
the
init/add-implementation
branch
from
July 28, 2026 02:11
4a27acf to
c17ecdd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
httpfx) that provides configured*http.Clientinstances via a newFactory.Configplus per-clientOptionoverrides for timeouts and connection pooling.ALL_PROXY, and SOCKS5), including bypass lists and validation, plus exported sentinel errors for invalid proxy/config and dial failures.