Skip to content

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Oct 15, 2025

video QA

2025-10-16.18-17-23.mp4

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR implements min/max range filtering functionality for secondary axis bar charts in dashboards. The changes span across frontend UI components, GraphQL resolvers, and backend services to enable users to set custom minimum and maximum values for chart axes.

Key changes include:

  • Adding new chart configuration settings (RANGE_MIN and RANGE_MAX) with corresponding UI components
  • Extending GraphQL group-by resolvers to accept and process range parameters
  • Creating a new CommandMenuItemNumberInput component for numeric input in chart settings
  • Adding comprehensive integration tests to validate range filtering behavior
  • Updating chart configuration utilities to handle range values throughout the data flow

The implementation follows established patterns in the codebase for chart configuration settings, with proper type definitions, internationalization support using lingui, and consistent naming conventions. The feature integrates seamlessly with existing chart settings by extending the configuration system rather than replacing it.

Important Files Changed

Changed Files
Filename Score Overview
packages/twenty-front/src/modules/command-menu/components/CommandMenuItemNumberInput.tsx 1/5 New number input component with critical validation logic error
packages/twenty-server/src/engine/api/graphql/graphql-query-runner/group-by/resolvers/graphql-query-group-by-resolver.service.ts 3/5 Adds range filtering to group-by resolver with potential type safety issues
packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartSettings.tsx 4/5 Integrates number input component into chart settings UI
packages/twenty-front/src/modules/page-layout/widgets/graph/utils/generateGroupByQueryVariablesFromBarChartConfiguration.ts 4/5 Adds range parameters to GraphQL query variable generation
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/LineChartSettings.ts 4/5 Extends line charts with range settings beyond PR scope
packages/twenty-front/src/modules/command-menu/pages/page-layout/hooks/useChartSettingsValues.ts 4/5 Handles range value extraction and string conversion for UI
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/getBarChartSettings.ts 4/5 Adds range settings to secondary axis configuration
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/settings/RangeMaxSetting.ts 5/5 Clean implementation of max range setting configuration
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/settings/RangeMinSetting.ts 5/5 Clean implementation of min range setting configuration
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/settings/ChartConfigurationSettingLabels.ts 5/5 Adds translatable labels for range settings
packages/twenty-front/src/modules/command-menu/pages/page-layout/types/ChartConfigurationSettingIds.ts 5/5 Adds new enum values and mappings for range settings
packages/twenty-front/src/modules/command-menu/pages/page-layout/types/ChartSettingsGroup.ts 5/5 Adds optional isInput field for input-based settings
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/tests/getBarChartSettings.test.ts 5/5 Updates tests to verify range settings integration
packages/twenty-front/src/modules/page-layout/widgets/graph/utils/generateGroupByQuery.ts 5/5 Adds range parameters to GraphQL query template
packages/twenty-server/src/engine/api/graphql/workspace-resolver-builder/interfaces/workspace-resolvers-builder.interface.ts 5/5 Extends interface with optional range parameters
packages/twenty-server/src/engine/api/graphql/workspace-schema-builder/utils/get-resolver-args.util.ts 5/5 Adds GraphQL arguments for range parameters
packages/twenty-server/test/integration/graphql/suites/group-by-resolvers.integration-spec.ts 5/5 Comprehensive integration tests for range functionality
packages/twenty-server/test/integration/graphql/utils/group-by-operation-factory.util.ts 5/5 Extends test utility to support range parameters
packages/twenty-ui/src/display/icon/components/TablerIcons.ts 5/5 Adds math min/max icons for range controls
packages/twenty-ui/src/display/index.ts 5/5 Exports new math icons for UI components

Confidence score: 2/5

  • This PR contains critical bugs that will cause functionality to fail, particularly in the number input validation logic
  • Score significantly lowered due to a logical error in CommandMenuItemNumberInput.tsx that prevents valid numeric input from being accepted, and potential type safety issues in the backend resolver that could cause runtime errors with non-numeric aggregate fields
  • Pay close attention to CommandMenuItemNumberInput.tsx which has inverted validation logic, and the group-by resolver service which applies numeric range filters to all aggregate types without validation

Sequence Diagram

