test: add comprehensive tests for request filter binding types#483
test: add comprehensive tests for request filter binding types#483miraserver wants to merge 4 commits intoding113:devfrom
Conversation
Allow Request Filters to be applied globally, to specific providers, or to provider groups.
## Key Changes
### Database Schema
- Add `bindingType` field ('global' | 'providers' | 'groups')
- Add `providerIds` field (array of provider IDs)
- Add `groupTags` field (array of group tags)
- Migration: 0041_sticky_jackal.sql
### Backend Logic
- Split filter engine into global and provider-specific filters
- `applyGlobal()`: runs BEFORE provider selection
- `applyForProvider()`: runs AFTER provider selection
- Provider matching: checks provider IDs or group tags
### Guard Pipeline
- Add new `providerRequestFilter` guard step
- CHAT_PIPELINE order: [..., provider, providerRequestFilter, messageContext]
- COUNT_TOKENS_PIPELINE includes provider filters
### UI Components
- New: `ProviderMultiSelect` - multi-select for providers
- New: `GroupMultiSelect` - multi-select for group tags
- Updated: `FilterDialog` - binding type selector moved after Name field
- Updated: `FilterTable` - compact layout with icon-only display
- Binding display: Global (icon only), Providers (icon + count), Groups (icon + count)
### Localization
- Add translations for 5 languages (en, ru, ja, zh-CN, zh-TW)
- Column name changed from "Apply To" to "Apply"
### Bug Fixes
- Fix binding type change validation (use explicit undefined check instead of ??)
- Prevent stale binding data when switching types
### Other Improvements
- Compact last 4 columns in filter table (Priority, Apply, Status, Actions)
- Improved UX: binding configuration at top of dialog
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add truncate to name, target, and replacement columns to keep table rows single-line - Add tooltips on Apply column showing provider names or group tags on hover - Fetch provider list to display names instead of IDs in tooltips - Optimize provider lookup with useMemo Map (O(1) access) - Add i18n translations for tooltip labels (en, ja, ru, zh-CN, zh-TW) - Optimize request filter engine with caching and early exits 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Set max-w-2xl width for consistency with error rules dialog - Add max-h-80vh height limit to prevent overflow - Restructure layout with fixed header and footer - Make only content area scrollable when form has many fields - Ensures Cancel/Save buttons and close button always accessible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add 28 unit tests for RequestFilterEngine binding functionality - Cover global, provider-specific, and group-specific filters - Test combined filters and edge cases - Fix setFiltersForTest to sort by priority (matching reload behavior) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @miraserver, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major enhancement to the request filtering system by implementing different binding types for filters. Filters can now be configured to apply globally, to specific providers, or to custom provider groups. This change provides much finer control over request modification and routing. The update includes necessary database schema migrations, a refactored backend engine with performance improvements, new proxy pipeline integration, and a user-friendly interface for managing these advanced filter settings, all backed by a comprehensive suite of new unit tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly enhances the request filtering functionality. The introduction of provider and group-based filter bindings is a great feature, and the implementation is solid across the stack, from the database schema and backend logic to the new UI components.
I'm particularly impressed with the performance optimizations in RequestFilterEngine, such as pre-compiling regexes and using Sets for faster lookups. The comprehensive suite of 28 new unit tests is also a fantastic addition that ensures the new logic is robust and reliable.
I have a couple of minor suggestions for code simplification and micro-optimization, but overall, this is a very high-quality contribution. Great work!
| if (row.groupTag) { | ||
| const tags = row.groupTag.split(",").map((tag) => tag.trim()); | ||
| for (const tag of tags) { | ||
| if (tag) allTags.add(tag); | ||
| } | ||
| } |
There was a problem hiding this comment.
For better readability and conciseness, you can use a more functional approach with method chaining to parse and add tags to the Set. This avoids nested loops and if conditions.
row.groupTag
?.split(",")
.map((tag) => tag.trim())
.filter(Boolean)
.forEach((tag) => allTags.add(tag));| } else if (filter.bindingType === "groups" && providerTagsSet) { | ||
| // Optimization #3: O(m) instead of O(m*n) | ||
| matches = filter.groupTagsSet | ||
| ? Array.from(filter.groupTagsSet).some((tag) => providerTagsSet!.has(tag)) | ||
| : false; | ||
| } |
There was a problem hiding this comment.
To slightly improve performance and readability, you can iterate directly over filter.groupTagsSet to check for intersection with providerTagsSet. This avoids creating an intermediate array with Array.from() and allows for an early exit using break once a match is found.
| } else if (filter.bindingType === "groups" && providerTagsSet) { | |
| // Optimization #3: O(m) instead of O(m*n) | |
| matches = filter.groupTagsSet | |
| ? Array.from(filter.groupTagsSet).some((tag) => providerTagsSet!.has(tag)) | |
| : false; | |
| } | |
| } else if (filter.bindingType === "groups" && providerTagsSet && filter.groupTagsSet) { | |
| for (const tag of filter.groupTagsSet) { | |
| if (providerTagsSet.has(tag)) { | |
| matches = true; | |
| break; | |
| } | |
| } | |
| } |
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Optimization #1: Memory leak cleanup | ||
| private eventEmitterCleanup: (() => void) | null = null; | ||
|
|
||
| // Optimization #5: Skip tag parsing when no group filters |
There was a problem hiding this comment.
[Critical] [ERROR-SILENT] Memory leak: destroy() method is never called
Why this is a problem: The PR adds a destroy() method to fix event listener memory leaks (Optimization #1), but this method is never called anywhere in the codebase. The event listener cleanup function is stored but never executed, meaning the memory leak is NOT fixed - it's just made possible to fix.
The singleton requestFilterEngine is created at module level and never destroyed. In a long-running Node.js process with hot reloads or module re-imports, the old event listeners will accumulate without being cleaned up.
Suggested fix:
// Call destroy() before setting up new listener in setupEventListener
private async setupEventListener(): Promise<void> {
if (typeof process !== "undefined" && process.env.NEXT_RUNTIME !== "edge") {
try {
// Clean up existing listener before adding new one
if (this.eventEmitterCleanup) {
this.eventEmitterCleanup();
}
const { eventEmitter } = await import("@/lib/event-emitter");
const handler = () => { void this.reload(); };
eventEmitter.on("requestFiltersUpdated", handler);
this.eventEmitterCleanup = () => {
eventEmitter.off("requestFiltersUpdated", handler);
};
} catch { /* ignore */ }
}
}If destroy() is intended for future use, add a TODO comment documenting this.
There was a problem hiding this comment.
Code Review Summary
This PR adds provider-specific and group-specific request filter bindings, along with comprehensive tests. The implementation is well-tested and documented, but has one critical issue that needs addressing.
PR Size: L
- Lines changed: +3669/-69 (net +3600)
- Files changed: 21
Note: This is an L-size PR. Consider splitting the database schema changes, engine refactor, and UI changes into separate PRs to improve reviewability and reduce merge risk.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
1. Memory leak: destroy() method is never called - src/lib/request-filter-engine.ts:125
The PR adds a destroy() method to fix event listener memory leaks (Optimization #1), but this method is never called anywhere in the codebase. The singleton requestFilterEngine is created at module level and never destroyed. In a long-running Node.js process with hot reloads or module re-imports, old event listeners will accumulate.
Fix: Either call destroy() in setupEventListener() before adding a new listener, or add a TODO comment documenting that this is for future graceful shutdown.
High Priority Issues (Should Fix)
None identified.
What Was Done Well
- Comprehensive test coverage: 28 new unit tests covering all binding types and edge cases
- Performance optimizations: Pre-compiled regex, Set-based lookups, early exit patterns
- Proper validation: Binding type constraints validated in actions layer
- Good documentation: Russian comments explaining the purpose of each method
- Type safety: Proper use of TypeScript discriminated unions for binding types
- Database indexing: Added composite index for efficient binding type queries
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - 1 issue found
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Excellent (28 new tests)
- Code clarity - Good
Automated review by Claude AI
1 participant
Summary
Enhances the Request Filter system with provider/group binding capability, allowing filters to be applied to specific providers or provider groups instead of only globally. Also adds comprehensive test coverage (28 tests) for the binding functionality.
Problem
Previously, request filters could only be applied globally to all providers. This made it impossible to:
Related to #459 - Provider grouping and batch operations feature request
Solution
Added a three-tier binding system for request filters:
Database Schema Changes
Migration: `drizzle/0041_sticky_jackal.sql`
```sql
ALTER TABLE "request_filters" ADD COLUMN "binding_type" varchar(20) DEFAULT 'global' NOT NULL;
ALTER TABLE "request_filters" ADD COLUMN "provider_ids" jsonb;
ALTER TABLE "request_filters" ADD COLUMN "group_tags" jsonb;
CREATE INDEX "idx_request_filters_binding" ON "request_filters" ("is_enabled", "binding_type");
```
Schema: `src/drizzle/schema.ts`
Core Engine Changes
`src/lib/request-filter-engine.ts`
`src/app/v1/_lib/proxy/provider-request-filter.ts` (NEW)
`src/app/v1/_lib/proxy/guard-pipeline.ts`
`src/app/v1/_lib/proxy/request-filter.ts`
UI Components
Filter Dialog (`src/app/[locale]/settings/request-filters/_components/filter-dialog.tsx`)
New Components:
Filter Table (`src/app/[locale]/settings/request-filters/_components/filter-table.tsx`)
Server Actions
`src/actions/request-filters.ts`
`src/repository/request-filters.ts`
Internationalization
Added translations to all 5 locales (`messages/{locale}/settings.json`):
Test Coverage
New Test File: `tests/unit/request-filter-binding.test.ts` (645 lines, 28 tests)
Test scenarios:
Changes
Core Changes
Supporting Changes
Breaking Changes
Database Migration Required
Backward Compatibility:
Testing
Automated Tests
Manual Testing
Global Filter (Existing Behavior)
Provider-Specific Filter
Group-Specific Filter
Combined Filters
Priority Testing
Checklist
🤖 Generated with Claude Code