-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Automatic Filter Title Fetching #8112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Automatic Filter Title Fetching #8112
Conversation
Add endpoint to fetch blocklist/allowlist titles automatically from URLs. Frontend includes 800ms debounce, respects user input, and has debug logging. Backend includes permissive validation for URLs without file extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other comments (2)
- internal/filtering/http.go (64-99) This function duplicates significant portions of `validateFilterURL`. Consider refactoring to extract common validation logic into a shared helper function to avoid future maintenance issues.
-
internal/filtering/http.go (681-681)
The error message format is inconsistent with other handlers. Consider using lowercase for consistency:
"failed to parse request body json: %s",
💡 To request another review, post a new comment with "/windsurf-review".
| debounceTimerRef.current = setTimeout(async () => { | ||
| setIsFetchingTitle(true); | ||
| try { | ||
| const title = await dispatch(fetchFilterTitle(urlValue) as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using as any bypasses TypeScript's type checking, which can hide potential errors. Consider properly typing the fetchFilterTitle action return value or using a more specific type assertion that preserves some type safety.
| // - Name field already has a value (user has typed something) | ||
| // - We're in edit mode (initialValues provided) | ||
| // - URL doesn't pass basic validation | ||
| if (!urlValue || nameValue || initialValues?.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependency array includes initialValues, but the condition only checks initialValues?.name. Consider either checking the entire initialValues object in the condition or only including initialValues?.name in the dependency array for consistency.
| p := rulelist.NewParser() | ||
| res, err := p.Parse(io.Discard, lr, *bufPtr) | ||
| if err != nil { | ||
| return "", fmt.Errorf("parsing filter: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function downloads and parses the entire filter content (up to 4KB) even though we only need the title. Consider adding a method to rulelist.Parser that can extract just the title without parsing all rules, which would be more efficient.
Closes #8101.
Overview
This PR adds automatic filter name fetching when users add custom blocklists or allowlists. Instead of manually typing both URL and name, users can now paste the filter URL and have the name auto-populated from the filter's metadata.
Implementation
Backend:
POST /control/filtering/fetch_title/hosts)rulelist.ParserFrontend:
Testing:
Screenshots
Breaking Changes
None. This is a purely additive feature with graceful degradation.