bito-code-review[bot] commented on code in PR #22604:
URL: https://github.com/apache/superset/pull/22604#discussion_r3485984498
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -518,6 +519,18 @@ export type ResolvedColorFormatterResult = {
color?: string;
};
+export type UrlLinkConfig = {
+ columnName?: string;
+ linkText?: string;
+ linkSchema?: string;
+};
+
+export type UrlLinks = {
+ column: string;
+ linkText: string;
+ getTextFromValues: (value: number, values: DataRecord) => string;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unused parameter in function type</b></div>
<div id="fix">
The `getTextFromValues` function in `UrlLinks` accepts `value: number` as
its first parameter, but this parameter is never used in the implementation at
`getUrlLinks.ts:39` (it's prefixed with `_`). This creates a misleading API
contract—callers (e.g., `TableChart.tsx:1267`) may expect the cell value to
influence the result, but it has no effect. Either remove the parameter or use
it.
</div>
</div>
<small><i>Code Review Run #3aa5ae</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getUrlLinks.ts:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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 memoizeOne from 'memoize-one';
+import { DataRecord } from '@superset-ui/core';
+import { UrlLinkConfig } from '../types';
+
+const parseLinkSchema = (schema: string) =>
schema.match(/(?<=\$\{).*?(?=\})/g);
+
+const genLinkHref = (values: DataRecord, schema: string) => {
+ let result = schema;
+ parseLinkSchema(schema)?.forEach(name => {
+ const val = values[name]?.toString() ?? '';
+ result = result.replace(`$\{${name}}`, val);
+ });
+ return result;
+};
+
+export const getTextFromValues = memoizeOne(
+ (columnConfig: UrlLinkConfig[] | undefined) =>
+ columnConfig?.map(({ columnName, linkText, linkSchema }) => ({
+ column: columnName!,
+ linkText: linkText!,
+ getTextFromValues: (_val: number, values: DataRecord) =>
+ genLinkHref(values, linkSchema!),
+ })),
+);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests</b></div>
<div id="fix">
New utility file has no unit tests. Add test coverage for `parseLinkSchema`,
`genLinkHref`, and `getTextFromValues` functions per project testing standards.
([11730])
</div>
</div>
<small><i>Code Review Run #3aa5ae</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -867,6 +867,27 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'url_link',
+ config: {
+ type: 'UrlLinkControl',
+ renderTrigger: true,
+ label: t('Url Link'),
+ description: t('Add a new column that can configure a link'),
+ shouldMapStateToProps() {
+ return true;
+ },
+ mapStateToProps(_explore: any, _: any, chart: any) {
+ const { colnames } = chart?.queriesResponse?.[0] ?? {};
+
+ return {
+ colnames,
+ };
+ },
+ },
+ },
+ ],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for url_link control</b></div>
<div id="fix">
The new `url_link` control configuration lacks test coverage. The existing
`controlPanel.test.tsx` file tests similar controls like
`conditional_formatting` and `metrics`, but no tests exist for `url_link`. Per
BITO.md [11730], new tools should have comprehensive unit tests covering
success paths, error scenarios, and edge cases.
</div>
</div>
<small><i>Code Review Run #3aa5ae</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-table/src/utils/isEqualColumns.ts:
##########
@@ -44,6 +44,8 @@ export default function isEqualColumns(
JSON.stringify(a.formData.extraFormData || null) ===
JSON.stringify(b.formData.extraFormData || null) &&
JSON.stringify(a.rawFormData.column_config || null) ===
- JSON.stringify(b.rawFormData.column_config || null)
+ JSON.stringify(b.rawFormData.column_config || null) &&
+ JSON.stringify(a.rawFormData.url_link || null) ===
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate column_config comparison</b></div>
<div id="fix">
Duplicate comparison detected: `formData.columnConfig` (line 36) and
`rawFormData.column_config` (line 47) reference identical data.
`transformProps.ts` line 215 confirms `columnConfig` is destructured from
`rawFormData.column_config`. Remove the redundant check at line 47-48 to
eliminate unnecessary JSON.stringify operations in memoization comparison.
</div>
</div>
<small><i>Code Review Run #3aa5ae</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -693,6 +695,28 @@ const transformProps = (
: '';
const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ const columnUrlLinks = getTextFromValues(urlLink) ?? [];
+
+ // append url link columns to columns
+ if (columnUrlLinks.length) {
+ const existingColumnKeys = new Set(columns.map(col => col.key));
+ columnUrlLinks.forEach(({ column }) => {
+ if (!existingColumnKeys.has(column)) {
+ existingColumnKeys.add(column);
+ columns.push({
+ key: column,
+ label: column,
+ dataType: GenericDataType.String,
+ isNumeric: false,
+ isMetric: false,
+ isPercentMetric: false,
+ config: {},
+ });
+ }
+ });
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test coverage gap</b></div>
<div id="fix">
New `url_link` feature adds column URL linking but has zero test coverage.
Per BITO.md rule [6262], tests must verify actual business logic rather than
just component rendering. Add unit tests covering url_link config parsing,
columnUrlLinks generation, and edge cases like empty configs.
</div>
</div>
<small><i>Code Review Run #3aa5ae</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
