refactor: clean code improvements for HQL by devinat1 · Pull Request #4686 · Helicone/helicone

devinat1 · 2025-09-07T20:10:57Z

Add a new config file for default SQL queries and editor options.
Create a QueryEditor component for SQL input with validation.
Introduce TableList component to display and manage database tables.
Refactor HQLPage to integrate new components and improve query handling.
Implement hooks for query execution and state management.
Enhance SQL validation to restrict write operations.
Add loading and row limit warning states for better user experience.

- Add a new config file for default SQL queries and editor options. - Create a QueryEditor component for SQL input with validation. - Introduce TableList component to display and manage database tables. - Refactor HQLPage to integrate new components and improve query handling. - Implement hooks for query execution and state management. - Enhance SQL validation to restrict write operations. - Add loading and row limit warning states for better user experience.

vercel · 2025-09-07T20:11:04Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
helicone	Error			Sep 7, 2025 8:16pm
helicone-eu	Error			Sep 7, 2025 8:16pm

1 Skipped Deployment

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

claude · 2025-09-07T20:11:19Z

Claude finished @devinat1's task —— View job

HQL Code Improvements Review

Todo List

Code Review Summary

This PR introduces significant improvements to the HQL (Helicone Query Language) interface with better componentization, type safety, and user experience. The refactoring follows React best practices with proper separation of concerns.

✅ Strengths

Excellent Component Architecture: Clean separation between QueryEditor, TableList, and state management hooks
Strong Type Safety: Comprehensive TypeScript interfaces in types.ts
Security-First Approach: SQL validation prevents write operations with clear error messaging
Performance Optimization: Proper use of React.memo, useCallback, and useMemo
User Experience: Row limit warnings, loading states, and keyboard shortcuts (Ctrl+Enter, Ctrl+S)
Code Organization: Well-structured hooks for different concerns (state, execution, Monaco setup)

🔍 Detailed Analysis & Issues Found

Potential Bugs & Issues

1. SQL Validation Timing Issue (QueryEditor.tsx:34-38, 62-67)

validateSqlQuery(sql, model, monacoInstance); // Uses stale 'sql' prop
editor.onDidChangeModelContent(() => {
  validateSqlQuery(sql, model, monacoInstance); // Same issue
});

Problem: The validation uses the sql prop instead of the current editor content, causing validation to be out of sync.
Fix: Get current value from editor: validateSqlQuery(editor.getValue(), model, monacoInstance)

2. Type Safety Issues

useAgentHandlers.ts:6 - clickhouseSchemas: any should use proper typing
constants.ts:120 - Complex nested type with potential undefined access

3. Memory Leak Risk (useMonacoSetup.ts:29)

Monaco completion provider registered but cleanup only happens in useEffect return, not on component unmount.

Security Concerns

1. SQL Injection Protection ✅

Good: Regex-based validation prevents write operations
Good: Server-side validation should complement client-side checks

2. XSS Prevention

Table/column names from API displayed without sanitization (TableList.tsx:45, 55)
Consider validating/sanitizing database metadata

Performance Considerations

1. Monaco Editor Optimization ✅

Good: Proper cleanup of completion providers
Good: Debounced validation on content changes

2. React Query Caching ✅

Efficient cache invalidation after mutations
Proper loading states

3. Minor Optimization Opportunities

constants.ts:83-100: SQL parsing regex executed on every keystroke
Consider memoizing parsed table aliases

Code Quality Issues

1. Consistency

TableList.tsx exists as both standalone component and inline in directory.tsx
Consider removing duplication

2. Error Handling

// constants.ts:120-138 - Complex nested error handling
if (data.error || !data.data?.data) {
  setQueryError(data.error?.error || "Query execution failed");
}

Could be simplified with better error type definitions.

3. Constants Organization

config.ts and constants.ts have overlapping concerns
Consider consolidating configuration

Recommendations

High Priority: Fix SQL validation timing issue in QueryEditor
Medium Priority: Improve type safety by replacing any types
Medium Priority: Add input sanitization for database metadata
Low Priority: Consolidate duplicate TableList implementations
Low Priority: Consider memoizing SQL parsing operations

Testing Considerations

Add tests for SQL validation edge cases
Test keyboard shortcuts functionality
Verify error handling paths in query execution
Test bulk delete operations

Confidence Score: 7/10

Reasoning: This is a well-architected refactor with significant improvements to code organization and user experience. The main concerns are a few potential bugs (especially the SQL validation timing issue) and some type safety improvements needed. The security approach is sound, and the performance optimizations are appropriate. With the identified fixes, this would be a solid 8-9/10.

Recommendation: Merge this PR after addressing the SQL validation timing issue and type safety concerns. The other improvements can be addressed in follow-up PRs.

greptile-apps

Greptile Summary

This PR refactors the HQL (Helicone Query Language) feature based on dogfooding feedback, breaking down a monolithic component into a well-structured, modular architecture. The changes extract complex logic from a large 280+ line HQLPage component into dedicated custom hooks and separate components for better maintainability and reusability.

Key architectural improvements include:

Custom Hooks: Created useQueryState for query management, useQueryExecution for SQL execution, useMonacoSetup for editor configuration, and useAgentHandlers for AI agent integration
Component Separation: Extracted a standalone QueryEditor component with Monaco editor integration and a TableList component for database schema browsing
Type Safety: Introduced a centralized types.ts file with proper TypeScript interfaces, replacing inline types and unsafe any usage throughout
Configuration Management: Added config.ts to centralize constants, default values, and editor options
Security Enhancements: Implemented SQL validation utilities to prevent write operations, restricting users to read-only SELECT queries

