From 37b6ded5f1f66ff84e1c80f26736b87950052e23 Mon Sep 17 00:00:00 2001 From: Julie Tibshirani Date: Tue, 25 Jun 2024 09:43:14 -0700 Subject: [PATCH] Search: always respect default pattern type (#63410) Previously, enabling then disabling the regexp toggle would always set the patterntype to `keyword`, even when the user has set `search.defaultPatternType: standard`. Now, toggles always revert back to the default pattern type. To support this, this PR adds `defaultPatternType` to the nav bar query state, which is updated every time there's a settings update. Having `defaultPatternType` available also lets us fix another bug: when the default pattern type has been set to `standard` we no longer awkwardly show `patterntype: standard` in the search bar. (This confusing behavior was introduced recently in #63326.) **Overall, this PR set us up to remove `experimentalFeatures.keywordSearch`, along with the keyword search toggle.** To opt out of keyword search, users can just set `search.defaultPatternType`, and have it work everywhere. --- .../src/search-ui/input/SearchBox.story.tsx | 13 +++++++- .../branded/src/search-ui/input/SearchBox.tsx | 1 + .../search-ui/input/toggles/Toggles.test.tsx | 27 ++++++++++++++-- .../src/search-ui/input/toggles/Toggles.tsx | 11 ++++--- .../__snapshots__/Toggles.test.tsx.snap | 27 ++++++++++++++++ client/shared/src/search/searchQueryState.tsx | 1 + .../webview/search-panel/SearchHomeView.tsx | 1 + .../search-panel/SearchResultsView.tsx | 3 +- .../sidebars/search/SearchSidebarView.tsx | 1 + .../NewGlobalNavigationBar.tsx | 21 ++++++++++-- .../web/src/search/input/SearchNavbarItem.tsx | 32 ++++++++++++++++--- .../web/src/stores/navbarSearchQueryState.ts | 32 ++++++++++++------- .../pages/SearchPage/SearchPageInput.tsx | 13 ++++++-- client/web/src/util/settings.ts | 4 +-- 14 files changed, 155 insertions(+), 32 deletions(-) diff --git a/client/branded/src/search-ui/input/SearchBox.story.tsx b/client/branded/src/search-ui/input/SearchBox.story.tsx index 7181dcbec7a..6becaea6b88 100644 --- a/client/branded/src/search-ui/input/SearchBox.story.tsx +++ b/client/branded/src/search-ui/input/SearchBox.story.tsx @@ -28,7 +28,8 @@ const defaultProps: SearchBoxProps = { telemetryRecorder: noOpTelemetryRecorder, queryState: { query: 'hello repo:test' }, isSourcegraphDotCom: false, - patternType: SearchPatternType.standard, + patternType: SearchPatternType.keyword, + defaultPatternType: SearchPatternType.keyword, setPatternType: () => {}, caseSensitive: false, setCaseSensitivity: () => {}, @@ -61,11 +62,21 @@ export const SearchBoxStory: StoryFn = () => ( +

Standard enabled

+
+ +
+

Structural enabled

+

Default patterntype

+
+ +
+

Case sensitivity enabled

diff --git a/client/branded/src/search-ui/input/SearchBox.tsx b/client/branded/src/search-ui/input/SearchBox.tsx index 2d0e1845b96..f491e9204de 100644 --- a/client/branded/src/search-ui/input/SearchBox.tsx +++ b/client/branded/src/search-ui/input/SearchBox.tsx @@ -194,6 +194,7 @@ export const SearchBox: FC = props => { {props.showKeywordSearchToggle ? ( { renderWithBrandedContext( undefined} caseSensitive={false} setCaseSensitivity={() => undefined} @@ -33,7 +34,8 @@ describe('Toggles', () => { renderWithBrandedContext( undefined} caseSensitive={false} setCaseSensitivity={() => undefined} @@ -50,7 +52,26 @@ describe('Toggles', () => { renderWithBrandedContext( undefined} + caseSensitive={false} + setCaseSensitivity={() => undefined} + searchMode={SearchMode.Precise} + setSearchMode={() => undefined} + telemetryService={NOOP_TELEMETRY_SERVICE} + telemetryRecorder={noOpTelemetryRecorder} + /> + ) + expect(screen.getAllByRole('checkbox', { name: 'Regular expression toggle' })).toMatchSnapshot() + }) + + test('regexp toggle with default patterntype', () => { + renderWithBrandedContext( + undefined} caseSensitive={false} setCaseSensitivity={() => undefined} diff --git a/client/branded/src/search-ui/input/toggles/Toggles.tsx b/client/branded/src/search-ui/input/toggles/Toggles.tsx index 1bdb3a9498a..dde6eee7afc 100644 --- a/client/branded/src/search-ui/input/toggles/Toggles.tsx +++ b/client/branded/src/search-ui/input/toggles/Toggles.tsx @@ -28,6 +28,7 @@ export interface TogglesProps TelemetryV2Props, Partial> { navbarSearchQuery: string + defaultPatternType: SearchPatternType className?: string /** * If set to false makes all buttons non-actionable. The main use case for @@ -46,6 +47,7 @@ export const Toggles: React.FunctionComponent { - const newPatternType = - patternType !== SearchPatternType.regexp ? SearchPatternType.regexp : SearchPatternType.keyword + const newPatternType = patternType !== SearchPatternType.regexp ? SearchPatternType.regexp : defaultPatternType setPatternType(newPatternType) submitOnToggle({ newPatternType }) telemetryService.log('ToggleRegexpPatternType', { currentStatus: patternType === SearchPatternType.regexp }) telemetryRecorder.recordEvent('search.regexpPatternType', 'toggle') - }, [patternType, setPatternType, submitOnToggle, telemetryService, telemetryRecorder]) + }, [patternType, defaultPatternType, setPatternType, submitOnToggle, telemetryService, telemetryRecorder]) const toggleStructuralSearch = useCallback((): void => { const newPatternType: SearchPatternType = - patternType !== SearchPatternType.structural ? SearchPatternType.structural : SearchPatternType.keyword + patternType !== SearchPatternType.structural ? SearchPatternType.structural : defaultPatternType setPatternType(newPatternType) submitOnToggle({ newPatternType }) telemetryRecorder.recordEvent('search.structuralPatternType', 'toggle') - }, [patternType, setPatternType, submitOnToggle, telemetryRecorder]) + }, [patternType, defaultPatternType, setPatternType, submitOnToggle, telemetryRecorder]) return (
diff --git a/client/branded/src/search-ui/input/toggles/__snapshots__/Toggles.test.tsx.snap b/client/branded/src/search-ui/input/toggles/__snapshots__/Toggles.test.tsx.snap index 37c255b6004..574ba9f76f5 100644 --- a/client/branded/src/search-ui/input/toggles/__snapshots__/Toggles.test.tsx.snap +++ b/client/branded/src/search-ui/input/toggles/__snapshots__/Toggles.test.tsx.snap @@ -80,3 +80,30 @@ Array [
, ] `; + +exports[`Toggles > Query input toggle state > regexp toggle with default patterntype 1`] = ` +Array [ + , +] +`; diff --git a/client/shared/src/search/searchQueryState.tsx b/client/shared/src/search/searchQueryState.tsx index 40c3fe365ec..38cc608a621 100644 --- a/client/shared/src/search/searchQueryState.tsx +++ b/client/shared/src/search/searchQueryState.tsx @@ -69,6 +69,7 @@ export interface SearchQueryState { queryState: QueryState searchCaseSensitivity: boolean searchPatternType: SearchPatternType + defaultPatternType: SearchPatternType searchQueryFromURL: string searchMode: SearchMode diff --git a/client/vscode/src/webview/search-panel/SearchHomeView.tsx b/client/vscode/src/webview/search-panel/SearchHomeView.tsx index 291767e9e99..d2fd66e10f0 100644 --- a/client/vscode/src/webview/search-panel/SearchHomeView.tsx +++ b/client/vscode/src/webview/search-panel/SearchHomeView.tsx @@ -167,6 +167,7 @@ export const SearchHomeView: React.FunctionComponent = React.memo(function queryState: { query: '' }, searchCaseSensitivity: false, searchPatternType: SearchPatternType.standard, + defaultPatternType: SearchPatternType.standard, // Not used here searchQueryFromURL: '', searchMode: SearchMode.Precise, diff --git a/client/web/src/nav/new-global-navigation/NewGlobalNavigationBar.tsx b/client/web/src/nav/new-global-navigation/NewGlobalNavigationBar.tsx index dea6e3ef48b..5117b2770bf 100644 --- a/client/web/src/nav/new-global-navigation/NewGlobalNavigationBar.tsx +++ b/client/web/src/nav/new-global-navigation/NewGlobalNavigationBar.tsx @@ -156,7 +156,13 @@ export const NewGlobalNavigationBar: FC = props => { type NavigationSearchBoxState = Pick< SearchQueryState, - 'queryState' | 'setQueryState' | 'submitSearch' | 'searchCaseSensitivity' | 'searchPatternType' | 'searchMode' + | 'queryState' + | 'setQueryState' + | 'submitSearch' + | 'searchCaseSensitivity' + | 'searchPatternType' + | 'defaultPatternType' + | 'searchMode' > /** @@ -170,6 +176,7 @@ const selectQueryState = (state: SearchQueryState): NavigationSearchBoxState => submitSearch: state.submitSearch, searchCaseSensitivity: state.searchCaseSensitivity, searchPatternType: state.searchPatternType, + defaultPatternType: state.defaultPatternType, searchMode: state.searchMode, }) @@ -191,8 +198,15 @@ const NavigationSearchBox: FC = props => { const location = useLocation() const showKeywordSearchToggle = useKeywordSearch() - const { searchMode, queryState, searchPatternType, searchCaseSensitivity, setQueryState, submitSearch } = - useNavbarQueryState(selectQueryState, shallow) + const { + searchMode, + queryState, + searchPatternType, + defaultPatternType, + searchCaseSensitivity, + setQueryState, + submitSearch, + } = useNavbarQueryState(selectQueryState, shallow) const submitSearchOnChange = useCallback( (parameters: Partial = {}) => { @@ -231,6 +245,7 @@ const NavigationSearchBox: FC = props => { => ({ queryState, setQueryState, submitSearch, searchCaseSensitivity, searchPatternType, searchMode }) + | 'queryState' + | 'setQueryState' + | 'submitSearch' + | 'searchCaseSensitivity' + | 'searchPatternType' + | 'defaultPatternType' + | 'searchMode' +> => ({ + queryState, + setQueryState, + submitSearch, + searchCaseSensitivity, + searchPatternType, + defaultPatternType, + searchMode, +}) /** * The search item in the navbar @@ -52,8 +67,15 @@ export const SearchNavbarItem: React.FunctionComponent((set, get) => ({ parametersSource: InitialParametersSource.DEFAULT, queryState: { query: '' }, searchCaseSensitivity: false, - searchPatternType: SearchPatternType.standard, + searchPatternType: SearchPatternType.keyword, + defaultPatternType: SearchPatternType.keyword, searchMode: SearchMode.SmartSearch, searchQueryFromURL: '', @@ -96,7 +92,8 @@ export function setSearchMode(searchMode: SearchMode): void { * the one contained in the URL (e.g. when the context:... filter got removed) */ export function setQueryStateFromURL(parsedSearchURL: ParsedSearchURL, query = parsedSearchURL.query ?? ''): void { - if (useNavbarQueryState.getState().parametersSource > InitialParametersSource.URL) { + const currentState = useNavbarQueryState.getState() + if (currentState.parametersSource > InitialParametersSource.URL) { return } @@ -121,9 +118,7 @@ export function setQueryStateFromURL(parsedSearchURL: ParsedSearchURL, query = p const parsedPatternType = parsedSearchURL.patternType if (parsedPatternType !== undefined) { newState.searchPatternType = parsedPatternType - // Only keyword, regexp, and structural are represented in the UI. For other pattern types, we make - // sure to surface them in the query input itself. - if (!explicitPatternTypes.has(parsedPatternType)) { + if (showPatternTypeInQuery(parsedPatternType, currentState.defaultPatternType)) { query = `${query} ${FilterType.patterntype}:${parsedPatternType}` } } @@ -137,6 +132,17 @@ export function setQueryStateFromURL(parsedSearchURL: ParsedSearchURL, query = p useNavbarQueryState.setState(newState as any) } +// The only pattern types explicitly represented in the UI are the default one, plus regexp and structural. For +// other pattern types, we make sure to surface them in the query input itself. +export function showPatternTypeInQuery( + patternType: SearchPatternType, + defaultPatternType?: SearchPatternType +): boolean { + return patternType !== defaultPatternType && !explicitPatternTypes.has(patternType) +} + +const explicitPatternTypes = new Set([SearchPatternType.regexp, SearchPatternType.structural]) + /** * Update or initialize query state related data from settings */ @@ -146,7 +152,10 @@ export function setQueryStateFromSettings(settings: SettingsCascadeOrError + Pick< + NavbarQueryState, + 'searchPatternType' | 'defaultPatternType' | 'searchCaseSensitivity' | 'parametersSource' | 'searchMode' + > > = { parametersSource: InitialParametersSource.USER_SETTINGS, } @@ -164,6 +173,7 @@ export function setQueryStateFromSettings(settings: SettingsCascadeOrError & SearchPatternTypeProps & Pick => ({ +): Pick & + SearchPatternTypeProps & + Pick & { defaultPatternType: SearchPatternType } => ({ caseSensitive: state.searchCaseSensitivity, patternType: state.searchPatternType, + defaultPatternType: state.defaultPatternType, searchMode: state.searchMode, }) @@ -73,7 +77,10 @@ export const SearchPageInput: FC = props => { const location = useLocation() const navigate = useNavigate() - const { caseSensitive, patternType, searchMode } = useNavbarQueryState(queryStateSelector, shallow) + const { caseSensitive, patternType, defaultPatternType, searchMode } = useNavbarQueryState( + queryStateSelector, + shallow + ) const [v2QueryInput] = useV2QueryInput() const { recentSearches } = useRecentSearches() @@ -151,6 +158,7 @@ export const SearchPageInput: FC = props => { {props.showKeywordSearchToggle ? ( = props => { showSearchContextManagement={true} caseSensitive={caseSensitive} patternType={patternType} + defaultPatternType={defaultPatternType} setPatternType={setSearchPatternType} setCaseSensitivity={setSearchCaseSensitivity} searchMode={searchMode} diff --git a/client/web/src/util/settings.ts b/client/web/src/util/settings.ts index 80cbad2cff5..5a72cbaa792 100644 --- a/client/web/src/util/settings.ts +++ b/client/web/src/util/settings.ts @@ -60,7 +60,7 @@ export function defaultSearchModeFromSettings(settingsCascade: SettingsCascadeOr * Returns the user-configured search pattern type or undefined if not * configured by the user. */ -export function defaultPatternTypeFromSettings(settingsCascade: SettingsCascadeOrError): SearchPatternType | undefined { +export function defaultPatternTypeFromSettings(settingsCascade: SettingsCascadeOrError): SearchPatternType { const defaultPatternType: SearchPatternType | undefined = getFromSettings( settingsCascade, 'search.defaultPatternType' @@ -69,7 +69,7 @@ export function defaultPatternTypeFromSettings(settingsCascade: SettingsCascadeO if (isKeywordSearchEnabled(settingsCascade)) { return defaultPatternType ?? SearchPatternType.keyword } - return defaultPatternType + return defaultPatternType ?? SearchPatternType.standard } /**