From f8bed47e3eea183f6ebcbf4df9b1756b98b633ea Mon Sep 17 00:00:00 2001 From: Vova Kulikov Date: Tue, 1 Jun 2021 19:40:29 +0300 Subject: [PATCH] Add support of non existent in a timeframe data-points (#21288) * Add support of non-existent in time-frame data-points * Add story for line chart with missing data --- .../web/src/insights/core/analytics.test.ts | 2 +- .../backend/api/get-search-insight-content.ts | 82 ++++++++++---- .../ChartViewContent.story.tsx | 47 ++++++++ .../ChartViewContent/charts/line/colors.ts | 4 - .../line/components/LineChartContent.tsx | 6 +- .../line/components/NonActiveBackground.tsx | 107 ++++++++++++++++++ .../charts/line/components/TooltipContent.tsx | 2 +- .../ChartViewContent/charts/line/constants.ts | 28 +++++ .../charts/line/helpers/generate-accessors.ts | 5 +- .../charts/line/helpers/use-scales.ts | 2 +- .../ChartViewContent/charts/line/types.ts | 4 +- package.json | 1 + yarn.lock | 10 ++ 13 files changed, 266 insertions(+), 34 deletions(-) delete mode 100644 client/web/src/views/ChartViewContent/charts/line/colors.ts create mode 100644 client/web/src/views/ChartViewContent/charts/line/components/NonActiveBackground.tsx create mode 100644 client/web/src/views/ChartViewContent/charts/line/constants.ts diff --git a/client/web/src/insights/core/analytics.test.ts b/client/web/src/insights/core/analytics.test.ts index 7d3331814b9..e9a7a902e04 100644 --- a/client/web/src/insights/core/analytics.test.ts +++ b/client/web/src/insights/core/analytics.test.ts @@ -1,5 +1,5 @@ -import { Settings, SettingsCascade } from '@sourcegraph/shared/out/src/settings/settings' import { IOrg } from '@sourcegraph/shared/src/graphql/schema' +import { Settings, SettingsCascade } from '@sourcegraph/shared/src/settings/settings' import { authUser } from '@sourcegraph/web/src/search/panels/utils' import { AuthenticatedUser } from '../../auth' diff --git a/client/web/src/insights/core/backend/api/get-search-insight-content.ts b/client/web/src/insights/core/backend/api/get-search-insight-content.ts index 7ec8237139f..fe3957de427 100644 --- a/client/web/src/insights/core/backend/api/get-search-insight-content.ts +++ b/client/web/src/insights/core/backend/api/get-search-insight-content.ts @@ -4,27 +4,55 @@ import { defer } from 'rxjs' import { retry } from 'rxjs/operators' import type { LineChartContent } from 'sourcegraph' +import { EMPTY_DATA_POINT_VALUE } from '../../../../views/ChartViewContent/charts/line/constants' import { fetchRawSearchInsightResults, fetchSearchInsightCommits } from '../requests/fetch-search-insight' import { SearchInsightSettings } from '../types' +interface RepoCommit { + date: Date + commit: string + repo: string +} + +interface InsightSeriesData { + date: number + [seriesName: string]: number +} + /** * This logic is a copy of fetch logic of search-based code insight extension. * See https://github.com/sourcegraph/sourcegraph-search-insights/blob/master/src/search-insights.ts * In order to have live preview for creation UI we had to copy this logic from * extension. - * * */ export async function getSearchInsightContent(insight: SearchInsightSettings): Promise> { const step = insight.step || { days: 1 } const { repositories: repos } = insight const dates = getDaysToQuery(step) - // Get commits to search for each day + const data: InsightSeriesData[] = [] + + for (const date of dates) { + const dataIndex = dates.indexOf(date) + + // Initialize data series object by all dates. + data[dataIndex] = { + date: date.getTime(), + // Initialize all series with EMPTY_DATA_POINT_VALUE + ...Object.fromEntries(insight.series.map(series => [series.name, EMPTY_DATA_POINT_VALUE])), + } + } + + // Get commits to search for each day. const repoCommits = ( await Promise.all( repos.map(async repo => (await determineCommitsToSearch(dates, repo)).map(commit => ({ repo, ...commit }))) ) - ).flat() + ) + .flat() + // For commit which we couldn't find we should not run search API request. + // Instead of it we will use just EMPTY_DATA_POINT_VALUE + .filter(commitData => commitData.commit !== null) as RepoCommit[] const searchQueries = insight.series.flatMap(({ query, name }) => repoCommits.map(({ date, repo, commit }) => ({ @@ -35,35 +63,36 @@ export async function getSearchInsightContent(insight: SearchInsightSettings): P query: `repo:^${escapeRegExp(repo)}$@${commit} ${query} count:99999`, })) ) + const rawSearchResults = await defer(() => fetchRawSearchInsightResults(searchQueries.map(search => search.query))) - // The bulk search may timeout, but a retry is then likely faster because caches are warm + // The bulk search may timeout, but a retry is then likely faster + // because caches are warm .pipe(retry(3)) .toPromise() const searchResults = Object.entries(rawSearchResults).map(([field, result]) => { const index = +field.slice('search'.length) const query = searchQueries[index] + return { ...query, result } }) - const data: { - date: number - [seriesName: string]: number - }[] = [] + // Merge initial data and search API data for (const { name, date, result } of searchResults) { const dataKey = name const dataIndex = dates.indexOf(date) - const object = - data[dataIndex] ?? - (data[dataIndex] = { - date: date.getTime(), - // Initialize all series to 0 - ...Object.fromEntries(insight.series.map(series => [series.name, 0])), - }) - // Sum across repos + const object = data[dataIndex] + const countForRepo = result?.results.matchCount - object[dataKey] += countForRepo ?? 0 + // If we got some data that means for this data points we got + // a valid commit in a git history therefore we need to write + // some data to this series. + if (object[dataKey] === EMPTY_DATA_POINT_VALUE) { + object[dataKey] = countForRepo ?? 0 + } else { + object[dataKey] += countForRepo ?? 0 + } } return { @@ -95,7 +124,12 @@ export async function getSearchInsightContent(insight: SearchInsightSettings): P } } -async function determineCommitsToSearch(dates: Date[], repo: string): Promise<{ date: Date; commit: string }[]> { +interface SearchCommit { + date: Date + commit: string | null +} + +async function determineCommitsToSearch(dates: Date[], repo: string): Promise { const commitQueries = dates.map(date => { const before = formatISO(date) return `repo:^${escapeRegExp(repo)}$ type:commit before:${before} count:1` @@ -103,25 +137,29 @@ async function determineCommitsToSearch(dates: Date[], repo: string): Promise<{ const commitResults = await fetchSearchInsightCommits(commitQueries).toPromise() - const commitOids = Object.entries(commitResults).map(([name, search], index) => { + return Object.entries(commitResults).map(([name, search], index) => { const index_ = +name.slice('search'.length) + const date = dates[index_] if (index_ !== index) { throw new Error(`Expected field ${name} to be at index ${index_} of object keys`) } if (search.results.results.length === 0) { - throw new Error(`No result for ${commitQueries[index_]}`) + console.warn(`No result for ${commitQueries[index_]}`) + + return { commit: null, date } } const commit = (search?.results.results[0]).commit // Sanity check const commitDate = commit.committer && new Date(commit.committer.date) - const date = dates[index_] + if (!commitDate) { throw new Error(`Expected commit to have committer: \`${commit.oid}\``) } + if (isAfter(commitDate, date)) { throw new Error( `Expected commit \`${commit.oid}\` to be before ${formatISO(date)}, but was after: ${formatISO( @@ -132,8 +170,6 @@ async function determineCommitsToSearch(dates: Date[], repo: string): Promise<{ return { commit: commit.oid, date } }) - - return commitOids } const NUMBER_OF_CHART_POINTS = 7 diff --git a/client/web/src/views/ChartViewContent/ChartViewContent.story.tsx b/client/web/src/views/ChartViewContent/ChartViewContent.story.tsx index 633ef7c30a2..0b6cd6fbb78 100644 --- a/client/web/src/views/ChartViewContent/ChartViewContent.story.tsx +++ b/client/web/src/views/ChartViewContent/ChartViewContent.story.tsx @@ -69,6 +69,53 @@ add('Line chart', () => ( /> )) +add('Line chart with missing data', () => ( + +)) + add('Bar chart', () => ( (props: LineChartContentPr onPointerUp={handlePointerUp} accessibilityLabel="Line chart content" > + {/* eslint-disable-next-line jsx-a11y/aria-role */} diff --git a/client/web/src/views/ChartViewContent/charts/line/components/NonActiveBackground.tsx b/client/web/src/views/ChartViewContent/charts/line/components/NonActiveBackground.tsx new file mode 100644 index 00000000000..f329fa1c59e --- /dev/null +++ b/client/web/src/views/ChartViewContent/charts/line/components/NonActiveBackground.tsx @@ -0,0 +1,107 @@ +import { PatternLines } from '@visx/pattern' +import { DataContext } from '@visx/xychart' +import React, { ReactElement, useContext, useMemo } from 'react' +import { LineChartContent } from 'sourcegraph' + +import { Accessors, YAccessor } from '../types' + +const patternId = 'xy-chart-pattern' + +function getFirstNonNullablePoint(data: Datum[], accessor: YAccessor): Datum | undefined { + return data.find(datum => accessor(datum) !== null) +} + +export interface NonActiveBackgroundProps { + data: Datum[] + series: LineChartContent['series'] + accessors: Accessors +} + +/** + * Displays custom pattern background for area where we don't have any data points. + * Example: + * ┌──────────────────────────────────┐ + * │`````````````````` │ 10 + * │`````````````````` │ + * │`````````````````` ▼ │ 9 + * │`````````````````` │ + * │`````````````````` ▼ │ 8 + * │`````````````````` │ + * │`````````````````` ▼ │ 7 + * │`````````````````` ▼ │ + * │`````````````````` │ 6 + * │`````````````````` │ + * │`````````````````` │ 5 + * └──────────────────────────────────┘ + * Where ` is a non-active background + * */ +export function NonActiveBackground( + props: NonActiveBackgroundProps +): ReactElement | null { + const { data, series, accessors } = props + const { theme, margin, width, height, innerHeight, xScale } = useContext(DataContext) + + const backgroundWidth = useMemo(() => { + // For non active background's width we need to find first non nullable element + const firstNonNullablePoints: (Datum | undefined)[] = [] + + for (const line of series) { + const lineKey = line.dataKey + const lineYAccessor = accessors.y[lineKey] + + firstNonNullablePoints.push(getFirstNonNullablePoint(data, lineYAccessor)) + } + + const lastNullablePointX = firstNonNullablePoints.reduce((xCoord, datum) => { + // In case if the first non nullable element is the first element + // of data that means we don't need to render non active background. + if (!datum || datum === data[0]) { + return null + } + + // Get x point from datum by x accessor + return accessors.x(datum) + }, null as Date | number | null) + + // If we didn't find any non-nullable elements we don't need to render + // non-active background. + if (!lastNullablePointX) { + return 0 + } + + // Convert x value of first non nullable point to reals svg coordinate + const xValue = xScale?.(lastNullablePointX) ?? 0 + + return +xValue + }, [data, series, accessors, xScale]) + + // Early return values not available in context or we don't need render + // non active background. + if (!backgroundWidth || !width || !height || !margin || !theme) { + return null + } + + return ( + <> + + + + + + + ) +} diff --git a/client/web/src/views/ChartViewContent/charts/line/components/TooltipContent.tsx b/client/web/src/views/ChartViewContent/charts/line/components/TooltipContent.tsx index 9456a8ee922..672a8a41731 100644 --- a/client/web/src/views/ChartViewContent/charts/line/components/TooltipContent.tsx +++ b/client/web/src/views/ChartViewContent/charts/line/components/TooltipContent.tsx @@ -3,7 +3,7 @@ import classNames from 'classnames' import React, { ReactElement } from 'react' import { LineChartContent } from 'sourcegraph' -import { DEFAULT_LINE_STROKE } from '../colors' +import { DEFAULT_LINE_STROKE } from '../constants' import { Accessors } from '../types' export interface TooltipContentProps extends RenderTooltipParams { diff --git a/client/web/src/views/ChartViewContent/charts/line/constants.ts b/client/web/src/views/ChartViewContent/charts/line/constants.ts new file mode 100644 index 00000000000..34f0aa99ed6 --- /dev/null +++ b/client/web/src/views/ChartViewContent/charts/line/constants.ts @@ -0,0 +1,28 @@ +/** + * Default value for line color in case if we didn't get color for line from content config. + */ +export const DEFAULT_LINE_STROKE = 'var(--color-bg-5)' + +/** + * Visx xy-chart supports data series with missing. To show the + * points but not the very beginning of the chart we should use + * this default empty value. See example below points that have + * EMPTY_DATA_POINT_VALUE value haven't been rendered instead of + * that we rendered non active background (``` area) + * + * ┌──────────────────────────────────┐ ┌──────────────────────────────────┐ + * │`````````````````` │ 10 │ ```````` │ 10 + * │`````````````````` │ │ ▼ ```````` │ + * │`````````````````` ▼ │ 9 │ ```````` ▼ │ 9 + * │`````````````````` │ │ ▼ ```````` │ + * │`````````````````` ▼ │ 8 │ ```````` ▼ │ 8 + * │`````````````````` │ │ ```````` │ + * │`````````````````` ▼ │ 7 │ ```````` ▼ │ 7 + * │`````````````````` ▼ │ │ ▼ ```````` ▼ │ + * │`````````````````` │ 6 │ ```````` │ 6 + * │`````````````````` │ │ ```````` │ + * │`````````````````` │ 5 │ ```````` │ 5 + * └──────────────────────────────────┘ └──────────────────────────────────┘ + * + */ +export const EMPTY_DATA_POINT_VALUE = null diff --git a/client/web/src/views/ChartViewContent/charts/line/helpers/generate-accessors.ts b/client/web/src/views/ChartViewContent/charts/line/helpers/generate-accessors.ts index e01cb8b40b8..bf02d157f05 100644 --- a/client/web/src/views/ChartViewContent/charts/line/helpers/generate-accessors.ts +++ b/client/web/src/views/ChartViewContent/charts/line/helpers/generate-accessors.ts @@ -1,5 +1,6 @@ import { ChartAxis } from 'sourcegraph' +import { EMPTY_DATA_POINT_VALUE } from '../constants' import { Accessors } from '../types' /** @@ -26,7 +27,9 @@ export function generateAccessors( // fix that when we will have a value type for LineChartContent generic const key = (dataKey as unknown) as keyof Datum - accessors[key] = data => +data[dataKey] + // If we get EMPTY_DATA_POINT_VALUE we should omit the '+' number casting + accessors[key] = data => + data[dataKey] === EMPTY_DATA_POINT_VALUE ? EMPTY_DATA_POINT_VALUE : +data[dataKey] return accessors }, {} as Record any>), diff --git a/client/web/src/views/ChartViewContent/charts/line/helpers/use-scales.ts b/client/web/src/views/ChartViewContent/charts/line/helpers/use-scales.ts index e50252bce74..c95310c6114 100644 --- a/client/web/src/views/ChartViewContent/charts/line/helpers/use-scales.ts +++ b/client/web/src/views/ChartViewContent/charts/line/helpers/use-scales.ts @@ -43,7 +43,7 @@ interface UseScalesOutput { } /** - * Hook to generate d3 scales according chart configuration. + * Hook to generate d3 scales according to chart configuration. */ export function useScales(props: UseScalesProps): UseScalesOutput { const { config, accessors, width, height, data } = props diff --git a/client/web/src/views/ChartViewContent/charts/line/types.ts b/client/web/src/views/ChartViewContent/charts/line/types.ts index 5ae1d3d5325..4c3dd4e8887 100644 --- a/client/web/src/views/ChartViewContent/charts/line/types.ts +++ b/client/web/src/views/ChartViewContent/charts/line/types.ts @@ -1,9 +1,11 @@ import { MouseEvent } from 'react' +export type YAccessor = (data: Datum) => any + /** Accessors map for getting values for x and y axes from datum object */ export interface Accessors { x: (d: Datum) => Date | number - y: Record any> + y: Record> } export interface DatumZoneClickEvent { diff --git a/package.json b/package.json index 71e0a2d9a12..3bbc8c72395 100644 --- a/package.json +++ b/package.json @@ -315,6 +315,7 @@ "@visx/grid": "^1.7.0", "@visx/group": "^1.7.0", "@visx/mock-data": "^1.7.0", + "@visx/pattern": "^1.7.0", "@visx/scale": "^1.7.0", "@visx/xychart": "^1.7.3", "bloomfilter": "^0.0.18", diff --git a/yarn.lock b/yarn.lock index 51e1212b5ff..eb5dc762088 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5050,6 +5050,16 @@ "@types/d3-random" "^2.2.0" d3-random "^2.2.2" +"@visx/pattern@^1.7.0": + version "1.7.0" + resolved "https://registry.npmjs.org/@visx/pattern/-/pattern-1.7.0.tgz#4b4392b976f57592d836b2055dc606aa7b0192d9" + integrity sha512-uIrGDtm7NqDfZZneRZ/DRdZA0nli/13sIUcxQ0oRL70b6ul+epNGiBVR8pvrgMhZ+SW3b05xhfEDnfnl2CqFQw== + dependencies: + "@types/classnames" "^2.2.9" + "@types/react" "*" + classnames "^2.2.5" + prop-types "^15.5.10" + "@visx/point@1.7.0": version "1.7.0" resolved "https://registry.npmjs.org/@visx/point/-/point-1.7.0.tgz#1df3c3425eae464f498473bcdda2fcae05c8ecbe"