From 72c00d397a3b06123db4a33e5e29e847cfe6639d Mon Sep 17 00:00:00 2001 From: Peter Guy Date: Thu, 27 Jun 2024 12:24:51 -0700 Subject: [PATCH] fix(search): VSCode Search extension: bring back matched lines in search results. (#63524) Due to changes in the code base, the search extension code when run from `main` shows file names only in the search results page - no matches in the files. This is a regression from the behavior in the deployed extension. Deployed extension: Screenshot 2024-06-27 at 10 04 37 Running from `main`: Screenshot 2024-06-27 at 10 11 17 Turns out the reason is because some shared code expects chunk matches, while the search queries were all returning line matches. Added support for line matches in the shared code, and then fixed an issue with the search results display not keeping up with `MatchGroups`. ## Test plan Build and run locally. ### Build ``` git switch peterguy/vscode-bring-back-matched-lines-in-search-results cd client/vscode pnpm run build ``` ### Run - Launch extension in VSCode: open the `Run and Debug` sidebar view in VS Code, then select `Launch VS Code Extension` from the dropdown menu. - Run a search using the search bar. - See that the results contain matched lines in the files and not just a list of file names. Compare to the currently-deployed extension - the search results should look generally the same. --- .../FileContentSearchResult.test.tsx | 1 + .../components/FileContentSearchResult.tsx | 28 ++++- .../search-panel/alias/FileMatchChildren.tsx | 100 +++++------------- 3 files changed, 51 insertions(+), 78 deletions(-) diff --git a/client/branded/src/search-ui/components/FileContentSearchResult.test.tsx b/client/branded/src/search-ui/components/FileContentSearchResult.test.tsx index ed4ec1c2519..2ea3c350068 100644 --- a/client/branded/src/search-ui/components/FileContentSearchResult.test.tsx +++ b/client/branded/src/search-ui/components/FileContentSearchResult.test.tsx @@ -37,6 +37,7 @@ describe('FileContentSearchResult', () => { expect(getByTestId(container, 'result-container')).toBeVisible() expect(getAllByTestId(container, 'result-container').length).toBe(1) expect(getAllByTestId(container, 'file-search-result').length).toBe(1) + expect(getAllByTestId(container, 'file-match-children').length).toBe(1) }) // TODO: add test that the collapse shows if there are too many results diff --git a/client/branded/src/search-ui/components/FileContentSearchResult.tsx b/client/branded/src/search-ui/components/FileContentSearchResult.tsx index 7458e94628f..590544b1894 100644 --- a/client/branded/src/search-ui/components/FileContentSearchResult.tsx +++ b/client/branded/src/search-ui/components/FileContentSearchResult.tsx @@ -21,6 +21,7 @@ import { getFileMatchUrl, getRepositoryUrl, getRevision, + type LineMatch, } from '@sourcegraph/shared/src/search/stream' import { useSettings, type SettingsCascadeProps } from '@sourcegraph/shared/src/settings/settings' import type { TelemetryV2Props } from '@sourcegraph/shared/src/telemetry' @@ -112,10 +113,7 @@ export const FileContentSearchResult: React.FunctionComponent settings?.['search.contextLines'] ?? 1, [settings]) - const unhighlightedGroups: MatchGroup[] = useMemo( - () => reranker(result.chunkMatches?.map(chunkToMatchGroup) ?? []), - [result, reranker] - ) + const unhighlightedGroups: MatchGroup[] = useMemo(() => reranker(matchesToMatchGroups(result)), [result, reranker]) const [expandedGroups, setExpandedGroups] = useState(unhighlightedGroups) const collapsedGroups = truncateGroups(expandedGroups, 5, contextLines) @@ -299,6 +297,12 @@ function countHighlightRanges(groups: MatchGroup[]): number { return groups.reduce((count, group) => count + group.matches.length, 0) } +function matchesToMatchGroups(result: ContentMatch): MatchGroup[] { + return [ + ...(result.lineMatches?.map(lineToMatchGroup) ?? []), + ...(result.chunkMatches?.map(chunkToMatchGroup) ?? []), + ] +} function chunkToMatchGroup(chunk: ChunkMatch): MatchGroup { const matches = chunk.ranges.map(range => ({ startLine: range.start.line, @@ -315,3 +319,19 @@ function chunkToMatchGroup(chunk: ChunkMatch): MatchGroup { endLine: chunk.contentStart.line + plaintextLines.length, } } + +function lineToMatchGroup(line: LineMatch): MatchGroup { + const matches = line.offsetAndLengths.map(offsetAndLength => ({ + startLine: line.lineNumber, + startCharacter: offsetAndLength[0], + endLine: line.lineNumber, + endCharacter: offsetAndLength[0] + offsetAndLength[1], + })) + return { + plaintextLines: [line.line], + highlightedHTMLRows: undefined, // populated lazily + matches, + startLine: line.lineNumber, + endLine: line.lineNumber + 1, // the matches support `endLine` == `startLine`, but MatchGroup requires `endLine` > `startLine` + } +} diff --git a/client/vscode/src/webview/search-panel/alias/FileMatchChildren.tsx b/client/vscode/src/webview/search-panel/alias/FileMatchChildren.tsx index 653df547731..496dc7016e6 100644 --- a/client/vscode/src/webview/search-panel/alias/FileMatchChildren.tsx +++ b/client/vscode/src/webview/search-panel/alias/FileMatchChildren.tsx @@ -3,14 +3,14 @@ import React, { type MouseEvent, type KeyboardEvent, useCallback } from 'react' import classNames from 'classnames' import type * as H from 'history' import type { Observable } from 'rxjs' -import { map } from 'rxjs/operators' +import { CodeExcerpt } from '@sourcegraph/branded/src/search-ui/components' import type { HoverMerged } from '@sourcegraph/client-api' import type { Hoverifier } from '@sourcegraph/codeintellify' import { SourcegraphURL } from '@sourcegraph/common' import type { ActionItemAction } from '@sourcegraph/shared/src/actions/ActionItem' import type { FetchFileParameters } from '@sourcegraph/shared/src/backend/file' -import type { MatchGroupMatch } from '@sourcegraph/shared/src/components/ranking/PerFileResultRanking' +import type { MatchGroup } from '@sourcegraph/shared/src/components/ranking/PerFileResultRanking' import type { Controller as ExtensionsController } from '@sourcegraph/shared/src/extensions/controller' import type { HoverContext } from '@sourcegraph/shared/src/hover/HoverOverlay.types' import { @@ -24,31 +24,10 @@ import { SymbolKind } from '@sourcegraph/shared/src/symbols/SymbolKind' import type { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService' import { Button, Code } from '@sourcegraph/wildcard' -import type { HighlightLineRange } from '../../../graphql-operations' -import { CodeExcerpt } from '../components/CodeExcerpt' import { useOpenSearchResultsContext } from '../MatchHandlersContext' import styles from './FileMatchChildren.module.scss' -export interface MatchGroup { - blobLines?: string[] - - // The matches in this group to display. - matches: MatchGroupMatch[] - - // The 1-based position of where the first match in the group. - position: { - line: number - character: number - } - - // The 0-based start line of the group (inclusive.) - startLine: number - - // The 0-based end line of the group (exclusive.) - endLine: number -} - interface FileMatchProps extends SettingsCascadeProps, TelemetryProps { location?: H.Location result: ContentMatch | SymbolMatch | PathMatch @@ -162,44 +141,29 @@ function navigateToFileOnMiddleMouseButtonClick(event: MouseEvent): } } +/** + * Convert a MatchGroup to a "position", which is just a tuple of line and character. + * Supports the "old" way of handling line-based matches without requiring a lot of redesign downstream. + * Limitation is that the `line` is taken from the first match in the group; other matches are ignored. + * @param group a MatchGroup, which could have multiple matches in it + * @returns a tuple of line and character + */ +function groupToPosition(group: MatchGroup): { line: number; character: number } { + return { + line: group.matches.length > 0 ? group.matches[0].startLine : group.startLine, + character: group.matches.length > 0 ? group.matches[0].startCharacter : 0, + } +} + export const FileMatchChildren: React.FunctionComponent> = props => { - const { result, grouped, fetchHighlightedFileLineRanges, telemetryService } = props + const { result, grouped } = props + + console.log('FILE_MATCH', { result, grouped }) const { openFile, openSymbol } = useOpenSearchResultsContext() - const fetchHighlightedFileRangeLines = React.useCallback( - (startLine: number, endLine: number) => { - const startTime = Date.now() - return fetchHighlightedFileLineRanges( - { - repoName: result.repository, - commitID: result.commit || '', - filePath: result.path, - disableTimeout: false, - ranges: grouped.map( - (group): HighlightLineRange => ({ - startLine: group.startLine, - endLine: group.endLine, - }) - ), - }, - false - ).pipe( - map(lines => { - telemetryService.log( - 'search.latencies.frontend.code-load', - { durationMs: Date.now() - startTime }, - { durationMs: Date.now() - startTime } - ) - return lines[grouped.findIndex(group => group.startLine === startLine && group.endLine === endLine)] - }) - ) - }, - [result, fetchHighlightedFileLineRanges, grouped, telemetryService] - ) - const createCodeExcerptLink = (group: MatchGroup): string => - SourcegraphURL.from(getFileMatchUrl(result)).setLineRange(group.position).toString() + SourcegraphURL.from(getFileMatchUrl(result)).setLineRange(groupToPosition(group)).toString() /** * This handler implements the logic to simulate the click/keyboard @@ -285,9 +249,7 @@ export const FileMatchChildren: React.FunctionComponent {grouped.map((group, index) => (
- navigateToFile(event, { - line: group.position.line, - character: group.position.character, - }) - } + onClick={event => navigateToFile(event, groupToPosition(group))} onMouseUp={navigateToFileOnMiddleMouseButtonClick} - onKeyDown={event => - navigateToFile(event, { - line: group.position.line, - character: group.position.character, - }) - } + onKeyDown={event => navigateToFile(event, groupToPosition(group))} data-testid="file-match-children-item" tabIndex={0} role="link" >