feat: dashboard transpose and dynamic columns #4592
Conversation
WalkthroughThe pull request introduces new configuration options for dashboard panels, specifically for table presentations. It adds optional fields to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ktx-vaidehi
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
9d95af8 to
321475b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (2)
web/src/utils/dashboard/convertTableData.ts (2)
54-85: Potential unnecessary mutation of 'columnData'When
table_dynamic_columnsis enabled, additional fields are appended tocolumnData. IfcolumnDatais used elsewhere, this mutation could have unintended side effects. Consider creating a new array or cloningcolumnDatabefore modification to prevent side effects.
248-260: Avoid reassigning 'tableRows' when transposing dataReassigning
tableRowsto a new structure can lead to confusion and may affect other parts of the code that rely on the original data. Consider using a new variable, such astransposedRows, to store the transposed data.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/common/meta/dashboards/v5/mod.rs (1 hunks)
- web/src/components/dashboards/addPanel/ConfigPanel.vue (2 hunks)
- web/src/composables/useDashboardPanel.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
- web/src/utils/dashboard/convertTableData.ts (3 hunks)
Additional context used
Path-based instructions (1)
src/common/meta/dashboards/v5/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (9)
src/common/meta/dashboards/v5/mod.rs (1)
222-225: LGTM!The addition of the new optional fields
table_transpose,table_dynamic_columns, andwrap_table_cellsto thePanelConfigstruct is a good way to expand the configuration options for table presentations in panels. Making the fields optional allows for backward compatibility and flexibility in panel configuration.The
serdeannotation#[serde(skip_serializing_if = "Option::is_none")]ensures that the fields are only serialized when they have a value, keeping the serialized representation concise.web/src/locales/languages/en.json (2)
575-575: Looks good!The addition of the
"tableTranspose": "Transpose"localization entry supports a new feature for transposing tables in dashboards. The key name and value are appropriate.
576-576: Looks good!The addition of the
"tableDynamicColumns": "Allow Dynamic Columns"localization entry supports a new feature for allowing dynamic columns in table panels in dashboards. The key name and value are appropriate.web/src/components/dashboards/addPanel/ConfigPanel.vue (3)
60-65: LGTM!The addition of the
table_transposeconfiguration option for table panels looks good. The toggle is properly bound to the panel configuration, uses localization for the label, and includes a data-test attribute for testability.
69-74: LGTM!The addition of the
table_dynamic_columnsconfiguration option for table panels looks good. The toggle is properly bound to the panel configuration, uses localization for the label, and includes a data-test attribute for testability.
998-1007: LGTM!The initialization logic for the
table_transposeandtable_dynamic_columnsproperties in theonBeforeMounthook looks good. Setting default values for these properties ensures consistent behavior and prevents undefined property access.web/src/composables/useDashboardPanel.ts (1)
80-81: Looks good! The added properties enhance the dashboard panel configuration.The new
table_transposeandtable_dynamic_columnsproperties in theconfigobject provide additional options for customizing the presentation of tables in dashboard panels. Setting the default values tofalseis appropriate.web/src/utils/dashboard/convertTableData.ts (2)
174-176: Verify that 'searchQueryData[0]' is an array before mappingThe code assumes that
searchQueryData[0]is an array when using.map(). IfsearchQueryData[0]is not an array, this will result in a runtime error. Add a check to ensure it is an array or adjust the data structure accordingly.To verify the data structure of
searchQueryData[0], consider logging its type or inspecting its contents during runtime.
231-242: Potential date formatting issues in transposed dataWhen formatting histogram fields in transposed data, the code assumes that the values are dates. Ensure that the values are valid date representations to prevent
NaNor invalid date errors during formatting.Run the following script to check for invalid date values in the transposed histogram fields:
Verification successful
Date formatting in histogram fields appears robust, but could benefit from error handling
The code handles various date formats and considers timezones, which is good practice. However, to further improve reliability, consider adding error handling for potentially invalid date inputs. For example:
- In
web/src/utils/dashboard/convertTableData.ts, wherehistogramFieldsare processed, you could add a try-catch block to handle potential errors when creating Date objects or formatting dates.Example:
obj["format"] = (val: any) => { try { return formatDate( toZonedTime( typeof val === "string" ? `${val}Z` : new Date(val)?.getTime() / 1000, store.state.timezone, ), ); } catch (error) { console.error(`Error formatting date: ${error.message}`); return 'Invalid Date'; } };This change would prevent potential runtime errors and provide a fallback for invalid date inputs.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for invalid date values in transposed histogram fields. # Test: Search for non-date values in histogram fields. # Expect: No matches indicating invalid dates. # Assuming histogramFields contains the field names. histogramFields=("field1" "field2") # Replace with actual field names. for field in "${histogramFields[@]}"; do rg --type ts "$field" -A 5 | grep -E "$field.*=.*(NaN|Invalid date)" doneLength of output: 253
Script:
#!/bin/bash # Find the definition and usage of histogramFields echo "Searching for histogramFields definition:" rg --type ts "histogramFields\s*=\s*\[" -A 5 echo "\nSearching for histogramFields usage:" rg --type ts "histogramFields\.includes\(" -A 5 echo "\nSearching for potential date formatting issues:" rg --type ts "new Date\(.*\)\.getTime\(\)" -C 3Length of output: 8255
| const xAxisKeys = x.map((key: any) => key.alias); | ||
| const yAxisKeys = y.map((key: any) => key.alias); | ||
| responseKeys = responseKeys?.filter( | ||
| (key: any) => !xAxisKeys.includes(key) && !yAxisKeys.includes(key), | ||
| ); |
There was a problem hiding this comment.
Ensure all field aliases exist before filtering
The code assumes that all entries in x and y have an alias property. If any entry lacks this property, it could lead to runtime errors. Consider adding checks or default values to handle cases where key.alias might be undefined.
| { | ||
| name: "label", | ||
| field: "label", | ||
| label: transposeColumnLabel, | ||
| align: "left", | ||
| }, // Add label column with the first column's label | ||
| ...uniqueTransposeColumns.map((it: any) => { | ||
| let obj: any = {}; | ||
| const isNumber = isSampleValuesNumbers(tableRows, it, 20); | ||
|
|
||
| obj["name"] = it; | ||
| obj["field"] = it; | ||
| obj["label"] = it; | ||
| obj["align"] = !isNumber ? "left" : "right"; | ||
| obj["sortable"] = true; | ||
|
|
||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | ||
| obj["format"] = (val: any) => { | ||
| return !Number.isNaN(val) | ||
| ? `${ | ||
| formatUnitValue( | ||
| getUnitValue( | ||
| val, | ||
| panelSchema.config?.unit, | ||
| panelSchema.config?.unit_custom, | ||
| panelSchema.config?.decimals ?? 2, | ||
| ), | ||
| ) ?? 0 | ||
| }` | ||
| : val; | ||
| }; | ||
| } | ||
|
|
||
| if (histogramFields.includes(it)) { | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone, | ||
| ), | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
| return obj; | ||
| }), |
There was a problem hiding this comment.
Duplicate logic in column definition for transposed data
Similar to the non-transposed case, the column formatting logic for transposed data is duplicated. Extract this logic into a shared helper function to reduce code duplication and enhance maintainability.
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | ||
| obj["format"] = (val: any) => { |
There was a problem hiding this comment.
Possible incorrect sort function
The custom sort function for numeric columns uses parseFloat(a) - parseFloat(b). However, a and b are entire row objects, not just the cell values. The sort function should access the specific field value for accurate sorting.
Apply this diff to correct the sort function:
- obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b);
+ obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | |
| obj["format"] = (val: any) => { | |
| obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]); | |
| obj["format"] = (val: any) => { |
| obj["format"] = (val: any) => { | ||
| return !Number.isNaN(val) | ||
| ? `${ | ||
| formatUnitValue( | ||
| getUnitValue( | ||
| val, | ||
| panelSchema.config?.unit, | ||
| panelSchema.config?.unit_custom, | ||
| panelSchema.config?.decimals ?? 2, | ||
| ), | ||
| ) ?? 0 | ||
| }` | ||
| : val; | ||
| }; |
There was a problem hiding this comment.
Improve number formatting logic
The formatting function checks if val is not NaN using Number.isNaN(val), but Number.isNaN returns false for non-number types. This might lead to improper formatting of values. Ensure that val is converted to a number before this check.
Apply this diff to correct the formatting function:
obj["format"] = (val: any) => {
- return !Number.isNaN(val)
+ const numVal = Number(val);
+ return !Number.isNaN(numVal)
? `${
formatUnitValue(
getUnitValue(
- val,
+ numVal,
panelSchema.config?.unit,
panelSchema.config?.unit_custom,
panelSchema.config?.decimals ?? 2,
),
) ?? 0
}`
: val;
};Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj["format"] = (val: any) => { | |
| return !Number.isNaN(val) | |
| ? `${ | |
| formatUnitValue( | |
| getUnitValue( | |
| val, | |
| panelSchema.config?.unit, | |
| panelSchema.config?.unit_custom, | |
| panelSchema.config?.decimals ?? 2, | |
| ), | |
| ) ?? 0 | |
| }` | |
| : val; | |
| }; | |
| obj["format"] = (val: any) => { | |
| const numVal = Number(val); | |
| return !Number.isNaN(numVal) | |
| ? `${ | |
| formatUnitValue( | |
| getUnitValue( | |
| numVal, | |
| panelSchema.config?.unit, | |
| panelSchema.config?.unit_custom, | |
| panelSchema.config?.decimals ?? 2, | |
| ), | |
| ) ?? 0 | |
| }` | |
| : val; | |
| }; |
ktx-kirtan
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
709d768 to
d0cb692
Compare
ktx-vaidehi
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
d0cb692 to
61b757f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/common/meta/dashboards/v5/mod.rs (1 hunks)
- web/src/components/dashboards/addPanel/ConfigPanel.vue (2 hunks)
- web/src/composables/useDashboardPanel.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
- web/src/utils/dashboard/convertTableData.ts (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/common/meta/dashboards/v5/mod.rs
- web/src/components/dashboards/addPanel/ConfigPanel.vue
- web/src/composables/useDashboardPanel.ts
- web/src/locales/languages/en.json
Additional comments not posted (3)
web/src/utils/dashboard/convertTableData.ts (3)
35-35: LGTM: Function signature and new variable declarationsThe changes to the function signature and the introduction of new variables for dynamic columns and transposition are well-structured and set up the groundwork for the new features.
Also applies to: 46-50, 53-54
123-128: LGTM: Transposition logic setupThe setup for the table transposition feature is clear, well-structured, and properly integrates with the existing column generation logic. The use of configuration flags (
isTransposeEnabled) and the selection of the transpose column are handled appropriately.
Line range hint
1-300: Overall assessment: Well-implemented features with room for refinementThe implementation of dynamic columns and table transposition features is well-structured and functional. The code effectively handles various scenarios and integrates well with existing functionality. However, there are opportunities for improvement:
- Enhance type safety by reducing the use of
anytypes where possible.- Implement more robust error handling, especially for date formatting.
- Reduce code duplication by extracting common logic into reusable functions.
- Consider minor optimizations, such as using
Setfor filtering operations.These refinements will further improve the code's maintainability, reliability, and performance. Great work on implementing these complex features!
| let responseKeys: any = new Set(); | ||
|
|
||
| // insert all keys of searchQueryData into responseKeys | ||
| tableRows?.forEach((row: any) => { | ||
| Object.keys(row).forEach((key) => { | ||
| responseKeys?.add(key); | ||
| }); | ||
| }); | ||
|
|
||
| // set to array | ||
| responseKeys = Array.from(responseKeys); | ||
|
|
||
| // remove x and y keys | ||
| const xAxisKeys = x.map((key: any) => key.alias); | ||
| const yAxisKeys = y.map((key: any) => key.alias); | ||
| responseKeys = responseKeys?.filter( | ||
| (key: any) => !xAxisKeys.includes(key) && !yAxisKeys.includes(key), | ||
| ); | ||
|
|
||
| // create panelSchema fields object | ||
| responseKeys = responseKeys.map((key: any) => ({ | ||
| label: key, | ||
| alias: key, | ||
| column: key, | ||
| color: null, | ||
| isDerived: false, | ||
| })); | ||
|
|
||
| // add responseKeys to columnData | ||
| columnData = [...columnData, ...responseKeys]; | ||
| } |
There was a problem hiding this comment.
LGTM: Dynamic columns logic with minor optimization suggestion
The implementation of dynamic columns is well-structured and efficiently handles the creation of additional columns based on the response data. It properly integrates with the existing columnData structure.
Consider using Set for xAxisKeys and yAxisKeys to improve the performance of the filtering operation:
const xAxisKeys = new Set(x.map((key: any) => key.alias));
const yAxisKeys = new Set(y.map((key: any) => key.alias));
responseKeys = responseKeys?.filter(
(key: any) => !xAxisKeys.has(key) && !yAxisKeys.has(key)
);This change would make the filtering operation more efficient, especially for larger datasets.
| columns = columnData.map((it: any) => { | ||
| let obj: any = {}; | ||
| const isNumber = isSampleValuesNumbers(tableRows, it.alias, 20); | ||
| obj["name"] = it.label; | ||
| obj["field"] = it.alias; | ||
| obj["label"] = it.label; | ||
| obj["align"] = !isNumber ? "left" : "right"; | ||
| obj["sortable"] = true; | ||
| // if number then sort by number and use decimal point config option in format | ||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | ||
| obj["format"] = (val: any) => { | ||
| return !Number.isNaN(val) | ||
| ? `${ | ||
| formatUnitValue( | ||
| getUnitValue( | ||
| val, | ||
| panelSchema.config?.unit, | ||
| panelSchema.config?.unit_custom, | ||
| panelSchema.config?.decimals ?? 2, | ||
| ), | ||
| ) ?? 0 | ||
| }` | ||
| : val; | ||
| }; | ||
| } | ||
|
|
||
| // if current field is histogram field then return formatted date | ||
| if (histogramFields.includes(it.alias)) { | ||
| // if current field is histogram field then return formatted date | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone | ||
| ) | ||
| ); | ||
| }; | ||
| } | ||
| return obj; | ||
| }); | ||
| if (histogramFields.includes(it.alias)) { | ||
| // if current field is histogram field then return formatted date | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone, | ||
| ), | ||
| ); | ||
| }; | ||
| } | ||
| return obj; | ||
| }); |
There was a problem hiding this comment.
LGTM with suggestions: Non-transposed column generation logic
The updated column generation logic for non-transposed data is comprehensive and handles various data types appropriately. However, there are some potential improvements in type safety and error handling:
- Consider using more specific types instead of
anywhere possible to improve type safety. - The
formatfunction for numeric values could be improved:
obj["format"] = (val: any) => {
const numVal = Number(val);
return !Number.isNaN(numVal)
? `${
formatUnitValue(
getUnitValue(
numVal,
panelSchema.config?.unit,
panelSchema.config?.unit_custom,
panelSchema.config?.decimals ?? 2,
),
) ?? 0
}`
: val;
};This change ensures that val is properly converted to a number before checking if it's NaN.
- For the histogram fields, consider adding error handling for invalid date values:
obj["format"] = (val: any) => {
try {
return formatDate(
toZonedTime(
typeof val === "string"
? `${val}Z`
: new Date(val)?.getTime() / 1000,
store.state.timezone,
),
);
} catch (error) {
console.error(`Error formatting date: ${val}`, error);
return val; // Return original value if formatting fails
}
};These changes will improve the robustness and reliability of the column generation logic.
| } else { | ||
| // lets get all columns from a particular field | ||
| const transposeColumns = searchQueryData[0].map( | ||
| (it: any) => it[transposeColumn] ?? "", | ||
| ); | ||
|
|
||
| const uniqueTransposeColumns: any = []; | ||
| const columnDuplicationMap: any = {}; | ||
|
|
||
| transposeColumns.forEach((col: any, index: any) => { | ||
| if (!columnDuplicationMap[col]) { | ||
| uniqueTransposeColumns.push(col); | ||
| columnDuplicationMap[col] = 1; | ||
| } else { | ||
| const uniqueCol = `${col}_${columnDuplicationMap[col]}`; | ||
| uniqueTransposeColumns.push(uniqueCol); | ||
| columnDuplicationMap[col] += 1; | ||
| } | ||
| }); | ||
|
|
||
| // Filter out the first column but retain the label | ||
| columnData = columnData.filter((it: any) => it.alias !== transposeColumn); | ||
|
|
||
| // Generate label and corresponding transposed data rows | ||
| columns = [ | ||
| { | ||
| name: "label", | ||
| field: "label", | ||
| label: transposeColumnLabel, | ||
| align: "left", | ||
| }, // Add label column with the first column's label | ||
| ...uniqueTransposeColumns.map((it: any) => { | ||
| let obj: any = {}; | ||
| const isNumber = isSampleValuesNumbers(tableRows, it, 20); | ||
|
|
||
| obj["name"] = it; | ||
| obj["field"] = it; | ||
| obj["label"] = it; | ||
| obj["align"] = !isNumber ? "left" : "right"; | ||
| obj["sortable"] = true; | ||
|
|
||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | ||
| obj["format"] = (val: any) => { | ||
| return !Number.isNaN(val) | ||
| ? `${ | ||
| formatUnitValue( | ||
| getUnitValue( | ||
| val, | ||
| panelSchema.config?.unit, | ||
| panelSchema.config?.unit_custom, | ||
| panelSchema.config?.decimals ?? 2, | ||
| ), | ||
| ) ?? 0 | ||
| }` | ||
| : val; | ||
| }; | ||
| } | ||
|
|
||
| if (histogramFields.includes(it)) { | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone, | ||
| ), | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
| return obj; | ||
| }), | ||
| ]; | ||
|
|
||
| // Transpose rows, adding 'label' as the first column | ||
| tableRows = columnData.map((it: any) => { | ||
| let obj = uniqueTransposeColumns.reduce( | ||
| (acc: any, curr: any, reduceIndex: any) => { | ||
| acc[curr] = searchQueryData[0][reduceIndex][it.alias] ?? ""; | ||
| return acc; | ||
| }, | ||
| {}, | ||
| ); | ||
| obj["label"] = it.label || transposeColumnLabel; // Add the label corresponding to each column | ||
| return obj; | ||
| }); | ||
| } |
There was a problem hiding this comment.
LGTM with suggestions: Transposed column and row generation logic
The new logic for handling column and row generation for transposed data is comprehensive and meets the transposition requirements well. It properly handles duplicate column names and integrates the 'label' column. However, there are some potential improvements:
-
Consider extracting the column generation logic into a separate function to improve readability and maintainability.
-
The numeric value formatting logic is duplicated. Consider extracting it into a reusable function:
const formatNumericValue = (val: any) => {
const numVal = Number(val);
return !Number.isNaN(numVal)
? `${
formatUnitValue(
getUnitValue(
numVal,
panelSchema.config?.unit,
panelSchema.config?.unit_custom,
panelSchema.config?.decimals ?? 2,
),
) ?? 0
}`
: val;
};- Similarly, the date formatting logic could be extracted into a reusable function with error handling:
const formatDateValue = (val: any) => {
try {
return formatDate(
toZonedTime(
typeof val === "string"
? `${val}Z`
: new Date(val)?.getTime() / 1000,
store.state.timezone,
),
);
} catch (error) {
console.error(`Error formatting date: ${val}`, error);
return val;
}
};- Consider using more specific types instead of
anywhere possible to improve type safety.
These changes will improve the code structure, reduce duplication, and enhance maintainability of the transposed data handling logic.
ktx-vaidehi
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
32d1a0f to
b362f42
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
web/src/utils/dashboard/convertTableData.ts (4)
53-85: LGTM: Dynamic column generation with optimization suggestionThe implementation of dynamic columns is well-structured and efficiently handles the creation of additional columns based on the response data. It properly integrates with the existing
columnDatastructure.Consider using
SetforxAxisKeysandyAxisKeysto improve the performance of the filtering operation:const xAxisKeys = new Set(x.map((key: any) => key.alias)); const yAxisKeys = new Set(y.map((key: any) => key.alias)); responseKeys = responseKeys?.filter( (key: any) => !xAxisKeys.has(key) && !yAxisKeys.has(key) );This change would make the filtering operation more efficient, especially for larger datasets.
123-171: LGTM with suggestions: Non-transposed column generation logicThe updated column generation logic for non-transposed data is comprehensive and handles various data types appropriately. However, there are some potential improvements in type safety and error handling:
- Consider using more specific types instead of
anywhere possible to improve type safety.- The
formatfunction for numeric values could be improved:obj["format"] = (val: any) => { const numVal = Number(val); return !Number.isNaN(numVal) ? `${ formatUnitValue( getUnitValue( numVal, panelSchema.config?.unit, panelSchema.config?.unit_custom, panelSchema.config?.decimals ?? 2, ), ) ?? 0 }` : val; };This change ensures that
valis properly converted to a number before checking if it'sNaN.
- For the histogram fields, consider adding error handling for invalid date values:
obj["format"] = (val: any) => { try { return formatDate( toZonedTime( typeof val === "string" ? `${val}Z` : new Date(val)?.getTime() / 1000, store.state.timezone, ), ); } catch (error) { console.error(`Error formatting date: ${val}`, error); return val; // Return original value if formatting fails } };These changes will improve the robustness and reliability of the column generation logic.
172-260: LGTM with suggestions: Transposed column and row generation logicThe new logic for handling column and row generation for transposed data is comprehensive and meets the transposition requirements well. It properly handles duplicate column names and integrates the 'label' column. However, there are some potential improvements:
Consider extracting the column generation logic into a separate function to improve readability and maintainability.
The numeric value formatting logic is duplicated. Consider extracting it into a reusable function:
const formatNumericValue = (val: any) => { const numVal = Number(val); return !Number.isNaN(numVal) ? `${ formatUnitValue( getUnitValue( numVal, panelSchema.config?.unit, panelSchema.config?.unit_custom, panelSchema.config?.decimals ?? 2, ), ) ?? 0 }` : val; };
- Similarly, the date formatting logic could be extracted into a reusable function with error handling:
const formatDateValue = (val: any) => { try { return formatDate( toZonedTime( typeof val === "string" ? `${val}Z` : new Date(val)?.getTime() / 1000, store.state.timezone, ), ); } catch (error) { console.error(`Error formatting date: ${val}`, error); return val; } };
- Consider using more specific types instead of
anywhere possible to improve type safety.These changes will improve the code structure, reduce duplication, and enhance maintainability of the transposed data handling logic.
Line range hint
1-260: Overall assessment: Significant improvements with room for optimizationThe changes to the
convertTableDatafunction introduce powerful new features for dynamic columns and table transposition. These enhancements greatly increase the flexibility and functionality of the dashboard. The implementation is generally well-structured and handles various edge cases appropriately.However, there are several opportunities for further optimization and improvement:
- Addressing type safety issues by replacing
anywith more specific types where possible.- Extracting repeated logic (like numeric and date formatting) into reusable functions.
- Optimizing performance in areas like the dynamic column generation and sorting functions.
- Improving error handling, particularly for date formatting.
Implementing these suggestions will further enhance the robustness, maintainability, and performance of the code. Great work on the new features, and with these refinements, the code will be even stronger.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/dashboards/v5/mod.rs (1 hunks)
- web/src/components/dashboards/VariablesValueSelector.vue (1 hunks)
- web/src/components/dashboards/addPanel/ConfigPanel.vue (2 hunks)
- web/src/composables/useDashboardPanel.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
- web/src/utils/dashboard/convertTableData.ts (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/common/meta/dashboards/v5/mod.rs
- web/src/components/dashboards/VariablesValueSelector.vue
- web/src/components/dashboards/addPanel/ConfigPanel.vue
- web/src/composables/useDashboardPanel.ts
- web/src/locales/languages/en.json
Additional comments not posted (1)
web/src/utils/dashboard/convertTableData.ts (1)
49-49: LGTM: Proper initialization of columnDataThe initialization of
columnDataby spreadingxandyis a clean and efficient way to combine these arrays.
ktx-vaidehi
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
4d8b353 to
46d6124
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/dashboards/v5/mod.rs (1 hunks)
- web/src/components/dashboards/VariablesValueSelector.vue (1 hunks)
- web/src/components/dashboards/addPanel/ConfigPanel.vue (2 hunks)
- web/src/composables/useDashboardPanel.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
- web/src/utils/dashboard/convertTableData.ts (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/common/meta/dashboards/v5/mod.rs
- web/src/components/dashboards/VariablesValueSelector.vue
- web/src/components/dashboards/addPanel/ConfigPanel.vue
- web/src/composables/useDashboardPanel.ts
- web/src/locales/languages/en.json
Additional context used
Biome
web/src/utils/dashboard/convertTableData.ts
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Additional comments not posted (6)
web/src/utils/dashboard/convertTableData.ts (6)
53-85: LGTM: Well-implemented dynamic columns logicThe implementation of dynamic columns is efficient and well-structured. It properly handles the creation of additional columns based on the response data and integrates well with the existing
columnDatastructure.Good practices observed:
- Using
Setfor collecting unique keys, ensuring efficiency.- Proper filtering of x and y axis keys to avoid duplication.
- Clear structure for creating new column objects.
This implementation enhances the flexibility of the table data conversion process.
123-171: LGTM: Well-structured column generation logicThe column generation logic is well-organized, clearly separating the transposed and non-transposed cases. The implementation for non-transposed data correctly handles various data types and formatting requirements.
Positive aspects:
- Clear conditional logic based on
isTransposeEnabled.- Proper handling of numeric and histogram fields.
- Correct application of formatting and alignment based on data types.
172-230: LGTM with suggestions: Comprehensive transposed column generationThe implementation for generating transposed columns is thorough and handles various edge cases well. It correctly manages column duplication and provides special handling for histogram fields.
Positive aspects:
- Effective handling of duplicate column names with numeric suffixes.
- Comprehensive logic for formatting histogram fields in the first column.
- Proper timezone conversion for date fields.
Suggestions for improvement:
- Consider extracting the histogram field formatting logic into a separate function to improve readability.
- The nested conditionals for date formatting (lines 198-230) could potentially be simplified or broken down into smaller functions for better maintainability.
These suggestions are optional and aimed at enhancing code readability and maintainability.
Tools
Biome
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
233-288: LGTM: Well-implemented column generation for transposed dataThe column generation logic for transposed data is well-structured and consistent with the non-transposed case. It correctly handles the addition of the 'label' column and applies appropriate formatting and sorting logic.
Positive aspects:
- Proper integration of the 'label' column.
- Consistent handling of numeric and histogram fields.
- Correct application of timezone conversion for histogram fields.
290-311: LGTM: Effective implementation of row transpositionThe row transposition logic is well-implemented and correctly handles the transformation of data for the transposed table view.
Positive aspects:
- Proper addition of the 'label' column as the first column in transposed rows.
- Correct application of histogram field formatting with timezone conversion.
- Efficient use of
reduceto create transposed row objects.The implementation ensures that the transposed data structure is consistent with the newly generated column structure, providing a coherent transposed view of the table data.
Line range hint
1-349: Overall assessment: Well-implemented feature with minor improvement opportunitiesThe changes to
convertTableData.tssuccessfully implement dynamic columns and table transposition features. The code is generally well-structured and handles various edge cases effectively.Key strengths:
- Efficient implementation of dynamic columns.
- Comprehensive handling of transposed data.
- Consistent formatting and sorting logic across different data types.
Areas for improvement:
- Replace deep cloning method with a more efficient alternative.
- Fix numeric sorting functions in both transposed and non-transposed cases.
- Use
Number.isNaNfor safer type checking.- Consider refactoring some complex logic (e.g., histogram field handling) into separate functions for improved readability.
These changes have significantly enhanced the flexibility and functionality of the table data conversion process. Addressing the mentioned issues will further improve the code's robustness and maintainability.
Tools
Biome
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
|
|
||
| const tableRows = JSON.parse(JSON.stringify(searchQueryData[0])); | ||
| let columnData = [...x, ...y]; | ||
| let tableRows = JSON.parse(JSON.stringify(searchQueryData[0])); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more efficient deep cloning method
The current implementation uses JSON.parse(JSON.stringify()) for deep cloning searchQueryData[0]. While this works, it can be inefficient for large objects and doesn't handle non-JSON-serializable values (e.g., functions, undefined, Symbol, etc.).
Consider using a more efficient and robust deep cloning method. You could use a library like lodash's cloneDeep or implement a custom deep clone function that handles your specific data types. For example:
import _ from 'lodash';
// ...
let tableRows = _.cloneDeep(searchQueryData[0]);This approach would be more efficient and handle a wider range of data types correctly.
| ? new Date(`${baseVal}Z`) | ||
| : new Date(baseVal); | ||
|
|
||
| if (!isNaN(parsedDate.getTime())) { |
There was a problem hiding this comment.
Use Number.isNaN for safer type checking
The current use of isNaN can lead to unexpected results due to type coercion.
Replace isNaN with Number.isNaN for more predictable and type-safe behavior.
Apply this diff to address the issue:
- if (!isNaN(parsedDate.getTime())) {
+ if (!Number.isNaN(parsedDate.getTime())) {This change ensures that only actual NaN values are detected, avoiding potential issues with type coercion.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isNaN(parsedDate.getTime())) { | |
| if (!Number.isNaN(parsedDate.getTime())) { |
Tools
Biome
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| obj["sortable"] = true; | ||
| // if number then sort by number and use decimal point config option in format | ||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); |
There was a problem hiding this comment.
Fix numeric sorting function
The current implementation of the numeric sorting function may not work as intended.
The sorting function (a: any, b: any) => parseFloat(a) - parseFloat(b) assumes that a and b are the cell values to be compared. However, in most table implementations, a and b are typically row objects, not individual cell values.
Apply this diff to correct the sorting function:
- obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b);
+ obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]);This change ensures that the sorting function compares the correct field values from each row.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | |
| obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]); |
| obj["sortable"] = true; | ||
|
|
||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); |
There was a problem hiding this comment.
Fix numeric sorting function for transposed data
The numeric sorting function for transposed data has the same issue as in the non-transposed case.
The sorting function (a: any, b: any) => parseFloat(a) - parseFloat(b) assumes that a and b are the cell values to be compared, but they are likely row objects.
Apply this diff to correct the sorting function:
- obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b);
+ obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]);This change ensures that the sorting function compares the correct field values from each row in the transposed data case.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | |
| obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]); |
ktx-vaidehi
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
46d6124 to
2e1de36
Compare
ktx-vaidehi
force-pushed
the
feat/dashboard-transpose-dynamic-columns
branch
from
3411088 to
14737d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
web/src/utils/dashboard/convertTableData.ts (1)
53-85: LGTM: Dynamic columns logic with minor optimization suggestionThe implementation of dynamic columns is well-structured and efficiently handles the creation of additional columns based on the response data. It properly integrates with the existing
columnDatastructure.Consider using
SetforxAxisKeysandyAxisKeysto improve the performance of the filtering operation:const xAxisKeys = new Set(x.map((key: any) => key.alias)); const yAxisKeys = new Set(y.map((key: any) => key.alias)); responseKeys = responseKeys?.filter( (key: any) => !xAxisKeys.has(key) && !yAxisKeys.has(key) );This change would make the filtering operation more efficient, especially for larger datasets.
web/src/composables/dashboard/usePanelDataLoader.ts (1)
Line range hint
1-1412: Overall, the change is acceptable but the file complexity warrants attention.The modification to the IntersectionObserver threshold is a minor change that doesn't introduce any syntax errors or logical issues. The core functionality of the panel data loading remains intact.
However, it's worth noting that this file is quite complex, with multiple nested functions, watchers, and intricate logic for handling variables and caching. While not directly related to this change, consider the following suggestions for future maintenance:
- Consider breaking down this large composable into smaller, more focused functions or composables. This could improve readability and maintainability.
- The caching logic and variable handling could potentially be extracted into separate modules.
- Consider adding more inline documentation or JSDoc comments to explain the purpose and behavior of complex sections, especially around the variable handling and caching mechanisms.
- Look into implementing unit tests for critical parts of this composable to ensure reliability during future refactoring or feature additions.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/common/meta/dashboards/v5/mod.rs (1 hunks)
- web/src/components/dashboards/VariablesValueSelector.vue (1 hunks)
- web/src/components/dashboards/addPanel/ConfigPanel.vue (2 hunks)
- web/src/composables/dashboard/usePanelDataLoader.ts (1 hunks)
- web/src/composables/useDashboardPanel.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
- web/src/utils/dashboard/convertTableData.ts (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/common/meta/dashboards/v5/mod.rs
- web/src/components/dashboards/VariablesValueSelector.vue
- web/src/components/dashboards/addPanel/ConfigPanel.vue
- web/src/composables/useDashboardPanel.ts
- web/src/locales/languages/en.json
Additional context used
Biome
web/src/utils/dashboard/convertTableData.ts
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Additional comments not posted (1)
web/src/utils/dashboard/convertTableData.ts (1)
216-216:⚠️ Potential issueUse Number.isNaN for safer type checking
The current use of
isNaNcan lead to unexpected results due to type coercion.Replace
isNaNwithNumber.isNaNfor more predictable and type-safe behavior.Apply this diff to address the issue:
- if (!isNaN(parsedDate.getTime())) { + if (!Number.isNaN(parsedDate.getTime())) {This change ensures that only actual
NaNvalues are detected, avoiding potential issues with type coercion.Likely invalid or redundant comment.
Tools
Biome
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| panelSchema: any, | ||
| searchQueryData: any, | ||
| store: any | ||
| store: any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more efficient deep cloning method
The current implementation uses JSON.parse(JSON.stringify()) for deep cloning searchQueryData[0]. While this works, it can be inefficient for large objects and doesn't handle non-JSON-serializable values (e.g., functions, undefined, Symbol, etc.).
Consider using a more efficient and robust deep cloning method. You could use a library like lodash's cloneDeep or implement a custom deep clone function that handles your specific data types. For example:
import _ from 'lodash';
// ...
let tableRows = _.cloneDeep(searchQueryData[0]);This approach would be more efficient and handle a wider range of data types correctly.
Also applies to: 49-50
| if (!isTransposeEnabled) { | ||
| columns = columnData.map((it: any) => { | ||
| let obj: any = {}; | ||
| const isNumber = isSampleValuesNumbers(tableRows, it.alias, 20); | ||
| obj["name"] = it.label; | ||
| obj["field"] = it.alias; | ||
| obj["label"] = it.label; | ||
| obj["align"] = !isNumber ? "left" : "right"; | ||
| obj["sortable"] = true; | ||
| // if number then sort by number and use decimal point config option in format | ||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | ||
| obj["format"] = (val: any) => { | ||
| return !Number.isNaN(val) | ||
| ? `${ | ||
| formatUnitValue( | ||
| getUnitValue( | ||
| val, | ||
| panelSchema.config?.unit, | ||
| panelSchema.config?.unit_custom, | ||
| panelSchema.config?.decimals ?? 2, | ||
| ), | ||
| ) ?? 0 | ||
| }` | ||
| : val; | ||
| }; | ||
| } | ||
|
|
||
| // if current field is histogram field then return formatted date | ||
| if (histogramFields.includes(it.alias)) { | ||
| // if current field is histogram field then return formatted date | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone | ||
| ) | ||
| ); | ||
| }; | ||
| if (histogramFields.includes(it.alias)) { | ||
| // if current field is histogram field then return formatted date | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone, | ||
| ), | ||
| ); | ||
| }; | ||
| } | ||
| return obj; | ||
| }); |
There was a problem hiding this comment.
LGTM with suggestions: Non-transposed column generation logic
The updated column generation logic for non-transposed data is comprehensive and handles various data types appropriately. However, there are some potential improvements in type safety and error handling:
- Consider using more specific types instead of
anywhere possible to improve type safety. - The
formatfunction for numeric values could be improved:
obj["format"] = (val: any) => {
const numVal = Number(val);
return !Number.isNaN(numVal)
? `${
formatUnitValue(
getUnitValue(
numVal,
panelSchema.config?.unit,
panelSchema.config?.unit_custom,
panelSchema.config?.decimals ?? 2,
),
) ?? 0
}`
: val;
};This change ensures that val is properly converted to a number before checking if it's NaN.
- For the histogram fields, consider adding error handling for invalid date values:
obj["format"] = (val: any) => {
try {
return formatDate(
toZonedTime(
typeof val === "string"
? `${val}Z`
: new Date(val)?.getTime() / 1000,
store.state.timezone,
),
);
} catch (error) {
console.error(`Error formatting date: ${val}`, error);
return val; // Return original value if formatting fails
}
};These changes will improve the robustness and reliability of the column generation logic.
The numeric sorting function (a: any, b: any) => parseFloat(a) - parseFloat(b) assumes that a and b are the cell values to be compared. However, in most table implementations, a and b are typically row objects, not individual cell values. Consider updating the sort function:
obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]);This change ensures that the sorting function compares the correct field values from each row.
| } else { | ||
| // lets get all columns from a particular field | ||
| const transposeColumns = searchQueryData[0].map( | ||
| (it: any) => it[transposeColumn] ?? "", | ||
| ); | ||
|
|
||
| let uniqueTransposeColumns: any = []; | ||
| const columnDuplicationMap: any = {}; | ||
|
|
||
| transposeColumns.forEach((col: any, index: any) => { | ||
| if (!columnDuplicationMap[col]) { | ||
| uniqueTransposeColumns.push(col); | ||
| columnDuplicationMap[col] = 1; | ||
| } else { | ||
| const uniqueCol = `${col}_${columnDuplicationMap[col]}`; | ||
| uniqueTransposeColumns.push(uniqueCol); | ||
| columnDuplicationMap[col] += 1; | ||
| } | ||
| }); | ||
|
|
||
| const isFirstColumnHistogram = histogramFields.includes(transposeColumn); | ||
|
|
||
| if (isFirstColumnHistogram) { | ||
| uniqueTransposeColumns = uniqueTransposeColumns.map((val: any) => { | ||
| let formattedDate = null; | ||
|
|
||
| if (val) { | ||
| // Check if the value contains an underscore (e.g., "2024-09-23T08:12:00_1") | ||
| let baseVal = val; | ||
| let underscorePart = ""; | ||
|
|
||
| // If underscore is found, split the value and retain the underscore part | ||
| if (typeof val === "string" && val.includes("_")) { | ||
| const parts = val.split("_"); | ||
| baseVal = parts[0]; // The date part (e.g., "2024-09-23T08:12:00") | ||
| underscorePart = `_${parts[1]}`; // The underscore and suffix part (e.g., "_1") | ||
| } | ||
|
|
||
| // Parse and format the base date part | ||
| const parsedDate = | ||
| typeof baseVal === "string" | ||
| ? new Date(`${baseVal}Z`) | ||
| : new Date(baseVal); | ||
|
|
||
| if (!isNaN(parsedDate.getTime())) { | ||
| formattedDate = formatDate( | ||
| toZonedTime(parsedDate, store.state.timezone), | ||
| ); | ||
| } | ||
|
|
||
| // Append the underscore part (if it exists) back to the formatted date | ||
| formattedDate = formattedDate | ||
| ? `${formattedDate}${underscorePart}` | ||
| : null; | ||
| } | ||
|
|
||
| // Return the formatted date with the underscore or the original value if it can't be parsed | ||
| return formattedDate || val; | ||
| }); | ||
| } | ||
| return obj; | ||
| }); | ||
|
|
||
| // Filter out the first column but retain the label | ||
| columnData = columnData.filter((it: any) => it.alias !== transposeColumn); | ||
|
|
||
| // Generate label and corresponding transposed data rows | ||
| columns = [ | ||
| { | ||
| name: "label", | ||
| field: "label", | ||
| label: transposeColumnLabel, | ||
| align: "left", | ||
| }, // Add label column with the first column's label | ||
| ...uniqueTransposeColumns.map((it: any) => { | ||
| let obj: any = {}; | ||
| const isNumber = isSampleValuesNumbers(tableRows, it, 20); | ||
|
|
||
| obj["name"] = it; | ||
| obj["field"] = it; | ||
| obj["label"] = it; | ||
| obj["align"] = !isNumber ? "left" : "right"; | ||
| obj["sortable"] = true; | ||
|
|
||
| if (isNumber) { | ||
| obj["sort"] = (a: any, b: any) => parseFloat(a) - parseFloat(b); | ||
| obj["format"] = (val: any) => { | ||
| return !Number.isNaN(val) | ||
| ? `${ | ||
| formatUnitValue( | ||
| getUnitValue( | ||
| val, | ||
| panelSchema.config?.unit, | ||
| panelSchema.config?.unit_custom, | ||
| panelSchema.config?.decimals ?? 2, | ||
| ), | ||
| ) ?? 0 | ||
| }` | ||
| : val; | ||
| }; | ||
| } | ||
|
|
||
| // Check if it's a histogram field and apply timezone conversion | ||
| if (histogramFields.includes(it)) { | ||
| obj["format"] = (val: any) => { | ||
| return formatDate( | ||
| toZonedTime( | ||
| typeof val === "string" | ||
| ? `${val}Z` | ||
| : new Date(val)?.getTime() / 1000, | ||
| store.state.timezone, | ||
| ), | ||
| ); | ||
| }; | ||
| } | ||
|
|
||
| return obj; | ||
| }), | ||
| ]; | ||
|
|
||
| // Transpose rows, adding 'label' as the first column | ||
| tableRows = columnData.map((it: any) => { | ||
| let obj = uniqueTransposeColumns.reduce( | ||
| (acc: any, curr: any, reduceIndex: any) => { | ||
| const value = searchQueryData[0][reduceIndex][it.alias] ?? ""; | ||
| acc[curr] = histogramFields.includes(it.alias) | ||
| ? formatDate( | ||
| toZonedTime( | ||
| typeof value === "string" | ||
| ? `${value}Z` | ||
| : new Date(value)?.getTime() / 1000, | ||
| store.state.timezone, | ||
| ), | ||
| ) | ||
| : value; | ||
| return acc; | ||
| }, | ||
| {}, | ||
| ); | ||
| obj["label"] = it.label || transposeColumnLabel; // Add the label corresponding to each column | ||
| return obj; | ||
| }); | ||
| } |
There was a problem hiding this comment.
LGTM with suggestions: Transposed column and row generation logic
The new logic for handling column and row generation for transposed data is comprehensive and meets the transposition requirements well. It properly handles duplicate column names and integrates the 'label' column. However, there are some potential improvements:
-
Consider extracting the column generation logic into a separate function to improve readability and maintainability.
-
The numeric value formatting logic is duplicated. Consider extracting it into a reusable function:
const formatNumericValue = (val: any) => {
const numVal = Number(val);
return !Number.isNaN(numVal)
? `${
formatUnitValue(
getUnitValue(
numVal,
panelSchema.config?.unit,
panelSchema.config?.unit_custom,
panelSchema.config?.decimals ?? 2,
),
) ?? 0
}`
: val;
};- Similarly, the date formatting logic could be extracted into a reusable function with error handling:
const formatDateValue = (val: any) => {
try {
return formatDate(
toZonedTime(
typeof val === "string"
? `${val}Z`
: new Date(val)?.getTime() / 1000,
store.state.timezone,
),
);
} catch (error) {
console.error(`Error formatting date: ${val}`, error);
return val;
}
};- Consider using more specific types instead of
anywhere possible to improve type safety.
These changes will improve the code structure, reduce duplication, and enhance maintainability of the transposed data handling logic.
The numeric sorting function for transposed data has the same issue as in the non-transposed case. Update the sort function:
obj["sort"] = (a: any, b: any) => parseFloat(a[obj.field]) - parseFloat(b[obj.field]);This change ensures that the sorting function compares the correct field values from each row in the transposed data case.
Tools
Biome
[error] 216-216: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| root: null, | ||
| rootMargin: "0px", | ||
| threshold: 0.1, // Adjust as needed | ||
| threshold: 0, // Adjust as needed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the performance implications of changing the IntersectionObserver threshold.
The threshold for the IntersectionObserver has been changed from 0.1 to 0. This means the callback will be triggered as soon as any part of the target element becomes visible in the viewport, rather than waiting for 10% visibility.
While this change could lead to earlier loading of panel data and potentially improve perceived performance, it might also increase the number of API calls if many panels are partially visible at once.
Consider the following:
- Monitor the impact of this change on overall performance and the number of API calls made.
- If you notice an increase in unnecessary API calls, you might want to implement a debounce mechanism or adjust the
rootMarginto fine-tune when the intersection callback is triggered. - Consider making the threshold configurable, allowing for easy adjustment based on performance metrics.
const observerOptions = {
root: null,
rootMargin: "0px",
threshold: config.panelLoadThreshold || 0, // Use a configurable threshold
};
- #4593 - #4629 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced new configuration options for dashboard table panels: `Transpose` and `Allow Dynamic Columns`. - Added toggle components in the configuration panel for the new table settings. - Enhanced data handling for dynamic column generation and table transposition. - **Localization** - Added new entries for table-related functionalities in the localization files. - **Bug Fixes** - Updated initialization logic to ensure default values for new configuration properties. - Improved error handling for variable assignments in the dashboard components. - **Enhancements** - Adjusted the functionality of the data loader to improve visibility tracking for dashboard components. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ktx-abhay <[email protected]>
Summary by CodeRabbit
New Features
TransposeandAllow Dynamic Columns.Localization
Bug Fixes
Enhancements