You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2025/07/14 17:11:22 UTC
[PR] chore: asked Claude Code to migrate `src/utils` from js -> ts [superset]
mistercrunch opened a new pull request, #34160:
URL: https://github.com/apache/superset/pull/34160
See title, Claude Code seem to be doing a decent job at migrating js->ts. This is a bit of PoC but could really help complete the ts migration.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: asked Claude Code to migrate `src/utils` from js -> ts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34160:
URL: https://github.com/apache/superset/pull/34160#discussion_r2205491141
##########
superset-frontend/src/utils/datasourceUtils.ts:
##########
@@ -16,7 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-export const getDatasourceAsSaveableDataset = source => ({
+import { Dataset } from '@superset-ui/chart-controls';
+
+interface SaveableDataset {
+ columns: any[];
+ name: string;
+ dbId?: number;
+ sql: string;
+ catalog?: string | null;
+ schema?: string;
+ templateParams?: string;
+}
+
+export const getDatasourceAsSaveableDataset = (
+ source: Partial<Dataset>,
+): SaveableDataset => ({
Review Comment:
### Input type too permissive for output guarantees <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function returns a SaveableDataset with required properties but accepts a Partial<Dataset>, which could lead to runtime errors if required properties are missing.
###### Why this matters
There's a potential mismatch between the input type (which may lack required properties) and the output type (which requires certain properties). This could cause runtime errors that TypeScript can't catch.
###### Suggested change ∙ *Feature Preview*
Either strengthen the input type constraints or handle missing properties explicitly:
```typescript
export const getDatasourceAsSaveableDataset = (
source: Pick<Dataset, 'columns' | 'sql' | 'name' | 'database'>,
): SaveableDataset => ({
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/585c5c99-8e56-44fc-97e2-a115b392cd77/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/585c5c99-8e56-44fc-97e2-a115b392cd77?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/585c5c99-8e56-44fc-97e2-a115b392cd77?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/585c5c99-8e56-44fc-97e2-a115b392cd77?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/585c5c99-8e56-44fc-97e2-a115b392cd77)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7bfdefe5-7802-432b-847c-f49ff07a7f96 -->
[](7bfdefe5-7802-432b-847c-f49ff07a7f96)
##########
superset-frontend/src/utils/datasourceUtils.ts:
##########
@@ -16,7 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-export const getDatasourceAsSaveableDataset = source => ({
+import { Dataset } from '@superset-ui/chart-controls';
+
+interface SaveableDataset {
+ columns: any[];
+ name: string;
+ dbId?: number;
+ sql: string;
+ catalog?: string | null;
+ schema?: string;
+ templateParams?: string;
+}
+
+export const getDatasourceAsSaveableDataset = (
+ source: Partial<Dataset>,
+): SaveableDataset => ({
columns: source.columns,
Review Comment:
### Missing Columns Property Fallback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
No null check or fallback value for the columns property, which is required according to the interface.
###### Why this matters
If source.columns is undefined, this will cause a runtime error as the property is required by the SaveableDataset interface.
###### Suggested change ∙ *Feature Preview*
Add a fallback for the columns property:
```typescript
columns: source.columns || [],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b15aa931-841a-4489-ae1a-4c173bb1cb37/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b15aa931-841a-4489-ae1a-4c173bb1cb37?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b15aa931-841a-4489-ae1a-4c173bb1cb37?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b15aa931-841a-4489-ae1a-4c173bb1cb37?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b15aa931-841a-4489-ae1a-4c173bb1cb37)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a930c56b-39c3-4477-8872-e633b7cfeb22 -->
[](a930c56b-39c3-4477-8872-e633b7cfeb22)
##########
superset-frontend/src/utils/datasourceUtils.ts:
##########
@@ -16,7 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-export const getDatasourceAsSaveableDataset = source => ({
+import { Dataset } from '@superset-ui/chart-controls';
+
+interface SaveableDataset {
+ columns: any[];
+ name: string;
+ dbId?: number;
+ sql: string;
+ catalog?: string | null;
+ schema?: string;
+ templateParams?: string;
+}
Review Comment:
### Required SQL Property Inconsistency <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'sql' property is marked as required in SaveableDataset interface, but the implementation provides an empty string fallback, indicating it should be optional.
###### Why this matters
This mismatch could force consumers to provide SQL when it's not actually required, or cause TypeScript compilation errors when valid data is provided without SQL.
###### Suggested change ∙ *Feature Preview*
Make the 'sql' property optional in the interface to match the implementation:
```typescript
interface SaveableDataset {
columns: any[];
name: string;
dbId?: number;
sql?: string;
catalog?: string | null;
schema?: string;
templateParams?: string;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea41d0f1-6144-4503-928a-1d09c77a1fb4/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea41d0f1-6144-4503-928a-1d09c77a1fb4?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea41d0f1-6144-4503-928a-1d09c77a1fb4?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea41d0f1-6144-4503-928a-1d09c77a1fb4?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea41d0f1-6144-4503-928a-1d09c77a1fb4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ac071011-d01a-451e-8004-bb973ae600c9 -->
[](ac071011-d01a-451e-8004-bb973ae600c9)
##########
superset-frontend/src/utils/getControlsForVizType.ts:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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 {
+ isControlPanelSectionConfig,
+ ControlStateMapping,
+ ControlPanelConfig,
+} from '@superset-ui/chart-controls';
+import { getChartControlPanelRegistry } from '@superset-ui/core';
+import { controls } from '../explore/controls';
+
+const memoizedControls = memoizeOne(
+ (vizType: string, controlPanel: ControlPanelConfig): ControlStateMapping => {
+ const controlsMap: ControlStateMapping = {};
+ (controlPanel?.controlPanelSections || [])
+ .filter(isControlPanelSectionConfig)
+ .forEach(section => {
+ section.controlSetRows.forEach(row => {
+ row.forEach(control => {
+ if (!control) return;
+ if (typeof control === 'string') {
+ // For now, we have to look in controls.jsx to get the config for some controls.
+ // Once everything is migrated out, delete this if statement.
+ controlsMap[control] = controls[control];
+ } else if (control.name && control.config) {
+ // condition needed because there are elements, e.g. <hr /> in some control configs (I'm looking at you, FilterBox!)
+ controlsMap[control.name] = control.config;
+ }
+ });
+ });
+ });
+ return controlsMap;
+ },
+);
+
+const getControlsForVizType = (vizType: string): ControlStateMapping => {
+ const controlPanel = getChartControlPanelRegistry().get(vizType);
+ return memoizedControls(vizType, controlPanel);
Review Comment:
### Missing Error Handling for Invalid Viz Types <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code doesn't handle the case where getChartControlPanelRegistry().get(vizType) returns undefined for an invalid or unregistered vizType.
###### Why this matters
This could lead to runtime errors when attempting to process undefined controlPanel values, potentially causing the application to crash.
###### Suggested change ∙ *Feature Preview*
Add error handling for undefined control panels:
```typescript
const getControlsForVizType = (vizType: string): ControlStateMapping => {
const controlPanel = getChartControlPanelRegistry().get(vizType);
if (!controlPanel) {
return {}; // or throw new Error(`Invalid visualization type: ${vizType}`);
}
return memoizedControls(vizType, controlPanel);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93e5a515-2c77-49aa-9bad-ace25d5091b1/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93e5a515-2c77-49aa-9bad-ace25d5091b1?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93e5a515-2c77-49aa-9bad-ace25d5091b1?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93e5a515-2c77-49aa-9bad-ace25d5091b1?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93e5a515-2c77-49aa-9bad-ace25d5091b1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2d2c12a7-e8b3-4bc1-bb91-12cae561306e -->
[](2d2c12a7-e8b3-4bc1-bb91-12cae561306e)
##########
superset-frontend/src/utils/DebouncedMessageQueue.ts:
##########
@@ -18,33 +18,49 @@
*/
import { debounce } from 'lodash';
+interface DebouncedMessageQueueConfig {
+ callback?: (events: any[]) => void;
+ sizeThreshold?: number;
+ delayThreshold?: number;
+}
+
class DebouncedMessageQueue {
+ private queue: any[];
+
+ private sizeThreshold: number;
+
+ private delayThreshold: number;
+
+ private callback: (events: any[]) => void;
+
+ private trigger: () => void;
+
constructor({
callback = () => {},
sizeThreshold = 1000,
delayThreshold = 1000,
- }) {
+ }: DebouncedMessageQueueConfig = {}) {
this.queue = [];
this.sizeThreshold = sizeThreshold;
this.delayThreshold = delayThreshold;
- this.trigger = debounce(this.trigger.bind(this), this.delayThreshold);
+ this.trigger = debounce(this.triggerQueue.bind(this), this.delayThreshold);
this.callback = callback;
}
- append(eventData) {
+ append(eventData: any): void {
this.queue.push(eventData);
this.trigger();
}
Review Comment:
### Unvalidated Input in Queue <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The append method accepts any type of data without validation, which could lead to arbitrary data being stored and processed in the queue.
###### Why this matters
Without input validation, malicious data could be injected into the queue and potentially executed or processed by the callback function, leading to potential security vulnerabilities if the data contains malicious code or oversized payloads.
###### Suggested change ∙ *Feature Preview*
```typescript
append(eventData: any): void {
if (!eventData || typeof eventData === 'function') {
throw new Error('Invalid event data');
}
this.queue.push(eventData);
this.trigger();
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b0d1c2b-293e-41c6-a09a-74aa9a0a0839/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b0d1c2b-293e-41c6-a09a-74aa9a0a0839?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b0d1c2b-293e-41c6-a09a-74aa9a0a0839?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b0d1c2b-293e-41c6-a09a-74aa9a0a0839?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b0d1c2b-293e-41c6-a09a-74aa9a0a0839)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8643b78a-6d84-42c5-8429-0c5bbca95a1e -->
[](8643b78a-6d84-42c5-8429-0c5bbca95a1e)
##########
superset-frontend/src/utils/DebouncedMessageQueue.ts:
##########
@@ -18,33 +18,49 @@
*/
import { debounce } from 'lodash';
+interface DebouncedMessageQueueConfig {
+ callback?: (events: any[]) => void;
+ sizeThreshold?: number;
+ delayThreshold?: number;
+}
+
class DebouncedMessageQueue {
+ private queue: any[];
+
+ private sizeThreshold: number;
+
+ private delayThreshold: number;
+
+ private callback: (events: any[]) => void;
+
+ private trigger: () => void;
+
constructor({
callback = () => {},
sizeThreshold = 1000,
delayThreshold = 1000,
- }) {
+ }: DebouncedMessageQueueConfig = {}) {
this.queue = [];
this.sizeThreshold = sizeThreshold;
this.delayThreshold = delayThreshold;
- this.trigger = debounce(this.trigger.bind(this), this.delayThreshold);
+ this.trigger = debounce(this.triggerQueue.bind(this), this.delayThreshold);
this.callback = callback;
}
- append(eventData) {
+ append(eventData: any): void {
this.queue.push(eventData);
this.trigger();
}
- trigger() {
+ private triggerQueue(): void {
if (this.queue.length > 0) {
const events = this.queue.splice(0, this.sizeThreshold);
this.callback.call(null, events);
Review Comment:
### Unprotected callback execution <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The callback invocation lacks error handling, despite being an existing point of error handling concern since it executes user-provided code.
###### Why this matters
If the callback throws an error, it will break the queue processing and potentially leave remaining items unprocessed, affecting the reliability of the message queue.
###### Suggested change ∙ *Feature Preview*
```typescript
private triggerQueue(): void {
if (this.queue.length > 0) {
const events = this.queue.splice(0, this.sizeThreshold);
try {
this.callback.call(null, events);
} catch (error) {
// Re-add failed events back to the front of the queue
this.queue.unshift(...events);
console.error('Error processing message queue:', error);
}
// If there are remaining items, call it again.
if (this.queue.length > 0) {
this.triggerQueue();
}
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fedcff2f-8cfe-41a9-b338-cb548ad1600b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fedcff2f-8cfe-41a9-b338-cb548ad1600b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fedcff2f-8cfe-41a9-b338-cb548ad1600b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fedcff2f-8cfe-41a9-b338-cb548ad1600b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fedcff2f-8cfe-41a9-b338-cb548ad1600b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:27b5e027-2896-4ce9-baea-b1ec33425846 -->
[](27b5e027-2896-4ce9-baea-b1ec33425846)
##########
superset-frontend/src/utils/DebouncedMessageQueue.ts:
##########
@@ -18,33 +18,49 @@
*/
import { debounce } from 'lodash';
+interface DebouncedMessageQueueConfig {
+ callback?: (events: any[]) => void;
+ sizeThreshold?: number;
+ delayThreshold?: number;
+}
+
class DebouncedMessageQueue {
+ private queue: any[];
+
+ private sizeThreshold: number;
+
+ private delayThreshold: number;
+
+ private callback: (events: any[]) => void;
+
+ private trigger: () => void;
+
constructor({
callback = () => {},
sizeThreshold = 1000,
delayThreshold = 1000,
- }) {
+ }: DebouncedMessageQueueConfig = {}) {
Review Comment:
### Unexplained Magic Numbers in Defaults <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic numbers 1000 for thresholds lack context about their units or significance.
###### Why this matters
Without understanding if these numbers represent milliseconds, bytes, or items, developers must dig through implementation details or documentation.
###### Suggested change ∙ *Feature Preview*
```typescript
private static readonly DEFAULT_SIZE_THRESHOLD = 1000; // maximum items per batch
private static readonly DEFAULT_DELAY_THRESHOLD = 1000; // milliseconds
constructor({
callback = () => {},
sizeThreshold = DebouncedMessageQueue.DEFAULT_SIZE_THRESHOLD,
delayThreshold = DebouncedMessageQueue.DEFAULT_DELAY_THRESHOLD,
}: DebouncedMessageQueueConfig = {}) {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45684f75-ab0a-4db2-898f-3dd9e687b1e6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45684f75-ab0a-4db2-898f-3dd9e687b1e6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45684f75-ab0a-4db2-898f-3dd9e687b1e6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45684f75-ab0a-4db2-898f-3dd9e687b1e6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45684f75-ab0a-4db2-898f-3dd9e687b1e6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:49231325-8fc1-494e-9078-4d0ca853f598 -->
[](49231325-8fc1-494e-9078-4d0ca853f598)
##########
superset-frontend/src/utils/getControlsForVizType.ts:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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 {
+ isControlPanelSectionConfig,
+ ControlStateMapping,
+ ControlPanelConfig,
+} from '@superset-ui/chart-controls';
+import { getChartControlPanelRegistry } from '@superset-ui/core';
+import { controls } from '../explore/controls';
+
+const memoizedControls = memoizeOne(
+ (vizType: string, controlPanel: ControlPanelConfig): ControlStateMapping => {
+ const controlsMap: ControlStateMapping = {};
+ (controlPanel?.controlPanelSections || [])
+ .filter(isControlPanelSectionConfig)
+ .forEach(section => {
+ section.controlSetRows.forEach(row => {
+ row.forEach(control => {
Review Comment:
### Nested forEach Loops Reduce Readability <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Nested forEach loops with arrow functions make the code harder to follow. The nested structure creates multiple levels of indentation and callback functions.
###### Why this matters
Deep nesting reduces code scanability and makes it harder to track the flow of data transformation. This affects maintainability and debugging.
###### Suggested change ∙ *Feature Preview*
```typescript
for (const row of section.controlSetRows) {
for (const control of row) {
// ... rest of the code
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/983473f2-efbc-4e94-b0f4-ad67591880ee/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/983473f2-efbc-4e94-b0f4-ad67591880ee?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/983473f2-efbc-4e94-b0f4-ad67591880ee?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/983473f2-efbc-4e94-b0f4-ad67591880ee?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/983473f2-efbc-4e94-b0f4-ad67591880ee)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5545d241-0b43-407e-bb50-00fa99636842 -->
[](5545d241-0b43-407e-bb50-00fa99636842)
##########
superset-frontend/src/utils/DebouncedMessageQueue.ts:
##########
@@ -18,33 +18,49 @@
*/
import { debounce } from 'lodash';
+interface DebouncedMessageQueueConfig {
+ callback?: (events: any[]) => void;
+ sizeThreshold?: number;
+ delayThreshold?: number;
+}
+
class DebouncedMessageQueue {
+ private queue: any[];
+
+ private sizeThreshold: number;
+
+ private delayThreshold: number;
+
+ private callback: (events: any[]) => void;
+
+ private trigger: () => void;
+
constructor({
callback = () => {},
sizeThreshold = 1000,
delayThreshold = 1000,
- }) {
+ }: DebouncedMessageQueueConfig = {}) {
this.queue = [];
Review Comment:
### Inefficient array resizing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Array resizing during splice operations can be inefficient for large queues with frequent operations.
###### Why this matters
Memory reallocation and copying during array operations can impact performance when handling large amounts of data.
###### Suggested change ∙ *Feature Preview*
Pre-allocate array capacity when possible or consider using a more efficient data structure like a circular buffer for queue operations:
```typescript
private queue: any[] = new Array(this.sizeThreshold);
private head = 0;
private tail = 0;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/feca26ca-1bc1-4779-a146-21ee3e3be3bd/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/feca26ca-1bc1-4779-a146-21ee3e3be3bd?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/feca26ca-1bc1-4779-a146-21ee3e3be3bd?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/feca26ca-1bc1-4779-a146-21ee3e3be3bd?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/feca26ca-1bc1-4779-a146-21ee3e3be3bd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4740db1f-5c06-48fc-bbd5-bcc238f83942 -->
[](4740db1f-5c06-48fc-bbd5-bcc238f83942)
##########
superset-frontend/src/utils/getControlsForVizType.ts:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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 {
+ isControlPanelSectionConfig,
+ ControlStateMapping,
+ ControlPanelConfig,
+} from '@superset-ui/chart-controls';
+import { getChartControlPanelRegistry } from '@superset-ui/core';
+import { controls } from '../explore/controls';
+
+const memoizedControls = memoizeOne(
+ (vizType: string, controlPanel: ControlPanelConfig): ControlStateMapping => {
+ const controlsMap: ControlStateMapping = {};
+ (controlPanel?.controlPanelSections || [])
+ .filter(isControlPanelSectionConfig)
+ .forEach(section => {
+ section.controlSetRows.forEach(row => {
+ row.forEach(control => {
+ if (!control) return;
+ if (typeof control === 'string') {
+ // For now, we have to look in controls.jsx to get the config for some controls.
+ // Once everything is migrated out, delete this if statement.
Review Comment:
### Temporary Solution Creating Technical Debt <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code contains a technical debt marker indicating a temporary solution that depends on an external controls.jsx file.
###### Why this matters
Temporary solutions can become permanent, leading to maintenance issues and unclear dependencies. This violates the Open-Closed Principle as future changes require modifying existing code.
###### Suggested change ∙ *Feature Preview*
Create an interface for control configurations and implement a proper dependency injection or factory pattern:
```typescript
interface ControlConfigProvider {
getConfig(controlName: string): ControlConfig;
}
const getControlsForVizType = (
vizType: string,
configProvider: ControlConfigProvider
): ControlStateMapping => {
// Use configProvider instead of direct controls import
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a53c81c4-9fc3-4e3d-84c8-7cb30220d9f9/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a53c81c4-9fc3-4e3d-84c8-7cb30220d9f9?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a53c81c4-9fc3-4e3d-84c8-7cb30220d9f9?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a53c81c4-9fc3-4e3d-84c8-7cb30220d9f9?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a53c81c4-9fc3-4e3d-84c8-7cb30220d9f9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:871b931f-9585-406c-880e-c3d9f04b7823 -->
[](871b931f-9585-406c-880e-c3d9f04b7823)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: asked Claude Code to migrate `src/utils` from js -> ts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #34160:
URL: https://github.com/apache/superset/pull/34160#discussion_r2206002149
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -215,7 +214,15 @@ const SaveQuery = ({
onHide={() => setShowSaveDatasetModal(false)}
buttonTextOnSave={t('Save & Explore')}
buttonTextOnOverwrite={t('Overwrite & Explore')}
- datasource={getDatasourceAsSaveableDataset(query)}
+ datasource={{
Review Comment:
seems wrong ... Claude!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: asked Claude Code to migrate `src/utils` from js -> ts [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch closed pull request #34160: chore: asked Claude Code to migrate `src/utils` from js -> ts
URL: https://github.com/apache/superset/pull/34160
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: asked Claude Code to migrate `src/utils` from js -> ts [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #34160:
URL: https://github.com/apache/superset/pull/34160#discussion_r2205582569
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -215,7 +214,15 @@ const SaveQuery = ({
onHide={() => setShowSaveDatasetModal(false)}
buttonTextOnSave={t('Save & Explore')}
buttonTextOnOverwrite={t('Overwrite & Explore')}
- datasource={getDatasourceAsSaveableDataset(query)}
+ datasource={{
Review Comment:
why did we remove `getDatasourceAsSaveableDataset` when we updated the definition below to be the same? Shouldn't we just keep 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org