Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds ad blocking functionality backed by sing-box rulesets. The implementation uses a logical gate mechanism to enable/disable ad blocking dynamically, with state persisted to disk.
Key Changes:
- New
AdBlockercomponent manages ad blocking state using logical gates (AND=disabled, OR=enabled) - Integration with sing-box routing rules to block ad-related traffic
- Public API methods added to Radiance for enabling/disabling ad blocking
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vpn/adblocker.go | Core ad blocking logic with state management and persistence |
| vpn/adblocker_test.go | Comprehensive tests for ad blocker initialization, enable/disable, and persistence |
| vpn/boxoptions.go | Integration of ad block rules into sing-box routing configuration |
| radiance.go | Public API for ad blocking control and AdBlocker initialization |
| go.mod | Promoted miekg/dns and sagernet/sing-tun from indirect to direct dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| adBlocker, err := vpn.NewAdBlocker() | ||
| if err != nil { | ||
| slog.Error("Unable to create ad blocker", "error", err) | ||
| } |
There was a problem hiding this comment.
If vpn.NewAdBlocker() fails, the error is logged but execution continues with a nil adBlocker. This means r.adBlocker will be nil, but the Radiance instance is still created and returned successfully. While the public methods AdBlockEnabled() and SetAdBlockEnabled() do handle nil checks, it would be clearer to either:
- Return an error from
NewRadiance()if ad blocker initialization is critical, or - Document this behavior explicitly that ad blocking is optional and may be unavailable
4da5b07 to
341f39a
Compare
No description provided.