Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions client/shared/src/settings/settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { cloneDeep, isFunction } from 'lodash'
import { createAggregateError, ErrorLike, isErrorLike, parseJSONCOrError } from '@sourcegraph/common'

import { DefaultSettingFields, OrgSettingFields, SiteSettingFields, UserSettingFields } from '../graphql-operations'
import { Settings as GeneratedSettingsType } from '../schema/settings.schema'
import { Settings as GeneratedSettingsType, SettingsExperimentalFeatures } from '../schema/settings.schema'

/**
* A dummy type to represent the "subject" for client settings (i.e., settings stored in the client application,
Expand Down Expand Up @@ -245,10 +245,10 @@ export interface SettingsCascadeProps<S extends Settings = Settings> {
}

interface SettingsContextData<S extends Settings = Settings> {
settingsCascade: SettingsCascadeOrError<S> | null
settingsCascade: SettingsCascadeOrError<S>
}
const SettingsContext = createContext<SettingsContextData>({
settingsCascade: null,
settingsCascade: EMPTY_SETTINGS_CASCADE,
})

interface SettingsProviderProps {
Expand All @@ -268,8 +268,11 @@ export const SettingsProvider: React.FC<React.PropsWithChildren<SettingsProvider
*/
export const useSettingsCascade = (): SettingsCascadeOrError => {
const { settingsCascade } = useContext(SettingsContext)
if (!settingsCascade) {
throw new Error('useSettingsCascade must be used within a SettingsProvider')
if (settingsCascade === EMPTY_SETTINGS_CASCADE && process.env.JEST_WORKER_ID === undefined) {
// eslint-disable-next-line no-console
console.error(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The console.x methods are banned so we don't forget them in our PRs and to allow us to control the console output in place, where we potentially can send these logs/errors to other services like Sentry, Honeycomb, etc.

import { logger } from '@sourcegraph/common'
logger.error(...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cool. Is there a way to extend the error message here to explain this?

Screenshot 2023-02-24 at 10 49 44

We have a lot of annoying eslint rules so I just try to make them go away quickly. In this case, I wasn't aware that this is for a good reason and the message didn't explain it either.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have a lot of annoying eslint rules so I just try to make them go away quickly.

True, we also have a bunch of duplicate rules because of multiple plugins we use. Let's raise this in #chat-frontend next time it happens so we can remove redundant rules and better document others.

Is there a way to extend the error message here to explain this?

There's no super straightforward way to do it AFAIK. We cannot use no-restricted-syntax because it already has a warning level. We cannot add a custom message to the no-console rule, according to the docs. We can probably create a custom rule and add our message there. Let me know if you see a better way. Here's the related section in our documentation that can be used in the custom error message.

'useSettingsCascade must be used within a SettingsProvider, falling back to an empty settings object'
)
}
return settingsCascade
}
Expand All @@ -281,3 +284,28 @@ export const useSettings = (): Settings | null => {
const settingsCascade = useSettingsCascade()
return isSettingsValid(settingsCascade) ? settingsCascade.final : null
}

const defaultFeatures: SettingsExperimentalFeatures = {
codeMonitoring: true,
/**
* Whether we show the multiline editor at /search/console
*/
showMultilineSearchConsole: false,
codeMonitoringWebHooks: true,
showCodeMonitoringLogs: true,
codeInsightsCompute: false,
editor: 'codemirror6',
codeInsightsRepoUI: 'search-query-or-strict-list',
applySearchQuerySuggestionOnEnter: false,
setupWizard: false,
isInitialized: true,
}

/**
* A React hooks that can be used to query specific feature flags. Prioritize this over the generic
* useSettings() hook if all you need is a feature flag.
*/
export function useExperimentalFeatures<T>(selector: (features: SettingsExperimentalFeatures) => T): T {
const settings = useSettings()
return selector({ ...defaultFeatures, ...settings?.experimentalFeatures })
}
16 changes: 10 additions & 6 deletions client/web/src/Layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import { PlatformContextProps } from '@sourcegraph/shared/src/platform/context'
import { Shortcut } from '@sourcegraph/shared/src/react-shortcuts'
import { Settings } from '@sourcegraph/shared/src/schema/settings.schema'
import { SearchContextProps } from '@sourcegraph/shared/src/search'
import { SettingsCascadeProps, SettingsSubjectCommonFields } from '@sourcegraph/shared/src/settings/settings'
import {
SettingsCascadeProps,
SettingsSubjectCommonFields,
useExperimentalFeatures,
} from '@sourcegraph/shared/src/settings/settings'
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
import { useTheme, Theme } from '@sourcegraph/shared/src/theme'
import { lazyComponent } from '@sourcegraph/shared/src/util/lazyComponent'
Expand Down Expand Up @@ -40,8 +44,6 @@ import { EnterprisePageRoutes, PageRoutes } from './routes.constants'
import { parseSearchURLQuery, SearchAggregationProps, SearchStreamingProps } from './search'
import { NotepadContainer } from './search/Notepad'
import { SearchQueryStateObserver } from './SearchQueryStateObserver'
import { useExperimentalFeatures } from './stores'
import { getExperimentalFeatures } from './util/get-experimental-features'
import { parseBrowserRepoURL } from './util/url'

import styles from './Layout.module.scss'
Expand Down Expand Up @@ -103,11 +105,13 @@ export const Layout: React.FC<LegacyLayoutProps> = props => {
)
const isSearchNotebookListPage = location.pathname === EnterprisePageRoutes.Notebooks

const { setupWizard } = useExperimentalFeatures()
const { setupWizard, fuzzyFinder } = useExperimentalFeatures(features => ({
setupWizard: features.setupWizard,
// enable fuzzy finder by default unless it's explicitly disabled in settings
fuzzyFinder: features.fuzzyFinder ?? true,
}))
const isSetupWizardPage = setupWizard && location.pathname.startsWith(PageRoutes.SetupWizard)

// enable fuzzy finder by default unless it's explicitly disabled in settings
const fuzzyFinder = getExperimentalFeatures(props.settingsCascade.final).fuzzyFinder ?? true
const [isFuzzyFinderVisible, setFuzzyFinderVisible] = useState(false)
const userHistory = useUserHistory(isRepositoryRelatedPage)

Expand Down
16 changes: 10 additions & 6 deletions client/web/src/LegacyLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ import { PlatformContextProps } from '@sourcegraph/shared/src/platform/context'
import { Shortcut } from '@sourcegraph/shared/src/react-shortcuts'
import { Settings } from '@sourcegraph/shared/src/schema/settings.schema'
import { SearchContextProps } from '@sourcegraph/shared/src/search'
import { SettingsCascadeProps, SettingsSubjectCommonFields } from '@sourcegraph/shared/src/settings/settings'
import {
SettingsCascadeProps,
SettingsSubjectCommonFields,
useExperimentalFeatures,
} from '@sourcegraph/shared/src/settings/settings'
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
import { useTheme, Theme } from '@sourcegraph/shared/src/theme'
import { lazyComponent } from '@sourcegraph/shared/src/util/lazyComponent'
Expand Down Expand Up @@ -55,12 +59,10 @@ import { NotepadContainer } from './search/Notepad'
import { SearchQueryStateObserver } from './SearchQueryStateObserver'
import type { SiteAdminAreaRoute } from './site-admin/SiteAdminArea'
import type { SiteAdminSideBarGroups } from './site-admin/SiteAdminSidebar'
import { useExperimentalFeatures } from './stores'
import type { UserAreaRoute } from './user/area/UserArea'
import type { UserAreaHeaderNavItem } from './user/area/UserAreaHeader'
import type { UserSettingsAreaRoute } from './user/settings/UserSettingsArea'
import type { UserSettingsSidebarItems } from './user/settings/UserSettingsSidebar'
import { getExperimentalFeatures } from './util/get-experimental-features'
import { parseBrowserRepoURL } from './util/url'

import styles from './Layout.module.scss'
Expand Down Expand Up @@ -134,11 +136,13 @@ export const LegacyLayout: React.FunctionComponent<React.PropsWithChildren<Legac
const isSearchNotebookListPage = location.pathname === EnterprisePageRoutes.Notebooks
const isRepositoryRelatedPage = routeMatch === PageRoutes.RepoContainer ?? false

const { setupWizard } = useExperimentalFeatures()
const { setupWizard, fuzzyFinder } = useExperimentalFeatures(features => ({
setupWizard: features.setupWizard,
// enable fuzzy finder by default unless it's explicitly disabled in settings
fuzzyFinder: features.fuzzyFinder ?? true,
}))
const isSetupWizardPage = setupWizard && location.pathname.startsWith(PageRoutes.SetupWizard)

// enable fuzzy finder by default unless it's explicitly disabled in settings
const fuzzyFinder = getExperimentalFeatures(props.settingsCascade.final).fuzzyFinder ?? true
const [isFuzzyFinderVisible, setFuzzyFinderVisible] = useState(false)
const userHistory = useUserHistory(isRepositoryRelatedPage)

Expand Down
3 changes: 1 addition & 2 deletions client/web/src/LegacySourcegraphWebApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ import { SearchResultsCacheProvider } from './search/results/SearchResultsCacheP
import { GLOBAL_SEARCH_CONTEXT_SPEC } from './SearchQueryStateObserver'
import type { SiteAdminAreaRoute } from './site-admin/SiteAdminArea'
import type { SiteAdminSideBarGroups } from './site-admin/SiteAdminSidebar'
import { setQueryStateFromSettings, setExperimentalFeaturesFromSettings, useNavbarQueryState } from './stores'
import { setQueryStateFromSettings, useNavbarQueryState } from './stores'
import { eventLogger } from './tracking/eventLogger'
import type { UserAreaRoute } from './user/area/UserArea'
import type { UserAreaHeaderNavItem } from './user/area/UserAreaHeader'
Expand Down Expand Up @@ -208,7 +208,6 @@ export class LegacySourcegraphWebApp extends React.Component<
// Start with `undefined` while we don't know if the viewer is authenticated or not.
authenticatedUserSubject,
]).subscribe(([settingsCascade, authenticatedUser]) => {
setExperimentalFeaturesFromSettings(settingsCascade)
setQueryStateFromSettings(settingsCascade)
this.setState({
settingsCascade,
Expand Down
3 changes: 1 addition & 2 deletions client/web/src/SourcegraphWebApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import { SearchResultsCacheProvider } from './search/results/SearchResultsCacheP
import { GLOBAL_SEARCH_CONTEXT_SPEC } from './SearchQueryStateObserver'
import type { SiteAdminAreaRoute } from './site-admin/SiteAdminArea'
import type { SiteAdminSideBarGroups } from './site-admin/SiteAdminSidebar'
import { setQueryStateFromSettings, setExperimentalFeaturesFromSettings, useNavbarQueryState } from './stores'
import { setQueryStateFromSettings, useNavbarQueryState } from './stores'
import { eventLogger } from './tracking/eventLogger'
import type { UserAreaRoute } from './user/area/UserArea'
import type { UserAreaHeaderNavItem } from './user/area/UserAreaHeader'
Expand Down Expand Up @@ -230,7 +230,6 @@ export const SourcegraphWebApp: React.FC<SourcegraphWebAppProps> = props => {
subscriptions.add(
combineLatest([from(platformContext.settings), authenticatedUserSubject]).subscribe(
([settingsCascade, authenticatedUser]) => {
setExperimentalFeaturesFromSettings(settingsCascade)
setQueryStateFromSettings(settingsCascade)
setSettingsCascade(settingsCascade)
setResolvedAuthenticatedUser(authenticatedUser ?? null)
Expand Down
16 changes: 9 additions & 7 deletions client/web/src/codeintel/ReferencesPanel.test.tsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
import { within, fireEvent } from '@testing-library/react'
import { createPath } from 'react-router-dom'

import { SettingsProvider } from '@sourcegraph/shared/src/settings/settings'
import { MockedTestProvider, waitForNextApolloResponse } from '@sourcegraph/shared/src/testing/apollo'
import '@sourcegraph/shared/dev/mockReactVisibilitySensor'
import { renderWithBrandedContext } from '@sourcegraph/wildcard/src/testing'

import { setExperimentalFeaturesFromSettings } from '../stores'

import { ReferencesPanel } from './ReferencesPanel'
import { buildReferencePanelMocks, defaultProps } from './ReferencesPanel.mocks'

describe('ReferencesPanel', () => {
async function renderReferencesPanel() {
const { url, requestMocks } = buildReferencePanelMocks()

// TODO: we won't need to set experimental features explicitly once we cover CodeMirror side blob view with tests:
// https://github.com/sourcegraph/sourcegraph/issues/48049
setExperimentalFeaturesFromSettings(defaultProps.settingsCascade)

const result = renderWithBrandedContext(
<MockedTestProvider mocks={requestMocks}>
<ReferencesPanel {...defaultProps} />
<SettingsProvider
settingsCascade={{
final: { experimentalFeatures: { enableCodeMirrorFileView: false } },
subjects: [],
}}
>
<ReferencesPanel {...defaultProps} />
</SettingsProvider>
</MockedTestProvider>,
{ route: url }
)
Expand Down
3 changes: 1 addition & 2 deletions client/web/src/codeintel/ReferencesPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { ExtensionsControllerProps } from '@sourcegraph/shared/src/extensions/co
import { HighlightResponseFormat } from '@sourcegraph/shared/src/graphql-operations'
import { getModeFromPath } from '@sourcegraph/shared/src/languages'
import { PlatformContextProps } from '@sourcegraph/shared/src/platform/context'
import { SettingsCascadeProps } from '@sourcegraph/shared/src/settings/settings'
import { SettingsCascadeProps, useExperimentalFeatures } from '@sourcegraph/shared/src/settings/settings'
import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
import {
RepoSpec,
Expand Down Expand Up @@ -63,7 +63,6 @@ import { CodeMirrorBlob } from '../repo/blob/CodeMirrorBlob'
import { LegacyBlob } from '../repo/blob/LegacyBlob'
import * as BlobAPI from '../repo/blob/use-blob-store'
import { HoverThresholdProps } from '../repo/RepoContainer'
import { useExperimentalFeatures } from '../stores'
import { parseBrowserRepoURL } from '../util/url'

import { Location, LocationGroup, locationGroupQuality, buildRepoLocationGroups, RepoLocationGroup } from './location'
Expand Down
13 changes: 8 additions & 5 deletions client/web/src/components/WebStory.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { FC } from 'react'

import { RouterProvider, createMemoryRouter, MemoryRouterProps } from 'react-router-dom'

import { EMPTY_SETTINGS_CASCADE, SettingsProvider } from '@sourcegraph/shared/src/settings/settings'
import { MockedStoryProvider, MockedStoryProviderProps } from '@sourcegraph/shared/src/stories'
import { NOOP_TELEMETRY_SERVICE, TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
import { ThemeContext, ThemeSetting } from '@sourcegraph/shared/src/theme'
import { WildcardThemeContext } from '@sourcegraph/wildcard'
import { usePrependStyles, useStorybookTheme } from '@sourcegraph/wildcard/src/stories'

import { SourcegraphContext } from '../jscontext'
import { setExperimentalFeaturesForTesting } from '../stores/experimentalFeatures'

import { BreadcrumbSetters, BreadcrumbsProps, useBreadcrumbs } from './Breadcrumbs'

Expand Down Expand Up @@ -50,7 +50,6 @@ export const WebStory: FC<WebStoryProps> = ({
const breadcrumbSetters = useBreadcrumbs()

usePrependStyles('web-styles', webStyles)
setExperimentalFeaturesForTesting()

const routes = [
{
Expand All @@ -73,9 +72,13 @@ export const WebStory: FC<WebStoryProps> = ({
return (
<MockedStoryProvider mocks={mocks} useStrictMocking={useStrictMocking}>
<WildcardThemeContext.Provider value={{ isBranded: true }}>
<ThemeContext.Provider value={{ themeSetting: isLightTheme ? ThemeSetting.Light : ThemeSetting.Dark }}>
<RouterProvider router={router} />
</ThemeContext.Provider>
<SettingsProvider settingsCascade={EMPTY_SETTINGS_CASCADE}>
<ThemeContext.Provider
value={{ themeSetting: isLightTheme ? ThemeSetting.Light : ThemeSetting.Dark }}
>
<RouterProvider router={router} />
</ThemeContext.Provider>
</SettingsProvider>
</WildcardThemeContext.Provider>
</MockedStoryProvider>
)
Expand Down
2 changes: 1 addition & 1 deletion client/web/src/components/fuzzyFinder/FuzzyFinder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const FuzzyFinderContainer: React.FunctionComponent<FuzzyFinderContainerP
[setActiveTab, setIsVisible, toggleScope, setQuery]
)

const shortcuts = useFuzzyShortcuts(props.settingsCascade.final)
const shortcuts = useFuzzyShortcuts()

useEffect(() => {
if (isVisible) {
Expand Down
37 changes: 21 additions & 16 deletions client/web/src/components/fuzzyFinder/FuzzyFinderFeatureFlag.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,26 @@
import { SettingsExperimentalFeatures } from '@sourcegraph/shared/src/schema/settings.schema'
import { Settings, SettingsCascadeOrError } from '@sourcegraph/shared/src/settings/settings'
import type { SettingsExperimentalFeatures } from '@sourcegraph/shared/src/schema/settings.schema'
import { useExperimentalFeatures } from '@sourcegraph/shared/src/settings/settings'

import { getExperimentalFeatures } from '../../util/get-experimental-features'

export function getFuzzyFinderFeatureFlags(
finalSettings?: SettingsCascadeOrError<Settings>['final']
): Pick<
export function useFuzzyFinderFeatureFlags(): Pick<
SettingsExperimentalFeatures,
'fuzzyFinderAll' | 'fuzzyFinderActions' | 'fuzzyFinderRepositories' | 'fuzzyFinderSymbols' | 'fuzzyFinderNavbar'
> {
let { fuzzyFinderAll, fuzzyFinderActions, fuzzyFinderRepositories, fuzzyFinderSymbols, fuzzyFinderNavbar } =
getExperimentalFeatures(finalSettings)
// enable fuzzy finder unless it's explicitly disabled in settings
fuzzyFinderAll = fuzzyFinderAll ?? true
// Intentionally skip fuzzyFinderActions because we don't have enough actions implemented
// Intentionally skip fuzzyFinderNavbar because the navbar is already too busy and we need to explore alternative solutions for the discoverability problem
fuzzyFinderRepositories = fuzzyFinderAll || fuzzyFinderRepositories
fuzzyFinderSymbols = fuzzyFinderAll || fuzzyFinderSymbols
return { fuzzyFinderAll, fuzzyFinderActions, fuzzyFinderRepositories, fuzzyFinderSymbols, fuzzyFinderNavbar }
return useExperimentalFeatures(features => {
let { fuzzyFinderAll, fuzzyFinderActions, fuzzyFinderRepositories, fuzzyFinderSymbols, fuzzyFinderNavbar } =
features

// enable fuzzy finder unless it's explicitly disabled in settings
fuzzyFinderAll = fuzzyFinderAll ?? true
// Intentionally skip fuzzyFinderActions because we don't have enough actions implemented
// Intentionally skip fuzzyFinderNavbar because the navbar is already too busy and we need to explore alternative solutions for the discoverability problem
fuzzyFinderRepositories = fuzzyFinderAll || fuzzyFinderRepositories
fuzzyFinderSymbols = fuzzyFinderAll || fuzzyFinderSymbols
return {
fuzzyFinderAll,
fuzzyFinderActions,
fuzzyFinderRepositories,
fuzzyFinderSymbols,
fuzzyFinderNavbar,
}
})
}
12 changes: 3 additions & 9 deletions client/web/src/components/fuzzyFinder/FuzzyShortcuts.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import { useMemo } from 'react'

import { KeyboardShortcut } from '@sourcegraph/shared/src/keyboardShortcuts'
import { useKeyboardShortcut } from '@sourcegraph/shared/src/keyboardShortcuts/useKeyboardShortcut'
import { Settings, SettingsCascadeOrError } from '@sourcegraph/shared/src/settings/settings'

import { getFuzzyFinderFeatureFlags } from './FuzzyFinderFeatureFlag'
import { useFuzzyFinderFeatureFlags } from './FuzzyFinderFeatureFlag'
import { FuzzyTabKey } from './FuzzyTabs'

interface FuzzyShortcut {
Expand All @@ -13,11 +10,8 @@ interface FuzzyShortcut {
shortcut: KeyboardShortcut | undefined
}

export function useFuzzyShortcuts(settings?: SettingsCascadeOrError<Settings>['final']): FuzzyShortcut[] {
const { fuzzyFinderActions, fuzzyFinderRepositories, fuzzyFinderSymbols } = useMemo(
() => getFuzzyFinderFeatureFlags(settings),
[settings]
)
export function useFuzzyShortcuts(): FuzzyShortcut[] {
const { fuzzyFinderActions, fuzzyFinderRepositories, fuzzyFinderSymbols } = useFuzzyFinderFeatureFlags()
const fuzzyFinderShortcut = useKeyboardShortcut('fuzzyFinder')
const fuzzyFinderActionsShortcut = useKeyboardShortcut('fuzzyFinderActions')
const fuzzyFinderReposShortcut = useKeyboardShortcut('fuzzyFinderRepos')
Expand Down
4 changes: 2 additions & 2 deletions client/web/src/components/fuzzyFinder/FuzzyTabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { UserHistory } from '../useUserHistory'

import { createActionsFSM, getAllFuzzyActions } from './FuzzyActions'
import { FuzzyFiles, FuzzyRepoFiles } from './FuzzyFiles'
import { getFuzzyFinderFeatureFlags } from './FuzzyFinderFeatureFlag'
import { useFuzzyFinderFeatureFlags } from './FuzzyFinderFeatureFlag'
import { FuzzyFSM } from './FuzzyFsm'
import { FuzzyRepoRevision } from './FuzzyRepoRevision'
import { FuzzyRepos } from './FuzzyRepos'
Expand Down Expand Up @@ -252,7 +252,7 @@ export function useFuzzyState(props: FuzzyTabsProps): FuzzyState {
repoRevisionRef.current = repoRevision

const { fuzzyFinderAll, fuzzyFinderActions, fuzzyFinderRepositories, fuzzyFinderSymbols } =
getFuzzyFinderFeatureFlags(props.settingsCascade.final)
useFuzzyFinderFeatureFlags()

const [activeTab, setActiveTab] = useState<FuzzyTabKey>('all')

Expand Down
Loading