Browse Source

fix(query-builder): Improve the stability of token keys (#70828)

Previously, the cursor would jump around when entering a free text
portion due to the component remounting because the key changed. This
makes some changes to avoid keys changing as much as possible:

- Updates token keys from `{type}:{cursor_location}` to
`{type}:{type_index}`
- Renames space tokens to free_text since they are treated the same and
it keeps the keys stable
Malachi Willey 10 months ago
parent
commit
8dd727037c

+ 10 - 0
static/app/components/searchQueryBuilder/index.spec.tsx

@@ -26,6 +26,10 @@ const MOCK_SUPPORTED_KEYS: TagCollection = {
 };
 
 describe('SearchQueryBuilder', function () {
+  afterEach(function () {
+    jest.restoreAllMocks();
+  });
+
   const defaultProps: ComponentProps<typeof SearchQueryBuilder> = {
     getTagValues: jest.fn(),
     initialQuery: '',
@@ -178,7 +182,13 @@ describe('SearchQueryBuilder', function () {
         screen.getByRole('combobox'),
         'some free text brow{ArrowDown}'
       );
+
+      // XXX(malwilley): SearchQueryBuilderInput updates state in the render
+      // function which causes an act warning despite using userEvent.click.
+      // Cannot find a way to avoid this warning.
+      jest.spyOn(console, 'error').mockImplementation(jest.fn());
       await userEvent.click(screen.getByRole('option', {name: 'Browser Name'}));
+      jest.restoreAllMocks();
 
       // Should have a free text token "some free text"
       expect(screen.getByRole('row', {name: 'some free text'})).toBeInTheDocument();

+ 4 - 5
static/app/components/searchQueryBuilder/index.tsx

@@ -42,7 +42,6 @@ interface GridProps extends AriaGridListOptions<ParseResultToken> {
 function Grid(props: GridProps) {
   const ref = useRef<HTMLDivElement>(null);
   const state = useListState<ParseResultToken>(props);
-
   const {gridProps} = useQueryBuilderGrid(props, state, ref);
 
   return (
@@ -55,17 +54,17 @@ function Grid(props: GridProps) {
           case Token.FILTER:
             return (
               <SearchQueryBuilderFilter
-                key={makeTokenKey(token)}
+                key={item.key}
                 token={token}
                 item={item}
                 state={state}
               />
             );
-          case Token.SPACES:
           case Token.FREE_TEXT:
+          case Token.SPACES:
             return (
               <SearchQueryBuilderInput
-                key={makeTokenKey(token)}
+                key={item.key}
                 token={token}
                 item={item}
                 state={state}
@@ -117,7 +116,7 @@ export function SearchQueryBuilder({
       <PanelProvider>
         <Grid aria-label={label ?? t('Create a search query')} items={parsedQuery}>
           {item => (
-            <Item key={makeTokenKey(item)}>
+            <Item key={makeTokenKey(item, parsedQuery)}>
               {item.text.trim() ? item.text : t('Space')}
             </Item>
           )}

+ 7 - 6
static/app/components/searchQueryBuilder/input.tsx

@@ -76,12 +76,13 @@ function SearchQueryBuilderInputInternal({
   token,
   tabIndex,
 }: SearchQueryBuilderInputInternalProps) {
-  const [inputValue, setInputValue] = useState(token.value.trim());
+  const trimmedTokenValue = token.value.trim();
+  const [inputValue, setInputValue] = useState(trimmedTokenValue);
   // TODO(malwilley): Use input ref to update cursor position on mount
   const [selectionIndex, setSelectionIndex] = useState(0);
 
   const resetInputValue = () => {
-    setInputValue(token.value.trim());
+    setInputValue(trimmedTokenValue);
     // TODO(malwilley): Reset cursor position using ref
   };
 
@@ -109,10 +110,10 @@ function SearchQueryBuilderInputInternal({
   }, [allKeys]);
 
   // When token value changes, reset the input value
-  const [prevValue, setPrevValue] = useState(token.value);
-  if (token.value.trim() !== prevValue) {
-    setPrevValue(token.value.trim());
-    setInputValue(token.value.trim());
+  const [prevValue, setPrevValue] = useState(inputValue);
+  if (trimmedTokenValue !== prevValue) {
+    setPrevValue(trimmedTokenValue);
+    setInputValue(trimmedTokenValue);
   }
 
   return (

+ 23 - 4
static/app/components/searchQueryBuilder/utils.tsx

@@ -9,8 +9,21 @@ import {
 } from 'sentry/components/searchSyntax/parser';
 import {escapeDoubleQuotes} from 'sentry/utils';
 
-export function makeTokenKey(token: TokenResult<Token>) {
-  return `${token.type}:${token.location.start.offset}`;
+/**
+ * Generates a unique key for the given token.
+ *
+ * It's important that the key is as stable as possible. Since we derive tokens
+ * from the a simple query string, this is difficult to guarantee. The best we
+ * can do is to use the token type and which iteration of that type it is.
+ *
+ * Example for query "is:unresolved foo assignee:me bar":
+ * Keys: ["freeText:0", "filter:0", "freeText:1" "filter:1", "freeText:2"]
+ */
+export function makeTokenKey(token: ParseResultToken, allTokens: ParseResult | null) {
+  const tokenTypeIndex =
+    allTokens?.filter(t => t.type === token.type).indexOf(token) ?? 0;
+
+  return `${token.type}:${tokenTypeIndex}`;
 }
 
 const isSimpleTextToken = (
@@ -29,6 +42,13 @@ export function collapseTextTokens(tokens: ParseResult | null) {
   }
 
   return tokens.reduce<ParseResult>((acc, token) => {
+    // For our purposes, SPACES are equivalent to FREE_TEXT
+    // Combining them ensures that keys don't change when text is added or removed,
+    // which would cause the cursor to jump around.
+    if (isSimpleTextToken(token)) {
+      token.type = Token.FREE_TEXT;
+    }
+
     if (acc.length === 0) {
       return [token];
     }
@@ -37,9 +57,8 @@ export function collapseTextTokens(tokens: ParseResult | null) {
 
     if (isSimpleTextToken(token) && isSimpleTextToken(lastToken)) {
       lastToken.value += token.value;
-      lastToken.text += token.value;
+      lastToken.text += token.text;
       lastToken.location.end = token.location.end;
-      lastToken.type = Token.FREE_TEXT;
       return acc;
     }