codeant-ai-for-open-source[bot] commented on code in PR #36349:
URL: https://github.com/apache/superset/pull/36349#discussion_r2590430987
##########
docs/sidebars.js:
##########
@@ -87,6 +87,11 @@ const sidebars = {
},
],
},
+ {
+ type: "doc",
+ label: "Glossary",
+ id: "glossary"
Review Comment:
**Suggestion:** Inconsistent string quoting: this added object uses double
quotes while the rest of the file uses single quotes, which will likely cause
ESLint/CI failures if the project enforces single-quote style; change the
strings to single quotes to match the file's style. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
type: 'doc',
label: 'Glossary',
id: 'glossary'
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new object uses double quotes while the surrounding file predominantly
uses single quotes for string literals. That likely triggers ESLint/CI in
projects enforcing single-quote style. Converting to single quotes is a
low-risk, correct stylistic fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/sidebars.js
**Line:** 91:93
**Comment:**
*Possible Bug: Inconsistent string quoting: this added object uses
double quotes while the rest of the file uses single quotes, which will likely
cause ESLint/CI failures if the project enforces single-quote style; change the
strings to single quotes to match the file's style.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
docs/docs/glossary.mdx:
##########
@@ -0,0 +1,108 @@
+---
+title: Glossary
+hide_title: true
+sidebar_position: 10
+---
+
+import { getAllGlossaryTopics } from
'../../superset-frontend/packages/superset-ui-core/src/glossary';
+import { Table, ConfigProvider, theme } from 'antd';
+import { useColorMode } from '@docusaurus/theme-common';
+import { useEffect, useRef } from 'react';
+
+export const GlossaryStructure = [
+ {
+ title: 'Term',
+ dataIndex: 'title',
+ key: 'title',
+ width: 200,
+ },
+ {
+ title: 'Short Description',
+ dataIndex: 'short',
+ key: 'short',
+ },
+];
+
+export const GlossaryContent = () => {
+ const { colorMode } = useColorMode();
+ const isDark = colorMode === 'dark';
+ const tableRefs = useRef({});
+
+ const scrollToRow = (topic, rowKey) => {
+ const topicId = encodeURIComponent(topic);
+ const row = tableRefs.current[topicId]?.[rowKey];
Review Comment:
**Suggestion:** Encoding mismatch when looking up a stored row: keys are
stored encoded (via encodeURIComponent) but `scrollToRow` looks up
`tableRefs.current[topicId][rowKey]` using the raw/unencoded `rowKey`, so it
will fail to find the DOM node and the scroll/highlight will not occur. Fix by
encoding the `rowKey` before lookup (or accepting already-encoded values
consistently). [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const encRowKey = encodeURIComponent(rowKey);
const row = tableRefs.current[topicId]?.[encRowKey];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The dataSource stores keys with encodeURIComponent(...), while scrollToRow
receives a decoded hash and looks up the unencoded key, so it will fail to
locate the node. Encoding the rowKey before lookup fixes a real logic bug that
prevents scrolling/highlighting.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/glossary.mdx
**Line:** 33:33
**Comment:**
*Logic Error: Encoding mismatch when looking up a stored row: keys are
stored encoded (via encodeURIComponent) but `scrollToRow` looks up
`tableRefs.current[topicId][rowKey]` using the raw/unencoded `rowKey`, so it
will fail to find the DOM node and the scroll/highlight will not occur. Fix by
encoding the `rowKey` before lookup (or accepting already-encoded values
consistently).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 { GlossaryMap, GlossaryTerm, type GlossaryTopic } from
'./glossaryModels';
+import { glossaryDefinition } from './glossary';
+import { t } from '@superset-ui/core';
+
+export const GLOSSARY_BASE_URL = 'http://localhost:3000/docs'; // TODO: Change
to use the url for the environment.
Review Comment:
**Suggestion:** Hardcoded base URL points to localhost and will produce
incorrect links in non-local environments or when the docs host differs; make
the base URL configurable (env var) and fallback to the current origin or a
relative path, and normalize trailing slashes. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
export const GLOSSARY_BASE_URL =
(typeof process !== 'undefined' && (process.env.SUPERSET_DOCS_URL as
string)) ||
(typeof window !== 'undefined' ? `${window.location.origin}/docs` :
'/docs');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion addresses a real production/usability problem: a hardcoded
localhost URL will produce broken links when the package runs in
staging/production. Making the base URL configurable (via env or window.origin
fallback) is appropriate for a frontend library. The proposed improved code is
a pragmatic one-liner; it will require ensuring the chosen env var (e.g.,
SUPERSET_DOCS_URL) is set in build/deployment or that using
window.location.origin is acceptable for consumption contexts.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts
**Line:** 24:24
**Comment:**
*Possible Bug: Hardcoded base URL points to localhost and will produce
incorrect links in non-local environments or when the docs host differs; make
the base URL configurable (env var) and fallback to the current origin or a
relative path, and normalize trailing slashes.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
docs/docs/glossary.mdx:
##########
@@ -0,0 +1,108 @@
+---
+title: Glossary
+hide_title: true
+sidebar_position: 10
+---
+
+import { getAllGlossaryTopics } from
'../../superset-frontend/packages/superset-ui-core/src/glossary';
+import { Table, ConfigProvider, theme } from 'antd';
+import { useColorMode } from '@docusaurus/theme-common';
+import { useEffect, useRef } from 'react';
+
+export const GlossaryStructure = [
+ {
+ title: 'Term',
+ dataIndex: 'title',
+ key: 'title',
+ width: 200,
+ },
+ {
+ title: 'Short Description',
+ dataIndex: 'short',
+ key: 'short',
+ },
+];
+
+export const GlossaryContent = () => {
+ const { colorMode } = useColorMode();
+ const isDark = colorMode === 'dark';
+ const tableRefs = useRef({});
+
+ const scrollToRow = (topic, rowKey) => {
+ const topicId = encodeURIComponent(topic);
+ const row = tableRefs.current[topicId]?.[rowKey];
+ if (row) {
+ row.scrollIntoView({ behavior: 'smooth', block: 'center' });
+ row.classList.add('table-row-highlight');
+ setTimeout(() => row.classList.remove('table-row-highlight'), 2000);
+ }
+ };
+
+ useEffect(() => {
+ const hash = decodeURIComponent(window.location.hash.slice(1));
+ if (!hash) return;
+
+ const [topic, term] = hash.split('__');
+ if (topic && term) scrollToRow(topic, hash);
+ }, []);
+
+ return (
+ <div>
+ <ConfigProvider
+ theme={{
+ algorithm: isDark ? theme.darkAlgorithm : theme.defaultAlgorithm,
+ }}
+ >
+ {getAllGlossaryTopics().map((topic) => {
+ const topicName = topic.getName();
+ const topicFragment = encodeURIComponent(topicName);
+ const terms = topic.getAllTerms();
+ return (
+ <div key={topicName} id={topicFragment}>
+ <h3>{topic.getDisplayName()}</h3>
+ <Table
+ dataSource={terms
+ .map((term) => {
+ const key = term.getTitle()
+ ? encodeURIComponent(`${topicName}__${term.getTitle()}`)
+ : undefined;
+ return key
+ ? {
+ title: term.getDisplayTitle(),
+ short: term.getShort(),
+ key,
+ }
+ : null;
+ })
+ .filter(Boolean)}
+ columns={GlossaryStructure}
+ rowKey="key"
+ pagination={false}
+ showHeader
+ bordered
+ onRow={(record) => {
+ if (!record?.key) return {};
+ const topicId = topicFragment;
+
+ return {
+ ref: (node) => {
+ if (node) {
+ if (!tableRefs.current[topicId])
tableRefs.current[topicId] = {};
+ tableRefs.current[topicId][record.key] = node;
Review Comment:
**Suggestion:** Stale DOM references / memory leak: the `ref` callback
stored nodes into `tableRefs` only when `node` is non-null, but does not remove
the stored reference when `node` becomes null (unmount). That leaves stale
references to DOM nodes and breaks future lookups. Fix by removing the entry
when `node` is null and cleaning up empty topic maps. [memory leak]
**Severity Level:** Minor ⚠️
```suggestion
if (!tableRefs.current[topicId])
tableRefs.current[topicId] = {};
if (node) {
tableRefs.current[topicId][record.key] = node;
} else {
// cleanup stale reference when row unmounts
delete tableRefs.current[topicId][record.key];
if (Object.keys(tableRefs.current[topicId]).length
=== 0) {
delete tableRefs.current[topicId];
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
React will call the ref with null on unmount; the current code never removes
the stored reference, leaking memory and leaving stale DOM refs. The suggested
cleanup (delete on null) is correct and safe.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/docs/glossary.mdx
**Line:** 89:91
**Comment:**
*Memory Leak: Stale DOM references / memory leak: the `ref` callback
stored nodes into `tableRefs` only when `node` is non-null, but does not remove
the stored reference when `node` becomes null (unmount). That leaves stale
references to DOM nodes and breaks future lookups. Fix by removing the entry
when `node` is null and cleaning up empty topic maps.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 { GlossaryMap, GlossaryTerm, type GlossaryTopic } from
'./glossaryModels';
+import { glossaryDefinition } from './glossary';
+import { t } from '@superset-ui/core';
+
+export const GLOSSARY_BASE_URL = 'http://localhost:3000/docs'; // TODO: Change
to use the url for the environment.
+
+// Pattern matches: [GLOSSARY]|topic|title
+// Captures: topic and title for lookup in glossary
+const GLOSSARY_ENCODING_PATTERN = /^\[GLOSSARY\]\|([^|]+)\|([^|]+)$/;
+
+/**
+ * The exported glossary object is a runtime structure where each entry is a
GlossaryTerm instance, but the key
+ * structure mirrors `glossaryDefinition` so IDEs can autocomplete, yet
callers can use methods like `getShort()`.
+ */
+export type Glossary = {
+ [Topic in keyof typeof glossaryDefinition]: {
+ [Title in keyof (typeof glossaryDefinition)[Topic]]: GlossaryTerm;
+ };
+};
+
+const glossary: Glossary = Object.fromEntries(
+ Object.entries(glossaryDefinition).map(([topic, termsByTitle]) => [
+ topic,
+ Object.fromEntries(
+ Object.entries(termsByTitle).map(([title, termStrings]) => [
+ title,
+ new GlossaryTerm({
+ topic,
+ title,
+ short: termStrings.short,
+ extended: termStrings.extended,
Review Comment:
**Suggestion:** When constructing GlossaryTerm the code assumes
`termStrings.short` and `termStrings.extended` always exist; if they are
missing this can propagate undefined into the GlossaryTerm and cause runtime
errors — provide safe defaults (e.g., empty strings) when properties are
absent. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
short: termStrings.short ?? '',
extended: termStrings.extended ?? '',
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Providing safe defaults (e.g., empty strings) for short/extended is a
defensive improvement: if glossaryDefinition ever omits those fields,
GlossaryTerm will receive predictable values instead of undefined. This
prevents potential runtime surprises when consumers call methods like
getShort(). It's a low-risk, beneficial change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts
**Line:** 49:50
**Comment:**
*Possible Bug: When constructing GlossaryTerm the code assumes
`termStrings.short` and `termStrings.extended` always exist; if they are
missing this can propagate undefined into the GlossaryTerm and cause runtime
errors — provide safe defaults (e.g., empty strings) when properties are absent.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 { GlossaryMap, GlossaryTerm, type GlossaryTopic } from
'./glossaryModels';
+import { glossaryDefinition } from './glossary';
+import { t } from '@superset-ui/core';
+
+export const GLOSSARY_BASE_URL = 'http://localhost:3000/docs'; // TODO: Change
to use the url for the environment.
+
+// Pattern matches: [GLOSSARY]|topic|title
+// Captures: topic and title for lookup in glossary
+const GLOSSARY_ENCODING_PATTERN = /^\[GLOSSARY\]\|([^|]+)\|([^|]+)$/;
+
+/**
+ * The exported glossary object is a runtime structure where each entry is a
GlossaryTerm instance, but the key
+ * structure mirrors `glossaryDefinition` so IDEs can autocomplete, yet
callers can use methods like `getShort()`.
+ */
+export type Glossary = {
+ [Topic in keyof typeof glossaryDefinition]: {
+ [Title in keyof (typeof glossaryDefinition)[Topic]]: GlossaryTerm;
+ };
+};
+
+const glossary: Glossary = Object.fromEntries(
+ Object.entries(glossaryDefinition).map(([topic, termsByTitle]) => [
+ topic,
+ Object.fromEntries(
+ Object.entries(termsByTitle).map(([title, termStrings]) => [
+ title,
+ new GlossaryTerm({
+ topic,
+ title,
+ short: termStrings.short,
+ extended: termStrings.extended,
+ }),
+ ]),
+ ),
+ ]),
+) as Glossary;
+
+const glossaryMap = new GlossaryMap(glossary);
+
+export const getAllGlossaryTopics = (): GlossaryTopic[] =>
glossaryMap.getAllTopics();
+
+const buildGlossaryUrl = (topic: string, title: string): string =>
+ `${GLOSSARY_BASE_URL}/glossary#${encodeURIComponent(topic + '__' + title)}`;
+
+export const resolveGlossaryString = (glossaryString: string): [string |
undefined, string] => {
+ const match = glossaryString.match(GLOSSARY_ENCODING_PATTERN);
+ if (!match) {
+ return [undefined, glossaryString];
+ }
+ const topic = match[1];
+ const title = match[2];
+
+ // Look up the term from the glossary to get the translated description
+ const glossaryTopic = glossaryMap.getTopic(topic);
+ const term = glossaryTopic?.getTerm(title);
+ const description = term ? term.getShort(t) : glossaryString;
Review Comment:
**Suggestion:** The code matches the encoded glossary string without
trimming, so leading/trailing whitespace will prevent a match and the fallback
returns the untrimmed string; trim the input before matching and use the
trimmed value as the fallback description. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const encoded = glossaryString.trim();
const match = encoded.match(GLOSSARY_ENCODING_PATTERN);
if (!match) {
return [undefined, encoded];
}
const topic = match[1];
const title = match[2];
// Look up the term from the glossary to get the translated description
const glossaryTopic = glossaryMap.getTopic(topic);
const term = glossaryTopic?.getTerm(title);
const description = term ? term.getShort(t) : encoded;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Trimming before matching is a harmless robustness improvement: accidental
whitespace around an encoded glossary token would currently cause a failed
match and return the untrimmed token as the description. Using trim() yields
more consistent behavior and is unlikely to break legitimate usage. The
improved code is straightforward and addresses a plausible edge case.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts
**Line:** 65:75
**Comment:**
*Possible Bug: The code matches the encoded glossary string without
trimming, so leading/trailing whitespace will prevent a match and the fallback
returns the untrimmed string; trim the input before matching and use the
trimmed value as the fallback description.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
