refactor: decouple zustand from filter store

[devinat1]

This pull request introduces unit tests for the filterStore logic, refactors the store to support both Zustand and standalone usage.

• Refactored filterStore.ts to extract store logic into a reusable buildFilterStore function, allowing both Zustand and standalone in-memory store creation via createFilterStore. This enables easier testing and future extensibility. [1] [2]

• Improved type safety and code clarity by adding explicit types to store actions and internal variables, and cleaning up navigation logic for filter expression manipulation. [1] [2] [3] [4] [5] [6]

• Introduced unit tests for the filter store in web/filterAST/store/__tests__/filterStore.test.ts, covering initialization, filter manipulation, and state management scenarios.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
helicone	Ready	Preview	Comment	Sep 7, 2025 8:01pm
helicone-eu	Ready	Preview	Comment	Sep 7, 2025 8:01pm

1 Skipped Deployment

Project	Deployment	Preview	Comments	Updated (UTC)
helicone-bifrost	Skipped			Sep 7, 2025 8:01pm

[claude]

Claude finished @devinat1's task —— View job

---

PR Review: Decouple Zustand from Filter Store

Todo List:

• Read all changed files to understand the refactor
• Analyze code quality and architecture changes
• Check for potential bugs and type safety issues
• Review performance considerations
• Assess security concerns
• Provide structured feedback with confidence score

Summary

This PR successfully refactors the filter store to decouple it from Zustand, making it testable and more modular. However, there is one critical type safety issue that should be addressed before merging.

⚠️ Critical Issues

Type Safety Violation in removeFilterExpression (Line 259)

(get() as any).removeFilterExpression(parentPath);

This dangerous type assertion bypasses TypeScript's type checking. The recursive call could lead to runtime errors if the state structure changes.

Fix:

// Extract the actions from buildFilterStore
const actions = buildFilterStore(set, get);
actions.removeFilterExpression(parentPath);

🐛 Minor Issues

Test File Issues

1. Duplicate getState declaration (lines 6-7 in test file):

   const getState = () => useFilterStore.getState();
   const getState = () => useFilterStore.getState(); // Duplicate

2. Jest config includes .jsx but excludes it from test pattern - Consider consistency.

3. Test coverage gaps: Missing tests for edge cases like deeply nested expressions and error conditions.

✅ Positive Aspects

Excellent Architecture Improvements

1. Clean Dependency Injection: The buildFilterStore function properly abstracts the store logic from Zustand specifics.

2. Improved Testability: createFilterStore enables isolated testing without React/Zustand dependencies.

3. Backward Compatibility: Existing useFilterStore usage remains unchanged.

4. Type Safety Improvements: Better typing for store actions and internal variables (except the one critical issue).

5. Comprehensive Test Suite: Good coverage of core functionality including initialization, CRUD operations, and state management.

🚀 Performance Considerations

Performance Analysis

• Deep cloning: Using JSON.parse(JSON.stringify()) for filter mutations is acceptable for typical filter sizes but could be optimized with a proper immutable update library for very large filter trees.
• Memory usage: The new standalone store implementation is lightweight and doesn't introduce memory leaks.
• Bundle size: No significant impact on bundle size.

🔒 Security Assessment

No security concerns identified. The refactor maintains the same data flow and doesn't introduce any new security vectors.

Recommendations

1. MUST FIX: Replace the (get() as any) type assertion with a proper solution
2. SHOULD FIX: Remove duplicate getState declaration in tests
3. CONSIDER: Add tests for deeply nested filter expressions and error conditions
4. CONSIDER: Document the new factory pattern in comments for future maintainers

Confidence Score: 7/10

✅ Merge with fixes

The architecture improvements are excellent and the refactor is well-executed. The critical type safety issue must be fixed, but once addressed, this PR significantly improves the codebase's testability and maintainability.

---

[greptile-apps]

Greptile Summary

This pull request refactors the filter store architecture to improve testability by decoupling it from Zustand's state management. The main change extracts the core store logic into a reusable buildFilterStore function that accepts generic set and get functions, enabling the same logic to work with different store implementations.

The refactor introduces a new createFilterStore function that provides a standalone, in-memory store implementation with subscribe/unsubscribe capabilities. This allows the filter store logic to be tested in isolation without requiring React or Zustand dependencies. The existing useFilterStore is updated to use the new buildFilterStore function while maintaining backward compatibility and the same Zustand persistence configuration.

Additionally, the PR adds comprehensive unit tests covering initialization, filter manipulation (adding, removing, updating expressions), state management scenarios, and edge cases. The testing infrastructure is supported by new Jest configuration and dependencies added to package.json.

The filter store manages complex filter expressions used throughout the Helicone application for data filtering. This refactor follows dependency injection patterns, making the codebase more modular and maintainable while enabling better test coverage for the critical filtering functionality.

Confidence score: 3/5

• This PR requires careful review due to a potential type safety issue in the recursive function call
• Score lowered due to unsafe type assertion (get() as any) in removeFilterExpression that could cause runtime errors
• Pay close attention to the type assertion in web/filterAST/store/filterStore.ts at line 259

_{4 files reviewed, 3 comments}

_{Edit Code Review Bot Settings | Greptile}

[greptile-apps]

style: Test pattern excludes .jsx files but includes them in moduleFileExtensions - consider adding **/__tests__/**/*.test.jsx if JSX tests are needed

[greptile-apps]

logic: Using (get() as any) bypasses TypeScript's type checking and could cause runtime errors. The recursive call should be typed properly or restructured to avoid the type assertion.

Suggested change

	(get() as any).removeFilterExpression(parentPath);
	removeFilterExpression(parentPath);

[chitalian]

peak "⚠️ Critical Issues" in the claude review. I think it's valid