sequenceDiagram
    participant User
    participant ChartSettings as "Chart Settings UI"
    participant CommandMenuItemNumberInput as "Number Input Component"
    participant useUpdateCurrentWidgetConfig as "Config Update Hook"
    participant GraphQLQuery as "GraphQL Group By Query"
    participant GraphqlQueryGroupByResolverService as "Backend Resolver"
    participant Database as "Database"

    User->>ChartSettings: "Opens chart configuration panel"
    ChartSettings->>ChartSettings: "Renders range min/max input fields"
    
    User->>CommandMenuItemNumberInput: "Enters minimum value"
    CommandMenuItemNumberInput->>CommandMenuItemNumberInput: "Validates numeric input on blur"
    CommandMenuItemNumberInput->>useUpdateCurrentWidgetConfig: "Calls handleInputChange with rangeMin value"
    useUpdateCurrentWidgetConfig->>useUpdateCurrentWidgetConfig: "Updates chart configuration with rangeMin"
    
    User->>CommandMenuItemNumberInput: "Enters maximum value" 
    CommandMenuItemNumberInput->>CommandMenuItemNumberInput: "Validates numeric input on blur"
    CommandMenuItemNumberInput->>useUpdateCurrentWidgetConfig: "Calls handleInputChange with rangeMax value"
    useUpdateCurrentWidgetConfig->>useUpdateCurrentWidgetConfig: "Updates chart configuration with rangeMax"
    
    ChartSettings->>GraphQLQuery: "Generates query with rangeMin/rangeMax variables"
    GraphQLQuery->>GraphqlQueryGroupByResolverService: "Executes group by query with range parameters"
    
    GraphqlQueryGroupByResolverService->>GraphqlQueryGroupByResolverService: "Applies range filters using HAVING clauses"
    Note over GraphqlQueryGroupByResolverService: "Filters aggregated results where value >= rangeMin AND value <= rangeMax"
    
    GraphqlQueryGroupByResolverService->>Database: "Executes filtered SQL query"
    Database->>GraphqlQueryGroupByResolverService: "Returns filtered aggregated data"
    GraphqlQueryGroupByResolverService->>GraphQLQuery: "Returns formatted results within range"
    GraphQLQuery->>ChartSettings: "Updates chart with filtered data"
    ChartSettings->>User: "Displays chart with only values in specified range"
Loading

Context used (6)

  • Rule from dashboard - Use lingui for all user-facing strings to make them translatable, including field type labels and UI... (source)
  • Context from dashboard - Always consider adding tests for new functionality, especially for edge cases like empty responses. (source)
  • Context from dashboard - Ensure that type assertions are safe and consider using type guards instead of direct assertions. (source)
  • Context from dashboard - Use the 'isNonEmptyString' utility function to check for non-empty strings instead of checking for e... (source)
  • Context from dashboard - Use validation libraries like 'zod' or 'class-validator' for input validation instead of manual chec... (source)
  • Context from dashboard - Always clear mocks before each test to prevent state leakage between tests, as Jest does not automat... (source)

20 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

github-actions bot commented Oct 15, 2025

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:40315

This environment will automatically shut down when the PR is closed or after 5 hours.

Base automatically changed from omit-zero-values to main October 16, 2025 09:30
@ehconitin
Copy link
Contributor Author

@greptileai trigger

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR implements min/max range controls for secondary axis bar charts in the dashboard system. The implementation introduces new chart configuration settings (RANGE_MIN and RANGE_MAX) that allow users to set custom bounds on bar chart axes. The changes span across the frontend chart configuration system, adding new settings constants, UI components, and GraphQL integration, as well as backend support with updated resolvers and query handling.

The frontend changes include new chart setting constants (RangeMinSetting.ts, RangeMaxSetting.ts), a number input component (CommandMenuItemNumberInput.tsx), and integration of range settings into bar chart and line chart configurations. The backend modifications extend GraphQL group-by resolvers to accept and process rangeMin and rangeMax parameters with appropriate filtering logic. The feature maintains backward compatibility through optional parameters and follows the existing patterns established in the codebase.

Important Files Changed

Changed Files
Filename Score Overview
packages/twenty-front/src/modules/command-menu/components/CommandMenuItemNumberInput.tsx 2/5 New number input component with critical logic error in validation
packages/twenty-server/src/engine/api/graphql/graphql-query-runner/group-by/resolvers/graphql-query-group-by-resolver.service.ts 3/5 Backend range filtering with potential SQL error risk for non-numeric aggregates
packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartSettings.tsx 4/5 Chart settings UI with range validation but lacking user feedback
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/getBarChartSettings.ts 4/5 Integration of range settings into bar chart configuration
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/settings/RangeMaxSetting.ts 5/5 New range maximum setting configuration constant
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/settings/RangeMinSetting.ts 5/5 New range minimum setting configuration constant
packages/twenty-front/src/modules/command-menu/pages/page-layout/types/ChartConfigurationSettingIds.ts 5/5 Added RANGE_MIN and RANGE_MAX setting identifiers
packages/twenty-front/src/modules/command-menu/pages/page-layout/types/ChartSettingsGroup.ts 5/5 Added optional isInput property to ChartSettingsItem type
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/LineChartSettings.ts 5/5 Added range settings to line chart Y axis configuration
packages/twenty-front/src/modules/page-layout/widgets/graph/utils/generateGroupByQueryVariablesFromBarChartConfiguration.ts 5/5 Added range parameters to GraphQL query variables generation
packages/twenty-front/src/modules/page-layout/widgets/graph/utils/generateGroupByQuery.ts 5/5 Extended GraphQL query schema with range parameters
packages/twenty-ui/src/display/icon/components/TablerIcons.ts 5/5 Added IconMathMax and IconMathMin icon exports
packages/twenty-front/src/modules/command-menu/pages/page-layout/hooks/useChartSettingsValues.ts 5/5 Extended hook to handle RANGE_MIN and RANGE_MAX settings
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/tests/getBarChartSettings.test.ts 5/5 Updated tests to include new range settings in expectations
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/settings/ChartConfigurationSettingLabels.ts 5/5 Added translatable labels for range settings
packages/twenty-ui/src/display/index.ts 5/5 Auto-generated icon exports for new math icons
packages/twenty-server/test/integration/graphql/suites/group-by-resolvers.integration-spec.ts 5/5 Comprehensive integration tests for range filtering
packages/twenty-server/src/engine/api/graphql/workspace-resolver-builder/interfaces/workspace-resolvers-builder.interface.ts 5/5 Added range parameters to GroupByResolverArgs interface
packages/twenty-server/src/engine/api/graphql/workspace-schema-builder/utils/get-resolver-args.util.ts 5/5 Added rangeMin and rangeMax GraphQL arguments
packages/twenty-server/test/integration/graphql/utils/group-by-operation-factory.util.ts 5/5 Updated test utility to support range parameters

