refactor(base,default-layout): address PR feedback

packages/@sanity/base/src/search/weighted/applyWeights.test.ts

@@ -42,7 +42,7 @@ describe('calculateWordScore', () => {
 
 describe('partitionAndSanitizeSearchTerms', () => {
   it('should separate words and phrases', () => {
-    const [phrases, words] = partitionAndSanitizeSearchTerms(['foo', 'bar', `"foo bar"`])
+    const {phrases, words} = partitionAndSanitizeSearchTerms(['foo', 'bar', `"foo bar"`])
     expect(phrases).toEqual(['foo bar'])
     expect(words).toEqual(['foo', 'bar'])
   })

packages/@sanity/base/src/search/weighted/applyWeights.ts

@@ -6,7 +6,9 @@ type SearchScore = [number, string]
 // takes a set of terms and a value and returns a [score, story] pair where score is a value between 0, 1 and story is the explanation
 export const calculateScore = (searchTerms: string[], value: string): SearchScore => {
   // Separate search terms by phrases (wrapped with quotes) and words.
-  const [uniqueSearchPhrases, uniqueSearchWords] = partitionAndSanitizeSearchTerms(searchTerms)
+  const {phrases: uniqueSearchPhrases, words: uniqueSearchWords} = partitionAndSanitizeSearchTerms(
+    searchTerms
+  )
 
   // Calculate an aggregated score of both phrase and word matches.
   const [phraseScore, phraseWhy] = calculatePhraseScore(uniqueSearchPhrases, value)
@@ -89,14 +91,19 @@ export function calculateWordScore(uniqueSearchTerms: string[], value: string):
       ]
 }
 
-export function partitionAndSanitizeSearchTerms(searchTerms: string[]): [string[], string[]] {
+export function partitionAndSanitizeSearchTerms(
+  searchTerms: string[]
+): {
+  phrases: string[]
+  words: string[]
+} {
   const uniqueSearchTerms = uniq(searchTerms.map(toLower))
 
   const [searchPhrases, searchWords] = partition(uniqueSearchTerms, (term) => /^".*"$/.test(term))
-  return [
-    uniq(searchPhrases).map(toLower).map(stripWrappingQuotes), //
-    uniq(searchWords.map(toLower)),
-  ]
+  return {
+    phrases: uniq(searchPhrases).map(toLower).map(stripWrappingQuotes), //
+    words: uniq(searchWords.map(toLower)),
+  }
 }
 
 function stripWrappingQuotes(str: string) {

packages/@sanity/base/src/search/weighted/createSearchQuery.test.ts

@@ -297,6 +297,22 @@ describe('createSearchQuery', () => {
 })
 
 describe('extractTermsFromQuery', () => {
+  describe('should handle orphaned double quotes', () => {
+    const tests: [string, string[]][] = [
+      [`"foo bar`, ['foo', 'bar']],
+      [`foo bar"`, ['foo', 'bar']],
+      [`foo "bar`, ['foo', 'bar']],
+    ]
+    it.each(tests)('%s', (input, expected) => {
+      expect(extractTermsFromQuery(input)).toEqual(expected)
+    })
+  })
+
+  it('should treat single quotes as regular characters', () => {
+    const terms = extractTermsFromQuery(`'foo ' bar'`)
+    expect(terms).toEqual([`'foo`, `'`, `bar'`])
+  })
+
   it('should tokenize all unquoted text', () => {
     const terms = extractTermsFromQuery('foo bar')
     expect(terms).toEqual(['foo', 'bar'])

packages/@sanity/default-layout/src/navbar/search/components/RecentSearches.tsx

@@ -97,7 +97,7 @@ export function RecentSearches({
                 <RecentSearchItem
                   data-index={index}
                   index={index}
-                  key={recentSearch.__recentTimestamp}
+                  key={recentSearch.__recent.timestamp}
                   maxVisibleTypePillChars={maxVisibleTypePillChars}
                   onClick={handleRecentSearchClick}
                   onDelete={handleRecentSearchDelete(index)}

packages/@sanity/default-layout/src/navbar/search/components/SearchHeader.tsx

@@ -1,5 +1,5 @@
 import {CloseIcon, ControlsIcon, SearchIcon} from '@sanity/icons'
-import {Box, Button, Card, Flex, Spinner, studioTheme} from '@sanity/ui'
+import {Box, Button, Card, Flex, Spinner, studioTheme, Theme} from '@sanity/ui'
 import React, {Dispatch, SetStateAction, useCallback, useEffect, useRef, useState} from 'react'
 import styled from 'styled-components'
 import {useSearchState} from '../contexts/search'
@@ -32,8 +32,8 @@ const SearchHeaderCard = styled(Card)`
 
 const NotificationBadge = styled.div`
   align-items: center;
-  background: ${({theme}) => theme?.sanity?.color?.selectable?.primary?.enabled?.fg};
-  color: ${({theme}) => theme?.sanity?.color?.selectable?.primary?.selected?.fg};
+  background: ${({theme}: {theme: Theme}) => theme.sanity.color.selectable?.primary.enabled.fg};
+  color: ${({theme}: {theme: Theme}) => theme.sanity.color.selectable?.primary.selected.fg};
   border-radius: 100%;
   display: flex;
   font-size: calc(8 / 16 * 1rem);

packages/@sanity/default-layout/src/navbar/search/components/SortMenu.tsx

@@ -12,7 +12,7 @@ import {
   MenuItem,
   Text,
 } from '@sanity/ui'
-import {isEqual} from 'lodash'
+import isEqual from 'lodash/isEqual'
 import React, {useCallback, useMemo} from 'react'
 import styled from 'styled-components'
 import {SUBHEADER_HEIGHT_LARGE, SUBHEADER_HEIGHT_SMALL} from '../constants'
@@ -30,13 +30,16 @@ interface SortMenuProps {
   small?: boolean
 }
 
-// null items are represented as dividers
-const MENU_ORDERINGS: (SearchOrdering | null)[] = [
+interface SearchDivider {
+  type: 'divider'
+}
+
+const MENU_ORDERINGS: (SearchDivider | SearchOrdering)[] = [
   ORDER_RELEVANCE,
-  null,
+  {type: 'divider'},
   ORDER_CREATED_ASC,
   ORDER_CREATED_DESC,
-  null,
+  {type: 'divider'},
   ORDER_UPDATED_ASC,
   ORDER_UPDATED_DESC,
 ]
@@ -50,6 +53,10 @@ const SortMenuContentFlex = styled(Flex)<{$small?: boolean}>`
   height: ${({$small}) => ($small ? SUBHEADER_HEIGHT_SMALL : SUBHEADER_HEIGHT_LARGE)}px;
 `
 
+function isSearchDivider(item: SearchDivider | SearchOrdering): item is SearchDivider {
+  return (item as SearchDivider).type === 'divider'
+}
+
 function CustomMenuItem({ordering}: {ordering: SearchOrdering}) {
   const {
     dispatch,
@@ -123,7 +130,7 @@ export function SortMenu({small}: SortMenuProps) {
           menu={
             <Menu>
               {MENU_ORDERINGS.map((item, index) => {
-                if (item === null) {
+                if (isSearchDivider(item)) {
                   // eslint-disable-next-line react/no-array-index-key
                   return <MenuDivider key={index} />
                 }

packages/@sanity/default-layout/src/navbar/search/components/TypePills.tsx

@@ -1,6 +1,6 @@
 import {TextWithTone} from '@sanity/base/components'
 import {SearchableType} from '@sanity/base'
-import {Box, Card, Flex, Text} from '@sanity/ui'
+import {Box, Card, Flex, Text, Theme} from '@sanity/ui'
 import React, {useMemo} from 'react'
 import styled from 'styled-components'
 
@@ -12,7 +12,8 @@ interface TypePillsProps {
 const DEFAULT_AVAILABLE_CHARS = 40 // excluding "+x more" suffix
 
 const PillCard = styled(Card)<{$collapsible?: boolean}>`
-  background: ${({theme}) => theme?.sanity?.color?.selectable?.primary?.enabled?.code?.bg};
+  background: ${({theme}: {theme: Theme}) =>
+    theme.sanity.color.selectable?.primary.enabled.code.bg};
   flex-shrink: ${({$collapsible}) => ($collapsible ? 1 : 0)};
   overflow: hidden;
 `

packages/@sanity/default-layout/src/navbar/search/contexts/commandList/index.tsx

@@ -202,33 +202,28 @@ export function CommandListProvider({
     []
   )
 
-  const scrollToNextItem = useCallback(() => {
-    const nextIndex = selectedIndexRef.current < childCount - 1 ? selectedIndexRef.current + 1 : 0
-
-    // Delegate scrolling to virtual list if necessary
-    if (virtualList) {
-      virtualListScrollToIndexRef?.current?.(nextIndex)
-      setActiveIndex({index: nextIndex, scrollIntoView: false})
-    } else {
-      setActiveIndex({index: nextIndex})
-    }
-
-    enableChildContainerPointerEvents(false)
-  }, [childCount, enableChildContainerPointerEvents, setActiveIndex, virtualList])
-
-  const scrollToPreviousItem = useCallback(() => {
-    const nextIndex = selectedIndexRef.current > 0 ? selectedIndexRef.current - 1 : childCount - 1
+  const scrollToAdjacentItem = useCallback(
+    (direction: 'previous' | 'next') => {
+      let nextIndex
+      if (direction === 'next') {
+        nextIndex = selectedIndexRef.current < childCount - 1 ? selectedIndexRef.current + 1 : 0
+      }
+      if (direction === 'previous') {
+        nextIndex = selectedIndexRef.current > 0 ? selectedIndexRef.current - 1 : childCount - 1
+      }
 
-    // Delegate scrolling to virtual list if necessary
-    if (virtualList) {
-      virtualListScrollToIndexRef?.current?.(nextIndex)
-      setActiveIndex({index: nextIndex, scrollIntoView: false})
-    } else {
-      setActiveIndex({index: nextIndex})
-    }
+      // Delegate scrolling to virtual list if necessary
+      if (virtualList) {
+        virtualListScrollToIndexRef?.current?.(nextIndex)
+        setActiveIndex({index: nextIndex, scrollIntoView: false})
+      } else {
+        setActiveIndex({index: nextIndex})
+      }
 
-    enableChildContainerPointerEvents(false)
-  }, [childCount, enableChildContainerPointerEvents, setActiveIndex, virtualList])
+      enableChildContainerPointerEvents(false)
+    },
+    [childCount, enableChildContainerPointerEvents, setActiveIndex, virtualList]
+  )
 
   /**
    * Set active index whenever initial index changes
@@ -263,11 +258,11 @@ export function CommandListProvider({
 
       if (event.key === 'ArrowDown') {
         event.preventDefault()
-        scrollToNextItem()
+        scrollToAdjacentItem('next')
       }
       if (event.key === 'ArrowUp') {
         event.preventDefault()
-        scrollToPreviousItem()
+        scrollToAdjacentItem('previous')
       }
       if (event.key === 'Enter') {
         event.preventDefault()
@@ -290,7 +285,7 @@ export function CommandListProvider({
     return () => {
       headerInputElement?.removeEventListener('keydown', handleKeyDown)
     }
-  }, [childContainerElement, headerInputElement, scrollToNextItem, scrollToPreviousItem])
+  }, [childContainerElement, headerInputElement, scrollToAdjacentItem])
 
   /**
    * Listen to keyboard arrow events on the 'closest' parent [data-overflow] element to the child container.
@@ -304,12 +299,12 @@ export function CommandListProvider({
       if (event.key === 'ArrowDown') {
         event.preventDefault()
         headerInputElement?.focus()
-        scrollToNextItem()
+        scrollToAdjacentItem('next')
       }
       if (event.key === 'ArrowUp') {
         event.preventDefault()
         headerInputElement?.focus()
-        scrollToPreviousItem()
+        scrollToAdjacentItem('previous')
       }
     }
 
@@ -318,7 +313,7 @@ export function CommandListProvider({
     return () => {
       parentOverflowElement?.removeEventListener('keydown', handleKeydown)
     }
-  }, [childContainerElement, headerInputElement, scrollToNextItem, scrollToPreviousItem])
+  }, [childContainerElement, headerInputElement, scrollToAdjacentItem])
 
   /**
    * Track focus / blur state on the list's input element and store state in `data-focused` attribute on

packages/@sanity/default-layout/src/navbar/search/contexts/search/index.tsx

@@ -3,7 +3,7 @@
 
 import type {SearchTerms} from '@sanity/base'
 import type {CurrentUser} from '@sanity/types'
-import {isEqual} from 'lodash'
+import isEqual from 'lodash/isEqual'
 import schema from 'part:@sanity/base/schema'
 import React, {
   createContext,
@@ -97,7 +97,9 @@ export function SearchProvider({children, currentUser}: SearchProviderProps) {
           // Comments prepended to each query for future measurement
           comments: [
             `findability-mvi:${FINDABILITY_MVI}`,
-            ...(isRecentSearchTerms(terms) ? [`findability-recent-search:${terms.__index}`] : []),
+            ...(isRecentSearchTerms(terms)
+              ? [`findability-recent-search:${terms.__recent.index}`]
+              : []),
             `findability-selected-types:${terms.types.length}`,
             `findability-sort:${sortLabel}`,
           ],

packages/@sanity/default-layout/src/navbar/search/contexts/search/reducer.test.ts

@@ -27,8 +27,10 @@ const mockSearchableType: SearchableType = {
 }
 
 const recentSearchTerms = {
-  __index: 0,
-  __recentTimestamp: new Date().getTime(),
+  __recent: {
+    index: 0,
+    timestamp: new Date().getTime(),
+  },
   query: 'foo',
   types: [],
 } as RecentSearchTerms
@@ -38,80 +40,73 @@ const initialState: SearchReducerState = {
 }
 
 describe('searchReducer', () => {
-  it('should clear __index and __recentTimestamp when page index is incremented', () => {
+  it('should clear __recent when page index is incremented', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({type: 'PAGE_INCREMENT'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 
-  it('should clear __index and __recentTimestamp after resetting sort order', () => {
+  it('should clear __recent after resetting sort order', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({type: 'SEARCH_ORDERING_RESET'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 
-  it('should clear __index and __recentTimestamp after updating sort order', () => {
+  it('should clear __recent after updating sort order', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({ordering: mockOrdering, type: 'SEARCH_ORDERING_SET'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 
-  it('should clear __index and __recentTimestamp after updating query', () => {
+  it('should clear __recent after updating query', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({query: 'bar', type: 'TERMS_QUERY_SET'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 
-  it('should clear __index and __recentTimestamp after adding a document type', () => {
+  it('should clear __recent after adding a document type', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({schemaType: mockSearchableType, type: 'TERMS_TYPE_ADD'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 
-  it('should clear __index and __recentTimestamp after remove a document type', () => {
+  it('should clear __recent after remove a document type', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({schemaType: mockSearchableType, type: 'TERMS_TYPE_REMOVE'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 
-  it('should clear __index and __recentTimestamp after clearing all document types', () => {
+  it('should clear __recent after clearing all document types', () => {
     const {result} = renderHook(() => useReducer(searchReducer, initialState))
     const [, dispatch] = result.current
 
     act(() => dispatch({type: 'TERMS_TYPES_CLEAR'}))
 
     const [state] = result.current
-    expect((state.terms as RecentSearchTerms).__index).toBeUndefined()
-    expect((state.terms as RecentSearchTerms).__recentTimestamp).toBeUndefined()
+    expect((state.terms as RecentSearchTerms).__recent).toBeUndefined()
   })
 })