Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR getsentry#102792

Type: Clean (correct implementation)

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

  • Export tokenSupportsMultipleValues function for reuse across components

  • Use single value CompactSelect for boolean and string filter types

  • Add IGNORE_DEFAULT_VALUES constant to exclude string/boolean from default value initialization

  • Refactor filter selector to support both multi-select and single-select modes

  • Extract menu header and trigger rendering into separate functions

  • Increase max menu width from 400 to 500 pixels


Diagram Walkthrough

flowchart LR
  A["valueCombobox.tsx<br/>Export tokenSupportsMultipleValues"] --> B["filterSelector.tsx<br/>Import and use function"]
  B --> C["Determine canSelectMultipleValues"]
  C --> D{Can select<br/>multiple?}
  D -->|No| E["Render CompactSelect<br/>single value mode"]
  D -->|Yes| F["Render HybridFilter<br/>multi-select mode"]
  G["addFilter.tsx<br/>Add IGNORE_DEFAULT_VALUES"] --> H["Skip default values<br/>for string/boolean"]
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 whether a filter token supports multiple values
    based on field definition
+1/-1     
addFilter.tsx
Skip default values for string and boolean types                 

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

  • Add IGNORE_DEFAULT_VALUES constant containing string and boolean field
    value types
  • Update default filter value logic to skip initialization for string
    and boolean types
  • Prevents unnecessary default values for fields that should remain
    empty
+2/-1     
filterSelector.tsx
Implement single value selector for non-multi-select filters

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

  • Import CompactSelect component and tokenSupportsMultipleValues
    function
  • Add canSelectMultipleValues state to determine which selector
    component to use
  • Extract renderMenuHeaderTrailingItems and renderFilterSelectorTrigger
    into separate functions
  • Implement conditional rendering: use CompactSelect for single-value
    filters, HybridFilter for multi-select
  • Update CompactSelect to handle single value selection with clear and
    remove filter buttons
  • Increase max menu width from 400 to 500 pixels
  • Add canSelectMultipleValues to options dependency array
+80/-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 (clearing selections, removing filters, changing single-value
selection) are not logged, but UI diffs may not handle logging here; verify if audit
logging occurs elsewhere.

Referred Code
<Flex gap="md">
  {activeFilterValues.length > 0 && (
    <StyledButton
      aria-label={t('Clear Selections')}
      size="zero"
      borderless
      onClick={() => {
        setSearchQuery('');
        handleChange([]);
        closeOverlay();
      }}
    >
      {t('Clear')}
    </StyledButton>
  )}
  <StyledButton
    aria-label={t('Remove Filter')}
    size="zero"
    onClick={() => onRemoveFilter(globalFilter)}
  >
    {t('Remove Filter')}


 ... (clipped 33 lines)

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 error handling: Newly added selection logic and hooks do not include error handling for query failures or
empty option generation, which may be handled upstream but is not visible in this diff.

Referred Code
  placeholderData: keepPreviousData,
  enabled: shouldFetchValues,
  staleTime: 5 * 60 * 1000,
});

const {data: fetchedFilterValues, isFetching} = queryResult;

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});


 ... (clipped 33 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 sanitization: User-influenced search queries and option values are passed through without visible
sanitization; this may be safe in UI context but requires confirmation of downstream
handling.

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…')}
emptyMessage={
  isFetching ? t('Loading filter values...') : t('No filter values found')
}
menuTitle={t('%s Filter', getDatasetLabel(dataset))}
menuHeaderTrailingItems={renderMenuHeaderTrailingItems}
triggerProps={{

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
Possible issue
Prevent runtime error on clear

To prevent a potential runtime error, make the closeOverlay function call
conditional.

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

-const renderMenuHeaderTrailingItems = ({closeOverlay}: any) => (
+const renderMenuHeaderTrailingItems = ({closeOverlay}: {closeOverlay?: () => void}) => (
   <Flex gap="md">
     {activeFilterValues.length > 0 && (
       <StyledButton
         aria-label={t('Clear Selections')}
         size="zero"
         borderless
         onClick={() => {
           setSearchQuery('');
           handleChange([]);
-          closeOverlay();
+          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 potential runtime error where closeOverlay may not be defined, and the proposed fix using optional chaining is robust and prevents the crash.

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.

2 participants