Confidence score: 3/5

  • This PR requires careful review due to critical logic issues in key components that could cause runtime errors
  • Score lowered due to a logic error in CommandMenuItemNumberInput that prevents valid numbers from being processed, and backend range filtering that could cause SQL errors when applied to non-numeric aggregates
  • Pay close attention to CommandMenuItemNumberInput.tsx and the GraphQL group-by resolver service for the logic issues mentioned in previous reviews

Sequence Diagram

sequenceDiagram
    participant User
    participant ChartSettings
    participant CommandMenuItemNumberInput
    participant useChartSettingsValues
    participant useUpdateCurrentWidgetConfig
    participant GraphQL
    participant Database

    User->>ChartSettings: "Opens chart settings panel"
    ChartSettings->>useChartSettingsValues: "getChartSettingsValues(RANGE_MIN/RANGE_MAX)"
    useChartSettingsValues-->>ChartSettings: "Returns current range values"
    
    ChartSettings->>CommandMenuItemNumberInput: "Renders min/max input fields"
    CommandMenuItemNumberInput-->>ChartSettings: "Displays current values with 'Auto' placeholder"
    
    User->>CommandMenuItemNumberInput: "Enters minimum range value"
    CommandMenuItemNumberInput->>CommandMenuItemNumberInput: "canBeCastAsNumberOrNull(text)"
    CommandMenuItemNumberInput->>CommandMenuItemNumberInput: "castAsNumberOrNull(text)"
    CommandMenuItemNumberInput->>ChartSettings: "handleInputChange(value)"
    
    ChartSettings->>ChartSettings: "Validates min <= max constraint"
    alt Valid range
        ChartSettings->>useUpdateCurrentWidgetConfig: "updateCurrentWidgetConfig({rangeMin: value})"
        useUpdateCurrentWidgetConfig->>GraphQL: "Updates widget configuration"
        GraphQL->>Database: "Stores updated configuration"
    else Invalid range (min > max)
        ChartSettings->>ChartSettings: "Early return - no update"
    end
    
    User->>CommandMenuItemNumberInput: "Enters maximum range value"
    CommandMenuItemNumberInput->>CommandMenuItemNumberInput: "canBeCastAsNumberOrNull(text)"
    CommandMenuItemNumberInput->>CommandMenuItemNumberInput: "castAsNumberOrNull(text)"
    CommandMenuItemNumberInput->>ChartSettings: "handleInputChange(value)"
    
    ChartSettings->>ChartSettings: "Validates max >= min constraint"
    alt Valid range
        ChartSettings->>useUpdateCurrentWidgetConfig: "updateCurrentWidgetConfig({rangeMax: value})"
        useUpdateCurrentWidgetConfig->>GraphQL: "Updates widget configuration"
        GraphQL->>Database: "Stores updated configuration"
    else Invalid range (max < min)
        ChartSettings->>ChartSettings: "Early return - no update"
    end
    
    Note over ChartSettings,Database: Chart re-renders with filtered data based on range constraints
    
    GraphQL->>GraphQL: "generateGroupByQueryVariablesFromBarChartConfiguration"
    GraphQL->>Database: "Query with rangeMin/rangeMax parameters"
    Database->>GraphQL: "Returns filtered aggregate data"
    GraphQL-->>User: "Updated chart with range-filtered results"
Loading

20 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add rangeMin, rangeMax or omitNull in the backend

If we do this, we should implement a proper "having" API (that would specify on which aggregate it applies). This is not a must + a bit expensive

Let's do these checks on FE side, there is no pagination issue at this point it should be fine for 99% of our cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants