Adsblock to master#5
Conversation
…management, and UI integration
… logs, moving logic from frontend to backend store.
…sktop integration, and implement a post-removal cleanup.
…cluding Wails bindings and updated build scripts.
…nd real-time statistics, and a new CopyButton component.
…d SQLite persistence for logs, rules, and settings.
There was a problem hiding this comment.
Pull request overview
This PR integrates comprehensive adblock functionality into the Custos application, enabling ad and tracker blocking through a Rust-based adblock engine with FFI bindings. The changes include filter list management, pagination for logs, and various UI/UX improvements.
Key changes:
- Adds Rust adblock engine with C FFI bindings and Go CGO integration
- Implements filter list management system with automatic downloading and caching
- Adds cursor-based pagination for traffic logs with filtering capabilities
Reviewed changes
Copilot reviewed 37 out of 40 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/adblock/src/lib.rs | New Rust library implementing adblock engine with C FFI interface |
| lib/adblock/Cargo.toml | Cargo configuration for the adblock static library |
| internal/adblock/engine.go | Go CGO bindings for the Rust adblock engine |
| internal/adblock/stub.go | Non-CGO stub implementation for builds without CGO |
| internal/proxy/server.go | Integrates adblock engine into proxy server request handling |
| internal/core/types.go | Adds adblock-related data structures (AdblockFilter, reason field) |
| internal/store/sqlite.go | Implements database operations for adblock filters and paginated logs |
| app.go | Adds filter management, seeding, and refresh logic |
| frontend/src/pages/AdblockFilters.tsx | New UI page for managing adblock filter lists |
| frontend/src/pages/Traffic.tsx | Adds pagination controls and reason column to traffic logs |
| nfpm.yaml | Adds desktop integration files and post-removal script |
| scripts/postremove.sh | Cleanup script for removing user data on uninstall |
Comments suppressed due to low confidence (1)
scripts/postremove.sh:1
- Using rm -rf without additional safety checks on user input is dangerous. Consider adding validation to ensure the path is within expected bounds and doesn't follow symlinks that could delete unintended directories.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </span> | ||
| <td className="p-4 items-center flex justify-start"> | ||
| {log.reason ? <span className={`px-2 py-0.5 rounded text-xs font-medium border ${log.reason === 'adsblock' ? 'bg-blue-500/10 text-blue-500 border-blue-500/20' : 'bg-purple-500/10 text-purple-500 border-purple-500/20'}`}> | ||
| {log.reason.toLocaleUpperCase()} |
There was a problem hiding this comment.
Corrected spelling of 'toLocaleUpperCase' to 'toLocaleUpperCase'. The method name should be 'toLocaleUpperCase()' (with 'Locale'), but more commonly 'toUpperCase()' is used for simple uppercase conversion.
| {log.reason.toLocaleUpperCase()} | |
| {log.reason.toUpperCase()} |
|
|
||
| if rule.Type == core.RuleBlock { | ||
| r.logBlock(req, domain, string(core.RuleSourceCustom), &core.Process{ | ||
| r.store.IncrementAdblockHit(domain) |
There was a problem hiding this comment.
Adblock hit is incremented for custom rule blocks (line 256) when it should only be incremented for actual adblock engine matches. This causes incorrect statistics as custom rule blocks are counted as adblock hits.
| r.store.IncrementAdblockHit(domain) |
| // ... continue to allow | ||
| // Check Blocklist | ||
| if r.blocklist.IsBlocked(domain) { | ||
| r.store.IncrementAdblockHit(domain) |
There was a problem hiding this comment.
Adblock hit is incremented for blocklist matches when this should be tracked separately or not at all, as blocklist hits are different from adblock filter hits.
| r.store.IncrementAdblockHit(domain) |
| RuleSourceProtocolHttpsBlocked RuleType = "protection_https_blocked" | ||
| RuleSourceProtocolHttpAllowed RuleType = "protection_http_allowed" | ||
| RuleSourceProtocolHttpsAllowed RuleType = "protection_https_allowed" | ||
| RuleSourceAdsblock RuleType = "adsblock" |
There was a problem hiding this comment.
Corrected spelling of 'adsblock' to 'adblock' for consistency with other naming in the codebase.
| RuleSourceAdsblock RuleType = "adsblock" | |
| RuleSourceAdblock RuleType = "adblock" |
| fmt.Printf("Failed to disable system proxy on shutdown: %v\n", err) | ||
| } | ||
| a.proxyServer.Stop() | ||
| ctx.Done() |
There was a problem hiding this comment.
Calling ctx.Done() has no effect as it only returns a channel. If the intention is to cancel the context or wait for cancellation, this line should be removed or replaced with appropriate context cancellation logic.
| ctx.Done() |
| // Force a final report in a goroutine to not block the connection close | ||
| go c.report() |
There was a problem hiding this comment.
Running the final report in a goroutine may cause the report to be lost if the program terminates before the goroutine completes. Consider using a sync.WaitGroup or ensuring the goroutine completes before shutdown.
| // Force a final report in a goroutine to not block the connection close | |
| go c.report() | |
| // Force a final report before closing to ensure stats are persisted | |
| c.report() |
| // Truncate before seeding as requested | ||
| a.store.ClearAdblockFilters() | ||
|
|
There was a problem hiding this comment.
Clearing all adblock filters on every startup will delete user-added filters. This should only be done on first run or when explicitly requested by the user.
| // Truncate before seeding as requested | |
| a.store.ClearAdblockFilters() |
| sEnabled := false | ||
| var engine *adblock.Engine | ||
|
|
||
| r.server.mu.RLock() | ||
| sEnabled = r.server.adblockEnabled | ||
| engine = r.server.adblockEngine | ||
| r.server.mu.RUnlock() | ||
|
|
||
| if sEnabled && engine != nil { |
There was a problem hiding this comment.
The variable name 'sEnabled' is unclear. It should be renamed to 'adblockEnabled' to clearly indicate it stores the adblock enabled state.
| sEnabled := false | |
| var engine *adblock.Engine | |
| r.server.mu.RLock() | |
| sEnabled = r.server.adblockEnabled | |
| engine = r.server.adblockEngine | |
| r.server.mu.RUnlock() | |
| if sEnabled && engine != nil { | |
| adblockEnabled := false | |
| var engine *adblock.Engine | |
| r.server.mu.RLock() | |
| adblockEnabled = r.server.adblockEnabled | |
| engine = r.server.adblockEngine | |
| r.server.mu.RUnlock() | |
| if adblockEnabled && engine != nil { |
| setStats(currentStats); | ||
| setConnections(fetchedConns || []); | ||
|
|
||
| console.log("fetchedStats", fetchedStats); |
There was a problem hiding this comment.
Debug console.log statement should be removed from production code.
| console.log("fetchedStats", fetchedStats); |
No description provided.