Refactor CodePaths and FileCodeSnippet components

This refactors the CodePaths and FileCodeSnippet components to be more readable and in style with the rest of the "new" components. It does the following: - Remove uses of the `style` and `sx` props; replace it by using `styled-components` instead - Remove uses of Primer icons - Split out the components into multiple files - Change the colors of the severity to match VSCode colors (and make them themable) I haven't removed the use of the Primer `Overlay` component yet, since this component seems to do quite a lot and the VSCode WebView UI Toolkit doesn't have a replacement for it.
2022-09-21 11:29:34 +02:00
parent bcf70c6962
commit 3817133b5b
5 changed files with 366 additions and 222 deletions
--- a/extensions/ql-vscode/src/stories/remote-queries/CodePaths.stories.tsx
+++ b/extensions/ql-vscode/src/stories/remote-queries/CodePaths.stories.tsx
@@ -3,7 +3,7 @@ import React from 'react';
 import { ComponentStory, ComponentMeta } from '@storybook/react';
 import { ThemeProvider } from '@primer/react';

-import CodePaths from '../../view/remote-queries/CodePaths';
+import { CodePaths } from '../../view/remote-queries/CodePaths';
 import type { CodeFlow } from '../../remote-queries/shared/analysis-result';

 export default {
@@ -112,8 +112,8 @@ PowerShell.args = {
  message: {
    tokens: [
      {
-        type: 'text',
-        t: 'This zip file may have a dangerous path'
+        t: 'text',
+        text: 'This zip file may have a dangerous path'
      }
    ]
  },
--- a/extensions/ql-vscode/src/stories/remote-queries/FileCodeSnippet.stories.tsx
+++ b/extensions/ql-vscode/src/stories/remote-queries/FileCodeSnippet.stories.tsx
@@ -2,7 +2,7 @@ import React from 'react';

 import { ComponentStory, ComponentMeta } from '@storybook/react';

-import FileCodeSnippet from '../../view/remote-queries/FileCodeSnippet';
+import { FileCodeSnippet } from '../../view/remote-queries/FileCodeSnippet';

 export default {
  title: 'File Code Snippet',
--- a/extensions/ql-vscode/src/view/remote-queries/AnalysisAlertResult.tsx
+++ b/extensions/ql-vscode/src/view/remote-queries/AnalysisAlertResult.tsx
@@ -1,7 +1,7 @@
 import * as React from 'react';
 import { AnalysisAlert } from '../../remote-queries/shared/analysis-result';
-import CodePaths from './CodePaths';
-import FileCodeSnippet from './FileCodeSnippet';
+import { CodePaths } from './CodePaths';
+import { FileCodeSnippet } from './FileCodeSnippet';

 const AnalysisAlertResult = ({ alert }: { alert: AnalysisAlert }) => {
  const showPathsLink = alert.codeFlows.length > 0;
--- a/extensions/ql-vscode/src/view/remote-queries/CodePaths.tsx
+++ b/extensions/ql-vscode/src/view/remote-queries/CodePaths.tsx
@@ -1,12 +1,13 @@
-import { XCircleIcon } from '@primer/octicons-react';
-import { Overlay } from '@primer/react';
-import { VSCodeDropdown, VSCodeLink, VSCodeOption, VSCodeTag } from '@vscode/webview-ui-toolkit/react';
 import * as React from 'react';
-import { ChangeEvent, useRef, useState } from 'react';
+import { ChangeEvent, SetStateAction, useCallback, useRef, useState } from 'react';
 import styled from 'styled-components';
-import { CodeFlow, AnalysisMessage, ResultSeverity } from '../../remote-queries/shared/analysis-result';
+import { VSCodeDropdown, VSCodeLink, VSCodeOption, VSCodeTag } from '@vscode/webview-ui-toolkit/react';
+
+import { Overlay } from '@primer/react';
+
+import { AnalysisMessage, CodeFlow, ResultSeverity, ThreadFlow } from '../../remote-queries/shared/analysis-result';
 import { SectionTitle, VerticalSpace } from '../common';
-import FileCodeSnippet from './FileCodeSnippet';
+import { FileCodeSnippet } from './FileCodeSnippet';

 const StyledCloseButton = styled.button`
  position: absolute;
@@ -15,13 +16,14 @@ const StyledCloseButton = styled.button`
  background-color: var(--vscode-editor-background);
  color: var(--vscode-editor-foreground);
  border: none;
+  cursor: pointer;
+
  &:focus-visible {
    outline: none
  }
 `;

 const OverlayContainer = styled.div`
-  padding: 1em;
  height: 100%;
  width: 100%;
  padding: 2em;
@@ -34,144 +36,246 @@ const OverlayContainer = styled.div`
 `;

 const CloseButton = ({ onClick }: { onClick: () => void }) => (
-  <StyledCloseButton onClick={onClick} tabIndex={-1} >
-    <XCircleIcon size={24} />
+  <StyledCloseButton onClick={onClick} tabIndex={-1}>
+    <span className="codicon codicon-chrome-close" />
  </StyledCloseButton>
 );

+const PathsContainer = styled.div`
+  display: flex;
+  justify-content: center;
+  align-items: center;
+`;
+
+const PathDetailsContainer = styled.div`
+  padding: 0;
+  border: 0;
+`;
+
+const PathDropdownContainer = styled.div`
+  flex-grow: 1;
+  padding: 0 0 0 0.2em;
+  border: none;
+`;
+
+const Container = styled.div`
+  max-width: 55em;
+  margin-bottom: 1.5em;
+`;
+
+const HeaderContainer = styled.div`
+  display: flex;
+  justify-content: center;
+  align-items: center;
+  margin-bottom: 1em;
+`;
+
+const TitleContainer = styled.div`
+  flex-grow: 1;
+  padding: 0;
+  border: none;
+`;
+
+const TagContainer = styled.div`
+  padding: 0;
+  border: none;
+`;
+
+const ShowPathsLink = styled(VSCodeLink)`
+  cursor: pointer;
+`;
+
+type ThreadPathProps = {
+  threadFlow: ThreadFlow;
+  step: number;
+  message: AnalysisMessage;
+  severity: ResultSeverity;
+  isSource?: boolean;
+  isSink?: boolean;
+}
+
+const ThreadPath = ({
+  threadFlow,
+  step,
+  message,
+  severity,
+  isSource,
+  isSink,
+}: ThreadPathProps) => (
+  <Container>
+    <HeaderContainer>
+      <TitleContainer>
+        <SectionTitle>Step {step}</SectionTitle>
+      </TitleContainer>
+      {isSource &&
+        <TagContainer>
+          <VSCodeTag>Source</VSCodeTag>
+        </TagContainer>
+      }
+      {isSink &&
+        <TagContainer>
+          <VSCodeTag>Sink</VSCodeTag>
+        </TagContainer>
+      }
+    </HeaderContainer>
+
+    <FileCodeSnippet
+      fileLink={threadFlow.fileLink}
+      codeSnippet={threadFlow.codeSnippet}
+      highlightedRegion={threadFlow.highlightedRegion}
+      severity={severity}
+      message={isSink ? message : threadFlow.message}
+    />
+  </Container>
+);
+
+type CodePathProps = {
+  codeFlow: CodeFlow;
+  message: AnalysisMessage;
+  severity: ResultSeverity;
+}
+
 const CodePath = ({
  codeFlow,
  message,
  severity
-}: {
-  codeFlow: CodeFlow;
-  message: AnalysisMessage;
-  severity: ResultSeverity;
-}) => {
-  return <>
+}: CodePathProps) => (
+  <>
    {codeFlow.threadFlows.map((threadFlow, index) =>
-      <div key={`thread-flow-${index}`} style={{ maxWidth: '55em' }}>
-        {index !== 0 && <VerticalSpace size={3} />}
-
-        <div style={{ display: 'flex', justifyContent: 'center', alignItems: 'center' }}>
-          <div style={{ flexGrow: 1, padding: 0, border: 'none' }}>
-            <SectionTitle>Step {index + 1}</SectionTitle>
-          </div>
-          {index === 0 &&
-            <div style={{ padding: 0, border: 'none' }}>
-              <VSCodeTag>Source</VSCodeTag>
-            </div>
-          }
-          {index === codeFlow.threadFlows.length - 1 &&
-            <div style={{ padding: 0, border: 'none' }}>
-              <VSCodeTag>Sink</VSCodeTag>
-            </div>
-          }
-        </div>
-
-        <VerticalSpace size={2} />
-        <FileCodeSnippet
-          fileLink={threadFlow.fileLink}
-          codeSnippet={threadFlow.codeSnippet}
-          highlightedRegion={threadFlow.highlightedRegion}
-          severity={severity}
-          message={index === codeFlow.threadFlows.length - 1 ? message : threadFlow.message} />
-      </div>
+      <ThreadPath
+        key={index}
+        threadFlow={threadFlow}
+        step={index + 1}
+        message={message}
+        severity={severity}
+        isSource={index === 0}
+        isSink={index === codeFlow.threadFlows.length - 1}
+      />
    )}
-  </>;
-};
+  </>
+);

 const getCodeFlowName = (codeFlow: CodeFlow) => {
  const filePath = codeFlow.threadFlows[codeFlow.threadFlows.length - 1].fileLink.filePath;
  return filePath.substring(filePath.lastIndexOf('/') + 1);
 };

-const Menu = ({
+type CodeFlowsDropdownProps = {
+  codeFlows: CodeFlow[];
+  setSelectedCodeFlow: (value: SetStateAction<CodeFlow>) => void;
+}
+
+const CodeFlowsDropdown = ({
  codeFlows,
  setSelectedCodeFlow
-}: {
-  codeFlows: CodeFlow[],
-  setSelectedCodeFlow: (value: React.SetStateAction<CodeFlow>) => void
-}) => {
-  return <VSCodeDropdown
-    onChange={(event: ChangeEvent<HTMLSelectElement>) => {
-      const selectedOption = event.target;
-      const selectedIndex = selectedOption.value as unknown as number;
-      setSelectedCodeFlow(codeFlows[selectedIndex]);
-    }}
-  >
-    {codeFlows.map((codeFlow, index) =>
-      <VSCodeOption
-        key={`codeflow-${index}'`}
-        value={index}
-      >
-        {getCodeFlowName(codeFlow)}
-      </VSCodeOption>
-    )}
-  </VSCodeDropdown>;
+}: CodeFlowsDropdownProps) => {
+  const handleChange = useCallback((e: ChangeEvent<HTMLSelectElement>) => {
+    const selectedOption = e.target;
+    const selectedIndex = selectedOption.value as unknown as number;
+    setSelectedCodeFlow(codeFlows[selectedIndex]);
+  }, [setSelectedCodeFlow, codeFlows]);
+
+  return (
+    <VSCodeDropdown onChange={handleChange}>
+      {codeFlows.map((codeFlow, index) =>
+        <VSCodeOption
+          key={index}
+          value={index}
+        >
+          {getCodeFlowName(codeFlow)}
+        </VSCodeOption>
+      )}
+    </VSCodeDropdown>
+  );
 };

-const CodePaths = ({
+type CodePathsOverlayProps = {
+  codeFlows: CodeFlow[];
+  ruleDescription: string;
+  message: AnalysisMessage;
+  severity: ResultSeverity;
+  onClose: () => void;
+}
+
+const CodePathsOverlay = ({
  codeFlows,
  ruleDescription,
  message,
-  severity
-}: {
+  severity,
+  onClose,
+}: CodePathsOverlayProps) => {
+  const [selectedCodeFlow, setSelectedCodeFlow] = useState(codeFlows[0]);
+
+  return (
+    <OverlayContainer>
+      <CloseButton onClick={onClose} />
+
+      <SectionTitle>{ruleDescription}</SectionTitle>
+      <VerticalSpace size={2} />
+
+      <PathsContainer>
+        <PathDetailsContainer>
+          {codeFlows.length} paths available: {selectedCodeFlow.threadFlows.length} steps in
+        </PathDetailsContainer>
+        <PathDropdownContainer>
+          <CodeFlowsDropdown codeFlows={codeFlows} setSelectedCodeFlow={setSelectedCodeFlow} />
+        </PathDropdownContainer>
+      </PathsContainer>
+
+      <VerticalSpace size={2} />
+      <CodePath
+        codeFlow={selectedCodeFlow}
+        severity={severity}
+        message={message}
+      />
+      <VerticalSpace size={3} />
+    </OverlayContainer>
+  );
+};
+
+type Props = {
  codeFlows: CodeFlow[],
  ruleDescription: string,
  message: AnalysisMessage,
  severity: ResultSeverity
-}) => {
-  const [isOpen, setIsOpen] = useState(false);
-  const [selectedCodeFlow, setSelectedCodeFlow] = useState(codeFlows[0]);
+};
+
+export const CodePaths = ({
+  codeFlows,
+  ruleDescription,
+  message,
+  severity
+}: Props) => {
+  const [isOpen, setIsOpen] = useState(false);

-  const anchorRef = useRef<HTMLDivElement>(null);
  const linkRef = useRef<HTMLAnchorElement>(null);

  const closeOverlay = () => setIsOpen(false);

  return (
-    <div ref={anchorRef}>
-      <VSCodeLink
+    <>
+      <ShowPathsLink
        onClick={() => setIsOpen(true)}
        ref={linkRef}
-        sx={{ cursor: 'pointer' }}>
+      >
        Show paths
-      </VSCodeLink>
+      </ShowPathsLink>
      {isOpen && (
        <Overlay
          returnFocusRef={linkRef}
          onEscape={closeOverlay}
          onClickOutside={closeOverlay}
-          anchorSide="outside-top">
-          <OverlayContainer>
-            <CloseButton onClick={closeOverlay} />
-
-            <SectionTitle>{ruleDescription}</SectionTitle>
-            <VerticalSpace size={2} />
-
-            <div style={{ display: 'flex', justifyContent: 'center', alignItems: 'center' }}>
-              <div style={{ padding: 0, border: 0 }}>
-                {codeFlows.length} paths available: {selectedCodeFlow.threadFlows.length} steps in
-              </div>
-              <div style={{ flexGrow: 1, padding: 0, paddingLeft: '0.2em', border: 'none' }}>
-                <Menu codeFlows={codeFlows} setSelectedCodeFlow={setSelectedCodeFlow} />
-              </div>
-            </div>
-
-            <VerticalSpace size={2} />
-            <CodePath
-              codeFlow={selectedCodeFlow}
-              severity={severity}
-              message={message} />
-
-            <VerticalSpace size={3} />
-
-          </OverlayContainer>
+          anchorSide="outside-top"
+        >
+          <CodePathsOverlay
+            codeFlows={codeFlows}
+            ruleDescription={ruleDescription}
+            message={message}
+            severity={severity}
+            onClose={closeOverlay}
+          />
        </Overlay>
      )}
-    </div>
+    </>
  );
 };
-
-export default CodePaths;
--- a/extensions/ql-vscode/src/view/remote-queries/FileCodeSnippet.tsx
+++ b/extensions/ql-vscode/src/view/remote-queries/FileCodeSnippet.tsx
@@ -1,23 +1,27 @@
 import * as React from 'react';
 import styled from 'styled-components';
-import { CodeSnippet, FileLink, HighlightedRegion, AnalysisMessage, ResultSeverity } from '../../remote-queries/shared/analysis-result';
+import { VSCodeLink } from '@vscode/webview-ui-toolkit/react';
+import {
+  AnalysisMessage,
+  CodeSnippet,
+  FileLink,
+  HighlightedRegion,
+  ResultSeverity
+} from '../../remote-queries/shared/analysis-result';
 import { createRemoteFileRef } from '../../pure/location-link-utils';
 import { parseHighlightedLine, shouldHighlightLine } from '../../pure/sarif-utils';
-import { VSCodeLink } from '@vscode/webview-ui-toolkit/react';
 import { VerticalSpace } from '../common';

 const borderColor = 'var(--vscode-editor-snippetFinalTabstopHighlightBorder)';
-const warningColor = '#966C23';
-const highlightColor = 'var(--vscode-editor-findMatchHighlightBackground)';

 const getSeverityColor = (severity: ResultSeverity) => {
  switch (severity) {
    case 'Recommendation':
-      return 'blue';
+      return 'var(--vscode-editorInfo-foreground)';
    case 'Warning':
-      return warningColor;
+      return 'var(--vscode-editorWarning-foreground)';
    case 'Error':
-      return 'red';
+      return 'var(--vscode-editorError-foreground)';
  }
 };

@@ -55,61 +59,106 @@ const MessageContainer = styled.div`
  padding-bottom: 0.5em;
 `;

+const HighlightedSpan = styled.span`
+  background-color: var(--vscode-editor-findMatchHighlightBackground);
+`;
+
+const LineContainer = styled.div`
+  display: flex;
+`;
+
+const LineNumberContainer = styled.div`
+  border-style: none;
+  padding: 0.01em 0.5em 0.2em;
+`;
+
+const CodeSnippetLineCodeContainer = styled.div`
+  flex-grow: 1;
+  border-style: none;
+  padding: 0.01em 0.5em 0.2em 1.5em;
+  word-break: break-word;
+`;
+
+type CodeSnippetMessageContainerProps = {
+  severity: ResultSeverity;
+};
+
+const CodeSnippetMessageContainer = styled.div<CodeSnippetMessageContainerProps>`
+  border-color: var(--vscode-editor-snippetFinalTabstopHighlightBorder);
+  border-width: 0.1em;
+  border-style: solid;
+  border-left-color: ${props => getSeverityColor(props.severity)};
+  border-left-width: 0.3em;
+  padding-top: 1em;
+  padding-bottom: 1em;
+`;
+
+const LocationLink = styled(VSCodeLink)`
+  font-family: var(--vscode-editor-font-family)
+`;
+
 const PlainCode = ({ text }: { text: string }) => {
  return <span>{replaceSpaceAndTabChar(text)}</span>;
 };

 const HighlightedCode = ({ text }: { text: string }) => {
-  return <span style={{ backgroundColor: highlightColor }}>{replaceSpaceAndTabChar(text)}</span>;
+  return <HighlightedSpan>{replaceSpaceAndTabChar(text)}</HighlightedSpan>;
 };

-const Message = ({
-  message,
-  borderLeftColor,
-  children
-}: {
+
+type CodeSnippetMessageProps = {
  message: AnalysisMessage,
-  borderLeftColor: string,
+  severity: ResultSeverity,
  children: React.ReactNode
-}) => {
-  return <div style={{
-    borderColor: borderColor,
-    borderWidth: '0.1em',
-    borderStyle: 'solid',
-    borderLeftColor: borderLeftColor,
-    borderLeftWidth: '0.3em',
-    paddingTop: '1em',
-    paddingBottom: '1em'
-  }}>
-    <MessageText>
-      {message.tokens.map((token, index) => {
-        switch (token.t) {
-          case 'text':
-            return <span key={`token-${index}`}>{token.text}</span>;
-          case 'location':
-            return <VSCodeLink
-              style={{ fontFamily: 'var(--vscode-editor-font-family)' }}
-              key={`token-${index}`}
-              href={createRemoteFileRef(
-                token.location.fileLink,
-                token.location.highlightedRegion?.startLine,
-                token.location.highlightedRegion?.endLine)}>
-              {token.text}
-            </VSCodeLink>;
-          default:
-            return <></>;
-        }
-      })}
-      {children && <>
-        <VerticalSpace size={2} />
-        {children}
-      </>
-      }
-    </MessageText>
-  </div>;
 };

-const Code = ({
+const CodeSnippetMessage = ({
+  message,
+  severity,
+  children
+}: CodeSnippetMessageProps) => {
+  return (
+    <CodeSnippetMessageContainer
+      severity={severity}
+    >
+      <MessageText>
+        {message.tokens.map((token, index) => {
+          switch (token.t) {
+            case 'text':
+              return <span key={index}>{token.text}</span>;
+            case 'location':
+              return (
+                <LocationLink
+                  key={index}
+                  href={
+                    createRemoteFileRef(
+                      token.location.fileLink,
+                      token.location.highlightedRegion?.startLine,
+                      token.location.highlightedRegion?.endLine
+                    )
+                  }
+                >
+                  {token.text}
+                </LocationLink>
+              );
+            default:
+              return <></>;
+          }
+        })}
+        {
+          children && (
+            <>
+              <VerticalSpace size={2} />
+              {children}
+            </>
+          )
+        }
+      </MessageText>
+    </CodeSnippetMessageContainer>
+  );
+};
+
+const CodeSnippetCode = ({
  line,
  lineNumber,
  highlightedRegion
@@ -133,15 +182,7 @@ const Code = ({
  );
 };

-const Line = ({
-  line,
-  lineIndex,
-  startingLineIndex,
-  highlightedRegion,
-  severity,
-  message,
-  messageChildren
-}: {
+type CodeSnippetLineProps = {
  line: string,
  lineIndex: number,
  startingLineIndex: number,
@@ -149,65 +190,65 @@ const Line = ({
  severity?: ResultSeverity,
  message?: AnalysisMessage,
  messageChildren?: React.ReactNode,
-}) => {
+};
+
+const CodeSnippetLine = ({
+  line,
+  lineIndex,
+  startingLineIndex,
+  highlightedRegion,
+  severity,
+  message,
+  messageChildren
+}: CodeSnippetLineProps) => {
  const shouldShowMessage = message &&
    severity &&
    highlightedRegion &&
    highlightedRegion.endLine == startingLineIndex + lineIndex;

-  return <div>
-    <div style={{ display: 'flex' }} >
-      <div style={{
-        borderStyle: 'none',
-        paddingTop: '0.01em',
-        paddingLeft: '0.5em',
-        paddingRight: '0.5em',
-        paddingBottom: '0.2em'
-      }}>
-        {startingLineIndex + lineIndex}
-      </div>
-      <div style={{
-        flexGrow: 1,
-        borderStyle: 'none',
-        paddingTop: '0.01em',
-        paddingLeft: '1.5em',
-        paddingRight: '0.5em',
-        paddingBottom: '0.2em',
-        wordBreak: 'break-word'
-      }}>
-        <Code
-          line={line}
-          lineNumber={startingLineIndex + lineIndex}
-          highlightedRegion={highlightedRegion} />
-      </div>
+  return (
+    <div>
+      <LineContainer>
+        <LineNumberContainer>{startingLineIndex + lineIndex}</LineNumberContainer>
+        <CodeSnippetLineCodeContainer>
+          <CodeSnippetCode
+            line={line}
+            lineNumber={startingLineIndex + lineIndex}
+            highlightedRegion={highlightedRegion}
+          />
+        </CodeSnippetLineCodeContainer>
+      </LineContainer>
+      {shouldShowMessage &&
+        <MessageContainer>
+          <CodeSnippetMessage
+            message={message}
+            severity={severity}
+          >
+            {messageChildren}
+          </CodeSnippetMessage>
+        </MessageContainer>
+      }
    </div>
-    {shouldShowMessage &&
-      <MessageContainer>
-        <Message
-          message={message}
-          borderLeftColor={getSeverityColor(severity)}>
-          {messageChildren}
-        </Message>
-      </MessageContainer>
-    }
-  </div>;
+  );
 };

-const FileCodeSnippet = ({
-  fileLink,
-  codeSnippet,
-  highlightedRegion,
-  severity,
-  message,
-  messageChildren,
-}: {
+type Props = {
  fileLink: FileLink,
  codeSnippet?: CodeSnippet,
  highlightedRegion?: HighlightedRegion,
  severity?: ResultSeverity,
  message?: AnalysisMessage,
  messageChildren?: React.ReactNode,
-}) => {
+};
+
+export const FileCodeSnippet = ({
+  fileLink,
+  codeSnippet,
+  highlightedRegion,
+  severity,
+  message,
+  messageChildren,
+}: Props) => {

  const startingLine = codeSnippet?.startLine || 0;
  const endingLine = codeSnippet?.endLine || 0;
@@ -224,11 +265,12 @@ const FileCodeSnippet = ({
          <VSCodeLink href={titleFileUri}>{fileLink.filePath}</VSCodeLink>
        </TitleContainer>
        {message && severity &&
-          <Message
+          <CodeSnippetMessage
            message={message}
-            borderLeftColor={getSeverityColor(severity)}>
+            severity={severity}
+          >
            {messageChildren}
-          </Message>}
+          </CodeSnippetMessage>}
      </Container>
    );
  }
@@ -242,8 +284,8 @@ const FileCodeSnippet = ({
      </TitleContainer>
      <CodeContainer>
        {code.map((line, index) => (
-          <Line
-            key={`line-${index}`}
+          <CodeSnippetLine
+            key={index}
            line={line}
            lineIndex={index}
            startingLineIndex={startingLine}
@@ -257,5 +299,3 @@ const FileCodeSnippet = ({
    </Container>
  );
 };
-
-export default FileCodeSnippet;