korbit-ai[bot] commented on code in PR #34707: URL: https://github.com/apache/superset/pull/34707#discussion_r2277436888
########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx: ########## @@ -0,0 +1,257 @@ +/** + * 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 { useState, useEffect, useMemo, useCallback } from 'react'; +import { styled, SupersetClient, t, css } from '@superset-ui/core'; +import rison from 'rison'; +import { + CertifiedBadge, + InfoTooltip, + Loading, +} from '@superset-ui/core/components'; +import Table, { TableSize } from '@superset-ui/core/components/Table'; +import { FacePile, ModifiedInfo, GenericLink } from 'src/components'; +import DashboardLinksExternal from '../DashboardLinksExternal'; + +const FlexRowContainer = styled.div` + align-items: center; + display: flex; + gap: ${({ theme }) => theme.sizeUnit}px; + + a { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + line-height: 1.2; + } + + svg { + margin-right: ${({ theme }) => theme.sizeUnit}px; + } +`; + +const StyledUsageTabWrapper = styled.div``; + +const PAGE_SIZE = 25; + +interface Chart { + id: number; + slice_name: string; + url: string; + certified_by?: string; + certification_details?: string; + description?: string; + owners: Array<{ + first_name: string; + last_name: string; + id: number; + }>; + changed_on_delta_humanized: string; + changed_on?: string; // Optional since it might not be in existing data + changed_by: { + first_name: string; + last_name: string; + id: number; + }; + dashboards: Array<{ + id: number; + dashboard_title: string; + url: string; + }>; +} + +interface DatasetUsageTabProps { + datasourceId: number; + charts?: Chart[]; + onDataLoad?: (charts: Chart[]) => void; + addDangerToast?: (msg: string) => void; +} + +const DatasetUsageTab = ({ + datasourceId, + charts: preloadedCharts = [], + onDataLoad, + addDangerToast, +}: DatasetUsageTabProps) => { + const [loading, setLoading] = useState(!preloadedCharts.length); + const [charts, setCharts] = useState<Chart[]>(preloadedCharts); + + const fetchCharts = useCallback(async () => { + if (!datasourceId || preloadedCharts.length > 0) return; + + setLoading(true); + + try { + const queryParams = rison.encode({ + columns: [ + 'slice_name', + 'url', + 'certified_by', + 'certification_details', + 'description', + 'owners.first_name', + 'owners.last_name', + 'owners.id', + 'changed_on_delta_humanized', + 'changed_on', + 'changed_by.first_name', + 'changed_by.last_name', + 'changed_by.id', + 'dashboards.id', + 'dashboards.dashboard_title', + 'dashboards.url', + ], + filters: [ + { + col: 'datasource_id', + opr: 'eq', + value: datasourceId, + }, + ], + order_column: 'changed_on_delta_humanized', + order_direction: 'desc', + page: 0, + page_size: PAGE_SIZE, + }); + + const { json = {} } = await SupersetClient.get({ + endpoint: `/api/v1/chart/?q=${queryParams}`, + }); + + const newCharts = json?.result || []; + setCharts(newCharts); + onDataLoad?.(newCharts); + } catch (error) { + addDangerToast?.(t('Error fetching charts')); + setCharts([]); + onDataLoad?.([]); + } finally { + setLoading(false); + } + }, [datasourceId, preloadedCharts.length, onDataLoad, addDangerToast]); + + useEffect(() => { + fetchCharts(); + }, [fetchCharts]); + + const columns = useMemo( + () => [ + { + title: t('Chart'), + dataIndex: 'slice_name', + key: 'slice_name', + render: (_: any, record: Chart) => ( + <FlexRowContainer> + <GenericLink + to={record.url} + target="_blank" + data-test={`${record.slice_name}-usage-chart-title`} + > + {record.certified_by && ( + <> + <CertifiedBadge + certifiedBy={record.certified_by} + details={record.certification_details} + />{' '} + </> + )} + {record.slice_name} + </GenericLink> + {record.description && <InfoTooltip tooltip={record.description} />} + </FlexRowContainer> + ), + sorter: (a: Chart, b: Chart) => + a.slice_name.localeCompare(b.slice_name), + width: 300, + }, + { + title: t('Chart owners'), + dataIndex: 'owners', + key: 'owners', + render: (_: any, record: Chart) => <FacePile users={record.owners} />, + sorter: false, + width: 180, + }, + { + title: t('Last modified'), + dataIndex: 'changed_on_delta_humanized', + key: 'changed_on_delta_humanized', + render: (_: any, record: Chart) => ( + <ModifiedInfo + date={record.changed_on_delta_humanized} + user={record.changed_by} + /> + ), + sorter: (a: Chart, b: Chart) => { + if (a.changed_on && b.changed_on) { + const dateA = new Date(a.changed_on).getTime(); + const dateB = new Date(b.changed_on).getTime(); + + if (!Number.isNaN(dateA) && !Number.isNaN(dateB)) + return dateB - dateA; + } + + return a.changed_on_delta_humanized.localeCompare( + b.changed_on_delta_humanized, + ); + }, + width: 200, + }, + { + title: t('Dashboard usage'), + dataIndex: 'dashboards', + key: 'dashboards', + render: (_: any, record: Chart) => ( + <DashboardLinksExternal dashboards={record.dashboards} /> + ), + sorter: false, + width: 200, + }, + ], + [], + ); + + if (loading && charts.length === 0) { + return ( + <StyledUsageTabWrapper> + <Loading /> + </StyledUsageTabWrapper> + ); + } + + return ( + <StyledUsageTabWrapper> + <Table + columns={columns} + data={charts} + sticky + pagination={false} + loading={loading} + size={TableSize.Middle} + rowKey="id" + tableLayout="auto" + css={css` + height: 388px; Review Comment: ### Inflexible table height <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The table has a fixed height which could limit functionality on different screen sizes or with varying amounts of data. ###### Why this matters Users with different screen sizes or more data than fits in 388px might have a degraded experience or miss important information. ###### Suggested change ∙ *Feature Preview* Use a dynamic height calculation or min/max height constraints: ```css height: min(calc(100vh - 200px), 800px); min-height: 200px; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/000c9d9d-e695-4c62-8b55-17316a2fd315/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/000c9d9d-e695-4c62-8b55-17316a2fd315?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/000c9d9d-e695-4c62-8b55-17316a2fd315?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/000c9d9d-e695-4c62-8b55-17316a2fd315?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/000c9d9d-e695-4c62-8b55-17316a2fd315) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:7db6cdcd-9d44-4851-8e8b-6c09ca05012a --> [](7db6cdcd-9d44-4851-8e8b-6c09ca05012a) ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx: ########## @@ -0,0 +1,257 @@ +/** + * 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 { useState, useEffect, useMemo, useCallback } from 'react'; +import { styled, SupersetClient, t, css } from '@superset-ui/core'; +import rison from 'rison'; +import { + CertifiedBadge, + InfoTooltip, + Loading, +} from '@superset-ui/core/components'; +import Table, { TableSize } from '@superset-ui/core/components/Table'; +import { FacePile, ModifiedInfo, GenericLink } from 'src/components'; +import DashboardLinksExternal from '../DashboardLinksExternal'; + +const FlexRowContainer = styled.div` + align-items: center; + display: flex; + gap: ${({ theme }) => theme.sizeUnit}px; + + a { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + line-height: 1.2; + } + + svg { + margin-right: ${({ theme }) => theme.sizeUnit}px; + } +`; + +const StyledUsageTabWrapper = styled.div``; + +const PAGE_SIZE = 25; + +interface Chart { + id: number; + slice_name: string; + url: string; + certified_by?: string; + certification_details?: string; + description?: string; + owners: Array<{ + first_name: string; + last_name: string; + id: number; + }>; + changed_on_delta_humanized: string; + changed_on?: string; // Optional since it might not be in existing data + changed_by: { + first_name: string; + last_name: string; + id: number; + }; + dashboards: Array<{ + id: number; + dashboard_title: string; + url: string; + }>; +} + +interface DatasetUsageTabProps { + datasourceId: number; + charts?: Chart[]; + onDataLoad?: (charts: Chart[]) => void; + addDangerToast?: (msg: string) => void; +} + +const DatasetUsageTab = ({ + datasourceId, + charts: preloadedCharts = [], + onDataLoad, + addDangerToast, +}: DatasetUsageTabProps) => { + const [loading, setLoading] = useState(!preloadedCharts.length); + const [charts, setCharts] = useState<Chart[]>(preloadedCharts); + + const fetchCharts = useCallback(async () => { + if (!datasourceId || preloadedCharts.length > 0) return; + + setLoading(true); + + try { + const queryParams = rison.encode({ + columns: [ + 'slice_name', + 'url', + 'certified_by', + 'certification_details', + 'description', + 'owners.first_name', + 'owners.last_name', + 'owners.id', + 'changed_on_delta_humanized', + 'changed_on', + 'changed_by.first_name', + 'changed_by.last_name', + 'changed_by.id', + 'dashboards.id', + 'dashboards.dashboard_title', + 'dashboards.url', + ], + filters: [ + { + col: 'datasource_id', + opr: 'eq', + value: datasourceId, + }, + ], + order_column: 'changed_on_delta_humanized', + order_direction: 'desc', + page: 0, + page_size: PAGE_SIZE, + }); + + const { json = {} } = await SupersetClient.get({ + endpoint: `/api/v1/chart/?q=${queryParams}`, + }); + + const newCharts = json?.result || []; + setCharts(newCharts); + onDataLoad?.(newCharts); + } catch (error) { + addDangerToast?.(t('Error fetching charts')); + setCharts([]); + onDataLoad?.([]); + } finally { + setLoading(false); + } + }, [datasourceId, preloadedCharts.length, onDataLoad, addDangerToast]); + + useEffect(() => { + fetchCharts(); + }, [fetchCharts]); + + const columns = useMemo( + () => [ + { + title: t('Chart'), + dataIndex: 'slice_name', + key: 'slice_name', + render: (_: any, record: Chart) => ( + <FlexRowContainer> + <GenericLink + to={record.url} + target="_blank" + data-test={`${record.slice_name}-usage-chart-title`} + > + {record.certified_by && ( + <> + <CertifiedBadge + certifiedBy={record.certified_by} + details={record.certification_details} + />{' '} + </> + )} + {record.slice_name} + </GenericLink> + {record.description && <InfoTooltip tooltip={record.description} />} + </FlexRowContainer> + ), + sorter: (a: Chart, b: Chart) => + a.slice_name.localeCompare(b.slice_name), + width: 300, + }, + { + title: t('Chart owners'), + dataIndex: 'owners', + key: 'owners', + render: (_: any, record: Chart) => <FacePile users={record.owners} />, + sorter: false, + width: 180, + }, + { + title: t('Last modified'), + dataIndex: 'changed_on_delta_humanized', + key: 'changed_on_delta_humanized', + render: (_: any, record: Chart) => ( + <ModifiedInfo + date={record.changed_on_delta_humanized} + user={record.changed_by} + /> + ), + sorter: (a: Chart, b: Chart) => { + if (a.changed_on && b.changed_on) { + const dateA = new Date(a.changed_on).getTime(); + const dateB = new Date(b.changed_on).getTime(); + + if (!Number.isNaN(dateA) && !Number.isNaN(dateB)) + return dateB - dateA; + } Review Comment: ### Inefficient Date Parsing in Sort Function <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Date parsing operations are performed repeatedly during sorting, creating unnecessary performance overhead. ###### Why this matters Each sort comparison creates new Date objects and performs date parsing, which is computationally expensive when sorting large datasets. ###### Suggested change ∙ *Feature Preview* Transform dates to timestamps when data is loaded and store in a separate property: ```typescript const processCharts = (charts: Chart[]) => charts.map(chart => ({ ...chart, changed_on_ts: chart.changed_on ? new Date(chart.changed_on).getTime() : null })); // Update sorter to use pre-computed timestamp sorter: (a, b) => (b.changed_on_ts ?? 0) - (a.changed_on_ts ?? 0) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a83365b2-81db-4a52-8f9a-93e79a271787/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a83365b2-81db-4a52-8f9a-93e79a271787?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a83365b2-81db-4a52-8f9a-93e79a271787?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a83365b2-81db-4a52-8f9a-93e79a271787?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a83365b2-81db-4a52-8f9a-93e79a271787) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f89a6936-71c0-4949-8646-6610bdfca055 --> [](f89a6936-71c0-4949-8646-6610bdfca055) -- 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-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
