korbit-ai[bot] commented on code in PR #34868: URL: https://github.com/apache/superset/pull/34868#discussion_r2304676510
########## superset-frontend/src/visualizations/TimeTable/utils/rowProcessing/rowProcessing.ts: ########## @@ -0,0 +1,35 @@ +/** + * 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 { TimeTableData, Entry } from '../../types'; + +/** + * Converts raw time table data into sorted entries + */ +export function processTimeTableData(data: TimeTableData): { + entries: Entry[]; + reversedEntries: Entry[]; +} { + const entries: Entry[] = Object.keys(data) + .sort() + .map(time => ({ ...data[time], time })); + + const reversedEntries = entries.concat().reverse(); Review Comment: ### Inefficient Array Reversal <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unnecessary array copying with concat() before reverse operation. ###### Why this matters The concat() operation creates a new array unnecessarily since reverse() already creates a new array in modern JavaScript/TypeScript. ###### Suggested change ∙ *Feature Preview* Simply create reversed entries using spread operator: ```typescript const reversedEntries = [...entries].reverse(); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ad25dbed-6939-4520-9f87-f47f8cda3b12/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ad25dbed-6939-4520-9f87-f47f8cda3b12?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ad25dbed-6939-4520-9f87-f47f8cda3b12?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ad25dbed-6939-4520-9f87-f47f8cda3b12?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ad25dbed-6939-4520-9f87-f47f8cda3b12) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:3e504611-784b-49a0-8682-cbb57d432245 --> [](3e504611-784b-49a0-8682-cbb57d432245) ########## superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx: ########## @@ -0,0 +1,65 @@ +/** + * 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 { ReactElement } from 'react'; +import Mustache from 'mustache'; +import { Typography } from '@superset-ui/core/components'; +import { MetricOption } from '@superset-ui/chart-controls'; +import type { Row } from '../../types'; + +interface LeftCellProps { + row: Row; + rowType: 'column' | 'metric'; + url?: string; +} + +/** + * Renders the left cell containing either column labels or metric information + */ +function LeftCell({ row, rowType, url }: LeftCellProps): ReactElement { + const context = { metric: row }; + const fullUrl = url ? Mustache.render(url, context) : undefined; Review Comment: ### Memoize URL Template Rendering <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Mustache template rendering is executed on every component render, even if the url and row props haven't changed. ###### Why this matters Unnecessary re-computation of the URL template on each render can impact performance, especially if the component re-renders frequently or the template is complex. ###### Suggested change ∙ *Feature Preview* Use useMemo to cache the rendered URL until the dependencies change: ```typescript const fullUrl = useMemo(() => { if (!url) return undefined; const context = { metric: row }; return Mustache.render(url, context); }, [url, row]); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcfdd090-6c81-452c-b6b7-eb13e0a3c2c2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcfdd090-6c81-452c-b6b7-eb13e0a3c2c2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcfdd090-6c81-452c-b6b7-eb13e0a3c2c2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcfdd090-6c81-452c-b6b7-eb13e0a3c2c2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bcfdd090-6c81-452c-b6b7-eb13e0a3c2c2) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:bfe012d4-98bd-4404-bd9c-1401fd8a3af9 --> [](bfe012d4-98bd-4404-bd9c-1401fd8a3af9) ########## superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx: ########## @@ -0,0 +1,65 @@ +/** + * 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 { ReactElement } from 'react'; +import Mustache from 'mustache'; +import { Typography } from '@superset-ui/core/components'; +import { MetricOption } from '@superset-ui/chart-controls'; +import type { Row } from '../../types'; + +interface LeftCellProps { + row: Row; + rowType: 'column' | 'metric'; + url?: string; +} + +/** + * Renders the left cell containing either column labels or metric information + */ +function LeftCell({ row, rowType, url }: LeftCellProps): ReactElement { + const context = { metric: row }; + const fullUrl = url ? Mustache.render(url, context) : undefined; + + if (rowType === 'column') { + const column = row; + + if (fullUrl) + return ( + <Typography.Link + href={fullUrl} + rel="noopener noreferrer" + target="_blank" + > + {column.label} + </Typography.Link> + ); + + return <span>{column.label || ''}</span>; + } + + return ( + <MetricOption + metric={row as any} Review Comment: ### Unsafe Type Assertion in MetricOption <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using 'as any' type assertion on the row prop passed to MetricOption could lead to runtime errors if the row data structure doesn't match what MetricOption expects. ###### Why this matters Runtime errors could occur if the data passed doesn't match the expected shape, potentially causing the component to crash or display incorrect data. ###### Suggested change ∙ *Feature Preview* Define and use proper type for the metric prop: ```typescript interface MetricRow { metric_name: string; label: string; // add other required properties } // Then update the component: metric={row as MetricRow} ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fd63217-76c6-409b-8d69-d0db4ed35092/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fd63217-76c6-409b-8d69-d0db4ed35092?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fd63217-76c6-409b-8d69-d0db4ed35092?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fd63217-76c6-409b-8d69-d0db4ed35092?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fd63217-76c6-409b-8d69-d0db4ed35092) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:41652a11-ddbf-4a25-bb3e-eeb3067b0af0 --> [](41652a11-ddbf-4a25-bb3e-eeb3067b0af0) ########## superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts: ########## @@ -0,0 +1,132 @@ +/** + * 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 { ColumnConfig, Entry, Stats } from '../../types'; + +export interface ValueCalculationResult { + value: number | null; + errorMsg?: string; +} + +/** + * Calculates time-based values with lag and comparison types + */ +export function calculateTimeValue( + recent: number | null, + valueField: string, + reversedEntries: Entry[], + column: ColumnConfig, +): ValueCalculationResult { + const timeLag = column.timeLag || 0; + const totalLag = Object.keys(reversedEntries).length; + + if (Math.abs(timeLag) >= totalLag) + return { + value: null, + errorMsg: `The time lag set at ${timeLag} is too large for the length of data at ${reversedEntries.length}. No data available.`, + }; + + let laggedValue: number | null; + + if (timeLag < 0) + laggedValue = reversedEntries[totalLag + timeLag][valueField]; + else laggedValue = reversedEntries[timeLag][valueField]; + + if (typeof laggedValue !== 'number' || typeof recent !== 'number') + return { value: null }; + + let calculatedValue: number; + + if (column.comparisonType === 'diff') calculatedValue = recent - laggedValue; + else if (column.comparisonType === 'perc') + calculatedValue = recent / laggedValue; + else if (column.comparisonType === 'perc_change') + calculatedValue = recent / laggedValue - 1; + else calculatedValue = laggedValue; + + return { value: calculatedValue }; +} + +/** + * Calculates contribution value (percentage of total) + */ +export function calculateContribution( + recent: number | null, + reversedEntries: Entry[], +): ValueCalculationResult { + if (typeof recent !== 'number' || reversedEntries.length === 0) + return { value: null }; + + const total = Object.keys(reversedEntries[0]) + .map(k => (k !== 'time' ? reversedEntries[0][k] : 0)) + .reduce((a: number, b: number) => a + b, 0); Review Comment: ### Inefficient Double Iteration for Sum Calculation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unnecessary double iteration over object values when calculating total. Using Object.keys().map() followed by reduce is less efficient than a single iteration. ###### Why this matters This creates additional memory overhead and processing time by creating an intermediate array unnecessarily. ###### Suggested change ∙ *Feature Preview* Use Object.values() with a single reduce operation or a simple for...in loop: ```typescript const total = Object.values(reversedEntries[0]) .reduce((sum, val) => (val === 'time' ? sum : sum + val), 0); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3dee170d-bab9-4804-9fc3-4ec85168fa2b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3dee170d-bab9-4804-9fc3-4ec85168fa2b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3dee170d-bab9-4804-9fc3-4ec85168fa2b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3dee170d-bab9-4804-9fc3-4ec85168fa2b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3dee170d-bab9-4804-9fc3-4ec85168fa2b) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:2ef769e9-a021-484a-8576-6ba480b139c4 --> [](2ef769e9-a021-484a-8576-6ba480b139c4) ########## superset-frontend/src/visualizations/TimeTable/components/FormattedNumber/FormattedNumber.tsx: ########## @@ -19,16 +19,23 @@ import { formatNumber } from '@superset-ui/core'; interface FormattedNumberProps { - num?: string | number; + num?: string | number | null; format?: string; } function FormattedNumber({ num = 0, format }: FormattedNumberProps) { - if (format) { - // @ts-expect-error formatNumber can actually accept strings, even though it's not typed as such - return <span title={`${num}`}>{formatNumber(format, num)}</span>; - } - return <span>{num}</span>; + const displayNum = num ?? 0; + const numericValue = + typeof displayNum === 'string' ? parseFloat(displayNum) : displayNum; + + if (format) + return ( + <span title={`${displayNum}`}> + {formatNumber(format, Number.isNaN(numericValue) ? 0 : numericValue)} + </span> + ); + + return <span>{displayNum}</span>; } Review Comment: ### Mixed presentation and data transformation concerns <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The component mixes presentation logic with data transformation logic. The numeric value parsing and validation should be extracted into a separate utility function. ###### Why this matters Violates the Single Responsibility Principle, making the component harder to test and maintain. The mixing of concerns also makes it harder to reuse the number parsing logic elsewhere. ###### Suggested change ∙ *Feature Preview* ```typescript const parseNumericValue = (value: string | number | null, defaultValue = 0): number => { const sanitizedValue = value ?? defaultValue; const numericValue = typeof sanitizedValue === 'string' ? parseFloat(sanitizedValue) : sanitizedValue; return Number.isNaN(numericValue) ? defaultValue : numericValue; }; function FormattedNumber({ num = 0, format }: FormattedNumberProps) { const displayNum = num ?? 0; const numericValue = parseNumericValue(num); if (format) return <span title={`${displayNum}`}>{formatNumber(format, numericValue)}</span>; return <span>{displayNum}</span>; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564b095a-1f68-4185-935c-6d3f21d2a95b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564b095a-1f68-4185-935c-6d3f21d2a95b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564b095a-1f68-4185-935c-6d3f21d2a95b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564b095a-1f68-4185-935c-6d3f21d2a95b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564b095a-1f68-4185-935c-6d3f21d2a95b) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:7623aa14-785f-4655-91c5-d74fc8da49f2 --> [](7623aa14-785f-4655-91c5-d74fc8da49f2) ########## superset-frontend/src/visualizations/TimeTable/utils/rowProcessing/rowProcessing.ts: ########## @@ -0,0 +1,35 @@ +/** + * 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 { TimeTableData, Entry } from '../../types'; + +/** + * Converts raw time table data into sorted entries + */ +export function processTimeTableData(data: TimeTableData): { + entries: Entry[]; + reversedEntries: Entry[]; +} { + const entries: Entry[] = Object.keys(data) + .sort() Review Comment: ### Incorrect Time Value Sorting <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The sorting of time entries is using default string comparison which may not correctly sort timestamps or numerical time values. ###### Why this matters Using default string sorting for time values can lead to incorrect chronological ordering, especially with timestamps that don't follow lexicographical order. ###### Suggested change ∙ *Feature Preview* Implement explicit time-based sorting using a proper comparison function: ```typescript .sort((a, b) => { const timeA = new Date(a).getTime(); const timeB = new Date(b).getTime(); return timeA - timeB; }) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/416ae3e5-0180-489c-9352-87e9b2cb5c55/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/416ae3e5-0180-489c-9352-87e9b2cb5c55?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/416ae3e5-0180-489c-9352-87e9b2cb5c55?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/416ae3e5-0180-489c-9352-87e9b2cb5c55?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/416ae3e5-0180-489c-9352-87e9b2cb5c55) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:744da932-dbba-4ae4-9562-cc22c9cb395c --> [](744da932-dbba-4ae4-9562-cc22c9cb395c) ########## superset-frontend/src/visualizations/TimeTable/components/FormattedNumber/FormattedNumber.tsx: ########## @@ -19,16 +19,23 @@ import { formatNumber } from '@superset-ui/core'; interface FormattedNumberProps { - num?: string | number; + num?: string | number | null; format?: string; } function FormattedNumber({ num = 0, format }: FormattedNumberProps) { - if (format) { - // @ts-expect-error formatNumber can actually accept strings, even though it's not typed as such - return <span title={`${num}`}>{formatNumber(format, num)}</span>; - } - return <span>{num}</span>; + const displayNum = num ?? 0; + const numericValue = + typeof displayNum === 'string' ? parseFloat(displayNum) : displayNum; Review Comment: ### Redundant Type Conversions Per Render <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code performs unnecessary type checks and conversions for every render, even when the input type hasn't changed. ###### Why this matters Unnecessary type checks and conversions on every render can impact performance, especially when the component is rendered frequently or in large lists. ###### Suggested change ∙ *Feature Preview* Memoize the numeric conversion using useMemo to avoid recalculating on every render when inputs haven't changed: ```typescript const displayNum = num ?? 0; const numericValue = useMemo(() => { if (typeof displayNum === 'string') return parseFloat(displayNum); return displayNum; }, [displayNum]); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ddd568a0-becb-401e-8a17-4913dc6bdcba/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ddd568a0-becb-401e-8a17-4913dc6bdcba?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ddd568a0-becb-401e-8a17-4913dc6bdcba?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ddd568a0-becb-401e-8a17-4913dc6bdcba?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ddd568a0-becb-401e-8a17-4913dc6bdcba) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:0834b247-1e26-4b43-b1e1-969522b95e29 --> [](0834b247-1e26-4b43-b1e1-969522b95e29) ########## superset-frontend/src/visualizations/TimeTable/components/LeftCell/LeftCell.tsx: ########## @@ -0,0 +1,65 @@ +/** + * 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 { ReactElement } from 'react'; +import Mustache from 'mustache'; +import { Typography } from '@superset-ui/core/components'; +import { MetricOption } from '@superset-ui/chart-controls'; +import type { Row } from '../../types'; + +interface LeftCellProps { + row: Row; + rowType: 'column' | 'metric'; + url?: string; +} + +/** + * Renders the left cell containing either column labels or metric information + */ +function LeftCell({ row, rowType, url }: LeftCellProps): ReactElement { + const context = { metric: row }; + const fullUrl = url ? Mustache.render(url, context) : undefined; Review Comment: ### Unsafe URL Template Rendering <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Direct template rendering of user-provided URL with unsanitized context data could lead to XSS vulnerabilities. ###### Why this matters If the URL template contains malicious JavaScript and the row data contains specially crafted values, it could result in execution of arbitrary JavaScript when the link is rendered or clicked. ###### Suggested change ∙ *Feature Preview* ```typescript const sanitizeContext = (ctx: any) => ({ metric: typeof ctx.metric === 'object' ? Object.fromEntries( Object.entries(ctx.metric) .map(([k, v]) => [k, String(v).replace(/[<>"']/g, '')]) ) : String(ctx.metric) }); const fullUrl = url ? Mustache.render(url, sanitizeContext(context)) : undefined; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2902802b-6234-424b-8f37-73c3dc3d5cea/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2902802b-6234-424b-8f37-73c3dc3d5cea?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2902802b-6234-424b-8f37-73c3dc3d5cea?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2902802b-6234-424b-8f37-73c3dc3d5cea?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2902802b-6234-424b-8f37-73c3dc3d5cea) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:d825e7da-5d60-48fd-88ba-b06def6a561d --> [](d825e7da-5d60-48fd-88ba-b06def6a561d) ########## superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts: ########## @@ -0,0 +1,132 @@ +/** + * 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 { ColumnConfig, Entry, Stats } from '../../types'; + +export interface ValueCalculationResult { + value: number | null; + errorMsg?: string; +} + +/** + * Calculates time-based values with lag and comparison types + */ +export function calculateTimeValue( + recent: number | null, + valueField: string, + reversedEntries: Entry[], + column: ColumnConfig, +): ValueCalculationResult { + const timeLag = column.timeLag || 0; + const totalLag = Object.keys(reversedEntries).length; + + if (Math.abs(timeLag) >= totalLag) + return { + value: null, + errorMsg: `The time lag set at ${timeLag} is too large for the length of data at ${reversedEntries.length}. No data available.`, + }; + + let laggedValue: number | null; + + if (timeLag < 0) + laggedValue = reversedEntries[totalLag + timeLag][valueField]; + else laggedValue = reversedEntries[timeLag][valueField]; + + if (typeof laggedValue !== 'number' || typeof recent !== 'number') + return { value: null }; + + let calculatedValue: number; + + if (column.comparisonType === 'diff') calculatedValue = recent - laggedValue; + else if (column.comparisonType === 'perc') + calculatedValue = recent / laggedValue; + else if (column.comparisonType === 'perc_change') + calculatedValue = recent / laggedValue - 1; + else calculatedValue = laggedValue; + + return { value: calculatedValue }; +} + +/** + * Calculates contribution value (percentage of total) + */ +export function calculateContribution( + recent: number | null, + reversedEntries: Entry[], +): ValueCalculationResult { + if (typeof recent !== 'number' || reversedEntries.length === 0) + return { value: null }; + + const total = Object.keys(reversedEntries[0]) + .map(k => (k !== 'time' ? reversedEntries[0][k] : 0)) + .reduce((a: number, b: number) => a + b, 0); + + if (total === 0) return { value: null }; + + return { value: recent / total }; +} + +/** + * Calculates average value over a time period + */ +export function calculateAverage( + valueField: string, + reversedEntries: Entry[], + column: ColumnConfig, +): ValueCalculationResult { + if (reversedEntries.length === 0) return { value: null }; + + const stats = reversedEntries + .slice(undefined, column.timeLag) Review Comment: ### Suboptimal Array Slice Usage <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using undefined as the first argument to slice is unnecessary and less readable. ###### Why this matters While functionally correct, it creates additional parameter processing overhead in the JavaScript engine. ###### Suggested change ∙ *Feature Preview* Omit the first argument when starting from index 0: ```typescript slice(0, column.timeLag) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f8c9035-d854-45c2-afed-60a757939c30/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f8c9035-d854-45c2-afed-60a757939c30?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f8c9035-d854-45c2-afed-60a757939c30?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f8c9035-d854-45c2-afed-60a757939c30?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f8c9035-d854-45c2-afed-60a757939c30) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:bc5c714a-e929-41e3-981c-782718b97fce --> [](bc5c714a-e929-41e3-981c-782718b97fce) ########## superset-frontend/src/visualizations/TimeTable/utils/valueCalculations/valueCalculations.ts: ########## @@ -0,0 +1,132 @@ +/** + * 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 { ColumnConfig, Entry, Stats } from '../../types'; + +export interface ValueCalculationResult { + value: number | null; + errorMsg?: string; +} + +/** + * Calculates time-based values with lag and comparison types + */ +export function calculateTimeValue( + recent: number | null, + valueField: string, + reversedEntries: Entry[], + column: ColumnConfig, +): ValueCalculationResult { + const timeLag = column.timeLag || 0; + const totalLag = Object.keys(reversedEntries).length; Review Comment: ### Inefficient Array Length Access <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using Object.keys() to get array length is unnecessary as arrays have a length property. ###### Why this matters Creates unnecessary overhead by generating an array of keys just to get the length of an array. ###### Suggested change ∙ *Feature Preview* Access the length property directly: ```typescript const totalLag = reversedEntries.length; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4dd006e-e162-4d0c-bda0-a81b585243a9/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4dd006e-e162-4d0c-bda0-a81b585243a9?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4dd006e-e162-4d0c-bda0-a81b585243a9?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4dd006e-e162-4d0c-bda0-a81b585243a9?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4dd006e-e162-4d0c-bda0-a81b585243a9) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:08e03028-0769-4e41-8290-c04ca4ff18d2 --> [](08e03028-0769-4e41-8290-c04ca4ff18d2) -- 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
