fix: target URL filter by devinat1 · Pull Request #4735 · Helicone/helicone

devinat1 · 2025-09-11T21:53:27Z

Added target_url to the request_response_rmt key mappings in filters.ts, enabling filtering by this field.
*Test buildFilterClickHouse with the target_url field

vercel · 2025-09-11T21:53:31Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
helicone	Ready	Preview	Comment	Sep 11, 2025 10:20pm
helicone-bifrost	Ready	Preview	Comment	Sep 11, 2025 10:20pm
helicone-eu	Ready	Preview	Comment	Sep 11, 2025 10:20pm

claude · 2025-09-11T21:53:48Z

Claude finished @devinat1's task —— View job

PR Review Complete

Todo List:

Read the modified filters.ts file
Analyze the new test file
Examine broader filtering system context
Provide comprehensive code review feedback
Assign confidence score with reasoning

Code Review Summary

This PR adds target_url filtering capability to the Helicone filtering system by making a minimal but necessary change to the request_response_rmt table mappings.

✅ Strengths

Minimal and Focused: The core change is just one line (target_url: "request_response_rmt.target_url"), which is exactly what's needed
Type Safety: The target_url field is already properly defined in filterDefs.ts:202 with SingleKey<TextOperators>
Comprehensive Testing: The test suite covers:
- All standard text operators (equals, like, ilike, contains, not-equals)
- Edge cases (empty strings, null values, special characters, unicode, very long URLs)
- Integration scenarios (existing argument arrays)
- Multiple URL formats from different providers
Consistent Implementation: Follows existing patterns in the codebase for filter mappings
No Breaking Changes: This is purely additive functionality

📋 Code Quality Assessment

Detailed Analysis

Core Implementation (filters.ts:280)

✅ Correctly maps target_url to request_response_rmt.target_url
✅ Positioned appropriately within the easyKeyMappings structure
✅ Uses the same pattern as other string fields like model, user_id, etc.

Test Coverage (targetUrl.test.ts)

✅ Tests all relevant operators for text fields
✅ Includes comprehensive edge cases (233 lines of tests)
✅ Verifies the fix prevents the original error
✅ Tests URL formats from major LLM providers
✅ Handles special characters and unicode properly

Type Definitions

✅ target_url is already properly typed in filterDefs.ts:202
✅ Uses SingleKey<TextOperators> which supports all tested operators

🔍 Potential Concerns

Minor Observations:

No Database Schema Verification: The code assumes request_response_rmt.target_url exists in the database, but this isn't verified in the PR
Test Dependencies: Tests couldn't be executed due to missing ts-node dependency in the CI environment, though this appears to be an environment issue rather than a code issue

🛡️ Security & Performance

✅ Security: No concerns - follows existing patterns for parameterized queries
✅ Performance: Minimal impact - just adds one more mappable field to existing system
✅ SQL Injection: Properly handled through existing placeValueSafely mechanism

📊 Confidence Score: 9/10

Rationale:

✅ Simple, focused change that follows established patterns
✅ Comprehensive test coverage
✅ Type-safe implementation
✅ No breaking changes or security concerns
⚠️ Minor uncertainty about database schema (not visible in this PR)

This is a well-implemented, low-risk change that enables an important filtering capability. The extensive test coverage demonstrates thorough consideration of edge cases and real-world usage scenarios.

greptile-apps

Greptile Summary

This PR addresses a missing filtering capability by adding support for the target_url field in the ClickHouse query filtering system. The change adds a single line mapping target_url: "request_response_rmt.target_url" to the existing key mappings in packages/filters/filters.ts. This enables users to filter requests based on the target URL field, which already exists in the request_response_rmt table schema but was previously inaccessible through the filter system.

The implementation follows the exact same pattern as all other field mappings in the request_response_rmt table, integrating seamlessly with the existing filtering infrastructure. The PR includes comprehensive test coverage in packages/__tests__/filters/targetUrl.test.ts that validates all supported filter operators (equals, like, ilike, contains, not-equals) and thoroughly tests edge cases including various URL formats, special characters, unicode handling, null values, and integration with existing argument arrays.

