Skip to content

Conversation

jasonyuezhang
Copy link
Owner

TODO

Ticket: EXP-357


Copied from getsentry#101070
Original PR: getsentry#101070

Copy link

Retain Position in Value Combobox When Multi-Selecting (Search Bar UX Improvements)

This PR improves the behavior of the multi-select value combobox in the search bar, ensuring selected values retain their position and order during the selection process. Extensive changes are made to internal state management and rendering logic so that when users select or deselect multiple values (e.g., with checkboxes or keyboard), the position and order of selected options are preserved across interactions and after the combobox is closed and reopened. The UI also ensures focused options are maintained properly and improves input focus handling during multi-selection. Corresponding test cases have been added to verify correct value reordering and overall stability.

Key Changes

• Refactored selection state in static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx to track initialSelectedValues and maintain value ordering.
• Introduced logic to separate the rendering of initially selected values from other suggestions, displaying them at the top of the combobox.
• Utilized useRef and dynamic lookup for selected state to avoid unnecessary re-creation of combobox items during checkbox interactions.
• Ensured that, after opening/closing the combobox, selected values remain in the order chosen by the user.
• Added state and effect logic to dynamically update initialSelectedValues with newly added custom values (typed by user) not present in suggestions.
• Updated event handler signatures to pass combobox state information, enabling better management of focus and selection state.
• Modified SearchQueryBuilderCombobox to pass and react to additional props such as preserveFocusOnInputChange and updated onOpenChange to provide up-to-date internal state.
• Enhanced valueListBox.tsx: tracks the focused key and restores focus/scroll position if react-aria clears it during updates, improving accessibility and UX.
• Revised test suite in index.spec.tsx to validate the correct order and persistence of selected values, ensuring the desired multi-select experience.
• Minor code quality/cleanup changes, including improved comments, removal of redundant code, and bug fixes for explicit selection logic.

Affected Areas

static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx
static/app/components/searchQueryBuilder/tokens/filter/valueListBox.tsx
static/app/components/searchQueryBuilder/tokens/combobox.tsx
static/app/components/searchQueryBuilder/index.spec.tsx

This summary was automatically generated by @propel-code-bot

Comment on lines +147 to +149
// Use the actual token location from the parser instead of indexOf to avoid
// matching substrings (e.g., "artifact_bundle.assemble" inside "artifact_bundle.assemble.find_projects")
const selected = text.charAt(item.value?.location.end.offset ?? 0) === ',';

Choose a reason for hiding this comment

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

[BestPractice]

Potential substring matching issue in selection detection:

// Use the actual token location from the parser instead of indexOf to avoid
// matching substrings (e.g., "artifact_bundle.assemble" inside "artifact_bundle.assemble.find_projects")
const selected = text.charAt(item.value?.location.end.offset ?? 0) === ',';

This fix is good, but there's still a potential issue: if item.value?.location.end.offset is 0 (falsy), the fallback ?? 0 will use index 0, which may not be correct for the actual token position. Consider using a more explicit check:

const endOffset = item.value?.location?.end?.offset;
if (endOffset === undefined) {
  // Handle case where location info is missing
  continue; // or some other fallback logic
}
const selected = text.charAt(endOffset) === ',';
Context for Agents
[**BestPractice**]

Potential substring matching issue in selection detection:

```typescript
// Use the actual token location from the parser instead of indexOf to avoid
// matching substrings (e.g., "artifact_bundle.assemble" inside "artifact_bundle.assemble.find_projects")
const selected = text.charAt(item.value?.location.end.offset ?? 0) === ',';
```

This fix is good, but there's still a potential issue: if `item.value?.location.end.offset` is 0 (falsy), the fallback `?? 0` will use index 0, which may not be correct for the actual token position. Consider using a more explicit check:

```typescript
const endOffset = item.value?.location?.end?.offset;
if (endOffset === undefined) {
  // Handle case where location info is missing
  continue; // or some other fallback logic
}
const selected = text.charAt(endOffset) === ',';
```

File: static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx
Line: 149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants