korbit-ai[bot] commented on code in PR #33340: URL: https://github.com/apache/superset/pull/33340#discussion_r2080105266
########## superset-frontend/src/database/reducers.ts: ########## @@ -0,0 +1,56 @@ +/** + * 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 type { QueryAdhocState } from './types'; + +const initialState: QueryAdhocState = { + isLoading: null, Review Comment: ### Incorrect Loading State Type <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The 'isLoading' state is initialized as null when it should be a boolean to properly represent loading state. ###### Why this matters Using null for a loading state can lead to ambiguous state handling and potentially incorrect UI rendering decisions. ###### Suggested change ∙ *Feature Preview* Initialize isLoading as a boolean false: ```typescript const initialState: QueryAdhocState = { isLoading: false, sql: null, queryResult: null, error: null, }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9a79524-66e0-4211-bf35-c6a29697b45c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9a79524-66e0-4211-bf35-c6a29697b45c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9a79524-66e0-4211-bf35-c6a29697b45c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9a79524-66e0-4211-bf35-c6a29697b45c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9a79524-66e0-4211-bf35-c6a29697b45c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:134e577c-d504-4777-837c-7cdda6a17ea9 --> [](134e577c-d504-4777-837c-7cdda6a17ea9) ########## superset-frontend/src/database/actions.ts: ########## @@ -0,0 +1,68 @@ +/** + * 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 { makeApi } from '@superset-ui/core'; +import { ThunkDispatch } from 'redux-thunk'; +import { AnyAction } from 'redux'; +import { QueryExecutePayload, QueryExecuteResponse } from './types'; + +export const executeQueryApi = makeApi< + QueryExecutePayload, + QueryExecuteResponse +>({ + method: 'POST', + endpoint: '/api/v1/sqllab/execute', +}); + +export function setQueryIsLoading(isLoading: boolean) { + return { + type: 'SET_QUERY_IS_LOADING', + payload: isLoading, + }; +} +export function setQueryResult(queryResult: QueryExecuteResponse) { + return { + type: 'SET_QUERY_RESULT', + payload: queryResult, + }; +} +export function resetDatabaseState() { + return { + type: 'RESET_DATABASE_STATE', + }; +} +export function setQueryError(error: string) { + return { + type: 'SET_QUERY_ERROR', + payload: error, + }; +} +export function executeQuery(payload: QueryExecutePayload) { + return async function (dispatch: ThunkDispatch<any, undefined, AnyAction>) { + try { + dispatch(setQueryIsLoading(true)); + const result = await executeQueryApi(payload); + dispatch(setQueryResult(result as QueryExecuteResponse)); Review Comment: ### Unsafe API Response Type Assertion <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The type assertion (as QueryExecuteResponse) bypasses TypeScript's type checking without validating the actual response structure. ###### Why this matters If the API response structure changes or is malformed, the application might crash when trying to access properties that don't exist. ###### Suggested change ∙ *Feature Preview* Implement runtime validation of the API response: ```typescript const result = await executeQueryApi(payload); if (!isValidQueryResponse(result)) { throw new Error('Invalid response format from server'); } dispatch(setQueryResult(result)); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2c188fa0-22f5-4a12-a693-fd641391133e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2c188fa0-22f5-4a12-a693-fd641391133e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2c188fa0-22f5-4a12-a693-fd641391133e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2c188fa0-22f5-4a12-a693-fd641391133e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2c188fa0-22f5-4a12-a693-fd641391133e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a53c4f13-0757-4d45-b7f1-56f01a30c94f --> [](a53c4f13-0757-4d45-b7f1-56f01a30c94f) ########## superset-frontend/src/components/Datasource/Field.tsx: ########## @@ -62,32 +66,51 @@ onChange: onControlChange, }); return ( - <FormItem - label={ - <FormLabel className="m-r-5"> - {label || fieldKey} - {compact && description && ( - <Tooltip id="field-descr" placement="right" title={description}> - {/* TODO: Remove fa-icon */} - {/* eslint-disable-next-line icons/no-fa-icons-usage */} - <i className="fa fa-info-circle m-l-5" /> - </Tooltip> - )} - </FormLabel> + <div + css={ + additionalControl && + css` + position: relative; + ` } - css={inline && formItemInlineCss} > - {hookedControl} - {!compact && description && ( + {additionalControl} + <FormItem + label={ + <FormLabel className="m-r-5"> + {label || fieldKey} + {compact && description && ( + <Tooltip id="field-descr" placement="right" title={description}> + <Icons.InfoCircleFilled iconSize="s" className="m-l-5" /> + </Tooltip> + )} + </FormLabel> + } + css={inline && formItemInlineCss} + > + {hookedControl} + {!compact && description && ( + <div + css={(theme: SupersetTheme) => ({ + color: theme.colors.grayscale.base, + [inline ? 'marginLeft' : 'marginTop']: theme.gridUnit, + })} + > + {description} + </div> + )} + </FormItem> + {errorMessage && ( <div css={(theme: SupersetTheme) => ({ - color: theme.colors.grayscale.base, - [inline ? 'marginLeft' : 'marginTop']: theme.gridUnit, + color: theme.colors.error.base, + marginTop: -16, Review Comment: ### Hardcoded Magic Number in Styling <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Magic number -16 is used for margin styling without explanation or variable reference. ###### Why this matters Hardcoded numbers make it difficult to understand the intent and relationship to other styling values, and complicate maintenance. ###### Suggested change ∙ *Feature Preview* ```typescript marginTop: theme.gridUnit * -4, // -16px for error message spacing ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ea690f1-22e5-4564-8a8f-817f5e435b55/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ea690f1-22e5-4564-8a8f-817f5e435b55?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ea690f1-22e5-4564-8a8f-817f5e435b55?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ea690f1-22e5-4564-8a8f-817f5e435b55?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ea690f1-22e5-4564-8a8f-817f5e435b55) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:8fc3968e-3b6d-4c69-bffe-22192900bcdc --> [](8fc3968e-3b6d-4c69-bffe-22192900bcdc) ########## superset-frontend/src/database/actions.ts: ########## @@ -0,0 +1,68 @@ +/** + * 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 { makeApi } from '@superset-ui/core'; +import { ThunkDispatch } from 'redux-thunk'; +import { AnyAction } from 'redux'; +import { QueryExecutePayload, QueryExecuteResponse } from './types'; + +export const executeQueryApi = makeApi< + QueryExecutePayload, + QueryExecuteResponse +>({ + method: 'POST', + endpoint: '/api/v1/sqllab/execute', +}); + +export function setQueryIsLoading(isLoading: boolean) { + return { + type: 'SET_QUERY_IS_LOADING', + payload: isLoading, + }; +} +export function setQueryResult(queryResult: QueryExecuteResponse) { + return { + type: 'SET_QUERY_RESULT', + payload: queryResult, + }; +} +export function resetDatabaseState() { + return { + type: 'RESET_DATABASE_STATE', + }; +} +export function setQueryError(error: string) { + return { + type: 'SET_QUERY_ERROR', + payload: error, + }; +} +export function executeQuery(payload: QueryExecutePayload) { + return async function (dispatch: ThunkDispatch<any, undefined, AnyAction>) { + try { + dispatch(setQueryIsLoading(true)); + const result = await executeQueryApi(payload); + dispatch(setQueryResult(result as QueryExecuteResponse)); Review Comment: ### Missing Query Rate Limiting <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? No debounce/throttle mechanism implemented for SQL query execution, which could lead to rapid-fire API calls if the user makes multiple quick changes. ###### Why this matters Without rate limiting, users could accidentally trigger multiple expensive SQL queries in quick succession, potentially overwhelming the server and degrading application performance. ###### Suggested change ∙ *Feature Preview* Implement debouncing using a utility like lodash's debounce: ```typescript import { debounce } from 'lodash'; export const debouncedExecuteQuery = debounce( (payload: QueryExecutePayload, dispatch: ThunkDispatch<any, undefined, AnyAction>) => { executeQuery(payload)(dispatch); }, 500 ); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18cd9755-f86a-4e0e-96ee-75661d7679cd/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18cd9755-f86a-4e0e-96ee-75661d7679cd?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18cd9755-f86a-4e0e-96ee-75661d7679cd?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18cd9755-f86a-4e0e-96ee-75661d7679cd?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18cd9755-f86a-4e0e-96ee-75661d7679cd) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:315bc01f-1e47-4ea7-8f4e-7ffb7f10f2e9 --> [](315bc01f-1e47-4ea7-8f4e-7ffb7f10f2e9) ########## superset-frontend/src/database/types.ts: ########## @@ -0,0 +1,57 @@ +/** + * 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. + */ + +export interface QueryExecutePayload { + client_id: string; + database_id: number; + json: boolean; + runAsync: boolean; + catalog: string | null; + schema: string; + sql: string; + tmp_table_name: string; + select_as_cta: boolean; + ctas_method: string; + queryLimit: number; + expand_data: boolean; +} +export interface Column { + name: string; + type: string; + is_dttm: boolean; + type_generic: number; + is_hidden: boolean; + column_name: string; +} Review Comment: ### Redundant Column Properties <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Column interface has redundant properties 'name' and 'column_name' which appear to represent the same concept. ###### Why this matters Having duplicate properties with different names increases maintenance burden and can lead to inconsistent usage across the codebase. It violates the DRY principle and can cause confusion about which property should be used. ###### Suggested change ∙ *Feature Preview* ```typescript export interface Column { name: string; type: string; is_dttm: boolean; type_generic: number; is_hidden: boolean; } ``` Standardize on using either 'name' or 'column_name', not both. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/317b2288-5434-431c-afc8-4c0b20ce8553/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/317b2288-5434-431c-afc8-4c0b20ce8553?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/317b2288-5434-431c-afc8-4c0b20ce8553?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/317b2288-5434-431c-afc8-4c0b20ce8553?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/317b2288-5434-431c-afc8-4c0b20ce8553) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:d9c92136-a816-4f6d-8023-a899eedb7d61 --> [](d9c92136-a816-4f6d-8023-a899eedb7d61) ########## superset-frontend/src/components/Datasource/DatasourceEditor.jsx: ########## @@ -1078,14 +1103,62 @@ <TextAreaControl language="sql" offerEditInModal={false} - minLines={20} + minLines={10} maxLines={Infinity} readOnly={!this.state.isEditMode} resize="both" tooltipOptions={sqlTooltipOptions} /> } + additionalControl={ + <div + css={css` + position: absolute; + right: 0; + top: 0; + z-index: 2; + `} + > + <Button + css={css` + align-self: flex-end; + height: 24px; + padding-left: 6px; + padding-right: 6px; + `} + size="small" + buttonStyle="primary" + onClick={() => { + this.onQueryRun(); + }} + > + <Icons.CaretRightFilled + iconSize="s" + css={theme => ({ + color: theme.colors.grayscale.light5, + })} + /> + </Button> + </div> + } + errorMessage={ + this.props.database?.error && t('Error executing query.') + } /> + {this.props.database?.queryResult && ( + <ResultTable + data={this.props.database.queryResult.data} + queryId={this.props.database.queryResult.query.id} + orderedColumnKeys={this.props.database.queryResult.columns.map( + col => col.column_name, + )} + height={100} Review Comment: ### Insufficient result table height <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The ResultTable has a fixed height of 100px which may not be sufficient for displaying query results effectively. ###### Why this matters A fixed small height could make it difficult for users to view and analyze query results, especially for queries returning many rows. ###### Suggested change ∙ *Feature Preview* Make the height dynamic based on content or viewport: ```javascript height={Math.min(window.innerHeight * 0.4, 400)} // 40% of viewport height, max 400px ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0c80c17-a0f6-41c5-bc94-1f0c52c2076e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0c80c17-a0f6-41c5-bc94-1f0c52c2076e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0c80c17-a0f6-41c5-bc94-1f0c52c2076e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0c80c17-a0f6-41c5-bc94-1f0c52c2076e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0c80c17-a0f6-41c5-bc94-1f0c52c2076e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:958ee215-bd01-459f-a2ba-0c5af8d23ccf --> [](958ee215-bd01-459f-a2ba-0c5af8d23ccf) ########## superset-frontend/src/components/Datasource/DatasourceEditor.jsx: ########## @@ -698,6 +706,23 @@ class DatasourceEditor extends PureComponent { this.validate(this.onChange); } + async onQueryRun() { + this.props.runQuery({ + client_id: this.props.clientId, + database_id: this.state.datasource.database.id, + json: true, + runAsync: false, + catalog: this.state.datasource.catalog, + schema: this.state.datasource.schema, + sql: this.state.datasource.sql, + tmp_table_name: '', + select_as_cta: false, + ctas_method: 'TABLE', + queryLimit: 25, + expand_data: true, + }); Review Comment: ### Missing required clientId prop <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The client_id is being accessed from props but isn't defined in the component's propTypes or being passed from the parent component. ###### Why this matters This could lead to undefined client_id in the query execution, potentially causing the query to fail or be tracked incorrectly. ###### Suggested change ∙ *Feature Preview* Add clientId to propTypes and ensure it's passed from the parent component: ```javascript const propTypes = { clientId: PropTypes.string.isRequired, // ... other propTypes }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f44ab94d-6601-4253-8f69-eaa181b10fb3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f44ab94d-6601-4253-8f69-eaa181b10fb3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f44ab94d-6601-4253-8f69-eaa181b10fb3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f44ab94d-6601-4253-8f69-eaa181b10fb3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f44ab94d-6601-4253-8f69-eaa181b10fb3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4a695d74-8d83-4731-b971-bc29457c988a --> [](4a695d74-8d83-4731-b971-bc29457c988a) ########## superset-frontend/src/components/Datasource/DatasourceEditor.jsx: ########## @@ -698,6 +706,23 @@ this.validate(this.onChange); } + async onQueryRun() { + this.props.runQuery({ + client_id: this.props.clientId, + database_id: this.state.datasource.database.id, + json: true, + runAsync: false, + catalog: this.state.datasource.catalog, + schema: this.state.datasource.schema, + sql: this.state.datasource.sql, + tmp_table_name: '', + select_as_cta: false, + ctas_method: 'TABLE', + queryLimit: 25, + expand_data: true, + }); + } Review Comment: ### Synchronous Query Execution <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The query execution is set to synchronous mode (runAsync: false) which can block the UI thread for long-running queries. ###### Why this matters Synchronous query execution can lead to poor user experience as the UI becomes unresponsive while waiting for query results. ###### Suggested change ∙ *Feature Preview* Set `runAsync: true` and implement proper loading states and error handling: ```jsx async onQueryRun() { this.setState({ isLoading: true }); try { await this.props.runQuery({ ..., runAsync: true, }); } catch (error) { // Handle error } finally { this.setState({ isLoading: false }); } } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd11079d-8fb7-418c-aa71-ecb06fc27323/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd11079d-8fb7-418c-aa71-ecb06fc27323?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd11079d-8fb7-418c-aa71-ecb06fc27323?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd11079d-8fb7-418c-aa71-ecb06fc27323?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd11079d-8fb7-418c-aa71-ecb06fc27323) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f8a558d1-0efa-461b-9b05-a1dfb2740c85 --> [](f8a558d1-0efa-461b-9b05-a1dfb2740c85) ########## superset-frontend/src/components/Datasource/Field.tsx: ########## @@ -21,23 +21,25 @@ import { useCallback, ReactNode, ReactElement, cloneElement } from 'react'; import { css, SupersetTheme } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; import { FormItem, FormLabel } from 'src/components/Form'; +import { Icons } from 'src/components/Icons'; const formItemInlineCss = css` .ant-form-item-control-input-content { display: flex; flex-direction: row; } `; - interface FieldProps<V> { fieldKey: string; value?: V; label: string; description?: ReactNode; control: ReactElement; + additionalControl?: ReactElement; onChange: (fieldKey: string, newValue: V) => void; compact: boolean; inline: boolean; + errorMessage?: string; } Review Comment: ### Missing interface property descriptions <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The interface properties lack descriptions explaining their purpose and usage implications ###### Why this matters This makes it harder for developers to understand how to properly use these props, potentially leading to incorrect implementation or requiring them to dive into the implementation details ###### Suggested change ∙ *Feature Preview* ```typescript interface FieldProps<V> { /** Unique identifier for the field */ fieldKey: string; /** Current value of the field */ value?: V; /** Display label for the field */ label: string; /** Optional help text shown below the field (or in tooltip if compact=true) */ description?: ReactNode; /** The form control element to render */ control: ReactElement; /** Optional secondary control element */ additionalControl?: ReactElement; /** Callback when field value changes */ onChange: (fieldKey: string, newValue: V) => void; /** Whether to show description in tooltip instead of below field */ compact: boolean; /** Whether to render the field inline */ inline: boolean; /** Error message to display below the field */ errorMessage?: string; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42f329c3-98f4-4df6-8c5d-b67bdb878d96/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42f329c3-98f4-4df6-8c5d-b67bdb878d96?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42f329c3-98f4-4df6-8c5d-b67bdb878d96?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42f329c3-98f4-4df6-8c5d-b67bdb878d96?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/42f329c3-98f4-4df6-8c5d-b67bdb878d96) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f2e66c1c-875d-4567-ace5-9617dd31652b --> [](f2e66c1c-875d-4567-ace5-9617dd31652b) -- 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]
