Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR getsentry#102792

Type: Corrupted (contains bugs)

Original PR Title: feat(dashboards): Use a single value selector for boolean filters
Original PR Description: Boolean filters previously used the multi select. If a list of booleans (e.g. [true,false]) is used in a widget query, an error is thrown by the backend because booleans are expected to be single values. To avoid this error completely, this PR uses a single value compact select for booleans (and any other filter token that doesn't support multi select).
Original PR URL: getsentry#102792


PR Type

Enhancement


Description

  • Use single value selector for boolean filters instead of multi-select

  • Export tokenSupportsMultipleValues function for reuse in filter selector

  • Add logic to detect filters that don't support multiple values

  • Implement conditional rendering: CompactSelect for single-value filters, HybridFilter for multi-value

  • Extract menu header and trigger rendering into separate functions

  • Adjust menu width and improve filter UI consistency


Diagram Walkthrough

flowchart LR
  A["Filter Token"] --> B{"Supports Multiple Values?"}
  B -->|Yes| C["HybridFilter<br/>Multi-select"]
  B -->|No| D["CompactSelect<br/>Single-value"]
  C --> E["Dashboard Global Filter"]
  D --> E
Loading

File Walkthrough

Relevant files
Enhancement
valueCombobox.tsx
Export tokenSupportsMultipleValues function                           

static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx

  • Export tokenSupportsMultipleValues function to make it available for
    external use
  • Function determines if a filter token supports multiple values based
    on field definition
+1/-1     
addFilter.tsx
Add default value handling for boolean filters                     

static/app/views/dashboards/globalFilter/addFilter.tsx

  • Add APPLY_DEFAULT_VALUES constant to specify which field value types
    should have default values applied
  • Update condition to check if valueType is in APPLY_DEFAULT_VALUES
    (STRING and BOOLEAN) instead of just checking if it's not STRING
  • Ensures boolean filters get proper default values on initialization
+2/-1     
filterSelector.tsx
Implement conditional single/multi-value filter selectors

static/app/views/dashboards/globalFilter/filterSelector.tsx

  • Import CompactSelect component and tokenSupportsMultipleValues
    function
  • Add canSelectMultipleValues state to determine which selector
    component to use
  • Implement conditional rendering: use CompactSelect for single-value
    filters, HybridFilter for multi-value filters
  • Extract renderMenuHeaderTrailingItems and renderFilterSelectorTrigger
    into separate functions for code reuse
  • Update menu width from 400 to 500 pixels for better visibility
  • Add canSelectMultipleValues to options dependency array
+79/-36 

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new UI actions (adding/removing filters, changing values) introduce critical state
changes without any visible audit logging in the new code.

Referred Code
if (!canSelectMultipleValues) {
  return (
    <CompactSelect
      multiple={false}
      disabled={false}
      options={options}
      value={activeFilterValues.length > 0 ? activeFilterValues[0] : undefined}
      onChange={option => {
        const newValue = option?.value;
        handleChange(newValue ? [newValue] : []);
      }}
      onClose={() => {
        setStagedFilterValues([]);
      }}
      menuTitle={t('%s Filter', getDatasetLabel(dataset))}
      menuHeaderTrailingItems={renderMenuHeaderTrailingItems}
      triggerProps={{
        children: renderFilterSelectorTrigger(),
      }}
    />

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing fallbacks: The new conditional rendering and option building rely on external data (predefined
values, fetched values) without explicit null/empty fallbacks or error handling for failed
queries.

Referred Code
const options = useMemo(() => {
  if (predefinedValues && canSelectMultipleValues) {
    return predefinedValues.flatMap(section =>
      section.suggestions.map(suggestion => ({
        label: suggestion.value,
        value: suggestion.value,
      }))
    );
  }

  const optionMap = new Map<string, SelectOption<string>>();
  const fixedOptionMap = new Map<string, SelectOption<string>>();
  const addOption = (value: string, map: Map<string, SelectOption<string>>) =>
    map.set(value, {label: value, value});

  // Filter values in the global filter
  activeFilterValues.forEach(value => addOption(value, optionMap));

  // Predefined values
  predefinedValues?.forEach(suggestionSection => {
    suggestionSection.suggestions.forEach(suggestion =>


 ... (clipped 25 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: User-entered search queries and selected values are passed through to handlers without
visible sanitization or validation in the new code changes.

Referred Code
searchPlaceholder={t('Search filter values...')}
onSearch={setSearchQuery}
defaultValue={[]}
onChange={handleChange}
onStagedValueChange={value => {
  setStagedFilterValues(value);
}}
sizeLimit={30}
maxMenuWidth={500}
onClose={() => {
  setSearchQuery('');
  setStagedFilterValues([]);
}}
sizeLimitMessage={t('Use search to find more filter values…')}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
The PR introduces a logical flaw

The logic for generating filter options is flawed. The condition if
(predefinedValues && canSelectMultipleValues) incorrectly excludes predefined
options for single-value filters, breaking functionality for boolean filters.

Examples:

static/app/views/dashboards/globalFilter/filterSelector.tsx [115-122]
    if (predefinedValues && canSelectMultipleValues) {
      return predefinedValues.flatMap(section =>
        section.suggestions.map(suggestion => ({
          label: suggestion.value,
          value: suggestion.value,
        }))
      );
    }

Solution Walkthrough:

Before:

// static/app/views/dashboards/globalFilter/filterSelector.tsx
const options = useMemo(() => {
  // This block only runs for multi-select filters with predefined values.
  // It returns early, skipping the logic below.
  if (predefinedValues && canSelectMultipleValues) {
    return predefinedValues.flatMap(/*...options from suggestions...*/);
  }

  // For single-value filters (like booleans), `canSelectMultipleValues` is false.
  // This `if` is skipped, and the logic below is executed.
  // However, the logic below does not correctly populate the predefined
  // 'true'/'false' options, leaving the dropdown empty.
  const optionMap = new Map();
  // ...
  return Array.from(optionMap.values());
}, [..., canSelectMultipleValues]);

After:

// static/app/views/dashboards/globalFilter/filterSelector.tsx
const options = useMemo(() => {
  // This block now runs for ANY filter with predefined values,
  // regardless of whether it is single- or multi-select.
  if (predefinedValues) {
    return predefinedValues.flatMap(/*...options from suggestions...*/);
  }

  // This logic is now only for filters without predefined values.
  const optionMap = new Map();
  // ...
  return Array.from(optionMap.values());
}, [...]);
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical logic flaw where the condition if (predefinedValues && canSelectMultipleValues) prevents single-value filters, like booleans, from displaying their predefined options, defeating the PR's main purpose.

High
Possible issue
Fix menu not closing on clear

Update renderMenuHeaderTrailingItems to accept and call both close and
closeOverlay functions to fix a regression where the menu does not close after
clearing selections.

static/app/views/dashboards/globalFilter/filterSelector.tsx [182-205]

-const renderMenuHeaderTrailingItems = ({closeOverlay}: any) => (
+const renderMenuHeaderTrailingItems = ({
+  close,
+  closeOverlay,
+}: {
+  close?: () => void;
+  closeOverlay?: () => void;
+}) => (
   <Flex gap="md">
     {activeFilterValues.length > 0 && (
       <StyledButton
         aria-label={t('Clear Selections')}
         size="zero"
         borderless
         onClick={() => {
           setSearchQuery('');
           handleChange([]);
+          close?.();
+          closeOverlay?.();
         }}
       >
         {t('Clear')}
       </StyledButton>
     )}
     <StyledButton
       aria-label={t('Remove Filter')}
       size="zero"
       onClick={() => onRemoveFilter(globalFilter)}
     >
       {t('Remove Filter')}
     </StyledButton>
   </Flex>
 );
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a UI regression where the closeOverlay() call was removed during refactoring, and proposes a robust fix that accounts for usage in both HybridFilter and the newly added CompactSelect.

Medium
  • More

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.

3 participants