The refactoring maintains existing functionality while significantly improving code organization. The HQL feature appears to be a SQL query interface similar to tools like DataGrip or pgAdmin, integrated into the Helicone platform for querying ClickHouse databases. The modular architecture makes components more testable and allows for better separation of concerns between query state management, editor functionality, and execution logic.

Confidence score: 2/5

This PR requires careful review due to potential breaking changes and several implementation issues that could cause runtime problems
Score lowered due to critical logic bugs in validation callbacks, component duplication concerns, and loose typing with any usage in multiple files
Pay close attention to QueryEditor.tsx (validation bug), TableList.tsx (potential duplication), and useAgentHandlers.ts (error handling issues)

_{11 files reviewed, 8 comments}

_{Edit Code Review Bot Settings | Greptile}

greptile-apps · 2025-09-07T20:11:40Z

web/components/templates/hql/hooks/useQueryState.ts

+  useEffect(() => {
+    if (savedQueryDetails?.data) {
+      setCurrentQuery({
+        id: savedQueryDetails.data.id,
+        name: savedQueryDetails.data.name,
+        sql: savedQueryDetails.data.sql,
+      });
+    }
+  }, [savedQueryDetails]);


logic: This effect could cause an infinite re-render loop. When savedQueryDetails changes, it updates currentQuery, which triggers the query to refetch (line 36), potentially causing the effect to run again.

greptile-apps · 2025-09-07T20:12:10Z

web/components/templates/hql/hooks/useQueryExecution.ts

+export interface QueryExecutionState {
+  result: components["schemas"]["ExecuteSqlResponse"];
+  loading: boolean;
+  error: string | null;
+}


style: The QueryExecutionState interface is defined but never used. Consider removing it or using it to type the hook's return value.

greptile-apps · 2025-09-07T20:12:11Z

web/components/templates/hql/hooks/useQueryExecution.ts

+  const [queryError, setQueryError] = useState<string | null>(null);
+
+  const { mutate: handleExecuteQuery, mutateAsync: handleExecuteQueryAsync } =
+    useMutation<any, Error, string>(


style: Using any as the mutation result type is not type-safe. Consider using a more specific type based on the API response structure.

greptile-apps · 2025-09-07T20:12:39Z

web/components/templates/hql/QueryEditor.tsx

+      editor.onDidChangeModelContent(() => {
+        validateSqlQuery(sql, model, monacoInstance);
+      });


logic: Logic error: validating with stale sql prop instead of current editor content. Should use editor.getValue() or the model's content.

Suggested change

editor.onDidChangeModelContent(() => {

validateSqlQuery(sql, model, monacoInstance);

});

editor.onDidChangeModelContent(() => {

validateSqlQuery(editor.getValue(), model, monacoInstance);

});

greptile-apps · 2025-09-07T20:12:40Z

web/components/templates/hql/QueryEditor.tsx

+      if (value && editorRef.current) {
+        const model = editorRef.current.getModel();
+        if (model && typeof window !== 'undefined' && (window as any).monaco) {
+          validateSqlQuery(value, model, (window as any).monaco);
+        }
+      }


logic: Validation inconsistency: using different Monaco instances (global vs parameter) and different content sources creates potential validation gaps.

greptile-apps · 2025-09-07T20:12:41Z

web/components/templates/hql/QueryEditor.tsx

+  onSave,
+  editorRef,
+  latestQueryRef,
+  currentQuery,


style: The currentQuery prop is passed but never used in the component implementation.

greptile-apps · 2025-09-07T20:12:56Z

web/components/templates/hql/utils/sqlValidation.ts

+        startLineNumber: 1,
+        startColumn: 1,
+        endLineNumber: 1,
+        endColumn: 1,


style: Error marker spans only column 1-1 on line 1, which may not clearly indicate where the problematic SQL keyword is located in the query

greptile-apps · 2025-09-07T20:12:57Z

web/components/templates/hql/utils/sqlValidation.ts

+export function isReadOnlyQuery(sql: string): boolean {
+  return !SQL_WRITE_OPERATIONS_REGEX.test(sql);


style: Both validation functions use the same regex - consider extracting the validation logic into a single internal function to avoid duplication

devinat1 · 2025-09-07T20:34:19Z

Closing since I made the PR from the wrong branch

devinat1 changed the title ~~feat: code improvements upon dogfooding HQL~~ feat: clean code improvements for HQL Sep 7, 2025

devinat1 changed the title ~~feat: clean code improvements for HQL~~ refactor: clean code improvements for HQL Sep 7, 2025

vercel bot had a problem deploying to Preview – helicone-eu September 7, 2025 20:12 Failure

greptile-apps bot reviewed Sep 7, 2025

View reviewed changes

vercel bot had a problem deploying to Preview – helicone September 7, 2025 20:16 Failure

devinat1 closed this Sep 7, 2025

		export function isReadOnlyQuery(sql: string): boolean {
		return !SQL_WRITE_OPERATIONS_REGEX.test(sql);

Conversation

devinat1 commented Sep 7, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

HQL Code Improvements Review

Todo List

Code Review Summary

✅ Strengths

Potential Bugs & Issues

1. SQL Validation Timing Issue (QueryEditor.tsx:34-38, 62-67)

2. Type Safety Issues

3. Memory Leak Risk (useMonacoSetup.ts:29)

Security Concerns

1. SQL Injection Protection ✅

2. XSS Prevention

Performance Considerations

1. Monaco Editor Optimization ✅

2. React Query Caching ✅

3. Minor Optimization Opportunities

Code Quality Issues

1. Consistency

2. Error Handling

3. Constants Organization

Recommendations

Testing Considerations

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Summary

Confidence score: 2/5

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

devinat1 commented Sep 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel bot commented Sep 7, 2025 •

edited

Loading

claude bot commented Sep 7, 2025 •

edited

Loading