This fix resolves issue ENG-2960 and fills a gap in the filtering capabilities where users couldn't filter by target URL despite the field being available in the database schema. The change is minimal, follows established patterns, and maintains backward compatibility while extending functionality.

Confidence score: 5/5

This PR is safe to merge with minimal risk as it only adds a missing field mapping following established patterns
Score reflects simple, well-tested changes that extend existing functionality without breaking existing code
No files require special attention as both the implementation and tests are straightforward

_{2 files reviewed, 2 comments}

_{Edit Code Review Bot Settings | Greptile}

greptile-apps · 2025-09-11T21:54:21Z

packages/__tests__/filters/targetUrl.test.ts

+    test('should work with target_url alongside other basic fields', () => {
+      // Test that target_url doesn't break when used with a simple status filter
+      const simpleFilter: FilterLeaf = {
+        request_response_rmt: {
+          target_url: { equals: 'https://api.openai.com/v1/chat/completions' }
+        }
+      };
+
+      const result = buildFilterClickHouse({ filter: simpleFilter, argsAcc: [] });
+
+      // Should successfully build target_url filter
+      expect(result.argsAcc).toEqual(['https://api.openai.com/v1/chat/completions']);
+      expect(result.filter).toBe('request_response_rmt.target_url = {val_0 : String}');
+    });


style: This test is redundant with the basic equals test on lines 10-21 and doesn't add new validation

greptile-apps · 2025-09-11T21:54:22Z

packages/__tests__/filters/targetUrl.test.ts

+      expect(() => {
+        const result = buildFilterClickHouse({ filter, argsAcc: [] });
+        expect(result.filter).toContain("request_response_rmt.target_url ILIKE '%' || {val_0 : String}::text || '%'");
+        expect(result.argsAcc).toEqual(['anthropic']);
+      }).not.toThrow();


style: Consider using toThrow() without the inner expectation, as the inner expects may not run if an error is thrown earlier

Suggested change

expect(() => {

const result = buildFilterClickHouse({ filter, argsAcc: [] });

expect(result.filter).toContain("request_response_rmt.target_url ILIKE '%' || {val_0 : String}::text || '%'");

expect(result.argsAcc).toEqual(['anthropic']);

}).not.toThrow();

// This should not throw an error now that target_url mapping is fixed

expect(() => {

buildFilterClickHouse({ filter, argsAcc: [] });

}).not.toThrow();

// Verify the actual result separately

const result = buildFilterClickHouse({ filter, argsAcc: [] });

expect(result.filter).toContain("request_response_rmt.target_url ILIKE '%' || {val_0 : String}::text || '%'");

expect(result.argsAcc).toEqual(['anthropic']);

devinat1 added 2 commits September 11, 2025 14:47

fix: add target_url mapping to request_response_rmt in filters

8fb66c1

test: target url filter

7e77f46

greptile-apps bot reviewed Sep 11, 2025

View reviewed changes

vercel bot deployed to Preview – helicone-eu September 11, 2025 22:11 View deployment

vercel bot deployed to Preview – helicone September 11, 2025 22:14 View deployment

vercel bot deployed to Preview – helicone-bifrost September 11, 2025 22:20 View deployment

tomharmon approved these changes Sep 12, 2025

View reviewed changes

devinat1 merged commit ad56701 into main Sep 12, 2025
13 of 14 checks passed

devinat1 deleted the devin/eng-2960-fix-target-filter branch September 12, 2025 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: target URL filter#4735

fix: target URL filter#4735
devinat1 merged 2 commits intomainfrom
devin/eng-2960-fix-target-filter

devinat1 commented Sep 11, 2025

Uh oh!

vercel bot commented Sep 11, 2025 •

edited

Loading

Uh oh!

claude bot commented Sep 11, 2025 •

edited

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Sep 11, 2025

Uh oh!

greptile-apps bot Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

devinat1 commented Sep 11, 2025

Uh oh!

vercel bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete

Code Review Summary

✅ Strengths

📋 Code Quality Assessment

🔍 Potential Concerns

🛡️ Security & Performance

📊 Confidence Score: 9/10

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Summary

Confidence score: 5/5

Uh oh!

greptile-apps bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel bot commented Sep 11, 2025 •

edited

Loading

claude bot commented Sep 11, 2025 •

edited

Loading