Copilot commented on code in PR #66809:
URL: https://github.com/apache/airflow/pull/66809#discussion_r3228269099
##########
airflow-core/src/airflow/ui/src/components/ReactMarkdown.tsx:
##########
@@ -106,81 +122,111 @@ const UlComponent = ({ children }: PropsWithChildren) =>
(
</List.Root>
);
-// Factory function for the code component that needs style
-const createCodeComponent =
- (style: typeof oneDark | typeof oneLight) =>
+type CodeElementProps = {
+ readonly children?: ReactNode;
+ readonly className?: string;
+};
+
+type MermaidDiagramProps = {
+ readonly chart: string;
+ readonly theme: "dark" | "default";
+};
+
+type MarkdownRendererProps = {
+ readonly mermaidTheme: MermaidDiagramProps["theme"];
+ readonly style: SyntaxTheme;
+};
+
+const extractTextContent = (children: CodeElementProps["children"]): string =>
{
+ if (typeof children === "number" || typeof children === "string") {
+ return String(children);
+ }
+
+ if (Array.isArray(children)) {
+ return children
+ .map((child) => (typeof child === "number" || typeof child === "string"
? String(child) : ""))
+ .join("");
+ }
+
+ return "";
+};
+
+const CodeComponent = ({ children }: PropsWithChildren) => <Code
display="inline">{children}</Code>;
+
+// Factory function for the pre component that needs style
+const createPreComponent =
+ (style: SyntaxTheme, mermaidTheme: MermaidDiagramProps["theme"]) =>
({
children,
- className,
- inline,
}: {
- readonly children: ReactNode;
- readonly className?: string;
- readonly inline?: boolean;
+ readonly children?: ReactNode;
}) => {
- if (inline) {
- return (
- <Code display="inline" p={2}>
- {children}
- </Code>
- );
+ const [codeElement] = Children.toArray(children);
+
+ if (!isValidElement<CodeElementProps>(codeElement)) {
+ return <Box my={3}>{children}</Box>;
}
// Extract language from className (format: "language-python")
+ const { children: codeChildren, className } = codeElement.props;
const match = /language-(?<lang>\w+)/u.exec(className ?? "");
Review Comment:
Language extraction for fenced code blocks uses `/language-(?<lang>\w+)/`,
which will truncate or fail for common hyphenated language identifiers (e.g.
`language-bash-session`, `language-objective-c`). This can lead to wrong labels
and incorrect highlighting/mermaid detection. Consider expanding the regex to
include `-` (and possibly other valid characters) when capturing the language.
##########
airflow-core/src/airflow/ui/src/utils/renderMermaid.ts:
##########
@@ -0,0 +1,48 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import mermaid from "mermaid";
+
+type MermaidTheme = "dark" | "default";
+
+let initializedTheme: MermaidTheme | undefined;
+
+const initializeMermaid = (theme: MermaidTheme) => {
+ if (initializedTheme === theme) {
+ return;
+ }
+
+ mermaid.initialize({ securityLevel: "strict", startOnLoad: false, theme });
+ initializedTheme = theme;
+};
Review Comment:
`renderMermaid.ts` imports `mermaid` eagerly at module load. Since
`ReactMarkdown` is used across multiple UI surfaces (not just DAG docs), this
likely pulls Mermaid (and its heavy dependency tree) into the main bundle even
when no Mermaid blocks are present. Consider switching to a dynamic import
inside `renderMermaidDiagram` (or inside the Mermaid block component) so
Mermaid code is only loaded when a Mermaid fenced block is actually rendered.
##########
airflow-core/src/airflow/ui/src/components/ReactMarkdownBlocks.tsx:
##########
@@ -0,0 +1,188 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, Flex, Text } from "@chakra-ui/react";
+import { useEffect, useId, useState, type ReactNode } from "react";
+
+import { LazyClipboard } from "src/components/ui";
+import { renderMermaidDiagram } from "src/utils/renderMermaid";
+import { SyntaxHighlighter, type SyntaxTheme } from
"src/utils/syntaxHighlighter";
+
+const MarkdownBlockFrame = ({
+ action,
+ children,
+ label,
+}: {
+ readonly action: ReactNode;
+ readonly children: ReactNode;
+ readonly label: string;
+}) => (
+ <Box maxWidth="100%" minWidth={0} my={3} width="100%">
+ <Flex
+ alignItems="center"
+ bg="bg.muted"
+ borderColor="border.emphasized"
+ borderTopRadius="md"
+ borderWidth="1px"
+ justifyContent="space-between"
+ minH={8}
+ px={3}
+ py={1}
+ >
+ <Text color="fg.muted" fontFamily="mono" fontSize="xs"
lineHeight="short">
+ {label}
+ </Text>
+ {action}
+ </Flex>
+ <Box
+ borderBottomRadius="md"
+ borderColor="border.emphasized"
+ borderTopWidth={0}
+ borderWidth="1px"
+ maxWidth="100%"
+ minWidth={0}
+ overflow="hidden"
+ >
+ {children}
+ </Box>
+ </Box>
+);
+
+export const MarkdownCodeBlock = ({
+ language,
+ style,
+ value,
+}: {
+ readonly language?: string;
+ readonly style: SyntaxTheme;
+ readonly value: string;
+}) => {
+ const languageLabel = language ?? "text";
+
+ return (
+ <MarkdownBlockFrame
+ action={
+ <LazyClipboard
+ aria-label="Copy code block"
+ data-testid="markdown-copy-button"
+ getValue={() => value}
+ />
+ }
+ label={languageLabel}
+ >
+ <Box data-testid="markdown-code-scroll-area" maxWidth="100%"
minWidth={0} overflowX="auto" overflowY="hidden" width="100%">
+ <Box data-testid="markdown-code-content" display="inline-block"
minWidth="100%">
+ <SyntaxHighlighter
+ codeTagProps={{ style: { overflowWrap: "normal", whiteSpace:
"pre", wordBreak: "normal" } }}
+ customStyle={{ borderRadius: 0, margin: 0, width: "max-content" }}
+ language={languageLabel}
+ lineNumberStyle={{ minWidth: "2.5em", opacity: 0.6, paddingRight:
"1em" }}
+ PreTag="div"
+ showLineNumbers
+ style={style}
+ >
+ {value}
+ </SyntaxHighlighter>
+ </Box>
+ </Box>
+ </MarkdownBlockFrame>
+ );
+};
+
+export const MarkdownMermaid = ({
+ chart,
+ fallbackStyle,
+ theme,
+}: {
+ readonly chart: string;
+ readonly fallbackStyle: SyntaxTheme;
+ readonly theme: "dark" | "default";
+}) => {
+ const diagramId = useId().replaceAll(":", "");
+ const [error, setError] = useState(false);
+ const [svg, setSvg] = useState<string>();
+
+ useEffect(() => {
+ let cancelled = false;
+
+ const renderDiagram = async () => {
+ try {
+ const renderedSvg = await renderMermaidDiagram({
+ chart,
+ diagramId: `markdown-mermaid-${diagramId}`,
+ theme,
+ });
+
+ if (!cancelled) {
+ setSvg(renderedSvg);
+ setError(false);
+ }
+ } catch {
+ if (!cancelled) {
+ setError(true);
+ setSvg(undefined);
+ }
+ }
+ };
+
+ void renderDiagram();
+
+ return () => {
+ cancelled = true;
+ };
+ }, [chart, diagramId, theme]);
+
+ if (error) {
+ return <MarkdownCodeBlock language="mermaid" style={fallbackStyle}
value={chart} />;
+ }
+
+ return (
+ <MarkdownBlockFrame
+ action={
+ <LazyClipboard
+ aria-label="Copy Mermaid source"
+ data-testid="markdown-mermaid-copy-button"
+ getValue={() => chart}
+ />
+ }
+ label="mermaid"
+ >
+ <Box maxWidth="100%" minHeight="8rem" minWidth={0} overflow="hidden"
p={3} width="100%">
+ {svg === undefined ? (
+ <Text color="fg.muted" fontSize="sm">
+ Rendering diagram...
+ </Text>
+ ) : (
+ <Box
+ css={{
+ "& svg": {
+ display: "block",
+ height: "auto",
+ marginInline: "auto",
+ maxWidth: "100%",
+ width: "100%",
+ },
+ }}
+ dangerouslySetInnerHTML={{ __html: svg }}
+ data-testid="markdown-mermaid-diagram"
+ />
Review Comment:
This renders Mermaid output via `dangerouslySetInnerHTML` using SVG
generated from `doc_md` content. Even with `securityLevel: "strict"`, this is
still an XSS-sensitive sink and relies entirely on Mermaid's sanitization
behavior. Please add an explicit sanitization step (or a defensive allowlist
check) before injecting, and/or add a regression test that ensures the
generated SVG cannot contain script/event-handler attributes.
##########
airflow-core/src/airflow/ui/src/components/ReactMarkdownBlocks.tsx:
##########
@@ -0,0 +1,188 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, Flex, Text } from "@chakra-ui/react";
+import { useEffect, useId, useState, type ReactNode } from "react";
+
+import { LazyClipboard } from "src/components/ui";
+import { renderMermaidDiagram } from "src/utils/renderMermaid";
+import { SyntaxHighlighter, type SyntaxTheme } from
"src/utils/syntaxHighlighter";
+
+const MarkdownBlockFrame = ({
+ action,
+ children,
+ label,
+}: {
+ readonly action: ReactNode;
+ readonly children: ReactNode;
+ readonly label: string;
+}) => (
+ <Box maxWidth="100%" minWidth={0} my={3} width="100%">
+ <Flex
+ alignItems="center"
+ bg="bg.muted"
+ borderColor="border.emphasized"
+ borderTopRadius="md"
+ borderWidth="1px"
+ justifyContent="space-between"
+ minH={8}
+ px={3}
+ py={1}
+ >
+ <Text color="fg.muted" fontFamily="mono" fontSize="xs"
lineHeight="short">
+ {label}
+ </Text>
+ {action}
+ </Flex>
+ <Box
+ borderBottomRadius="md"
+ borderColor="border.emphasized"
+ borderTopWidth={0}
+ borderWidth="1px"
+ maxWidth="100%"
+ minWidth={0}
+ overflow="hidden"
+ >
+ {children}
+ </Box>
+ </Box>
+);
+
+export const MarkdownCodeBlock = ({
+ language,
+ style,
+ value,
+}: {
+ readonly language?: string;
+ readonly style: SyntaxTheme;
+ readonly value: string;
+}) => {
+ const languageLabel = language ?? "text";
+
+ return (
+ <MarkdownBlockFrame
+ action={
+ <LazyClipboard
+ aria-label="Copy code block"
+ data-testid="markdown-copy-button"
+ getValue={() => value}
+ />
Review Comment:
New user-visible strings are hard-coded (e.g. the copy button aria-label).
The UI generally routes aria-label/title text through i18n (see other
`LazyClipboard` usages). Please use the translation mechanism here as well so
these labels are localizable and consistent.
##########
airflow-core/src/airflow/ui/src/components/ReactMarkdownBlocks.tsx:
##########
@@ -0,0 +1,188 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, Flex, Text } from "@chakra-ui/react";
+import { useEffect, useId, useState, type ReactNode } from "react";
+
+import { LazyClipboard } from "src/components/ui";
+import { renderMermaidDiagram } from "src/utils/renderMermaid";
+import { SyntaxHighlighter, type SyntaxTheme } from
"src/utils/syntaxHighlighter";
+
+const MarkdownBlockFrame = ({
+ action,
+ children,
+ label,
+}: {
+ readonly action: ReactNode;
+ readonly children: ReactNode;
+ readonly label: string;
+}) => (
+ <Box maxWidth="100%" minWidth={0} my={3} width="100%">
+ <Flex
+ alignItems="center"
+ bg="bg.muted"
+ borderColor="border.emphasized"
+ borderTopRadius="md"
+ borderWidth="1px"
+ justifyContent="space-between"
+ minH={8}
+ px={3}
+ py={1}
+ >
+ <Text color="fg.muted" fontFamily="mono" fontSize="xs"
lineHeight="short">
+ {label}
+ </Text>
+ {action}
+ </Flex>
+ <Box
+ borderBottomRadius="md"
+ borderColor="border.emphasized"
+ borderTopWidth={0}
+ borderWidth="1px"
+ maxWidth="100%"
+ minWidth={0}
+ overflow="hidden"
+ >
+ {children}
+ </Box>
+ </Box>
+);
+
+export const MarkdownCodeBlock = ({
+ language,
+ style,
+ value,
+}: {
+ readonly language?: string;
+ readonly style: SyntaxTheme;
+ readonly value: string;
+}) => {
+ const languageLabel = language ?? "text";
+
+ return (
+ <MarkdownBlockFrame
+ action={
+ <LazyClipboard
+ aria-label="Copy code block"
+ data-testid="markdown-copy-button"
+ getValue={() => value}
+ />
+ }
+ label={languageLabel}
+ >
+ <Box data-testid="markdown-code-scroll-area" maxWidth="100%"
minWidth={0} overflowX="auto" overflowY="hidden" width="100%">
+ <Box data-testid="markdown-code-content" display="inline-block"
minWidth="100%">
+ <SyntaxHighlighter
+ codeTagProps={{ style: { overflowWrap: "normal", whiteSpace:
"pre", wordBreak: "normal" } }}
+ customStyle={{ borderRadius: 0, margin: 0, width: "max-content" }}
+ language={languageLabel}
+ lineNumberStyle={{ minWidth: "2.5em", opacity: 0.6, paddingRight:
"1em" }}
+ PreTag="div"
+ showLineNumbers
+ style={style}
+ >
+ {value}
+ </SyntaxHighlighter>
+ </Box>
+ </Box>
+ </MarkdownBlockFrame>
+ );
+};
+
+export const MarkdownMermaid = ({
+ chart,
+ fallbackStyle,
+ theme,
+}: {
+ readonly chart: string;
+ readonly fallbackStyle: SyntaxTheme;
+ readonly theme: "dark" | "default";
+}) => {
+ const diagramId = useId().replaceAll(":", "");
+ const [error, setError] = useState(false);
+ const [svg, setSvg] = useState<string>();
+
+ useEffect(() => {
+ let cancelled = false;
+
+ const renderDiagram = async () => {
+ try {
+ const renderedSvg = await renderMermaidDiagram({
+ chart,
+ diagramId: `markdown-mermaid-${diagramId}`,
+ theme,
+ });
+
+ if (!cancelled) {
+ setSvg(renderedSvg);
+ setError(false);
+ }
+ } catch {
+ if (!cancelled) {
+ setError(true);
+ setSvg(undefined);
+ }
+ }
+ };
+
+ void renderDiagram();
+
+ return () => {
+ cancelled = true;
+ };
+ }, [chart, diagramId, theme]);
+
+ if (error) {
+ return <MarkdownCodeBlock language="mermaid" style={fallbackStyle}
value={chart} />;
+ }
+
+ return (
+ <MarkdownBlockFrame
+ action={
+ <LazyClipboard
+ aria-label="Copy Mermaid source"
+ data-testid="markdown-mermaid-copy-button"
+ getValue={() => chart}
+ />
+ }
+ label="mermaid"
+ >
+ <Box maxWidth="100%" minHeight="8rem" minWidth={0} overflow="hidden"
p={3} width="100%">
+ {svg === undefined ? (
+ <Text color="fg.muted" fontSize="sm">
+ Rendering diagram...
+ </Text>
+ ) : (
Review Comment:
The Mermaid block UI hard-codes user-visible strings (copy button aria-label
and the loading message). Please route these through i18n (consistent with
other UI components) so they can be localized, and update the tests accordingly.
##########
airflow-core/src/airflow/ui/src/components/ReactMarkdown.test.tsx:
##########
@@ -0,0 +1,153 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import "@testing-library/jest-dom";
Review Comment:
`testsSetup.ts` already imports `@testing-library/jest-dom/vitest`, so
importing `@testing-library/jest-dom` again here is redundant and may register
matchers in a non-Vitest-specific way. Consider removing this import and
relying on the global test setup.
##########
airflow-core/src/airflow/ui/src/utils/syntaxHighlighter.ts:
##########
@@ -16,20 +16,10 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { PrismLight as SyntaxHighlighter } from "react-syntax-highlighter";
-import bash from "react-syntax-highlighter/dist/esm/languages/prism/bash";
-import json from "react-syntax-highlighter/dist/esm/languages/prism/json";
-import python from "react-syntax-highlighter/dist/esm/languages/prism/python";
-import sql from "react-syntax-highlighter/dist/esm/languages/prism/sql";
-import yaml from "react-syntax-highlighter/dist/esm/languages/prism/yaml";
+import type { CSSProperties } from "react";
-// Register all supported languages once
-SyntaxHighlighter.registerLanguage("python", python);
-SyntaxHighlighter.registerLanguage("json", json);
-SyntaxHighlighter.registerLanguage("yaml", yaml);
-SyntaxHighlighter.registerLanguage("sql", sql);
-SyntaxHighlighter.registerLanguage("bash", bash);
+export { atomOneDark as oneDark, atomOneLight as oneLight } from
"react-syntax-highlighter/dist/esm/styles/hljs";
-export { oneDark, oneLight } from
"react-syntax-highlighter/dist/esm/styles/prism";
+export { default as SyntaxHighlighter } from "react-syntax-highlighter";
-export { PrismLight as SyntaxHighlighter } from "react-syntax-highlighter";
+export type SyntaxTheme = Record<string, CSSProperties>;
Review Comment:
Switching from `PrismLight` with explicit language registration to the
default `react-syntax-highlighter` export will typically bundle a much larger
highlighter/language set. Given this helper is used across the UI, consider
using a light/async build (and/or selective language registration) to keep
bundle size and initial load time under control while still supporting the
needed languages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
