all: add per-client filter list assignment#8274
Open
fegauthier-paragon wants to merge 1 commit intoAdguardTeam:masterfrom
Open
all: add per-client filter list assignment#8274fegauthier-paragon wants to merge 1 commit intoAdguardTeam:masterfrom
fegauthier-paragon wants to merge 1 commit intoAdguardTeam:masterfrom
Conversation
10 tasks
There was a problem hiding this comment.
Other comments (2)
- internal/filtering/filter.go (668-669) The new `enabledBlockFilterIDs` and `enabledAllowFilterIDs` maps are key to the per-client filtering functionality, but they lack documentation. Consider adding comments explaining their purpose and how they're used in the filtering process.
- internal/filtering/filter.go (218-221) The code now loads all filters regardless of their enabled state, which is a significant change in behavior. Consider adding a comment explaining this design decision to help future maintainers understand the rationale.
💡 To request another review, post a new comment with "?url=https%3A%2F%2Fgithub.com%2Fwindsurf-review".
28b699c to
249a7d3
Compare
Author
|
/windsurf-review |
There was a problem hiding this comment.
Other comments (1)
- internal/filtering/filtering.go (1031-1038) When both `setts.ClientFilterListIDs` and `d.enabledBlockFilterIDs` are nil, no filtering is applied to the DNS result. Consider adding a fallback behavior to keep all rules in this case, or document that this is the intended behavior.
💡 To request another review, post a new comment with "?url=https%3A%2F%2Fgithub.com%2Fwindsurf-review".
Add the ability to assign specific blocklists and allowlists to individual clients. When UseOwnFilterLists is true, only the assigned filter lists apply to that client instead of the global ones. User rules (ID 0) always apply regardless of assignment. All filters are now loaded into the engine even when globally disabled, so that per-client assignments work for any filter. Globally-disabled filters are excluded at match time for clients using global settings.
249a7d3 to
944544d
Compare
Author
|
/windsurf-review |
There was a problem hiding this comment.
Other comments (4)
-
internal/client/storage_test.go (1330-1331)
Consider using the two-value lookup pattern for map access here as well:
require.NotNil(t, setts.ClientAllowListIDs) val, ok := setts.ClientAllowListIDs[5] assert.True(t, ok && val) - internal/filtering/filter.go (209-214) While the new comment at line 209-214 explains why disabled filters are loaded, it would be helpful to add a note about the memory usage implications. Consider mentioning approximate memory overhead per rule or filter to help users understand the tradeoff between functionality and resource usage.
-
internal/filtering/filter.go (649-658)
Variable naming inconsistency: `enabledBlockIDs` and `enabledAllowIDs` are declared but then assigned to `d.enabledBlockFilterIDs` and `d.enabledAllowFilterIDs`. Consider using consistent naming to avoid confusion.
enabledBlockFilterIDs := make(map[rules.ListID]bool, len(d.conf.Filters)) for _, filter := range d.conf.Filters { filters = append(filters, Filter{ ID: filter.ID, FilePath: filter.Path(d.conf.DataDir), }) if filter.Enabled { enabledBlockFilterIDs[filter.ID] = true } } -
internal/filtering/filter.go (660-670)
Similar naming inconsistency for allowlist filters. Consider using consistent variable names.
enabledAllowFilterIDs := make(map[rules.ListID]bool, len(d.conf.WhitelistFilters)) var allowFilters []Filter for _, filter := range d.conf.WhitelistFilters { allowFilters = append(allowFilters, Filter{ ID: filter.ID, FilePath: filter.Path(d.conf.DataDir), }) if filter.Enabled { enabledAllowFilterIDs[filter.ID] = true } }
💡 To request another review, post a new comment with "?url=https%3A%2F%2Fgithub.com%2Fwindsurf-review".
This was referenced Mar 2, 2026
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
UseOwnFilterLists,FilterListIDs, andAllowFilterListIDsfields to the client data model, following the existingUseOwnBlockedServicespatternmatchHost(): when a client has its own filter lists, only rules from assigned lists apply; user rules (ID 0) always applyfilterDNSResultByListIDs,CheckHostwith client filter lists, andApplyClientFilteringTest plan
go test ./internal/filtering/...passesgo test ./internal/client/...passesgo test ./internal/home/...passesuse_global_filter_lists: falseand specificfilter_list_idsvia the API